From 4e6b88d58c7e4788881d53a944238273730be396 Mon Sep 17 00:00:00 2001 From: Steven Clark Date: Wed, 12 Apr 2023 09:05:42 -0400 Subject: [PATCH] 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 | 44 +++++-- builtin/logical/pki/backend.go | 6 + builtin/logical/pki/backend_test.go | 4 + builtin/logical/pki/path_acme_new_account.go | 109 +++++++++++++++- builtin/logical/pki/path_acme_test.go | 130 ++++++++++++++----- 5 files changed, 242 insertions(+), 51 deletions(-) diff --git a/builtin/logical/pki/acme_state.go b/builtin/logical/pki/acme_state.go index 5fef9b329..8c9d84c48 100644 --- a/builtin/logical/pki/acme_state.go +++ b/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" - StatusDeactivated = "deactivated" - StatusRevoked = "revoked" + StatusValid ACMEAccountStatus = "valid" + StatusDeactivated ACMEAccountStatus = "deactivated" + StatusRevoked ACMEAccountStatus = "revoked" ) type acmeAccount struct { - KeyId string `json:"-"` - Status ACMEStates `json:"state"` - Contact []string `json:"contact"` - TermsOfServiceAgreed bool `json:"termsOfServiceAgreed"` - Jwk []byte `json:"jwk"` + KeyId string `json:"-"` + Status ACMEAccountStatus `json:"status"` + Contact []string `json:"contact"` + TermsOfServiceAgreed bool `json:"termsOfServiceAgreed"` + 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 } diff --git a/builtin/logical/pki/backend.go b/builtin/logical/pki/backend.go index f0883f5f8..4df320b74 100644 --- a/builtin/logical/pki/backend.go +++ b/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 { diff --git a/builtin/logical/pki/backend_test.go b/builtin/logical/pki/backend_test.go index 93bca248a..409ec0550 100644 --- a/builtin/logical/pki/backend_test.go +++ b/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 { diff --git a/builtin/logical/pki/path_acme_new_account.go b/builtin/logical/pki/path_acme_new_account.go index 67cfe094d..b8e2745a5 100644 --- a/builtin/logical/pki/path_acme_new_account.go +++ b/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. - if !termsOfServiceAgreed { - return nil, fmt.Errorf("terms of service not agreed to: %w", ErrUserActionRequired) - } + // TODO: Limit this only when ToS are required or set by the operator, since we don't have a + // ToS URL in the directory at the moment, we can not enforce this. + //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 +} diff --git a/builtin/logical/pki/path_acme_test.go b/builtin/logical/pki/path_acme_test.go index e2e878ed4..b8090d982 100644 --- a/builtin/logical/pki/path_acme_test.go +++ b/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 -// are available and produce the correct responses. -func TestAcmeDirectory(t *testing.T) { +// TestAcmeBasicWorkflow a basic test that will validate a basic ACME workflow using the Golang ACME client. +func TestAcmeBasicWorkflow(t *testing.T) { t.Parallel() - cluster, client, pathConfig := setupAcmeBackend(t) + cluster, client, _ := setupAcmeBackend(t) defer cluster.Cleanup() - cases := []struct { - name string - prefixUrl string - directoryUrl string + name string + prefixUrl string }{ - {"root", "", "pki/acme/directory"}, - {"role", "/roles/test-role", "pki/roles/test-role/acme/directory"}, - {"issuer", "/issuer/default", "pki/issuer/default/acme/directory"}, - {"issuer_role", "/issuer/default/roles/test-role", "pki/issuer/default/roles/test-role/acme/directory"}, - {"issuer_role_acme", "/issuer/acme/roles/acme", "pki/issuer/acme/roles/acme/acme/directory"}, + {"root", ""}, + {"role", "/roles/test-role"}, + {"issuer", "/issuer/default"}, + {"issuer_role", "/issuer/default/roles/test-role"}, + {"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) - require.NoError(t, err, "failed reading ACME directory configuration") + baseAcmeURL := "/v1/pki" + tc.prefixUrl + "/acme/" + key, err := rsa.GenerateKey(rand.Reader, 2048) + require.NoError(t, err, "failed creating rsa key") - require.Equal(t, 200, dirResp.StatusCode) - require.Equal(t, "application/json", dirResp.Header.Get("Content-Type")) + acmeClient := getAcmeClientForCluster(t, cluster, baseAcmeURL, key) - requiredUrls := map[string]string{ - "newNonce": pathConfig + tc.prefixUrl + "/acme/new-nonce", - "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", - } + discovery, err := acmeClient.Discover(testCtx) + require.NoError(t, err, "failed acme discovery call") - rawBodyBytes, err := io.ReadAll(dirResp.Body) - require.NoError(t, err, "failed reading from directory response body") - _ = dirResp.Body.Close() + discoveryBaseUrl := client.Address() + baseAcmeURL + require.Equal(t, discoveryBaseUrl+"new-nonce", discovery.NonceURL) + 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{}{} - err = json.Unmarshal(rawBodyBytes, &respType) - require.NoError(t, err, "failed unmarshalling ACME directory response body") + // Attempt to update prior to creating an account + _, err = acmeClient.UpdateReg(testCtx, &acme.Account{Contact: []string{"mailto:shouldfail@example.com"}}) + require.ErrorIs(t, err, acme.ErrNoAccount, "expected failure attempting to update prior to account registration") - for key, expectedUrl := range requiredUrls { - require.Contains(t, respType, key, "missing required value %s from data", key) - require.Equal(t, expectedUrl, respType[key], "different URL returned for %s", key) - } + // Create new account + acct, err := acmeClient.Register(testCtx, &acme.Account{ + 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", + } +}