Enforce ACME accounts to a specific directory path (#20363)

* Enforce ACME accounts to a specific directory path

 - Accounts and correspondingly orders, authz should not cross
   the path boundaries. So we now tag an ACME account with a specific
   directory based on the requested role/issuer values in the path.
 - If an operation occurs on a different acme directory path it will
   cause a failure of the request.
 - Add some go doc to a few places and reorder the methods in the
   acme_wrappers.go class to highlight the wrappers and not intertwine
   the helper functions
 - Rename path_acme_new_account.go to path_acme_account.go as it has
   several account related methods now.

* Get rid of bad test case

 - The previous commit contained a bug fix for us properly
   loading issuers within the ACME path, that exposed
   this broken/bad test case. Simply remove it.

builtin/logical/pki/acme_state.go

@@ -160,6 +160,7 @@ type acmeAccount struct {
 	Contact              []string          `json:"contact"`
 	TermsOfServiceAgreed bool              `json:"termsOfServiceAgreed"`
 	Jwk                  []byte            `json:"jwk"`
+	AcmeDirectory        string            `json:"acme-directory"`
 }
 
 type acmeOrder struct {
@@ -171,7 +172,8 @@ type acmeOrder struct {
 	AuthorizationIds        []string            `json:"authorization-ids"`
 	CertificateSerialNumber string              `json:"cert-serial-number"`
 	CertificateExpiry       time.Time           `json:"cert-expiry"`
-	IssuerId                issuerID            `json:"issuer-id"`
+	// The actual issuer UUID that issued the certificate, blank if an order exists but no certificate was issued.
+	IssuerId issuerID `json:"issuer-id"`
 }
 
 func (o acmeOrder) getIdentifierDNSValues() []string {
@@ -229,6 +231,7 @@ func (a *acmeState) CreateAccount(ac *acmeContext, c *jwsCtx, contact []string,
 		TermsOfServiceAgreed: termsOfServiceAgreed,
 		Jwk:                  c.Jwk,
 		Status:               StatusValid,
+		AcmeDirectory:        ac.acmeDirectory,
 	}
 	json, err := logical.StorageEntryJSON(acmeAccountPrefix+c.Kid, acct)
 	if err != nil {
@@ -272,6 +275,10 @@ func (a *acmeState) LoadAccount(ac *acmeContext, keyId string) (*acmeAccount, er
 		return nil, fmt.Errorf("error decoding account: %w", err)
 	}
 
+	if acct.AcmeDirectory != ac.acmeDirectory {
+		return nil, fmt.Errorf("%w: account part of different ACME directory path", ErrMalformed)
+	}
+
 	acct.KeyId = keyId
 
 	return &acct, nil

builtin/logical/pki/acme_wrappers.go

@@ -22,6 +22,9 @@ type acmeContext struct {
 	sc         *storageContext
 	role       *roleEntry
 	issuer     *issuerEntry
+	// acmeDirectory is a string that can distinguish the various acme directories we have configured
+	// if something needs to remain locked into a directory path structure.
+	acmeDirectory string
 }
 
 type (
@@ -30,6 +33,7 @@ type (
 	acmeAccountRequiredOperation func(acmeCtx *acmeContext, r *logical.Request, fields *framework.FieldData, userCtx *jwsCtx, data map[string]interface{}, acct *acmeAccount) (*logical.Response, error)
 )
 
+// acmeErrorWrapper the lowest level wrapper that will translate errors into proper ACME error responses
 func acmeErrorWrapper(op framework.OperationFunc) framework.OperationFunc {
 	return func(ctx context.Context, r *logical.Request, data *framework.FieldData) (*logical.Response, error) {
 		resp, err := op(ctx, r, data)
@@ -41,6 +45,10 @@ func acmeErrorWrapper(op framework.OperationFunc) framework.OperationFunc {
 	}
 }
 
+// acmeWrapper a basic wrapper that all ACME handlers should leverage as the basis.
+// This will create a basic ACME context, validate basic ACME configuration is setup
+// for operations. This pulls in acmeErrorWrapper to translate error messages for users,
+// but does not enforce any sort of ACME authentication.
 func (b *backend) acmeWrapper(op acmeOperation) framework.OperationFunc {
 	return acmeErrorWrapper(func(ctx context.Context, r *logical.Request, data *framework.FieldData) (*logical.Response, error) {
 		sc := b.makeStorageContext(ctx, r.Storage)
@@ -64,86 +72,25 @@ func (b *backend) acmeWrapper(op acmeOperation) framework.OperationFunc {
 			return nil, err
 		}
 
+		acmeDirectory := getAcmeDirectory(data)
+
 		acmeCtx := &acmeContext{
-			baseUrl:    acmeBaseUrl,
-			clusterUrl: clusterBase,
-			sc:         sc,
-			role:       role,
-			issuer:     issuer,
+			baseUrl:       acmeBaseUrl,
+			clusterUrl:    clusterBase,
+			sc:            sc,
+			role:          role,
+			issuer:        issuer,
+			acmeDirectory: acmeDirectory,
 		}
 
 		return op(acmeCtx, r, data)
 	})
 }
 
-func getAcmeIssuer(sc *storageContext, issuerName string) (*issuerEntry, error) {
-	issuerId, err := sc.resolveIssuerReference(issuerName)
-	if err != nil {
-		return nil, fmt.Errorf("%w: issuer does not exist", ErrMalformed)
-	}
-
-	issuer, err := sc.fetchIssuerById(issuerId)
-	if err != nil {
-		return nil, fmt.Errorf("issuer failed to load: %w", err)
-	}
-
-	if issuer.Usage.HasUsage(IssuanceUsage) && len(issuer.KeyID) > 0 {
-		return issuer, nil
-	}
-
-	return nil, fmt.Errorf("%w: issuer missing proper issuance usage or key", ErrServerInternal)
-}
-
-func getAcmeRoleAndIssuer(sc *storageContext, data *framework.FieldData) (*roleEntry, *issuerEntry, error) {
-	requestedIssuer := defaultRef
-	requestedIssuerRaw, present := data.GetOk("issuer")
-	if present {
-		requestedIssuer = requestedIssuerRaw.(string)
-	}
-
-	var role *roleEntry
-	roleNameRaw, present := data.GetOk("role")
-	if present {
-		roleName := roleNameRaw.(string)
-		if len(roleName) > 0 {
-			var err error
-			role, err = sc.Backend.getRole(sc.Context, sc.Storage, roleName)
-			if err != nil {
-				return nil, nil, fmt.Errorf("%w: err loading role", ErrServerInternal)
-			}
-
-			if role == nil {
-				return nil, nil, fmt.Errorf("%w: role does not exist", ErrMalformed)
-			}
-
-			if role.NoStore {
-				return nil, nil, fmt.Errorf("%w: role can not be used as NoStore is set to true", ErrServerInternal)
-			}
-
-			if len(role.Issuer) > 0 {
-				requestedIssuer = role.Issuer
-			}
-		}
-	} else {
-		role = buildSignVerbatimRoleWithNoData(&roleEntry{
-			Issuer:  requestedIssuer,
-			NoStore: false,
-			Name:    "",
-		})
-	}
-
-	issuer, err := getAcmeIssuer(sc, requestedIssuer)
-	if err != nil {
-		return nil, nil, err
-	}
-
-	role.Issuer = requestedIssuer
-
-	// TODO: Need additional configuration validation here, for allowed roles/issuers.
-
-	return role, issuer, nil
-}
-
+// acmeParsedWrapper is an ACME wrapper that will parse out the ACME request parameters, validate
+// that we have a proper signature and pass to the operation a decoded map of arguments received.
+// This wrapper builds on top of acmeWrapper. Note that this does perform signature verification
+// it does not enforce the account being in a valid state nor existing.
 func (b *backend) acmeParsedWrapper(op acmeParsedOperation) framework.OperationFunc {
 	return b.acmeWrapper(func(acmeCtx *acmeContext, r *logical.Request, fields *framework.FieldData) (*logical.Response, error) {
 		user, data, err := b.acmeState.ParseRequestParams(acmeCtx, r, fields)
@@ -217,6 +164,10 @@ func (b *backend) acmeParsedWrapper(op acmeParsedOperation) framework.OperationF
 	})
 }
 
+// acmeAccountRequiredWrapper builds on top of acmeParsedWrapper, enforcing the
+// request has a proper signature for an existing account, and that account is
+// in a valid status. It passes to the operation a decoded form of the request
+// parameters as well as the ACME account the request is for.
 func (b *backend) acmeAccountRequiredWrapper(op acmeAccountRequiredOperation) framework.OperationFunc {
 	return b.acmeParsedWrapper(func(acmeCtx *acmeContext, r *logical.Request, fields *framework.FieldData, uc *jwsCtx, data map[string]interface{}) (*logical.Response, error) {
 		if !uc.Existing {
@@ -281,3 +232,94 @@ func getAcmeBaseUrl(sc *storageContext, path string) (*url.URL, *url.URL, error)
 
 	return baseUrl.JoinPath(directoryPrefix, "/acme/"), baseUrl, nil
 }
+
+func getAcmeIssuer(sc *storageContext, issuerName string) (*issuerEntry, error) {
+	if issuerName == "" {
+		issuerName = defaultRef
+	}
+	issuerId, err := sc.resolveIssuerReference(issuerName)
+	if err != nil {
+		return nil, fmt.Errorf("%w: issuer does not exist", ErrMalformed)
+	}
+
+	issuer, err := sc.fetchIssuerById(issuerId)
+	if err != nil {
+		return nil, fmt.Errorf("issuer failed to load: %w", err)
+	}
+
+	if issuer.Usage.HasUsage(IssuanceUsage) && len(issuer.KeyID) > 0 {
+		return issuer, nil
+	}
+
+	return nil, fmt.Errorf("%w: issuer missing proper issuance usage or key", ErrServerInternal)
+}
+
+func getAcmeDirectory(data *framework.FieldData) string {
+	requestedIssuer := getRequestedAcmeIssuerFromPath(data)
+	requestedRole := getRequestedAcmeRoleFromPath(data)
+
+	return fmt.Sprintf("issuer-%s::role-%s", requestedIssuer, requestedRole)
+}
+
+func getAcmeRoleAndIssuer(sc *storageContext, data *framework.FieldData) (*roleEntry, *issuerEntry, error) {
+	requestedIssuer := getRequestedAcmeIssuerFromPath(data)
+	requestedRole := getRequestedAcmeRoleFromPath(data)
+	issuerToLoad := requestedIssuer
+
+	var role *roleEntry
+
+	if len(requestedRole) > 0 {
+		var err error
+		role, err = sc.Backend.getRole(sc.Context, sc.Storage, requestedRole)
+		if err != nil {
+			return nil, nil, fmt.Errorf("%w: err loading role", ErrServerInternal)
+		}
+
+		if role == nil {
+			return nil, nil, fmt.Errorf("%w: role does not exist", ErrMalformed)
+		}
+
+		if role.NoStore {
+			return nil, nil, fmt.Errorf("%w: role can not be used as NoStore is set to true", ErrServerInternal)
+		}
+
+		// If we haven't loaded an issuer directly from our path and the specified
+		// role does specify an issuer prefer the role's issuer rather than the default issuer.
+		if len(role.Issuer) > 0 && len(requestedIssuer) == 0 {
+			issuerToLoad = role.Issuer
+		}
+	} else {
+		role = buildSignVerbatimRoleWithNoData(&roleEntry{
+			Issuer:  requestedIssuer,
+			NoStore: false,
+			Name:    requestedRole,
+		})
+	}
+
+	issuer, err := getAcmeIssuer(sc, issuerToLoad)
+	if err != nil {
+		return nil, nil, err
+	}
+
+	// TODO: Need additional configuration validation here, for allowed roles/issuers.
+
+	return role, issuer, nil
+}
+
+func getRequestedAcmeRoleFromPath(data *framework.FieldData) string {
+	requestedRole := ""
+	roleNameRaw, present := data.GetOk("role")
+	if present {
+		requestedRole = roleNameRaw.(string)
+	}
+	return requestedRole
+}
+
+func getRequestedAcmeIssuerFromPath(data *framework.FieldData) string {
+	requestedIssuer := ""
+	requestedIssuerRaw, present := data.GetOk(issuerRefParam)
+	if present {
+		requestedIssuer = requestedIssuerRaw.(string)
+	}
+	return requestedIssuer
+}

builtin/logical/pki/path_acme_test.go

@@ -52,7 +52,6 @@ func TestAcmeBasicWorkflow(t *testing.T) {
 		{"role", "/roles/test-role"},
 		{"issuer", "/issuer/int-ca"},
 		{"issuer_role", "/issuer/int-ca/roles/test-role"},
-		{"issuer_role_acme", "/issuer/acme/roles/acme"},
 	}
 	testCtx := context.Background()
 
@@ -369,6 +368,34 @@ func TestAcmeClusterPathNotConfigured(t *testing.T) {
 	}
 }
 
+// TestAcmeAccountsCrossingDirectoryPath make sure that if an account attempts to use a different ACME
+// directory path that we get an error.
+func TestAcmeAccountsCrossingDirectoryPath(t *testing.T) {
+	t.Parallel()
+	cluster, _, _ := setupAcmeBackend(t)
+	defer cluster.Cleanup()
+
+	baseAcmeURL := "/v1/pki/acme/"
+	accountKey, err := rsa.GenerateKey(rand.Reader, 2048)
+	require.NoError(t, err, "failed creating rsa key")
+
+	testCtx := context.Background()
+	acmeClient := getAcmeClientForCluster(t, cluster, baseAcmeURL, accountKey)
+
+	// Create new account
+	acct, err := acmeClient.Register(testCtx, &acme.Account{}, func(tosURL string) bool { return true })
+	require.NoError(t, err, "failed registering account")
+
+	// Try to update the account under another ACME directory
+	baseAcmeURL2 := "/v1/pki/roles/test-role/acme/"
+	acmeClient2 := getAcmeClientForCluster(t, cluster, baseAcmeURL2, accountKey)
+	acct.Contact = []string{"mailto:test3@example.com"}
+	_, err = acmeClient2.UpdateReg(testCtx, acct)
+	require.Error(t, err, "successfully updated account when we should have failed due to different directory")
+	// We don't test for the specific error about using the wrong directory, as the golang library
+	// swallows the error we are sending back to a no account error
+}
+
 func setupAcmeBackend(t *testing.T) (*vault.TestCluster, *api.Client, string) {
 	cluster, client := setupTestPkiCluster(t)