From 6e05f43f8e5beeacfe998a75b4987b9499257324 Mon Sep 17 00:00:00 2001 From: Richard Wall Date: Tue, 22 Nov 2022 09:59:03 +0000 Subject: [PATCH 1/6] Set the Vault namespace using the official method in the vault SDK Signed-off-by: Richard Wall --- internal/vault/vault.go | 49 +++++++++++++++++++++-------------------- 1 file changed, 25 insertions(+), 24 deletions(-) diff --git a/internal/vault/vault.go b/internal/vault/vault.go index 25f9ee9cb..a4ac2daca 100644 --- a/internal/vault/vault.go +++ b/internal/vault/vault.go @@ -68,7 +68,22 @@ type Vault struct { issuer v1.GenericIssuer namespace string + // The pattern below, of namespaced and non-namespaced Vault clients, is copied from Hashicorp Nomad: + // https://github.com/hashicorp/nomad/blob/6e4410a9b13ce167bc7ef53da97c621b5c9dcd12/nomad/vault.go#L180-L190 + + // client is the Vault API client used for Namespace-relative integrations + // with the Vault API (anything except `/v1/sys`). + // The namespace feature is only available in Vault Enterprise. + // The namespace HTTP header (X-Vault-Namespace) is ignored by the open source version of Vault. + // See https://www.vaultproject.io/docs/enterprise/namespaces client Client + + // clientSys is the Vault API client used for non-Namespace-relative integrations + // with the Vault API (anything involving `/v1/sys`). This client is never configured + // with a Vault namespace, because these endpoints may return errors if a namespace + // header is provided + // See https://developer.hashicorp.com/vault/docs/enterprise/namespaces#root-only-api-paths + clientSys Client } // New returns a new Vault instance with the given namespace, issuer and @@ -87,16 +102,21 @@ func New(namespace string, secretsLister corelisters.SecretLister, issuer v1.Gen return nil, err } - client, err := vault.NewClient(cfg) + clientSys, err := vault.NewClient(cfg) if err != nil { return nil, fmt.Errorf("error initializing Vault client: %s", err.Error()) } - if err := v.setToken(client); err != nil { + // Set the Vault namespace. + // An empty namespace string will cause the client to not send the namespace related HTTP headers to Vault. + clientNS := clientSys.WithNamespace(issuer.GetSpec().Vault.Namespace) + + if err := v.setToken(clientNS); err != nil { return nil, err } - v.client = client + v.client = clientNS + v.clientSys = clientSys return v, nil } @@ -124,8 +144,6 @@ func (v *Vault) Sign(csrPEM []byte, duration time.Duration) (cert []byte, ca []b request := v.client.NewRequest("POST", url) - v.addVaultNamespaceToRequest(request) - if err := request.SetJSONBody(parameters); err != nil { return nil, nil, fmt.Errorf("failed to build vault request: %s", err) } @@ -312,8 +330,6 @@ func (v *Vault) requestTokenWithAppRoleRef(client Client, appRole *v1.VaultAppRo return "", fmt.Errorf("error encoding Vault parameters: %s", err.Error()) } - v.addVaultNamespaceToRequest(request) - resp, err := client.RawRequest(request) if err != nil { return "", fmt.Errorf("error logging in to Vault server: %s", err.Error()) @@ -373,8 +389,6 @@ func (v *Vault) requestTokenWithKubernetesAuth(client Client, kubernetesAuth *v1 return "", fmt.Errorf("error encoding Vault parameters: %s", err.Error()) } - v.addVaultNamespaceToRequest(request) - resp, err := client.RawRequest(request) if err != nil { return "", fmt.Errorf("error calling Vault server: %s", err.Error()) @@ -425,8 +439,8 @@ func extractCertificatesFromVaultCertificateSecret(secret *certutil.Secret) ([]b func (v *Vault) IsVaultInitializedAndUnsealed() error { healthURL := path.Join("/v1", "sys", "health") - healthRequest := v.client.NewRequest("GET", healthURL) - healthResp, err := v.client.RawRequest(healthRequest) + healthRequest := v.clientSys.NewRequest("GET", healthURL) + healthResp, err := v.clientSys.RawRequest(healthRequest) if healthResp != nil { defer healthResp.Body.Close() @@ -448,16 +462,3 @@ func (v *Vault) IsVaultInitializedAndUnsealed() error { return nil } - -func (v *Vault) addVaultNamespaceToRequest(request *vault.Request) { - vaultIssuer := v.issuer.GetSpec().Vault - if vaultIssuer != nil && vaultIssuer.Namespace != "" { - if request.Headers != nil { - request.Headers.Add("X-VAULT-NAMESPACE", vaultIssuer.Namespace) - } else { - vaultReqHeaders := http.Header{} - vaultReqHeaders.Add("X-VAULT-NAMESPACE", vaultIssuer.Namespace) - request.Headers = vaultReqHeaders - } - } -} From 51ac6fe181fe95bc6714c7e53a8fb7c243af1a51 Mon Sep 17 00:00:00 2001 From: Richard Wall Date: Tue, 22 Nov 2022 14:35:22 +0000 Subject: [PATCH 2/6] Test Signed-off-by: Richard Wall --- internal/vault/vault_test.go | 67 ++++++++++++++++++++++++++++++++++++ 1 file changed, 67 insertions(+) diff --git a/internal/vault/vault_test.go b/internal/vault/vault_test.go index 74518af66..0fb4007e5 100644 --- a/internal/vault/vault_test.go +++ b/internal/vault/vault_test.go @@ -32,11 +32,15 @@ import ( vault "github.com/hashicorp/vault/api" "github.com/hashicorp/vault/sdk/helper/certutil" "github.com/hashicorp/vault/sdk/helper/jsonutil" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" clientcorev1 "k8s.io/client-go/listers/core/v1" vaultfake "github.com/cert-manager/cert-manager/internal/vault/fake" cmapi "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1" + v1 "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1" cmmeta "github.com/cert-manager/cert-manager/pkg/apis/meta/v1" "github.com/cert-manager/cert-manager/pkg/util/pki" "github.com/cert-manager/cert-manager/test/unit/gen" @@ -1169,3 +1173,66 @@ func TestRequestTokenWithAppRoleRef(t *testing.T) { }) } } + +// TestNewWithVaultNamespaces demonstrates that New initializes two Vault +// clients, one with a namespace and one without a namespace which is used for +// interacting with root-only APIs. +func TestNewWithVaultNamespaces(t *testing.T) { + type testCase struct { + name string + vaultNS string + } + + tests := []testCase{ + { + name: "without-namespace", + vaultNS: "", + }, + { + name: "with-namespace", + vaultNS: "vault-ns-1", + }, + } + + for _, tc := range tests { + tc := tc + t.Run(tc.name, func(t *testing.T) { + c, err := New( + "k8s-ns1", + listers.FakeSecretListerFrom(listers.NewFakeSecretLister(), + listers.SetFakeSecretNamespaceListerGet( + &corev1.Secret{ + Data: map[string][]byte{ + "key1": []byte("not-used"), + }, + }, nil), + ), + &cmapi.Issuer{ + ObjectMeta: metav1.ObjectMeta{ + Name: "issuer1", + Namespace: "k8s-ns1", + }, + Spec: v1.IssuerSpec{ + IssuerConfig: v1.IssuerConfig{ + Vault: &v1.VaultIssuer{ + Namespace: tc.vaultNS, + Auth: cmapi.VaultAuth{ + TokenSecretRef: &cmmeta.SecretKeySelector{ + LocalObjectReference: cmmeta.LocalObjectReference{ + Name: "secret1", + }, + Key: "key1", + }, + }, + }, + }, + }, + }) + require.NoError(t, err) + assert.Equal(t, tc.vaultNS, c.(*Vault).client.(*vault.Client).Namespace(), + "The vault client should have the namespace provided in the Issuer recource") + assert.Equal(t, "", c.(*Vault).clientSys.(*vault.Client).Namespace(), + "The vault sys client should never have a namespace") + }) + } +} From 23437dfbbcc87c42e74ffda88081c9a42ecb491d Mon Sep 17 00:00:00 2001 From: Richard Wall Date: Tue, 22 Nov 2022 16:25:22 +0000 Subject: [PATCH 3/6] Remove unused Sys methods Signed-off-by: Richard Wall --- internal/vault/fake/client.go | 4 ---- internal/vault/fake/vault.go | 6 ------ internal/vault/vault.go | 7 ------- 3 files changed, 17 deletions(-) diff --git a/internal/vault/fake/client.go b/internal/vault/fake/client.go index 64aa0c6aa..bed9462ef 100644 --- a/internal/vault/fake/client.go +++ b/internal/vault/fake/client.go @@ -64,7 +64,3 @@ func (c *Client) Token() string { func (c *Client) RawRequest(r *vault.Request) (*vault.Response, error) { return c.RawRequestFn(r) } - -func (c *Client) Sys() *vault.Sys { - return nil -} diff --git a/internal/vault/fake/vault.go b/internal/vault/fake/vault.go index 529a0a54f..1ccdcbdf1 100644 --- a/internal/vault/fake/vault.go +++ b/internal/vault/fake/vault.go @@ -20,7 +20,6 @@ package fake import ( "time" - vault "github.com/hashicorp/vault/api" corelisters "k8s.io/client-go/listers/core/v1" v1 "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1" @@ -80,11 +79,6 @@ func (v *Vault) New(ns string, sl corelisters.SecretLister, iss v1.GenericIssuer return v, nil } -// Sys returns an empty `vault.Sys`. -func (v *Vault) Sys() *vault.Sys { - return new(vault.Sys) -} - // IsVaultInitializedAndUnsealed always returns nil func (v *Vault) IsVaultInitializedAndUnsealed() error { return nil diff --git a/internal/vault/vault.go b/internal/vault/vault.go index a4ac2daca..6b00195a1 100644 --- a/internal/vault/vault.go +++ b/internal/vault/vault.go @@ -45,10 +45,8 @@ type ClientBuilder func(namespace string, secretsLister corelisters.SecretLister // Interface implements various high level functionality related to connecting // with a Vault server, verifying its status and signing certificate request for // Vault's certificate. -// TODO: Sys() is duplicated here and in Client interface type Interface interface { Sign(csrPEM []byte, duration time.Duration) (certPEM []byte, caPEM []byte, err error) - Sys() *vault.Sys IsVaultInitializedAndUnsealed() error } @@ -58,7 +56,6 @@ type Client interface { RawRequest(r *vault.Request) (*vault.Response, error) SetToken(v string) Token() string - Sys() *vault.Sys } // Vault implements Interface and holds a Vault issuer, secrets lister and a @@ -409,10 +406,6 @@ func (v *Vault) requestTokenWithKubernetesAuth(client Client, kubernetesAuth *v1 return token, nil } -func (v *Vault) Sys() *vault.Sys { - return v.client.Sys() -} - func extractCertificatesFromVaultCertificateSecret(secret *certutil.Secret) ([]byte, []byte, error) { parsedBundle, err := certutil.ParsePKIMap(secret.Data) if err != nil { From 6b2c3b5295a83bbac052c3ce01a1be1730aa566f Mon Sep 17 00:00:00 2001 From: Richard Wall Date: Tue, 22 Nov 2022 16:47:41 +0000 Subject: [PATCH 4/6] Remove unused Token method Signed-off-by: Richard Wall --- internal/vault/vault.go | 1 - 1 file changed, 1 deletion(-) diff --git a/internal/vault/vault.go b/internal/vault/vault.go index 6b00195a1..78aad557a 100644 --- a/internal/vault/vault.go +++ b/internal/vault/vault.go @@ -55,7 +55,6 @@ type Client interface { NewRequest(method, requestPath string) *vault.Request RawRequest(r *vault.Request) (*vault.Response, error) SetToken(v string) - Token() string } // Vault implements Interface and holds a Vault issuer, secrets lister and a From e1740afedfb4fa18a1797242a06fb2134025a8b1 Mon Sep 17 00:00:00 2001 From: Richard Wall Date: Wed, 23 Nov 2022 09:58:39 +0000 Subject: [PATCH 5/6] Recreate the original behaviour of sending a Vault token to the unauthenticated sys/health endpoint. Signed-off-by: Richard Wall --- internal/vault/vault.go | 16 +++++++++-- internal/vault/vault_test.go | 53 ++++++++++++++++++++++++++++++++++++ 2 files changed, 66 insertions(+), 3 deletions(-) diff --git a/internal/vault/vault.go b/internal/vault/vault.go index 78aad557a..9f18a7554 100644 --- a/internal/vault/vault.go +++ b/internal/vault/vault.go @@ -98,21 +98,31 @@ func New(namespace string, secretsLister corelisters.SecretLister, issuer v1.Gen return nil, err } - clientSys, err := vault.NewClient(cfg) + client, err := vault.NewClient(cfg) if err != nil { return nil, fmt.Errorf("error initializing Vault client: %s", err.Error()) } // Set the Vault namespace. // An empty namespace string will cause the client to not send the namespace related HTTP headers to Vault. - clientNS := clientSys.WithNamespace(issuer.GetSpec().Vault.Namespace) + clientNS := client.WithNamespace(issuer.GetSpec().Vault.Namespace) + // Use the (maybe) namespaced client to authenticate. + // If a Vault namespace is configured, then the authentication endpoints are + // expected to be in that namespace. if err := v.setToken(clientNS); err != nil { return nil, err } + // A client for use with namespaced API paths v.client = clientNS - v.clientSys = clientSys + + // Create duplicate Vault client without a namespace, for interacting with root-only API paths. + // For backwards compatibility, this client will use the token from the namespaced client, + // although this is probably unnecessary / bad practice, since we only + // interact with the sys/health endpoint which is an unauthenticated endpoint: + // https://github.com/hashicorp/vault/issues/209#issuecomment-102485565. + v.clientSys = clientNS.WithNamespace("") return v, nil } diff --git a/internal/vault/vault_test.go b/internal/vault/vault_test.go index 0fb4007e5..b984c3f26 100644 --- a/internal/vault/vault_test.go +++ b/internal/vault/vault_test.go @@ -25,6 +25,7 @@ import ( "fmt" "io" "net/http" + "net/http/httptest" "strings" "testing" "time" @@ -1236,3 +1237,55 @@ func TestNewWithVaultNamespaces(t *testing.T) { }) } } + +// TestIsVaultInitiatedAndUnsealedIntegration demonstrates that it interacts only with the +// sys/health endpoint and that it supplies the Vault token but not a Vault namespace header. +func TestIsVaultInitiatedAndUnsealedIntegration(t *testing.T) { + + const vaultToken = "token1" + + mux := http.NewServeMux() + mux.HandleFunc("/v1/sys/health", func(response http.ResponseWriter, request *http.Request) { + assert.Empty(t, request.Header.Values("X-Vault-Namespace"), "Unexpected Vault namespace header for root-only API path") + assert.Equal(t, vaultToken, request.Header.Get("X-Vault-Token"), "Expected the Vault token for root-only API path") + }) + server := httptest.NewServer(mux) + defer server.Close() + + v, err := New( + "k8s-ns1", + listers.FakeSecretListerFrom(listers.NewFakeSecretLister(), + listers.SetFakeSecretNamespaceListerGet( + &corev1.Secret{ + Data: map[string][]byte{ + "key1": []byte(vaultToken), + }, + }, nil), + ), + &cmapi.Issuer{ + ObjectMeta: metav1.ObjectMeta{ + Name: "issuer1", + Namespace: "k8s-ns1", + }, + Spec: v1.IssuerSpec{ + IssuerConfig: v1.IssuerConfig{ + Vault: &v1.VaultIssuer{ + Server: server.URL, + Namespace: "ns1", + Auth: cmapi.VaultAuth{ + TokenSecretRef: &cmmeta.SecretKeySelector{ + LocalObjectReference: cmmeta.LocalObjectReference{ + Name: "secret1", + }, + Key: "key1", + }, + }, + }, + }, + }, + }) + require.NoError(t, err) + + err = v.IsVaultInitializedAndUnsealed() + require.NoError(t, err) +} From 75b2ba12dc0e9437d3b2a6f4133572f194416ba0 Mon Sep 17 00:00:00 2001 From: Richard Wall Date: Wed, 23 Nov 2022 10:18:48 +0000 Subject: [PATCH 6/6] Test that the Sign function *does* use the Vault namespace Signed-off-by: Richard Wall --- internal/vault/vault_test.go | 67 ++++++++++++++++++++++++++++++++++++ 1 file changed, 67 insertions(+) diff --git a/internal/vault/vault_test.go b/internal/vault/vault_test.go index b984c3f26..9720e4271 100644 --- a/internal/vault/vault_test.go +++ b/internal/vault/vault_test.go @@ -1289,3 +1289,70 @@ func TestIsVaultInitiatedAndUnsealedIntegration(t *testing.T) { err = v.IsVaultInitializedAndUnsealed() require.NoError(t, err) } + +// TestSignIntegration demonstrates that it interacts only with the API endpoint +// path supplied in the Issuer resource and that it supplies the Vault namespace +// and token to that endpoint. +func TestSignIntegration(t *testing.T) { + const ( + vaultToken = "token1" + vaultNamespace = "vault-ns-1" + vaultPath = "my_pki_mount/sign/my-role-name" + ) + + privatekey := generateRSAPrivateKey(t) + csrPEM := generateCSR(t, privatekey) + + rootBundleData, err := bundlePEM(testIntermediateCa, testRootCa) + require.NoError(t, err) + + mux := http.NewServeMux() + mux.HandleFunc(fmt.Sprintf("/v1/%s", vaultPath), func(response http.ResponseWriter, request *http.Request) { + assert.Equal(t, vaultNamespace, request.Header.Get("X-Vault-Namespace"), "Expected Vault namespace header for namespaced API path") + assert.Equal(t, vaultToken, request.Header.Get("X-Vault-Token"), "Expected the Vault token for root-only API path") + _, err := response.Write(rootBundleData) + require.NoError(t, err) + }) + server := httptest.NewServer(mux) + defer server.Close() + + v, err := New( + "k8s-ns1", + listers.FakeSecretListerFrom(listers.NewFakeSecretLister(), + listers.SetFakeSecretNamespaceListerGet( + &corev1.Secret{ + Data: map[string][]byte{ + "key1": []byte(vaultToken), + }, + }, nil), + ), + &cmapi.Issuer{ + ObjectMeta: metav1.ObjectMeta{ + Name: "issuer1", + Namespace: "k8s-ns1", + }, + Spec: v1.IssuerSpec{ + IssuerConfig: v1.IssuerConfig{ + Vault: &v1.VaultIssuer{ + Server: server.URL, + Path: vaultPath, + Namespace: vaultNamespace, + Auth: cmapi.VaultAuth{ + TokenSecretRef: &cmmeta.SecretKeySelector{ + LocalObjectReference: cmmeta.LocalObjectReference{ + Name: "secret1", + }, + Key: "key1", + }, + }, + }, + }, + }, + }) + require.NoError(t, err) + + certPEM, caPEM, err := v.Sign(csrPEM, time.Hour) + require.NoError(t, err) + require.NotEmpty(t, certPEM) + require.NotEmpty(t, caPEM) +}