Skip to content

Commit

Permalink
Merge pull request #1235 from smallstep/herman/acme-da-subject-check
Browse files Browse the repository at this point in the history 
Improve validation and error messages for Orders with Permanent Identifier
  • Loading branch information
@hslatman
hslatman authored Feb 2, 2023
2 parents 067f9c9 + 3a6fc5e commit da00046
Show file tree
Hide file tree
Showing 15 changed files with 1,913 additions and 669 deletions.
1 change: 0 additions & 1 deletion acme/account_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,6 @@ func TestExternalAccountKey_BindTo(t *testing.T) {
if assert.True(t, errors.As(err, &ae)) {
assert.Equals(t, ae.Type, tt.err.Type)
assert.Equals(t, ae.Detail, tt.err.Detail)
assert.Equals(t, ae.Identifier, tt.err.Identifier)
assert.Equals(t, ae.Subproblems, tt.err.Subproblems)
}
} else {
Expand Down
3 changes: 0 additions & 3 deletions acme/api/account_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -388,7 +388,6 @@ func TestHandler_GetOrdersByAccountID(t *testing.T) {

assert.Equals(t, ae.Type, tc.err.Type)
assert.Equals(t, ae.Detail, tc.err.Detail)
assert.Equals(t, ae.Identifier, tc.err.Identifier)
assert.Equals(t, ae.Subproblems, tc.err.Subproblems)
assert.Equals(t, res.Header["Content-Type"], []string{"application/problem+json"})
} else {
Expand Down Expand Up @@ -828,7 +827,6 @@ func TestHandler_NewAccount(t *testing.T) {

assert.Equals(t, ae.Type, tc.err.Type)
assert.Equals(t, ae.Detail, tc.err.Detail)
assert.Equals(t, ae.Identifier, tc.err.Identifier)
assert.Equals(t, ae.Subproblems, tc.err.Subproblems)
assert.Equals(t, res.Header["Content-Type"], []string{"application/problem+json"})
} else {
Expand Down Expand Up @@ -1032,7 +1030,6 @@ func TestHandler_GetOrUpdateAccount(t *testing.T) {

assert.Equals(t, ae.Type, tc.err.Type)
assert.Equals(t, ae.Detail, tc.err.Detail)
assert.Equals(t, ae.Identifier, tc.err.Identifier)
assert.Equals(t, ae.Subproblems, tc.err.Subproblems)
assert.Equals(t, res.Header["Content-Type"], []string{"application/problem+json"})
} else {
Expand Down
2 changes: 0 additions & 2 deletions acme/api/eab_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -866,7 +866,6 @@ func TestHandler_validateExternalAccountBinding(t *testing.T) {
assert.Equals(t, ae.Status, tc.err.Status)
assert.HasPrefix(t, ae.Err.Error(), tc.err.Err.Error())
assert.Equals(t, ae.Detail, tc.err.Detail)
assert.Equals(t, ae.Identifier, tc.err.Identifier)
assert.Equals(t, ae.Subproblems, tc.err.Subproblems)
}
} else {
Expand Down Expand Up @@ -1145,7 +1144,6 @@ func Test_validateEABJWS(t *testing.T) {
assert.Equals(t, tc.err.Status, err.Status)
assert.HasPrefix(t, err.Err.Error(), tc.err.Err.Error())
assert.Equals(t, tc.err.Detail, err.Detail)
assert.Equals(t, tc.err.Identifier, err.Identifier)
assert.Equals(t, tc.err.Subproblems, err.Subproblems)
} else {
assert.Nil(t, err)
Expand Down
4 changes: 0 additions & 4 deletions acme/api/handler_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,6 @@ func TestHandler_GetDirectory(t *testing.T) {

assert.Equals(t, ae.Type, tc.err.Type)
assert.Equals(t, ae.Detail, tc.err.Detail)
assert.Equals(t, ae.Identifier, tc.err.Identifier)
assert.Equals(t, ae.Subproblems, tc.err.Subproblems)
assert.Equals(t, res.Header["Content-Type"], []string{"application/problem+json"})
} else {
Expand Down Expand Up @@ -366,7 +365,6 @@ func TestHandler_GetAuthorization(t *testing.T) {

assert.Equals(t, ae.Type, tc.err.Type)
assert.Equals(t, ae.Detail, tc.err.Detail)
assert.Equals(t, ae.Identifier, tc.err.Identifier)
assert.Equals(t, ae.Subproblems, tc.err.Subproblems)
assert.Equals(t, res.Header["Content-Type"], []string{"application/problem+json"})
} else {
Expand Down Expand Up @@ -509,7 +507,6 @@ func TestHandler_GetCertificate(t *testing.T) {

assert.Equals(t, ae.Type, tc.err.Type)
assert.HasPrefix(t, ae.Detail, tc.err.Detail)
assert.Equals(t, ae.Identifier, tc.err.Identifier)
assert.Equals(t, ae.Subproblems, tc.err.Subproblems)
assert.Equals(t, res.Header["Content-Type"], []string{"application/problem+json"})
} else {
Expand Down Expand Up @@ -768,7 +765,6 @@ func TestHandler_GetChallenge(t *testing.T) {

assert.Equals(t, ae.Type, tc.err.Type)
assert.Equals(t, ae.Detail, tc.err.Detail)
assert.Equals(t, ae.Identifier, tc.err.Identifier)
assert.Equals(t, ae.Subproblems, tc.err.Subproblems)
assert.Equals(t, res.Header["Content-Type"], []string{"application/problem+json"})
} else {
Expand Down
11 changes: 0 additions & 11 deletions acme/api/middleware_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,6 @@ func TestHandler_addNonce(t *testing.T) {

assert.Equals(t, ae.Type, tc.err.Type)
assert.Equals(t, ae.Detail, tc.err.Detail)
assert.Equals(t, ae.Identifier, tc.err.Identifier)
assert.Equals(t, ae.Subproblems, tc.err.Subproblems)
assert.Equals(t, res.Header["Content-Type"], []string{"application/problem+json"})
} else {
Expand Down Expand Up @@ -147,7 +146,6 @@ func TestHandler_addDirLink(t *testing.T) {

assert.Equals(t, ae.Type, tc.err.Type)
assert.Equals(t, ae.Detail, tc.err.Detail)
assert.Equals(t, ae.Identifier, tc.err.Identifier)
assert.Equals(t, ae.Subproblems, tc.err.Subproblems)
assert.Equals(t, res.Header["Content-Type"], []string{"application/problem+json"})
} else {
Expand Down Expand Up @@ -252,7 +250,6 @@ func TestHandler_verifyContentType(t *testing.T) {

assert.Equals(t, ae.Type, tc.err.Type)
assert.Equals(t, ae.Detail, tc.err.Detail)
assert.Equals(t, ae.Identifier, tc.err.Identifier)
assert.Equals(t, ae.Subproblems, tc.err.Subproblems)
assert.Equals(t, res.Header["Content-Type"], []string{"application/problem+json"})
} else {
Expand Down Expand Up @@ -320,7 +317,6 @@ func TestHandler_isPostAsGet(t *testing.T) {

assert.Equals(t, ae.Type, tc.err.Type)
assert.Equals(t, ae.Detail, tc.err.Detail)
assert.Equals(t, ae.Identifier, tc.err.Identifier)
assert.Equals(t, ae.Subproblems, tc.err.Subproblems)
assert.Equals(t, res.Header["Content-Type"], []string{"application/problem+json"})
} else {
Expand Down Expand Up @@ -410,7 +406,6 @@ func TestHandler_parseJWS(t *testing.T) {

assert.Equals(t, ae.Type, tc.err.Type)
assert.Equals(t, ae.Detail, tc.err.Detail)
assert.Equals(t, ae.Identifier, tc.err.Identifier)
assert.Equals(t, ae.Subproblems, tc.err.Subproblems)
assert.Equals(t, res.Header["Content-Type"], []string{"application/problem+json"})
} else {
Expand Down Expand Up @@ -606,7 +601,6 @@ func TestHandler_verifyAndExtractJWSPayload(t *testing.T) {

assert.Equals(t, ae.Type, tc.err.Type)
assert.Equals(t, ae.Detail, tc.err.Detail)
assert.Equals(t, ae.Identifier, tc.err.Identifier)
assert.Equals(t, ae.Subproblems, tc.err.Subproblems)
assert.Equals(t, res.Header["Content-Type"], []string{"application/problem+json"})
} else {
Expand Down Expand Up @@ -808,7 +802,6 @@ func TestHandler_lookupJWK(t *testing.T) {

assert.Equals(t, ae.Type, tc.err.Type)
assert.Equals(t, ae.Detail, tc.err.Detail)
assert.Equals(t, ae.Identifier, tc.err.Identifier)
assert.Equals(t, ae.Subproblems, tc.err.Subproblems)
assert.Equals(t, res.Header["Content-Type"], []string{"application/problem+json"})
} else {
Expand Down Expand Up @@ -1008,7 +1001,6 @@ func TestHandler_extractJWK(t *testing.T) {

assert.Equals(t, ae.Type, tc.err.Type)
assert.Equals(t, ae.Detail, tc.err.Detail)
assert.Equals(t, ae.Identifier, tc.err.Identifier)
assert.Equals(t, ae.Subproblems, tc.err.Subproblems)
assert.Equals(t, res.Header["Content-Type"], []string{"application/problem+json"})
} else {
Expand Down Expand Up @@ -1384,7 +1376,6 @@ func TestHandler_validateJWS(t *testing.T) {

assert.Equals(t, ae.Type, tc.err.Type)
assert.Equals(t, ae.Detail, tc.err.Detail)
assert.Equals(t, ae.Identifier, tc.err.Identifier)
assert.Equals(t, ae.Subproblems, tc.err.Subproblems)
assert.Equals(t, res.Header["Content-Type"], []string{"application/problem+json"})
} else {
Expand Down Expand Up @@ -1567,7 +1558,6 @@ func TestHandler_extractOrLookupJWK(t *testing.T) {

assert.Equals(t, ae.Type, tc.err.Type)
assert.Equals(t, ae.Detail, tc.err.Detail)
assert.Equals(t, ae.Identifier, tc.err.Identifier)
assert.Equals(t, ae.Subproblems, tc.err.Subproblems)
assert.Equals(t, res.Header["Content-Type"], []string{"application/problem+json"})
} else {
Expand Down Expand Up @@ -1652,7 +1642,6 @@ func TestHandler_checkPrerequisites(t *testing.T) {
assert.FatalError(t, json.Unmarshal(bytes.TrimSpace(body), &ae))
assert.Equals(t, ae.Type, tc.err.Type)
assert.Equals(t, ae.Detail, tc.err.Detail)
assert.Equals(t, ae.Identifier, tc.err.Identifier)
assert.Equals(t, ae.Subproblems, tc.err.Subproblems)
assert.Equals(t, res.Header["Content-Type"], []string{"application/problem+json"})
} else {
Expand Down
3 changes: 0 additions & 3 deletions acme/api/order_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -486,7 +486,6 @@ func TestHandler_GetOrder(t *testing.T) {

assert.Equals(t, ae.Type, tc.err.Type)
assert.Equals(t, ae.Detail, tc.err.Detail)
assert.Equals(t, ae.Identifier, tc.err.Identifier)
assert.Equals(t, ae.Subproblems, tc.err.Subproblems)
assert.Equals(t, res.Header["Content-Type"], []string{"application/problem+json"})
} else {
Expand Down Expand Up @@ -1846,7 +1845,6 @@ func TestHandler_NewOrder(t *testing.T) {

assert.Equals(t, ae.Type, tc.err.Type)
assert.Equals(t, ae.Detail, tc.err.Detail)
assert.Equals(t, ae.Identifier, tc.err.Identifier)
assert.Equals(t, ae.Subproblems, tc.err.Subproblems)
assert.Equals(t, res.Header["Content-Type"], []string{"application/problem+json"})
} else {
Expand Down Expand Up @@ -2144,7 +2142,6 @@ func TestHandler_FinalizeOrder(t *testing.T) {

assert.Equals(t, ae.Type, tc.err.Type)
assert.Equals(t, ae.Detail, tc.err.Detail)
assert.Equals(t, ae.Identifier, tc.err.Identifier)
assert.Equals(t, ae.Subproblems, tc.err.Subproblems)
assert.Equals(t, res.Header["Content-Type"], []string{"application/problem+json"})
} else {
Expand Down
3 changes: 0 additions & 3 deletions acme/api/revoke_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1090,7 +1090,6 @@ func TestHandler_RevokeCert(t *testing.T) {

assert.Equals(t, ae.Type, tc.err.Type)
assert.Equals(t, ae.Detail, tc.err.Detail)
assert.Equals(t, ae.Identifier, tc.err.Identifier)
assert.Equals(t, ae.Subproblems, tc.err.Subproblems)
assert.Equals(t, res.Header["Content-Type"], []string{"application/problem+json"})
} else {
Expand Down Expand Up @@ -1230,7 +1229,6 @@ func TestHandler_isAccountAuthorized(t *testing.T) {
assert.Equals(t, acmeErr.Type, tc.err.Type)
assert.Equals(t, acmeErr.Status, tc.err.Status)
assert.Equals(t, acmeErr.Detail, tc.err.Detail)
assert.Equals(t, acmeErr.Identifier, tc.err.Identifier)
assert.Equals(t, acmeErr.Subproblems, tc.err.Subproblems)

})
Expand Down Expand Up @@ -1323,7 +1321,6 @@ func Test_wrapUnauthorizedError(t *testing.T) {
assert.Equals(t, acmeErr.Type, tc.want.Type)
assert.Equals(t, acmeErr.Status, tc.want.Status)
assert.Equals(t, acmeErr.Detail, tc.want.Detail)
assert.Equals(t, acmeErr.Identifier, tc.want.Identifier)
assert.Equals(t, acmeErr.Subproblems, tc.want.Subproblems)
})
}
Expand Down
34 changes: 19 additions & 15 deletions acme/challenge.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,10 @@ import (
"time"

"github.com/fxamacker/cbor/v2"
"github.com/smallstep/certificates/authority/provisioner"
"go.step.sm/crypto/jose"
"go.step.sm/crypto/pemutil"

"github.com/smallstep/certificates/authority/provisioner"
)

type ChallengeType string
Expand Down Expand Up @@ -79,10 +80,9 @@ func (ch *Challenge) ToLog() (interface{}, error) {
return string(b), nil
}

// Validate attempts to validate the challenge. Stores changes to the Challenge
// type using the DB interface.
// satisfactorily validated, the 'status' and 'validated' attributes are
// updated.
// Validate attempts to validate the Challenge. Stores changes to the Challenge
// type using the DB interface. If the Challenge is validated, the 'status' and
// 'validated' attributes are updated.
func (ch *Challenge) Validate(ctx context.Context, db DB, jwk *jose.JSONWebKey, payload []byte) error {
// If already valid or invalid then return without performing validation.
if ch.Status != StatusPending {
Expand Down Expand Up @@ -335,20 +335,19 @@ func dns01Validate(ctx context.Context, ch *Challenge, db DB, jwk *jose.JSONWebK
return nil
}

type Payload struct {
type payloadType struct {
AttObj string `json:"attObj"`
Error string `json:"error"`
}

type AttestationObject struct {
type attestationObject struct {
Format string `json:"fmt"`
AttStatement map[string]interface{} `json:"attStmt,omitempty"`
}

// TODO(bweeks): move attestation verification to a shared package.
// TODO(bweeks): define new error type for failed attestation validation.
func deviceAttest01Validate(ctx context.Context, ch *Challenge, db DB, jwk *jose.JSONWebKey, payload []byte) error {
var p Payload
var p payloadType
if err := json.Unmarshal(payload, &p); err != nil {
return WrapErrorISE(err, "error unmarshalling JSON")
}
Expand All @@ -362,7 +361,7 @@ func deviceAttest01Validate(ctx context.Context, ch *Challenge, db DB, jwk *jose
return WrapErrorISE(err, "error base64 decoding attObj")
}

att := AttestationObject{}
att := attestationObject{}
if err := cbor.Unmarshal(attObj, &att); err != nil {
return WrapErrorISE(err, "error unmarshalling CBOR")
}
Expand Down Expand Up @@ -415,12 +414,17 @@ 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.
if data.SerialNumber != ch.Value {
return storeError(ctx, db, ch, true, NewError(ErrorBadAttestationStatementType, "permanent identifier does not match"))
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,
)
return storeError(ctx, db, ch, true, NewError(ErrorBadAttestationStatementType, "permanent identifier does not match").AddSubproblems(subproblem))
}
default:
return storeError(ctx, db, ch, true, NewError(ErrorBadAttestationStatementType, "unexpected attestation object format"))
Expand Down Expand Up @@ -469,7 +473,7 @@ type appleAttestationData struct {
Certificate *x509.Certificate
}

func doAppleAttestationFormat(ctx context.Context, prov Provisioner, ch *Challenge, att *AttestationObject) (*appleAttestationData, error) {
func doAppleAttestationFormat(ctx context.Context, prov Provisioner, ch *Challenge, att *attestationObject) (*appleAttestationData, error) {
// Use configured or default attestation roots if none is configured.
roots, ok := prov.GetAttestationRoots()
if !ok {
Expand Down Expand Up @@ -570,7 +574,7 @@ type stepAttestationData struct {
SerialNumber string
}

func doStepAttestationFormat(ctx context.Context, prov Provisioner, ch *Challenge, jwk *jose.JSONWebKey, att *AttestationObject) (*stepAttestationData, error) {
func doStepAttestationFormat(ctx context.Context, prov Provisioner, ch *Challenge, jwk *jose.JSONWebKey, att *attestationObject) (*stepAttestationData, error) {
// Use configured or default attestation roots if none is configured.
roots, ok := prov.GetAttestationRoots()
if !ok {
Expand Down
Loading

0 comments on commit da00046

Please sign in to comment.