From 1c38113e44d6baa3d87b9fc540cdcd28371a5a9b Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Thu, 26 Jan 2023 13:24:25 +0100 Subject: [PATCH] Add ACME `Subproblem` for more detailed ACME client-side errors When validating an ACME challenge (`device-attest-01` in this case, but it's also true for others), and validation fails, the CA didn't return a lot of information about why the challenge had failed. By introducing the ACME `Subproblem` type, an ACME `Error` can include some additional information about what went wrong when validating the challenge. This is a WIP commit. The `Subproblem` isn't created in many code paths yet, just for the `step` format at the moment. Will probably follow up with some more improvements to how the ACME error is handled. Also need to cleanup some debug things (q.Q) --- acme/api/handler.go | 3 ++ acme/challenge.go | 19 +++++++++++-- acme/db/nosql/challenge.go | 5 +++- acme/errors.go | 56 +++++++++++++++++++++++++++++++++----- go.mod | 5 +++- go.sum | 7 +++++ 6 files changed, 83 insertions(+), 12 deletions(-) diff --git a/acme/api/handler.go b/acme/api/handler.go index e6aad131..8f3b51db 100644 --- a/acme/api/handler.go +++ b/acme/api/handler.go @@ -10,6 +10,7 @@ import ( "time" "github.com/go-chi/chi" + "github.com/ryboe/q" "github.com/smallstep/certificates/acme" "github.com/smallstep/certificates/api" @@ -355,6 +356,8 @@ func GetChallenge(w http.ResponseWriter, r *http.Request) { return } + q.Q(ch) + linker.LinkChallenge(ctx, ch, azID) w.Header().Add("Link", link(linker.GetLink(ctx, acme.AuthzLinkType, azID), "up")) diff --git a/acme/challenge.go b/acme/challenge.go index 1a45a252..9eca34a5 100644 --- a/acme/challenge.go +++ b/acme/challenge.go @@ -29,6 +29,8 @@ import ( "github.com/smallstep/certificates/authority/provisioner" "go.step.sm/crypto/jose" "go.step.sm/crypto/pemutil" + + "github.com/ryboe/q" ) type ChallengeType string @@ -404,6 +406,8 @@ func deviceAttest01Validate(ctx context.Context, ch *Challenge, db DB, jwk *jose } case "step": data, err := doStepAttestationFormat(ctx, prov, ch, jwk, &att) + q.Q(data) + q.Q(err) if err != nil { var acmeError *Error if errors.As(err, &acmeError) { @@ -415,12 +419,20 @@ func deviceAttest01Validate(ctx context.Context, ch *Challenge, db DB, jwk *jose return WrapErrorISE(err, "error validating attestation") } - // Validate Apple's ClientIdentifier (Identifier.Value) with device - // identifiers. + // Validate the YubiKey serial number from the attestation + // certificate with the challenged Order value. // // Note: We might want to use an external service for this. + q.Q(data.SerialNumber, ch.Value) if data.SerialNumber != ch.Value { - return storeError(ctx, db, ch, true, NewError(ErrorBadAttestationStatementType, "permanent identifier does not match")) + q.Q("not the same") + subproblem := NewSubproblemWithIdentifier( + ErrorMalformedType, + Identifier{Type: "permanent-identifier", Value: ch.Value}, + "challenge identifier %q doesn't match the attested hardware identifier %q", ch.Value, data.SerialNumber, + ) + s2 := NewSubproblem(ErrorMalformedType, "test") + return storeError(ctx, db, ch, true, NewError(ErrorBadAttestationStatementType, "permanent identifier does not match").AddSubproblems(subproblem, s2)) } default: return storeError(ctx, db, ch, true, NewError(ErrorBadAttestationStatementType, "unexpected attestation object format")) @@ -752,6 +764,7 @@ func KeyAuthorization(token string, jwk *jose.JSONWebKey) (string, error) { // storeError the given error to an ACME error and saves using the DB interface. func storeError(ctx context.Context, db DB, ch *Challenge, markInvalid bool, err *Error) error { ch.Error = err + q.Q(err) if markInvalid { ch.Status = StatusInvalid } diff --git a/acme/db/nosql/challenge.go b/acme/db/nosql/challenge.go index f84a6f4e..05d23a1f 100644 --- a/acme/db/nosql/challenge.go +++ b/acme/db/nosql/challenge.go @@ -6,6 +6,7 @@ import ( "time" "github.com/pkg/errors" + "github.com/ryboe/q" "github.com/smallstep/certificates/acme" "github.com/smallstep/nosql" ) @@ -19,7 +20,7 @@ type dbChallenge struct { Value string `json:"value"` ValidatedAt string `json:"validatedAt"` CreatedAt time.Time `json:"createdAt"` - Error *acme.Error `json:"error"` + Error *acme.Error `json:"error"` // TODO(hs): a bit dangerous; should become db-specific type } func (dbc *dbChallenge) clone() *dbChallenge { @@ -29,6 +30,7 @@ func (dbc *dbChallenge) clone() *dbChallenge { func (db *DB) getDBChallenge(ctx context.Context, id string) (*dbChallenge, error) { data, err := db.db.Get(challengeTable, []byte(id)) + q.Q(data) if nosql.IsErrNotFound(err) { return nil, acme.NewError(acme.ErrorMalformedType, "challenge %s not found", id) } else if err != nil { @@ -39,6 +41,7 @@ func (db *DB) getDBChallenge(ctx context.Context, id string) (*dbChallenge, erro if err := json.Unmarshal(data, dbch); err != nil { return nil, errors.Wrap(err, "error unmarshaling dbChallenge") } + q.Q(dbch) return dbch, nil } diff --git a/acme/errors.go b/acme/errors.go index a969bd96..95053908 100644 --- a/acme/errors.go +++ b/acme/errors.go @@ -270,14 +270,36 @@ var ( } ) -// Error represents an ACME +// Error represents an ACME Error type Error struct { - Type string `json:"type"` - Detail string `json:"detail"` - Subproblems []interface{} `json:"subproblems,omitempty"` - Identifier interface{} `json:"identifier,omitempty"` - Err error `json:"-"` - Status int `json:"-"` + Type string `json:"type"` + Detail string `json:"detail"` + Subproblems []Subproblem `json:"subproblems,omitempty"` + + // The "identifier" field MUST NOT be present at the top level in ACME + // problem documents. It can only be present in subproblems. + // Subproblems need not all have the same type, and they do not need to + // match the top level type. + Identifier Identifier `json:"identifier,omitempty"` // TODO(hs): seems unused and MUST NOT be present; this can likely be removed + Err error `json:"-"` + Status int `json:"-"` +} + +// Subproblem represents an ACME subproblem. It's fairly +// similar to an ACME error, but differs in that it can't +// include subproblems itself, the error is reflected +// in the Detail property and doesn't have a Status. +type Subproblem struct { + Type string `json:"type"` + Detail string `json:"detail"` + Identifier *Identifier `json:"identifier,omitempty"` +} + +// AddSubproblems adds the Subproblems to Error. It +// returns the Error, allowing for fluent addition. +func (e *Error) AddSubproblems(subproblems ...Subproblem) *Error { + e.Subproblems = append(e.Subproblems, subproblems...) + return e } // NewError creates a new Error type. @@ -285,6 +307,26 @@ func NewError(pt ProblemType, msg string, args ...interface{}) *Error { return newError(pt, errors.Errorf(msg, args...)) } +// NewSubproblem creates a new Subproblem. The msg and args +// are used to create a new error, which is set as the Detail, allowing +// for more detailed error messages to be returned to the ACME client. +func NewSubproblem(pt ProblemType, msg string, args ...interface{}) Subproblem { + e := newError(pt, fmt.Errorf(msg, args...)) + s := Subproblem{ + Type: e.Type, + Detail: e.Err.Error(), + } + return s +} + +// NewSubproblemWithIdentifier creates a new Subproblem with a specific ACME +// Identifier. It calls NewSubproblem and sets the Identifier. +func NewSubproblemWithIdentifier(pt ProblemType, identifier Identifier, msg string, args ...interface{}) Subproblem { + s := NewSubproblem(pt, msg, args...) + s.Identifier = &identifier + return s +} + func newError(pt ProblemType, err error) *Error { meta, ok := errorMap[pt] if !ok { diff --git a/go.mod b/go.mod index 4fcfae3e..34bd40fd 100644 --- a/go.mod +++ b/go.mod @@ -28,7 +28,7 @@ require ( github.com/hashicorp/vault/api/auth/approle v0.3.0 github.com/hashicorp/vault/api/auth/kubernetes v0.3.0 github.com/jhump/protoreflect v1.9.0 // indirect - github.com/kr/pretty v0.3.0 // indirect + github.com/kr/pretty v0.3.1 // indirect github.com/mattn/go-colorable v0.1.8 // indirect github.com/mattn/go-isatty v0.0.13 // indirect github.com/micromdm/scep/v2 v2.1.0 @@ -122,6 +122,7 @@ require ( github.com/jackc/pgx/v4 v4.17.2 // indirect github.com/jmespath/go-jmespath v0.4.0 // indirect github.com/klauspost/compress v1.15.11 // indirect + github.com/kr/text v0.2.0 // indirect github.com/manifoldco/promptui v0.9.0 // indirect github.com/mgutz/ansi v0.0.0-20200706080929-d51e80ef957d // indirect github.com/miekg/pkcs11 v1.1.1 // indirect @@ -133,8 +134,10 @@ require ( github.com/oklog/run v1.0.0 // indirect github.com/pierrec/lz4 v2.5.2+incompatible // indirect github.com/pmezard/go-difflib v1.0.0 // indirect + github.com/rogpeppe/go-internal v1.9.0 // indirect github.com/russross/blackfriday/v2 v2.0.1 // indirect github.com/ryanuber/go-glob v1.0.0 // indirect + github.com/ryboe/q v1.0.18 // indirect github.com/shopspring/decimal v1.2.0 // indirect github.com/shurcooL/sanitized_anchor_name v1.0.0 // indirect github.com/spf13/cast v1.4.1 // indirect diff --git a/go.sum b/go.sum index 2bb94368..343d38e8 100644 --- a/go.sum +++ b/go.sum @@ -444,6 +444,8 @@ github.com/kr/pretty v0.2.0/go.mod h1:ipq/a2n7PKx3OHsz4KJII5eveXtPO4qwEXGdVfWzfn github.com/kr/pretty v0.2.1/go.mod h1:ipq/a2n7PKx3OHsz4KJII5eveXtPO4qwEXGdVfWzfnI= github.com/kr/pretty v0.3.0 h1:WgNl7dwNpEZ6jJ9k1snq4pZsg7DOEN8hP9Xw0Tsjwk0= github.com/kr/pretty v0.3.0/go.mod h1:640gp4NfQd8pI5XOwp5fnNeVWj67G7CFk/SaSQn7NBk= +github.com/kr/pretty v0.3.1 h1:flRD4NNwYAUpkphVc1HcthR4KEIFJ65n8Mw5qdRn3LE= +github.com/kr/pretty v0.3.1/go.mod h1:hoEshYVHaxMs3cyo3Yncou5ZscifuDolrwPKZanG3xk= github.com/kr/pty v1.1.1/go.mod h1:pFQYn66WHrOpPYNljwOMqo10TkYh1fy3cYio2l3bCsQ= github.com/kr/pty v1.1.8/go.mod h1:O1sed60cT9XZ5uDucP5qwvh+TE3NnUj51EiZO/lmSfw= github.com/kr/text v0.1.0/go.mod h1:4Jbv+DJW3UT/LiOwJeYQe1efqtUx/iVham/4vfdArNI= @@ -549,6 +551,7 @@ github.com/pierrec/lz4 v1.0.2-0.20190131084431-473cd7ce01a1/go.mod h1:3/3N9NVKO0 github.com/pierrec/lz4 v2.0.5+incompatible/go.mod h1:pdkljMzZIN41W+lC3N2tnIh5sFi+IEE17M5jbnwPHcY= github.com/pierrec/lz4 v2.5.2+incompatible h1:WCjObylUIOlKy/+7Abdn34TLIkXiA4UWUMhxq9m9ZXI= github.com/pierrec/lz4 v2.5.2+incompatible/go.mod h1:pdkljMzZIN41W+lC3N2tnIh5sFi+IEE17M5jbnwPHcY= +github.com/pkg/diff v0.0.0-20210226163009-20ebb0f2a09e/go.mod h1:pJLUxLENpZxwdsKMEsNbx1VGcRFpLqf3715MtcvvzbA= github.com/pkg/errors v0.8.0/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINEl0= github.com/pkg/errors v0.8.1/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINEl0= github.com/pkg/errors v0.9.1 h1:FEBLx1zS214owpjy7qsBeixbURkuhQAwrK5UwLGTwt4= @@ -582,6 +585,8 @@ github.com/rogpeppe/fastuuid v1.2.0/go.mod h1:jVj6XXZzXRy/MSR5jhDC/2q6DgLz+nrA6L github.com/rogpeppe/go-internal v1.3.0/go.mod h1:M8bDsm7K2OlrFYOpmOWEs/qY81heoFRclV5y23lUDJ4= github.com/rogpeppe/go-internal v1.6.1 h1:/FiVV8dS/e+YqF2JvO3yXRFbBLTIuSDkuC7aBOAvL+k= github.com/rogpeppe/go-internal v1.6.1/go.mod h1:xXDCJY+GAPziupqXw64V24skbSoqbTEfhy4qGm1nDQc= +github.com/rogpeppe/go-internal v1.9.0 h1:73kH8U+JUqXU8lRuOHeVHaa/SZPifC7BkcraZVejAe8= +github.com/rogpeppe/go-internal v1.9.0/go.mod h1:WtVeX8xhTBvf0smdhujwtBcq4Qrzq/fJaraNFVN+nFs= github.com/rs/xid v1.2.1/go.mod h1:+uKXf+4Djp6Md1KODXJxgGQPKngRmWyn10oCKFzNHOQ= github.com/rs/xid v1.4.0 h1:qd7wPTDkN6KQx2VmMBLrpHkiyQwgFXRnkOLacUiaSNY= github.com/rs/xid v1.4.0/go.mod h1:trrq9SKmegXys3aeAKXMUTdJsYXVwGY3RLcfgqegfbg= @@ -594,6 +599,8 @@ github.com/ryanuber/columnize v0.0.0-20160712163229-9b3edd62028f/go.mod h1:sm1tb github.com/ryanuber/columnize v2.1.0+incompatible/go.mod h1:sm1tb6uqfes/u+d4ooFouqFdy9/2g9QGwK3SQygK0Ts= github.com/ryanuber/go-glob v1.0.0 h1:iQh3xXAumdQ+4Ufa5b25cRpC5TYKlno6hsv6Cb3pkBk= github.com/ryanuber/go-glob v1.0.0/go.mod h1:807d1WSdnB0XRJzKNil9Om6lcp/3a0v4qIHxIXzX/Yc= +github.com/ryboe/q v1.0.18 h1:uTonPt1eZjy7GSpB0XpYpsCvX+Yf9f+M4CUKuH2r+vg= +github.com/ryboe/q v1.0.18/go.mod h1:elqvVf/GBuZHvZ9gvHv4MKM6NZAMz2rFajnTgQZ46wU= github.com/samuel/go-zookeeper v0.0.0-20190923202752-2cc03de413da/go.mod h1:gi+0XIa01GRL2eRQVjQkKGqKF3SF9vZR/HnPullcV2E= github.com/satori/go.uuid v1.2.0/go.mod h1:dA0hQrYB0VpLJoorglMZABFdXlWrHn1NEOzdhQKdks0= github.com/sean-/seed v0.0.0-20170313163322-e2103e2c3529/go.mod h1:DxrIzT+xaE7yg65j358z/aeFdxmN0P9QXhEzd20vsDc=