Rework ACME workflow test to leverage Golang's ACME client library (#19949)

* Rework ACME workflow test to leverage Golang's ACME client library

 - Instead of testing manually, leverage the Golang ACME library
   to test against our implementation from the unit tests.

* Add tests for new-account and misc fixes

 - Set and return the account status for registration
 - Add handlers for the account/ api/updates
 - Switch acme/ to cluster local storage
 - Disable terms of service checks for now as we don't set the url

* PR feedback

 - Implement account deactivation
 - Create separate account update handler, to not mix account creation
   logic
 - Add kid field to account update definition
 - Add support to update contact details on an existing account

builtin/logical/pki/acme_state.go

@@ -107,20 +107,24 @@ func (a *acmeState) TidyNonces() {
 	a.nextExpiry.Store(nextRun.Unix())
 }
 
-type ACMEStates string
+type ACMEAccountStatus string
+
+func (aas ACMEAccountStatus) String() string {
+	return string(aas)
+}
 
 const (
-	StatusValid       = "valid"
+	StatusValid       ACMEAccountStatus = "valid"
-	StatusDeactivated = "deactivated"
+	StatusDeactivated ACMEAccountStatus = "deactivated"
-	StatusRevoked     = "revoked"
+	StatusRevoked     ACMEAccountStatus = "revoked"
 )
 
 type acmeAccount struct {
-	KeyId                string     `json:"-"`
+	KeyId                string            `json:"-"`
-	Status               ACMEStates `json:"state"`
+	Status               ACMEAccountStatus `json:"status"`
-	Contact              []string   `json:"contact"`
+	Contact              []string          `json:"contact"`
-	TermsOfServiceAgreed bool       `json:"termsOfServiceAgreed"`
+	TermsOfServiceAgreed bool              `json:"termsOfServiceAgreed"`
-	Jwk                  []byte     `json:"jwk"`
+	Jwk                  []byte            `json:"jwk"`
 }
 
 func (a *acmeState) CreateAccount(ac *acmeContext, c *jwsCtx, contact []string, termsOfServiceAgreed bool) (*acmeAccount, error) {
@@ -129,9 +133,12 @@ func (a *acmeState) CreateAccount(ac *acmeContext, c *jwsCtx, contact []string,
 		Contact:              contact,
 		TermsOfServiceAgreed: termsOfServiceAgreed,
 		Jwk:                  c.Jwk,
+		Status:               StatusValid,
 	}
 
-	json, err := logical.StorageEntryJSON(acmeAccountPrefix+c.Kid, acct)
+	kid := cleanKid(c.Kid)
+
+	json, err := logical.StorageEntryJSON(acmeAccountPrefix+kid, acct)
 	if err != nil {
 		return nil, fmt.Errorf("error creating account entry: %w", err)
 	}
@@ -143,6 +150,21 @@ func (a *acmeState) CreateAccount(ac *acmeContext, c *jwsCtx, contact []string,
 	return acct, nil
 }
 
+func (a *acmeState) UpdateAccount(ac *acmeContext, acct *acmeAccount) error {
+	kid := cleanKid(acct.KeyId)
+
+	json, err := logical.StorageEntryJSON(acmeAccountPrefix+kid, acct)
+	if err != nil {
+		return fmt.Errorf("error creating account entry: %w", err)
+	}
+
+	if err := ac.sc.Storage.Put(ac.sc.Context, json); err != nil {
+		return fmt.Errorf("error writing account entry: %w", err)
+	}
+
+	return nil
+}
+
 func cleanKid(keyID string) string {
 	pieces := strings.Split(keyID, "/")
 	return pieces[len(pieces)-1]
@@ -165,6 +187,8 @@ func (a *acmeState) LoadAccount(ac *acmeContext, keyID string) (*acmeAccount, er
 		return nil, fmt.Errorf("error loading account: %w", err)
 	}
 
+	acct.KeyId = keyID
+
 	return &acct, nil
 }
 

builtin/logical/pki/backend.go

@@ -126,6 +126,7 @@ func Backend(conf *logical.BackendConfig) *backend {
 				clusterConfigPath,
 				"crls/",
 				"certs/",
+				acmePathPrefix,
 			},
 
 			Root: []string{
@@ -228,6 +229,10 @@ func Backend(conf *logical.BackendConfig) *backend {
 			pathAcmeRoleNewAccount(&b),
 			pathAcmeIssuerNewAccount(&b),
 			pathAcmeIssuerAndRoleNewAccount(&b),
+			pathAcmeRootUpdateAccount(&b),
+			pathAcmeRoleUpdateAccount(&b),
+			pathAcmeIssuerUpdateAccount(&b),
+			pathAcmeIssuerAndRoleUpdateAccount(&b),
 		},
 
 		Secrets: []*framework.Secret{
@@ -248,6 +253,7 @@ func Backend(conf *logical.BackendConfig) *backend {
 		b.PathsSpecial.Unauthenticated = append(b.PathsSpecial.Unauthenticated, acmePrefix+"acme/new-order")
 		b.PathsSpecial.Unauthenticated = append(b.PathsSpecial.Unauthenticated, acmePrefix+"acme/revoke-cert")
 		b.PathsSpecial.Unauthenticated = append(b.PathsSpecial.Unauthenticated, acmePrefix+"acme/key-change")
+		b.PathsSpecial.Unauthenticated = append(b.PathsSpecial.Unauthenticated, acmePrefix+"acme/account/+")
 	}
 
 	if constants.IsEnterprise {

builtin/logical/pki/backend_test.go

@@ -6813,6 +6813,7 @@ func TestProperAuthing(t *testing.T) {
 		paths[acmePrefix+"acme/directory"] = shouldBeUnauthedReadList
 		paths[acmePrefix+"acme/new-nonce"] = shouldBeUnauthedReadList
 		paths[acmePrefix+"acme/new-account"] = shouldBeUnauthedWriteOnly
+		paths[acmePrefix+"acme/account/hrKmDYTvicHoHGVN2-3uzZV_BPGdE0W_dNaqYTtYqeo="] = shouldBeUnauthedWriteOnly
 	}
 
 	for path, checkerType := range paths {
@@ -6858,6 +6859,9 @@ func TestProperAuthing(t *testing.T) {
 		if strings.Contains(raw_path, "{serial}") {
 			raw_path = strings.ReplaceAll(raw_path, "{serial}", serial)
 		}
+		if strings.Contains(raw_path, "acme/account/") && strings.Contains(raw_path, "{kid}") {
+			raw_path = strings.ReplaceAll(raw_path, "{kid}", "hrKmDYTvicHoHGVN2-3uzZV_BPGdE0W_dNaqYTtYqeo=")
+		}
 
 		handler, present := paths[raw_path]
 		if !present {

builtin/logical/pki/path_acme_new_account.go

@@ -6,6 +6,8 @@ import (
 	"net/http"
 	"strings"
 
+	"github.com/hashicorp/go-secure-stdlib/strutil"
+
 	"github.com/hashicorp/vault/sdk/framework"
 	"github.com/hashicorp/vault/sdk/logical"
 )
@@ -28,6 +30,24 @@ func pathAcmeIssuerAndRoleNewAccount(b *backend) *framework.Path {
 			"/roles/"+framework.GenericNameRegex("role")+"/acme/new-account")
 }
 
+func pathAcmeRootUpdateAccount(b *backend) *framework.Path {
+	return patternAcmeNewAccount(b, "acme/account/"+framework.MatchAllRegex("kid"))
+}
+
+func pathAcmeRoleUpdateAccount(b *backend) *framework.Path {
+	return patternAcmeNewAccount(b, "roles/"+framework.GenericNameRegex("role")+"/acme/account/"+framework.MatchAllRegex("kid"))
+}
+
+func pathAcmeIssuerUpdateAccount(b *backend) *framework.Path {
+	return patternAcmeNewAccount(b, "issuer/"+framework.GenericNameRegex(issuerRefParam)+"/acme/account/"+framework.MatchAllRegex("kid"))
+}
+
+func pathAcmeIssuerAndRoleUpdateAccount(b *backend) *framework.Path {
+	return patternAcmeNewAccount(b,
+		"issuer/"+framework.GenericNameRegex(issuerRefParam)+
+			"/roles/"+framework.GenericNameRegex("role")+"/acme/account/"+framework.MatchAllRegex("kid"))
+}
+
 func addFieldsForACMEPath(fields map[string]*framework.FieldSchema, pattern string) map[string]*framework.FieldSchema {
 	if strings.Contains(pattern, framework.GenericNameRegex("role")) {
 		fields["role"] = &framework.FieldSchema{
@@ -69,10 +89,23 @@ func addFieldsForACMERequest(fields map[string]*framework.FieldSchema) map[strin
 	return fields
 }
 
+func addFieldsForACMEKidRequest(fields map[string]*framework.FieldSchema, pattern string) map[string]*framework.FieldSchema {
+	if strings.Contains(pattern, framework.GenericNameRegex("kid")) {
+		fields["kid"] = &framework.FieldSchema{
+			Type:        framework.TypeString,
+			Description: `The key identifier provided by the CA`,
+			Required:    true,
+		}
+	}
+
+	return fields
+}
+
 func patternAcmeNewAccount(b *backend, pattern string) *framework.Path {
 	fields := map[string]*framework.FieldSchema{}
 	addFieldsForACMEPath(fields, pattern)
 	addFieldsForACMERequest(fields)
+	addFieldsForACMEKidRequest(fields, pattern)
 
 	return &framework.Path{
 		Pattern: pattern,
@@ -171,6 +204,7 @@ func (b *backend) acmeNewAccountHandler(acmeCtx *acmeContext, r *logical.Request
 	var onlyReturnExisting bool
 	var contacts []string
 	var termsOfServiceAgreed bool
+	var status string
 
 	rawContact, present := data["contact"]
 	if present {
@@ -205,6 +239,15 @@ func (b *backend) acmeNewAccountHandler(acmeCtx *acmeContext, r *logical.Request
 		}
 	}
 
+	// Per RFC 8555 7.3.6 Account deactivation, we will handle it within our update API.
+	rawStatus, present := data["status"]
+	if present {
+		status, ok = rawStatus.(string)
+		if !ok {
+			return nil, fmt.Errorf("invalid type (%T) for field 'onlyReturnExisting': %w", rawOnlyReturnExisting, ErrMalformed)
+		}
+	}
+
 	// We ignore the EAB parameter as it is currently not supported.
 
 	// We have two paths here: search or create.
@@ -212,7 +255,13 @@ func (b *backend) acmeNewAccountHandler(acmeCtx *acmeContext, r *logical.Request
 		return b.acmeNewAccountSearchHandler(acmeCtx, r, fields, userCtx, data)
 	}
 
-	return b.acmeNewAccountCreateHandler(acmeCtx, r, fields, userCtx, data, contacts, termsOfServiceAgreed)
+	// Pass through the /new-account API calls to this specific handler as its requirements are different
+	// from the account update handler.
+	if strings.HasSuffix(r.Path, "/new-account") {
+		return b.acmeNewAccountCreateHandler(acmeCtx, r, fields, userCtx, data, contacts, termsOfServiceAgreed)
+	}
+
+	return b.acmeNewAccountUpdateHandler(acmeCtx, userCtx, contacts, status)
 }
 
 func formatAccountResponse(location string, acct *acmeAccount) *logical.Response {
@@ -265,10 +314,11 @@ func (b *backend) acmeNewAccountCreateHandler(acmeCtx *acmeContext, r *logical.R
 		return b.acmeNewAccountSearchHandler(acmeCtx, r, fields, userCtx, data)
 	}
 
-	// TODO: Limit this only when ToS are required by the operator.
+	// TODO: Limit this only when ToS are required or set by the operator, since we don't have a
-	if !termsOfServiceAgreed {
+	//       ToS URL in the directory at the moment, we can not enforce this.
-		return nil, fmt.Errorf("terms of service not agreed to: %w", ErrUserActionRequired)
+	//if !termsOfServiceAgreed {
-	}
+	//	return nil, fmt.Errorf("terms of service not agreed to: %w", ErrUserActionRequired)
+	//}
 
 	account, err := b.acmeState.CreateAccount(acmeCtx, userCtx, contact, termsOfServiceAgreed)
 	if err != nil {
@@ -285,3 +335,52 @@ func (b *backend) acmeNewAccountCreateHandler(acmeCtx *acmeContext, r *logical.R
 	resp.Data[logical.HTTPStatusCode] = http.StatusCreated
 	return resp, nil
 }
+
+func (b *backend) acmeNewAccountUpdateHandler(acmeCtx *acmeContext, userCtx *jwsCtx, contact []string, status string) (*logical.Response, error) {
+	if !userCtx.Existing {
+		return nil, fmt.Errorf("cannot submit to account updates without a 'kid': %w", ErrMalformed)
+	}
+
+	if !b.acmeState.DoesAccountExist(acmeCtx, userCtx.Kid) {
+		return nil, fmt.Errorf("an account with this key does not exist: %w", ErrAccountDoesNotExist)
+	}
+
+	account, err := b.acmeState.LoadAccount(acmeCtx, userCtx.Kid)
+	if err != nil {
+		return nil, fmt.Errorf("error loading account: %w", err)
+	}
+
+	// Per RFC 8555 7.3.6 Account deactivation, if we were previously deactivated, we should return
+	// unauthorized. There is no way to reactivate any accounts per ACME RFC.
+	if account.Status != StatusValid {
+		// Treating "revoked" and "deactivated" as the same here.
+		return nil, ErrUnauthorized
+	}
+
+	shouldUpdate := false
+	// Check to see if we should update, we don't really care about ordering
+	if !strutil.EquivalentSlices(account.Contact, contact) {
+		shouldUpdate = true
+		account.Contact = contact
+	}
+
+	// Check to process account de-activation status was requested.
+	// 7.3.6. Account Deactivation
+	if string(StatusDeactivated) == status {
+		shouldUpdate = true
+		// TODO: This should cancel any ongoing operations (do not revoke certs),
+		//       perhaps we should delete this account here?
+		account.Status = StatusDeactivated
+	}
+
+	if shouldUpdate {
+		err = b.acmeState.UpdateAccount(acmeCtx, account)
+		if err != nil {
+			return nil, fmt.Errorf("failed to update account: %w", err)
+		}
+	}
+
+	location := acmeCtx.baseUrl.String() + "account/" + userCtx.Kid
+	resp := formatAccountResponse(location, account)
+	return resp, nil
+}

builtin/logical/pki/path_acme_test.go

@@ -2,11 +2,19 @@ package pki
 
 import (
 	"context"
+	"crypto"
+	"crypto/rand"
+	"crypto/rsa"
 	"fmt"
 	"io"
 	"net/http"
+	"strings"
 	"testing"
 
+	"github.com/hashicorp/go-cleanhttp"
+	"golang.org/x/crypto/acme"
+	"golang.org/x/net/http2"
+
 	"github.com/hashicorp/vault/api"
 	vaulthttp "github.com/hashicorp/vault/http"
 	"github.com/hashicorp/vault/vault"
@@ -17,54 +25,80 @@ import (
 	"gopkg.in/square/go-jose.v2/json"
 )
 
-// TestAcmeDirectory a basic test that will validate the various directory APIs
+// TestAcmeBasicWorkflow a basic test that will validate a basic ACME workflow using the Golang ACME client.
-// are available and produce the correct responses.
+func TestAcmeBasicWorkflow(t *testing.T) {
-func TestAcmeDirectory(t *testing.T) {
 	t.Parallel()
-	cluster, client, pathConfig := setupAcmeBackend(t)
+	cluster, client, _ := setupAcmeBackend(t)
 	defer cluster.Cleanup()
-
 	cases := []struct {
-		name         string
+		name      string
-		prefixUrl    string
+		prefixUrl string
-		directoryUrl string
 	}{
-		{"root", "", "pki/acme/directory"},
+		{"root", ""},
-		{"role", "/roles/test-role", "pki/roles/test-role/acme/directory"},
+		{"role", "/roles/test-role"},
-		{"issuer", "/issuer/default", "pki/issuer/default/acme/directory"},
+		{"issuer", "/issuer/default"},
-		{"issuer_role", "/issuer/default/roles/test-role", "pki/issuer/default/roles/test-role/acme/directory"},
+		{"issuer_role", "/issuer/default/roles/test-role"},
-		{"issuer_role_acme", "/issuer/acme/roles/acme", "pki/issuer/acme/roles/acme/acme/directory"},
+		{"issuer_role_acme", "/issuer/acme/roles/acme"},
 	}
 	testCtx := context.Background()
 
 	for _, tc := range cases {
 		t.Run(tc.name, func(t *testing.T) {
-			dirResp, err := client.Logical().ReadRawWithContext(testCtx, tc.directoryUrl)
+			baseAcmeURL := "/v1/pki" + tc.prefixUrl + "/acme/"
-			require.NoError(t, err, "failed reading ACME directory configuration")
+			key, err := rsa.GenerateKey(rand.Reader, 2048)
+			require.NoError(t, err, "failed creating rsa key")
 
-			require.Equal(t, 200, dirResp.StatusCode)
+			acmeClient := getAcmeClientForCluster(t, cluster, baseAcmeURL, key)
-			require.Equal(t, "application/json", dirResp.Header.Get("Content-Type"))
 
-			requiredUrls := map[string]string{
+			discovery, err := acmeClient.Discover(testCtx)
-				"newNonce":   pathConfig + tc.prefixUrl + "/acme/new-nonce",
+			require.NoError(t, err, "failed acme discovery call")
-				"newAccount": pathConfig + tc.prefixUrl + "/acme/new-account",
-				"newOrder":   pathConfig + tc.prefixUrl + "/acme/new-order",
-				"revokeCert": pathConfig + tc.prefixUrl + "/acme/revoke-cert",
-				"keyChange":  pathConfig + tc.prefixUrl + "/acme/key-change",
-			}
 
-			rawBodyBytes, err := io.ReadAll(dirResp.Body)
+			discoveryBaseUrl := client.Address() + baseAcmeURL
-			require.NoError(t, err, "failed reading from directory response body")
+			require.Equal(t, discoveryBaseUrl+"new-nonce", discovery.NonceURL)
-			_ = dirResp.Body.Close()
+			require.Equal(t, discoveryBaseUrl+"new-account", discovery.RegURL)
+			require.Equal(t, discoveryBaseUrl+"new-order", discovery.OrderURL)
+			require.Equal(t, discoveryBaseUrl+"revoke-cert", discovery.RevokeURL)
+			require.Equal(t, discoveryBaseUrl+"key-change", discovery.KeyChangeURL)
 
-			respType := map[string]interface{}{}
+			// Attempt to update prior to creating an account
-			err = json.Unmarshal(rawBodyBytes, &respType)
+			_, err = acmeClient.UpdateReg(testCtx, &acme.Account{Contact: []string{"mailto:shouldfail@example.com"}})
-			require.NoError(t, err, "failed unmarshalling ACME directory response body")
+			require.ErrorIs(t, err, acme.ErrNoAccount, "expected failure attempting to update prior to account registration")
 
-			for key, expectedUrl := range requiredUrls {
+			// Create new account
-				require.Contains(t, respType, key, "missing required value %s from data", key)
+			acct, err := acmeClient.Register(testCtx, &acme.Account{
-				require.Equal(t, expectedUrl, respType[key], "different URL returned for %s", key)
+				Contact: []string{"mailto:test@example.com", "mailto:test2@test.com"},
-			}
+			}, func(tosURL string) bool { return true })
+			require.NoError(t, err, "failed registering account")
+			require.Equal(t, acme.StatusValid, acct.Status)
+			require.Contains(t, acct.Contact, "mailto:test@example.com")
+			require.Contains(t, acct.Contact, "mailto:test2@test.com")
+			require.Len(t, acct.Contact, 2)
+
+			// Call register again we should get existing account
+			_, err = acmeClient.Register(testCtx, acct, func(tosURL string) bool { return true })
+			require.ErrorIs(t, err, acme.ErrAccountAlreadyExists,
+				"We should have returned a 200 status code which would have triggered an error in the golang acme"+
+					" library")
+
+			// Update contact
+			acct.Contact = []string{"mailto:test3@example.com"}
+			acct2, err := acmeClient.UpdateReg(testCtx, acct)
+			require.NoError(t, err, "failed updating account")
+			require.Equal(t, acme.StatusValid, acct2.Status)
+			// We should get this back, not the original values.
+			require.Contains(t, acct2.Contact, "mailto:test3@example.com")
+			require.Len(t, acct2.Contact, 1)
+
+			// Deactivate account
+			err = acmeClient.DeactivateReg(testCtx)
+			require.NoError(t, err, "failed deactivating account")
+
+			// Make sure we get an unauthorized error trying to update the account again.
+			_, err = acmeClient.UpdateReg(testCtx, acct)
+			require.Error(t, err, "expected account to be deactivated")
+			require.IsType(t, &acme.Error{}, err, "expected acme error type")
+			acmeErr := err.(*acme.Error)
+			require.Equal(t, "urn:ietf:params:acme:error:unauthorized", acmeErr.ProblemType)
 		})
 	}
 }
@@ -172,7 +206,7 @@ func setupAcmeBackend(t *testing.T) (*vault.TestCluster, *api.Client, string) {
 	cluster, client := setupTestPkiCluster(t)
 
 	// Setting templated AIAs should succeed.
-	pathConfig := "https://localhost:8200/v1/pki"
+	pathConfig := client.Address() + "/v1/pki"
 
 	_, err := client.Logical().WriteWithContext(context.Background(), "pki/config/cluster", map[string]interface{}{
 		"path":     pathConfig,
@@ -182,7 +216,7 @@ func setupAcmeBackend(t *testing.T) (*vault.TestCluster, *api.Client, string) {
 
 	// Allow certain headers to pass through for ACME support
 	_, err = client.Logical().WriteWithContext(context.Background(), "sys/mounts/pki/tune", map[string]interface{}{
-		"allowed_response_headers": []string{"Last-Modified", "Replay-Nonce", "Link"},
+		"allowed_response_headers": []string{"Last-Modified", "Replay-Nonce", "Link", "Location"},
 	})
 	require.NoError(t, err, "failed tuning mount response headers")
 
@@ -203,3 +237,27 @@ func setupTestPkiCluster(t *testing.T) (*vault.TestCluster, *api.Client) {
 	mountPKIEndpoint(t, client, "pki")
 	return cluster, client
 }
+
+func getAcmeClientForCluster(t *testing.T, cluster *vault.TestCluster, baseUrl string, key crypto.Signer) acme.Client {
+	coreAddr := cluster.Cores[0].Listeners[0].Address
+	tlsConfig := cluster.Cores[0].TLSConfig()
+
+	transport := cleanhttp.DefaultPooledTransport()
+	transport.TLSClientConfig = tlsConfig.Clone()
+	if err := http2.ConfigureTransport(transport); err != nil {
+		t.Fatal(err)
+	}
+	httpClient := &http.Client{Transport: transport}
+	if baseUrl[0] == '/' {
+		baseUrl = baseUrl[1:]
+	}
+	if !strings.HasPrefix(baseUrl, "v1/") {
+		baseUrl = "v1/" + baseUrl
+	}
+	baseAcmeURL := fmt.Sprintf("https://%s/%s", coreAddr.String(), baseUrl)
+	return acme.Client{
+		Key:          key,
+		HTTPClient:   httpClient,
+		DirectoryURL: baseAcmeURL + "directory",
+	}
+}