Rework the ACME test suite to use full Vault cluster to validate behavior (#19874)

- Instead of using tests that just test the plugin storage/interface layer, use a full Vault instance to validate that we can send/receive the proper headers and responses back to a client. - Found an issue with HEAD new-nounce api calls returning 500 errors. - Add the /acme/ suffix to the baseUrl in the acme context so we don't have to keep adding it a bit everywhere.
2023-04-03 09:38:20 -04:00 · 2023-04-03 09:38:20 -04:00 · 8ddead7a17
parent b1c557bd73
commit 8ddead7a17
4 changed files with 97 additions and 60 deletions
--- a/builtin/logical/pki/path_acme_directory.go
+++ b/builtin/logical/pki/path_acme_directory.go
@ -106,7 +106,7 @@ func getAcmeBaseUrl(sc *storageContext, path string) (*url.URL, error) {
 		directoryPrefix = path[0:lastIndex]
 	}

-	return baseUrl.JoinPath(directoryPrefix), nil
+	return baseUrl.JoinPath(directoryPrefix, "/acme/"), nil
 }

 func acmeErrorWrapper(op framework.OperationFunc) framework.OperationFunc {
@ -122,11 +122,11 @@ func acmeErrorWrapper(op framework.OperationFunc) framework.OperationFunc {

 func (b *backend) acmeDirectoryHandler(acmeCtx acmeContext, r *logical.Request, _ *framework.FieldData) (*logical.Response, error) {
 	rawBody, err := json.Marshal(map[string]interface{}{
-		"newNonce":   acmeCtx.baseUrl.JoinPath("/acme/new-nonce").String(),
-		"newAccount": acmeCtx.baseUrl.JoinPath("/acme/new-account").String(),
-		"newOrder":   acmeCtx.baseUrl.JoinPath("/acme/new-order").String(),
-		"revokeCert": acmeCtx.baseUrl.JoinPath("/acme/revoke-cert").String(),
-		"keyChange":  acmeCtx.baseUrl.JoinPath("/acme/key-change").String(),
+		"newNonce":   acmeCtx.baseUrl.JoinPath("new-nonce").String(),
+		"newAccount": acmeCtx.baseUrl.JoinPath("new-account").String(),
+		"newOrder":   acmeCtx.baseUrl.JoinPath("new-order").String(),
+		"revokeCert": acmeCtx.baseUrl.JoinPath("revoke-cert").String(),
+		"keyChange":  acmeCtx.baseUrl.JoinPath("key-change").String(),
 		"meta": map[string]interface{}{
 			"externalAccountRequired": false,
 		},
--- a/builtin/logical/pki/path_acme_new_account.go
+++ b/builtin/logical/pki/path_acme_new_account.go
@ -169,7 +169,7 @@ func (b *backend) acmeNewAccountSearchHandler(acmeCtx acmeContext, r *logical.Re
 			return nil, fmt.Errorf("error loading account: %w", err)
 		}

-		location := acmeCtx.baseUrl.String() + "/acme/account/" + userCtx.Kid
+		location := acmeCtx.baseUrl.String() + "account/" + userCtx.Kid
 		return formatAccountResponse(location, account["status"].(string), account["contact"].([]string)), nil
 	}

@ -201,6 +201,6 @@ func (b *backend) acmeNewAccountCreateHandler(acmeCtx acmeContext, r *logical.Re
 		return nil, fmt.Errorf("failed to create account: %w", err)
 	}

-	location := acmeCtx.baseUrl.String() + "/acme/account/" + userCtx.Kid
+	location := acmeCtx.baseUrl.String() + "account/" + userCtx.Kid
 	return formatAccountResponse(location, account["status"].(string), account["contact"].([]string)), nil
 }
--- a/builtin/logical/pki/path_acme_nonce.go
+++ b/builtin/logical/pki/path_acme_nonce.go
@ -71,11 +71,14 @@ func (b *backend) acmeNonceHandler(ctx acmeContext, r *logical.Request, _ *frame
 		},
 		Data: map[string]interface{}{
 			logical.HTTPStatusCode: httpStatus,
+			// Get around Vault limitation of requiring a body set if the status is not http.StatusNoContent
+			// for our HEAD request responses.
+			logical.HTTPContentType: "",
 		},
 	}, nil
 }

 func genAcmeLinkHeader(ctx acmeContext) []string {
-	path := fmt.Sprintf("<%s>;rel=\"index\"", ctx.baseUrl.JoinPath("/acme/directory").String())
+	path := fmt.Sprintf("<%s>;rel=\"index\"", ctx.baseUrl.JoinPath("directory").String())
 	return []string{path}
 }
--- a/builtin/logical/pki/path_acme_test.go
+++ b/builtin/logical/pki/path_acme_test.go
@ -1,10 +1,16 @@
 package pki

 import (
+	"context"
 	"fmt"
+	"io"
 	"net/http"
 	"testing"

+	"github.com/hashicorp/vault/api"
+	vaulthttp "github.com/hashicorp/vault/http"
+	"github.com/hashicorp/vault/vault"
+
 	"github.com/hashicorp/vault/sdk/logical"

 	"github.com/stretchr/testify/require"
@ -15,28 +21,29 @@ import (
 // are available and produce the correct responses.
 func TestAcmeDirectory(t *testing.T) {
 	t.Parallel()
-	b, s, pathConfig := setupAcmeBackend(t)
+	cluster, client, pathConfig := setupAcmeBackend(t)
+	defer cluster.Cleanup()

 	cases := []struct {
 		name         string
 		prefixUrl    string
 		directoryUrl string
 	}{
-		{"root", "", "acme/directory"},
-		{"role", "/roles/test-role", "roles/test-role/acme/directory"},
-		{"issuer", "/issuer/default", "issuer/default/acme/directory"},
-		{"issuer_role", "/issuer/default/roles/test-role", "issuer/default/roles/test-role/acme/directory"},
-		{"issuer_role_acme", "/issuer/acme/roles/acme", "issuer/acme/roles/acme/acme/directory"},
+		{"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"},
 	}
+	testCtx := context.Background()

 	for _, tc := range cases {
 		t.Run(tc.name, func(t *testing.T) {
-			dirResp, err := CBRead(b, s, tc.directoryUrl)
+			dirResp, err := client.Logical().ReadRawWithContext(testCtx, tc.directoryUrl)
 			require.NoError(t, err, "failed reading ACME directory configuration")

-			require.Contains(t, dirResp.Data, "http_content_type", "missing Content-Type header")
-			require.Contains(t, dirResp.Data["http_content_type"], "application/json",
-				"missing appropriate content type in header")
+			require.Equal(t, 200, dirResp.StatusCode)
+			require.Equal(t, "application/json", dirResp.Header.Get("Content-Type"))

 			requiredUrls := map[string]string{
 				"newNonce":   pathConfig + tc.prefixUrl + "/acme/new-nonce",
@ -46,7 +53,10 @@ func TestAcmeDirectory(t *testing.T) {
 				"keyChange":  pathConfig + tc.prefixUrl + "/acme/key-change",
 			}

-			rawBodyBytes := dirResp.Data["http_raw_body"].([]byte)
+			rawBodyBytes, err := io.ReadAll(dirResp.Body)
+			require.NoError(t, err, "failed reading from directory response body")
+			_ = dirResp.Body.Close()
+
 			respType := map[string]interface{}{}
 			err = json.Unmarshal(rawBodyBytes, &respType)
 			require.NoError(t, err, "failed unmarshalling ACME directory response body")
@ -59,60 +69,60 @@ func TestAcmeDirectory(t *testing.T) {
 	}
 }

+// TestAcmeNonce a basic test that will validate we get back a nonce with the proper status codes
+// based on the
 func TestAcmeNonce(t *testing.T) {
 	t.Parallel()
-	b, s, pathConfig := setupAcmeBackend(t)
+	cluster, client, pathConfig := setupAcmeBackend(t)
+	defer cluster.Cleanup()

 	cases := []struct {
 		name         string
 		prefixUrl    string
 		directoryUrl string
 	}{
-		{"root", "", "acme/new-nonce"},
-		{"role", "/roles/test-role", "roles/test-role/acme/new-nonce"},
-		{"issuer", "/issuer/default", "issuer/default/acme/new-nonce"},
-		{"issuer_role", "/issuer/default/roles/test-role", "issuer/default/roles/test-role/acme/new-nonce"},
+		{"root", "", "pki/acme/new-nonce"},
+		{"role", "/roles/test-role", "pki/roles/test-role/acme/new-nonce"},
+		{"issuer", "/issuer/default", "pki/issuer/default/acme/new-nonce"},
+		{"issuer_role", "/issuer/default/roles/test-role", "pki/issuer/default/roles/test-role/acme/new-nonce"},
 	}
+
 	for _, tc := range cases {
 		for _, httpOp := range []string{"get", "header"} {
 			t.Run(fmt.Sprintf("%s-%s", tc.name, httpOp), func(t *testing.T) {
-				var resp *logical.Response
-				var err error
+				var req *api.Request
 				switch httpOp {
 				case "get":
-					resp, err = CBRead(b, s, tc.directoryUrl)
+					req = client.NewRequest(http.MethodGet, "/v1/"+tc.directoryUrl)
 				case "header":
-					resp, err = CBHeader(b, s, tc.directoryUrl)
+					req = client.NewRequest(http.MethodHead, "/v1/"+tc.directoryUrl)
 				}
-				require.NoError(t, err, "failed %s op for new-nouce", httpOp)
+				res, err := client.RawRequestWithContext(ctx, req)
+				require.NoError(t, err, "failed sending raw request")
+				_ = res.Body.Close()

 				// Proper Status Code
 				switch httpOp {
 				case "get":
-					require.Equal(t, http.StatusNoContent, resp.Data["http_status_code"])
+					require.Equal(t, http.StatusNoContent, res.StatusCode)
 				case "header":
-					require.Equal(t, http.StatusOK, resp.Data["http_status_code"])
+					require.Equal(t, http.StatusOK, res.StatusCode)
 				}

+				// Make sure we don't have a Content-Type header.
+				require.Equal(t, "", res.Header.Get("Content-Type"))
+
 				// Make sure we return the Cache-Control header
-				require.Contains(t, resp.Headers, "Cache-Control", "missing Cache-Control header")
-				require.Contains(t, resp.Headers["Cache-Control"], "no-store",
+				require.Contains(t, res.Header.Get("Cache-Control"), "no-store",
 					"missing Cache-Control header with no-store header value")
-				require.Len(t, resp.Headers["Cache-Control"], 1,
-					"Cache-Control header should have only a single header")

 				// Test for our nonce header value
-				require.Contains(t, resp.Headers, "Replay-Nonce", "missing Replay-Nonce header")
-				require.NotEmpty(t, resp.Headers["Replay-Nonce"], "missing Replay-Nonce header with an actual value")
-				require.Len(t, resp.Headers["Replay-Nonce"], 1,
-					"Replay-Nonce header should have only a single header")
+				require.NotEmpty(t, res.Header.Get("Replay-Nonce"), "missing Replay-Nonce header with an actual value")

 				// Test Link header value
-				require.Contains(t, resp.Headers, "Link", "missing Link header")
 				expectedLinkHeader := fmt.Sprintf("<%s>;rel=\"index\"", pathConfig+tc.prefixUrl+"/acme/directory")
-				require.Contains(t, resp.Headers["Link"], expectedLinkHeader,
+				require.Contains(t, res.Header.Get("Link"), expectedLinkHeader,
 					"different value for link header than expected")
-				require.Len(t, resp.Headers["Link"], 1, "Link header should have only a single header")
 			})
 		}
 	}
@ -121,31 +131,33 @@ func TestAcmeNonce(t *testing.T) {
 // TestAcmeClusterPathNotConfigured basic testing of the ACME error handler.
 func TestAcmeClusterPathNotConfigured(t *testing.T) {
 	t.Parallel()
-	b, s := CreateBackendWithStorage(t)
+	cluster, client := setupTestPkiCluster(t)
+	defer cluster.Cleanup()

 	// Do not fill in the path option within the local cluster configuration
 	cases := []struct {
 		name         string
 		directoryUrl string
 	}{
-		{"root", "acme/directory"},
-		{"role", "roles/test-role/acme/directory"},
-		{"issuer", "issuer/default/acme/directory"},
-		{"issuer_role", "issuer/default/roles/test-role/acme/directory"},
+		{"root", "pki/acme/directory"},
+		{"role", "pki/roles/test-role/acme/directory"},
+		{"issuer", "pki/issuer/default/acme/directory"},
+		{"issuer_role", "pki/issuer/default/roles/test-role/acme/directory"},
 	}
+	testCtx := context.Background()
+
 	for _, tc := range cases {
 		t.Run(tc.name, func(t *testing.T) {
-			dirResp, err := CBRead(b, s, tc.directoryUrl)
-			require.NoError(t, err, "failed reading ACME directory configuration")
+			dirResp, err := client.Logical().ReadRawWithContext(testCtx, tc.directoryUrl)
+			require.Error(t, err, "expected failure reading ACME directory configuration got none")

-			require.Contains(t, dirResp.Data, "http_content_type", "missing Content-Type header")
-			require.Contains(t, dirResp.Data["http_content_type"], "application/problem+json",
-				"missing appropriate content type in header")
+			require.Equal(t, "application/problem+json", dirResp.Header.Get("Content-Type"))
+			require.Equal(t, http.StatusInternalServerError, dirResp.StatusCode)

-			require.Equal(t, http.StatusInternalServerError, dirResp.Data["http_status_code"])
+			rawBodyBytes, err := io.ReadAll(dirResp.Body)
+			require.NoError(t, err, "failed reading from directory response body")
+			_ = dirResp.Body.Close()

-			require.Contains(t, dirResp.Data, "http_raw_body", "missing http_raw_body from data")
-			rawBodyBytes := dirResp.Data["http_raw_body"].([]byte)
 			respType := map[string]interface{}{}
 			err = json.Unmarshal(rawBodyBytes, &respType)
 			require.NoError(t, err, "failed unmarshalling ACME directory response body")
@ -156,16 +168,38 @@ func TestAcmeClusterPathNotConfigured(t *testing.T) {
 	}
 }

-func setupAcmeBackend(t *testing.T) (*backend, logical.Storage, string) {
-	b, s := CreateBackendWithStorage(t)
+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"

-	_, err := CBWrite(b, s, "config/cluster", map[string]interface{}{
+	_, err := client.Logical().WriteWithContext(context.Background(), "pki/config/cluster", map[string]interface{}{
 		"path":     pathConfig,
 		"aia_path": "http://localhost:8200/cdn/pki",
 	})
 	require.NoError(t, err)
-	return b, s, pathConfig
+
+	// 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"},
+	})
+	require.NoError(t, err, "failed tuning mount response headers")
+
+	return cluster, client, pathConfig
+}
+
+func setupTestPkiCluster(t *testing.T) (*vault.TestCluster, *api.Client) {
+	coreConfig := &vault.CoreConfig{
+		LogicalBackends: map[string]logical.Factory{
+			"pki": Factory,
+		},
+	}
+	cluster := vault.NewTestCluster(t, coreConfig, &vault.TestClusterOptions{
+		HandlerFunc: vaulthttp.Handler,
+	})
+	cluster.Start()
+	client := cluster.Cores[0].Client
+	mountPKIEndpoint(t, client, "pki")
+	return cluster, client
 }