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 25f9ee9cb..9f18a7554 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 } @@ -57,8 +55,6 @@ type Client interface { NewRequest(method, requestPath string) *vault.Request 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 @@ -68,7 +64,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 @@ -92,11 +103,26 @@ func New(namespace string, secretsLister corelisters.SecretLister, issuer v1.Gen 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 := 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 } - v.client = client + // A client for use with namespaced API paths + v.client = clientNS + + // 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 } @@ -124,8 +150,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 +336,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 +395,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()) @@ -395,10 +415,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 { @@ -425,8 +441,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 +464,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 - } - } -} diff --git a/internal/vault/vault_test.go b/internal/vault/vault_test.go index 74518af66..9720e4271 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" @@ -32,11 +33,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 +1174,185 @@ 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") + }) + } +} + +// 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) +} + +// 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) +}