From 0524122191cecc55086d1eadedfe4f47158ba219 Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Wed, 8 Dec 2021 16:28:14 +0100 Subject: [PATCH] Remove authorization flow for different Account private keys As discussed in https://github.com/smallstep/certificates/issues/767, we opted for not including this authorization flow to prevent users from getting OOMs. We can add the functionality back when the underlying data store can provide access to a long list of Authorizations more efficiently, for example when a callback is implemented. --- acme/api/revoke.go | 95 ++-------- acme/api/revoke_test.go | 379 ++++++++++------------------------------ 2 files changed, 103 insertions(+), 371 deletions(-) diff --git a/acme/api/revoke.go b/acme/api/revoke.go index 4d31c7be..d01e401c 100644 --- a/acme/api/revoke.go +++ b/acme/api/revoke.go @@ -153,91 +153,22 @@ func (h *Handler) isAccountAuthorized(ctx context.Context, dbCert *acme.Certific } certificateBelongsToAccount := dbCert.AccountID == account.ID if certificateBelongsToAccount { - return nil // return early; skip relatively expensive database check - } - requiredIdentifiers := extractIdentifiers(certToBeRevoked) - if len(requiredIdentifiers) == 0 { - return wrapUnauthorizedError(certToBeRevoked, nil, "cannot authorize revocation without providing identifiers to authorize", nil) - } - authzs, err := h.db.GetAuthorizationsByAccountID(ctx, account.ID) - if err != nil { - return acme.WrapErrorISE(err, "error retrieving authorizations for Account %s", account.ID) - } - authorizedIdentifiers := map[string]acme.Identifier{} - for _, authz := range authzs { - // Only valid Authorizations are included - if authz.Status != acme.StatusValid { - continue - } - authorizedIdentifiers[identifierKey(authz.Identifier)] = authz.Identifier - } - if len(authorizedIdentifiers) == 0 { - unauthorizedIdentifiers := []acme.Identifier{} - for _, identifier := range requiredIdentifiers { - unauthorizedIdentifiers = append(unauthorizedIdentifiers, identifier) - } - return wrapUnauthorizedError(certToBeRevoked, unauthorizedIdentifiers, fmt.Sprintf("account '%s' does not have valid authorizations", account.ID), nil) - } - unauthorizedIdentifiers := []acme.Identifier{} - for key := range requiredIdentifiers { - _, ok := authorizedIdentifiers[key] - if !ok { - unauthorizedIdentifiers = append(unauthorizedIdentifiers, requiredIdentifiers[key]) - } - } - if len(unauthorizedIdentifiers) != 0 { - return wrapUnauthorizedError(certToBeRevoked, unauthorizedIdentifiers, fmt.Sprintf("account '%s' does not have authorizations for all identifiers", account.ID), nil) + return nil // return early } - return nil -} + // TODO(hs): according to RFC8555: 7.6, a server MUST consider the following accounts authorized + // to revoke a certificate: + // + // o the account that issued the certificate. + // o an account that holds authorizations for all of the identifiers in the certificate. + // + // We currently only support the first case. The second might result in step going OOM when + // large numbers of Authorizations are involved when the current nosql interface is in use. + // We want to protect users from this failure scenario, so that's why it hasn't been added yet. + // This issue is tracked in https://github.com/smallstep/certificates/issues/767 -// identifierKey creates a unique key for an ACME identifier using -// the following format: ip|127.0.0.1; dns|*.example.com -func identifierKey(identifier acme.Identifier) string { - if identifier.Type == acme.IP { - return "ip|" + identifier.Value - } - if identifier.Type == acme.DNS { - return "dns|" + identifier.Value - } - return "unsupported|" + identifier.Value -} - -// extractIdentifiers extracts ACME identifiers from an x509 certificate and -// creates a map from them. The map ensures that duplicate SANs are deduplicated. -// The Subject CommonName is included, because RFC8555 7.4 states that DNS -// identifiers can come from either the CommonName or a DNS SAN or both. When -// authorizing issuance, the DNS identifier must be in the request and will be -// included in the validation (see Order.sans()) as of now. This means that the -// CommonName will in fact have an authorization available. -func extractIdentifiers(cert *x509.Certificate) map[string]acme.Identifier { - result := map[string]acme.Identifier{} - for _, name := range cert.DNSNames { - identifier := acme.Identifier{ - Type: acme.DNS, - Value: name, - } - result[identifierKey(identifier)] = identifier - } - for _, ip := range cert.IPAddresses { - identifier := acme.Identifier{ - Type: acme.IP, - Value: ip.String(), - } - result[identifierKey(identifier)] = identifier - } - if cert.Subject.CommonName != "" { - identifier := acme.Identifier{ - // assuming only DNS can be in Common Name (RFC8555, 7.4); RFC8738 - // IP Identifier Validation Extension does not state anything about this. - // This logic is in accordance with the logic in order.canonicalize() - Type: acme.DNS, - Value: cert.Subject.CommonName, - } - result[identifierKey(identifier)] = identifier - } - return result + // not authorized; fail closed. + return wrapUnauthorizedError(certToBeRevoked, nil, fmt.Sprintf("account '%s' is not authorized", account.ID), nil) } // wrapRevokeErr is a best effort implementation to transform an error during diff --git a/acme/api/revoke_test.go b/acme/api/revoke_test.go index cf036abb..4ff54405 100644 --- a/acme/api/revoke_test.go +++ b/acme/api/revoke_test.go @@ -715,35 +715,6 @@ func TestHandler_RevokeCert(t *testing.T) { }, } }, - "fail/db.GetAuthorizationsByAccountID-error": func(t *testing.T) test { - acc := &acme.Account{ID: "accountID", Status: acme.StatusValid} - ctx := context.WithValue(context.Background(), provisionerContextKey, prov) - ctx = context.WithValue(ctx, accContextKey, acc) - ctx = context.WithValue(ctx, payloadContextKey, &payloadInfo{value: payloadBytes}) - ctx = context.WithValue(ctx, jwsContextKey, jws) - ctx = context.WithValue(ctx, baseURLContextKey, baseURL) - ctx = context.WithValue(ctx, chi.RouteCtxKey, chiCtx) - db := &acme.MockDB{ - MockGetCertificateBySerial: func(ctx context.Context, serial string) (*acme.Certificate, error) { - assert.Equals(t, cert.SerialNumber.String(), serial) - return &acme.Certificate{ - AccountID: "differentAccountID", - Leaf: cert, - }, nil - }, - MockGetAuthorizationsByAccountID: func(ctx context.Context, accountID string) ([]*acme.Authorization, error) { - return nil, errors.New("force") - }, - } - ca := &mockCA{} - return test{ - db: db, - ca: ca, - ctx: ctx, - statusCode: 500, - err: acme.NewErrorISE("error retrieving authorizations for Account %s", "accountID"), - } - }, "fail/account-not-authorized": func(t *testing.T) test { acc := &acme.Account{ID: "accountID", Status: acme.StatusValid} ctx := context.WithValue(context.Background(), provisionerContextKey, prov) @@ -1150,101 +1121,13 @@ func TestHandler_isAccountAuthorized(t *testing.T) { }, } }, - "fail/no-certificate-identifiers": func(t *testing.T) test { - account := &acme.Account{ - ID: accountID, - Status: acme.StatusValid, - } - certToBeRevoked := &x509.Certificate{} - existingCert := &acme.Certificate{ - AccountID: "differentAccountID", - } - return test{ - ctx: context.TODO(), - existingCert: existingCert, - certToBeRevoked: certToBeRevoked, - account: account, - err: &acme.Error{ - Type: "urn:ietf:params:acme:error:unauthorized", - Status: http.StatusForbidden, - Detail: "No authorization provided", - Err: errors.New("cannot authorize revocation without providing identifiers to authorize"), - }, - } - }, - "fail/db.GetAuthorizationsByAccountID-error": func(t *testing.T) test { + "fail/different-account": func(t *testing.T) test { account := &acme.Account{ ID: accountID, Status: acme.StatusValid, } certToBeRevoked := &x509.Certificate{ - Subject: pkix.Name{ - CommonName: "127.0.0.1", - }, - } - existingCert := &acme.Certificate{ - AccountID: "differentAccountID", - } - return test{ - db: &acme.MockDB{ - MockGetAuthorizationsByAccountID: func(ctx context.Context, accountID string) ([]*acme.Authorization, error) { - assert.Equals(t, "accountID", accountID) - return nil, errors.New("force") - }, - }, - ctx: context.TODO(), - existingCert: existingCert, - certToBeRevoked: certToBeRevoked, - account: account, - err: acme.NewErrorISE("error retrieving authorizations for Account %s: force", accountID), - } - }, - "fail/no-valid-authorizations": func(t *testing.T) test { - account := &acme.Account{ - ID: accountID, - Status: acme.StatusValid, - } - certToBeRevoked := &x509.Certificate{ - Subject: pkix.Name{ - CommonName: "127.0.0.1", - }, - } - existingCert := &acme.Certificate{ - AccountID: "differentAccountID", - } - return test{ - db: &acme.MockDB{ - MockGetAuthorizationsByAccountID: func(ctx context.Context, accountID string) ([]*acme.Authorization, error) { - assert.Equals(t, "accountID", accountID) - return []*acme.Authorization{ - { - AccountID: accountID, - Status: acme.StatusInvalid, - }, - }, nil - }, - }, - ctx: context.TODO(), - existingCert: existingCert, - certToBeRevoked: certToBeRevoked, - account: account, - err: &acme.Error{ - Type: "urn:ietf:params:acme:error:unauthorized", - Status: http.StatusForbidden, - Detail: "No authorization provided for name 127.0.0.1", - Err: errors.New("account 'accountID' does not have valid authorizations"), - }, - } - }, - "fail/authorizations-do-not-match-identifiers": func(t *testing.T) test { - account := &acme.Account{ - ID: accountID, - Status: acme.StatusValid, - } - certToBeRevoked := &x509.Certificate{ - Subject: pkix.Name{ - CommonName: "127.0.0.1", - }, + IPAddresses: []net.IP{net.ParseIP("127.0.0.1")}, } existingCert := &acme.Certificate{ AccountID: "differentAccountID", @@ -1259,7 +1142,7 @@ func TestHandler_isAccountAuthorized(t *testing.T) { Status: acme.StatusValid, Identifier: acme.Identifier{ Type: acme.IP, - Value: "127.0.0.2", + Value: "127.0.0.1", }, }, }, nil @@ -1272,8 +1155,8 @@ func TestHandler_isAccountAuthorized(t *testing.T) { err: &acme.Error{ Type: "urn:ietf:params:acme:error:unauthorized", Status: http.StatusForbidden, - Detail: "No authorization provided for name 127.0.0.1", - Err: errors.New("account 'accountID' does not have authorizations for all identifiers"), + Detail: "No authorization provided", + Err: errors.New("account 'accountID' is not authorized"), }, } }, @@ -1286,7 +1169,7 @@ func TestHandler_isAccountAuthorized(t *testing.T) { IPAddresses: []net.IP{net.ParseIP("127.0.0.1")}, } existingCert := &acme.Certificate{ - AccountID: "differentAccountID", + AccountID: "accountID", } return test{ db: &acme.MockDB{ @@ -1340,176 +1223,94 @@ func TestHandler_isAccountAuthorized(t *testing.T) { } } -func Test_identifierKey(t *testing.T) { - tests := []struct { - name string - identifier acme.Identifier - want string - }{ - { - name: "ip", - identifier: acme.Identifier{ - Type: acme.IP, - Value: "10.0.0.1", - }, - want: "ip|10.0.0.1", +func Test_wrapUnauthorizedError(t *testing.T) { + type test struct { + cert *x509.Certificate + unauthorizedIdentifiers []acme.Identifier + msg string + err error + want *acme.Error + } + var tests = map[string]func(t *testing.T) test{ + "unauthorizedIdentifiers": func(t *testing.T) test { + acmeErr := acme.NewError(acme.ErrorUnauthorizedType, "account 'accountID' is not authorized") + acmeErr.Status = http.StatusForbidden + acmeErr.Detail = "No authorization provided for name 127.0.0.1" + return test{ + err: nil, + cert: nil, + unauthorizedIdentifiers: []acme.Identifier{ + { + Type: acme.IP, + Value: "127.0.0.1", + }, + }, + msg: "account 'accountID' is not authorized", + want: acmeErr, + } }, - { - name: "dns", - identifier: acme.Identifier{ - Type: acme.DNS, - Value: "*.example.com", - }, - want: "dns|*.example.com", + "subject": func(t *testing.T) test { + acmeErr := acme.NewError(acme.ErrorUnauthorizedType, "account 'accountID' is not authorized") + acmeErr.Status = http.StatusForbidden + acmeErr.Detail = "No authorization provided for name test.example.com" + cert := &x509.Certificate{ + Subject: pkix.Name{ + CommonName: "test.example.com", + }, + } + return test{ + err: nil, + cert: cert, + unauthorizedIdentifiers: []acme.Identifier{}, + msg: "account 'accountID' is not authorized", + want: acmeErr, + } }, - { - name: "unknown", - identifier: acme.Identifier{ - Type: "InvalidType", - Value: "<<>>", - }, - want: "unsupported|<<>>", + "wrap-subject": func(t *testing.T) test { + acmeErr := acme.NewError(acme.ErrorUnauthorizedType, "verification of jws using certificate public key failed: square/go-jose: error in cryptographic primitive") + acmeErr.Status = http.StatusForbidden + acmeErr.Detail = "No authorization provided for name test.example.com" + cert := &x509.Certificate{ + Subject: pkix.Name{ + CommonName: "test.example.com", + }, + } + return test{ + err: errors.New("square/go-jose: error in cryptographic primitive"), + cert: cert, + unauthorizedIdentifiers: []acme.Identifier{}, + msg: "verification of jws using certificate public key failed", + want: acmeErr, + } + }, + "default": func(t *testing.T) test { + acmeErr := acme.NewError(acme.ErrorUnauthorizedType, "account 'accountID' is not authorized") + acmeErr.Status = http.StatusForbidden + acmeErr.Detail = "No authorization provided" + cert := &x509.Certificate{ + Subject: pkix.Name{ + CommonName: "", + }, + } + return test{ + err: nil, + cert: cert, + unauthorizedIdentifiers: []acme.Identifier{}, + msg: "account 'accountID' is not authorized", + want: acmeErr, + } }, } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - got := identifierKey(tt.identifier) - if !cmp.Equal(tt.want, got) { - t.Errorf("identifierKey() diff = \n%s", cmp.Diff(tt.want, got)) - } - }) - } -} - -func Test_extractIdentifiers(t *testing.T) { - tests := []struct { - name string - cert *x509.Certificate - want map[string]acme.Identifier - }{ - { - name: "ip", - cert: &x509.Certificate{ - IPAddresses: []net.IP{net.ParseIP("127.0.0.1")}, - }, - want: map[string]acme.Identifier{ - "ip|127.0.0.1": { - Type: acme.IP, - Value: "127.0.0.1", - }, - }, - }, - { - name: "dns", - cert: &x509.Certificate{ - DNSNames: []string{"*.example.com"}, - }, - want: map[string]acme.Identifier{ - "dns|*.example.com": { - Type: acme.DNS, - Value: "*.example.com", - }, - }, - }, - { - name: "dns-subject", - cert: &x509.Certificate{ - Subject: pkix.Name{ - CommonName: "www.example.com", - }, - }, - want: map[string]acme.Identifier{ - "dns|www.example.com": { - Type: acme.DNS, - Value: "www.example.com", - }, - }, - }, - { - name: "ip-subject", - cert: &x509.Certificate{ - Subject: pkix.Name{ - CommonName: "127.0.0.1", - }, - }, - want: map[string]acme.Identifier{ - "dns|127.0.0.1": { // this is the currently expected behavior - Type: acme.DNS, - Value: "127.0.0.1", - }, - }, - }, - { - name: "combined", - cert: &x509.Certificate{ - Subject: pkix.Name{ - CommonName: "127.0.0.1", - }, - IPAddresses: []net.IP{net.ParseIP("127.0.0.1"), net.ParseIP("127.0.0.2")}, - DNSNames: []string{"*.example.com", "www.example.com"}, - }, - want: map[string]acme.Identifier{ - "ip|127.0.0.1": { - Type: acme.IP, - Value: "127.0.0.1", - }, - "ip|127.0.0.2": { - Type: acme.IP, - Value: "127.0.0.2", - }, - "dns|*.example.com": { - Type: acme.DNS, - Value: "*.example.com", - }, - "dns|www.example.com": { - Type: acme.DNS, - Value: "www.example.com", - }, - "dns|127.0.0.1": { // this is the currently expected behavior - Type: acme.DNS, - Value: "127.0.0.1", - }, - }, - }, - { - name: "ip-duplicates", - cert: &x509.Certificate{ - IPAddresses: []net.IP{net.ParseIP("127.0.0.1"), net.ParseIP("127.0.0.1"), net.ParseIP("127.0.0.2")}, - }, - want: map[string]acme.Identifier{ - "ip|127.0.0.1": { - Type: acme.IP, - Value: "127.0.0.1", - }, - "ip|127.0.0.2": { - Type: acme.IP, - Value: "127.0.0.2", - }, - }, - }, - { - name: "dns-duplicates", - cert: &x509.Certificate{ - DNSNames: []string{"*.example.com", "www.example.com", "www.example.com"}, - }, - want: map[string]acme.Identifier{ - "dns|*.example.com": { - Type: acme.DNS, - Value: "*.example.com", - }, - "dns|www.example.com": { - Type: acme.DNS, - Value: "www.example.com", - }, - }, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - got := extractIdentifiers(tt.cert) - if !cmp.Equal(tt.want, got) { - t.Errorf("extractIdentifiers() diff=\n%s", cmp.Diff(tt.want, got)) - } + for name, prep := range tests { + tc := prep(t) + t.Run(name, func(t *testing.T) { + acmeErr := wrapUnauthorizedError(tc.cert, tc.unauthorizedIdentifiers, tc.msg, tc.err) + assert.Equals(t, acmeErr.Err.Error(), tc.want.Err.Error()) + 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) }) } }