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

@@ -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
+		return nil // return early
-	}
-	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
+	// 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
+	// not authorized; fail closed.
-// the following format: ip|127.0.0.1; dns|*.example.com
+	return wrapUnauthorizedError(certToBeRevoked, nil, fmt.Sprintf("account '%s' is not authorized", account.ID), nil)
-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
 }
 
 // wrapRevokeErr is a best effort implementation to transform an error during

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 {
+		"fail/different-account": 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 {
 			account := &acme.Account{
 				ID:     accountID,
 				Status: acme.StatusValid,
 			}
 			certToBeRevoked := &x509.Certificate{
-				Subject: pkix.Name{
+				IPAddresses: []net.IP{net.ParseIP("127.0.0.1")},
-					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",
-				},
 			}
 			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",
+					Detail: "No authorization provided",
-					Err:    errors.New("account 'accountID' does not have authorizations for all identifiers"),
+					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) {
+func Test_wrapUnauthorizedError(t *testing.T) {
-	tests := []struct {
+	type test struct {
-		name       string
+		cert                    *x509.Certificate
-		identifier acme.Identifier
+		unauthorizedIdentifiers []acme.Identifier
-		want       string
+		msg                     string
-	}{
+		err                     error
-		{
+		want                    *acme.Error
-			name: "ip",
+	}
-			identifier: acme.Identifier{
+	var tests = map[string]func(t *testing.T) test{
-				Type:  acme.IP,
+		"unauthorizedIdentifiers": func(t *testing.T) test {
-				Value: "10.0.0.1",
+			acmeErr := acme.NewError(acme.ErrorUnauthorizedType, "account 'accountID' is not authorized")
-			},
+			acmeErr.Status = http.StatusForbidden
-			want: "ip|10.0.0.1",
+			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,
+			}
 		},
-		{
+		"subject": func(t *testing.T) test {
-			name: "dns",
+			acmeErr := acme.NewError(acme.ErrorUnauthorizedType, "account 'accountID' is not authorized")
-			identifier: acme.Identifier{
+			acmeErr.Status = http.StatusForbidden
-				Type:  acme.DNS,
+			acmeErr.Detail = "No authorization provided for name test.example.com"
-				Value: "*.example.com",
+			cert := &x509.Certificate{
-			},
+				Subject: pkix.Name{
-			want: "dns|*.example.com",
+					CommonName: "test.example.com",
+				},
+			}
+			return test{
+				err:                     nil,
+				cert:                    cert,
+				unauthorizedIdentifiers: []acme.Identifier{},
+				msg:                     "account 'accountID' is not authorized",
+				want:                    acmeErr,
+			}
 		},
-		{
+		"wrap-subject": func(t *testing.T) test {
-			name: "unknown",
+			acmeErr := acme.NewError(acme.ErrorUnauthorizedType, "verification of jws using certificate public key failed: square/go-jose: error in cryptographic primitive")
-			identifier: acme.Identifier{
+			acmeErr.Status = http.StatusForbidden
-				Type:  "InvalidType",
+			acmeErr.Detail = "No authorization provided for name test.example.com"
-				Value: "<<>>",
+			cert := &x509.Certificate{
-			},
+				Subject: pkix.Name{
-			want: "unsupported|<<>>",
+					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 {
+	for name, prep := range tests {
-		t.Run(tt.name, func(t *testing.T) {
+		tc := prep(t)
-			got := identifierKey(tt.identifier)
+		t.Run(name, func(t *testing.T) {
-			if !cmp.Equal(tt.want, got) {
+			acmeErr := wrapUnauthorizedError(tc.cert, tc.unauthorizedIdentifiers, tc.msg, tc.err)
-				t.Errorf("identifierKey() diff = \n%s", cmp.Diff(tt.want, got))
+			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)
-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))
-			}
 		})
 	}
 }