From 298ceb3b2a72706097f03271ec0998d496610ca5 Mon Sep 17 00:00:00 2001 From: Vincent Sabatini Date: Thu, 19 Oct 2023 07:53:28 -0500 Subject: [PATCH 1/3] fix error message when setting up vault issuer * Ensure Vault URL can be parsed * Separate generic http errors from vault specific errors when checking health endpoint Signed-off-by: Vincent Sabatini --- internal/vault/vault.go | 9 +++++ internal/vault/vault_test.go | 9 ++++- .../certificaterequests/vault/vault_test.go | 6 +++- .../vault/vault_test.go | 1 + pkg/issuer/vault/setup.go | 12 +++++-- pkg/issuer/vault/setup_test.go | 36 +++++++++++++++++++ 6 files changed, 68 insertions(+), 5 deletions(-) diff --git a/internal/vault/vault.go b/internal/vault/vault.go index 4a5aaf2c8..6c51ec8c0 100644 --- a/internal/vault/vault.go +++ b/internal/vault/vault.go @@ -22,6 +22,7 @@ import ( "errors" "fmt" "net/http" + "net/url" "path" "path/filepath" "strings" @@ -225,6 +226,14 @@ func (v *Vault) newConfig() (*vault.Config, error) { cfg := vault.DefaultConfig() cfg.Address = v.issuer.GetSpec().Vault.Server + urlParse, err := url.Parse(cfg.Address) + if err != nil { + return nil, fmt.Errorf("error parsing vault url: %w", err) + } + if urlParse.Host == "" { + return nil, fmt.Errorf("host not found in vault server url: %s", cfg.Address) + } + caBundle, err := v.caBundle() if err != nil { return nil, fmt.Errorf("failed to load vault CA bundle: %w", err) diff --git a/internal/vault/vault_test.go b/internal/vault/vault_test.go index 6e2aa31f6..5e457a203 100644 --- a/internal/vault/vault_test.go +++ b/internal/vault/vault_test.go @@ -989,6 +989,7 @@ func TestNewConfig(t *testing.T) { "a bad cert bundle should error": { issuer: gen.Issuer("vault-issuer", gen.SetIssuerVault(cmapi.VaultIssuer{ + Server: "https://vault.example.com", CABundle: []byte("a bad cert bundle"), }), ), @@ -998,6 +999,7 @@ func TestNewConfig(t *testing.T) { "a good cert bundle should be added to the config": { issuer: gen.Issuer("vault-issuer", gen.SetIssuerVault(cmapi.VaultIssuer{ + Server: "https://vault.example.com", CABundle: []byte(testLeafCertificate), }), ), @@ -1025,6 +1027,7 @@ func TestNewConfig(t *testing.T) { "a good bundle from a caBundleSecretRef should be added to the config": { issuer: gen.Issuer("vault-issuer", gen.SetIssuerVault(cmapi.VaultIssuer{ + Server: "https://vault.example.com", CABundleSecretRef: &cmmeta.SecretKeySelector{ Key: "my-bundle.crt", LocalObjectReference: cmmeta.LocalObjectReference{ @@ -1060,6 +1063,7 @@ func TestNewConfig(t *testing.T) { "a good bundle from a caBundleSecretRef with default key should be added to the config": { issuer: gen.Issuer("vault-issuer", gen.SetIssuerVault(cmapi.VaultIssuer{ + Server: "https://vault.example.com", CABundleSecretRef: &cmmeta.SecretKeySelector{ LocalObjectReference: cmmeta.LocalObjectReference{ Name: "bundle", @@ -1094,6 +1098,7 @@ func TestNewConfig(t *testing.T) { "a bad bundle from a caBundleSecretRef should error": { issuer: gen.Issuer("vault-issuer", gen.SetIssuerVault(cmapi.VaultIssuer{ + Server: "https://vault.example.com", CABundleSecretRef: &cmmeta.SecretKeySelector{ Key: "my-bundle.crt", LocalObjectReference: cmmeta.LocalObjectReference{ @@ -1108,7 +1113,8 @@ func TestNewConfig(t *testing.T) { "the tokenCreate func should be called with the correct namespace": { issuer: gen.Issuer("vault-issuer", gen.SetIssuerVault(cmapi.VaultIssuer{ - Path: "my-path", + Server: "https://vault.example.com", + Path: "my-path", Auth: cmapi.VaultAuth{ Kubernetes: &cmapi.VaultKubernetesAuth{ Role: "my-role", @@ -1320,6 +1326,7 @@ func TestNewWithVaultNamespaces(t *testing.T) { Spec: v1.IssuerSpec{ IssuerConfig: v1.IssuerConfig{ Vault: &v1.VaultIssuer{ + Server: "https://vault.example.com", Namespace: tc.vaultNS, Auth: cmapi.VaultAuth{ TokenSecretRef: &cmmeta.SecretKeySelector{ diff --git a/pkg/controller/certificaterequests/vault/vault_test.go b/pkg/controller/certificaterequests/vault/vault_test.go index 35dc0207b..0cb69b5fc 100644 --- a/pkg/controller/certificaterequests/vault/vault_test.go +++ b/pkg/controller/certificaterequests/vault/vault_test.go @@ -88,7 +88,9 @@ func generateSelfSignedCertFromCR(cr *cmapi.CertificateRequest, key crypto.Signe func TestSign(t *testing.T) { metaFixedClockStart := metav1.NewTime(fixedClockStart) baseIssuer := gen.Issuer("vault-issuer", - gen.SetIssuerVault(cmapi.VaultIssuer{}), + gen.SetIssuerVault(cmapi.VaultIssuer{ + Server: "https://example.vault.com", + }), gen.AddIssuerCondition(cmapi.IssuerCondition{ Type: cmapi.IssuerConditionReady, Status: cmmeta.ConditionTrue, @@ -234,6 +236,7 @@ func TestSign(t *testing.T) { }, }, }, + Server: "https://example.vault.com", })), }, ExpectedEvents: []string{ @@ -274,6 +277,7 @@ func TestSign(t *testing.T) { }, }, }, + Server: "https://example.vault.com", }), )}, ExpectedEvents: []string{ diff --git a/pkg/controller/certificatesigningrequests/vault/vault_test.go b/pkg/controller/certificatesigningrequests/vault/vault_test.go index 0d03c6547..f4b528977 100644 --- a/pkg/controller/certificatesigningrequests/vault/vault_test.go +++ b/pkg/controller/certificatesigningrequests/vault/vault_test.go @@ -70,6 +70,7 @@ func TestProcessItem(t *testing.T) { }, }, }, + Server: "https://example.vault.com", }), gen.AddIssuerCondition(cmapi.IssuerCondition{ Type: cmapi.IssuerConditionReady, diff --git a/pkg/issuer/vault/setup.go b/pkg/issuer/vault/setup.go index 9a3c450a4..886c9ed79 100644 --- a/pkg/issuer/vault/setup.go +++ b/pkg/issuer/vault/setup.go @@ -19,6 +19,7 @@ package vault import ( "context" "fmt" + "strings" vaultinternal "github.com/cert-manager/cert-manager/internal/vault" apiutil "github.com/cert-manager/cert-manager/pkg/api/util" @@ -135,9 +136,14 @@ func (v *Vault) Setup(ctx context.Context) error { } if err := client.IsVaultInitializedAndUnsealed(); err != nil { - logf.V(logf.WarnLevel).Infof("%s: %s: error: %s", v.issuer.GetObjectMeta().Name, messageVaultStatusVerificationFailed, err.Error()) - apiutil.SetIssuerCondition(v.issuer, v.issuer.GetGeneration(), v1.IssuerConditionReady, cmmeta.ConditionFalse, errorVault, messageVaultStatusVerificationFailed) - return fmt.Errorf(messageVaultStatusVerificationFailed) + if strings.Contains(err.Error(), "error calling Vault") { + logf.V(logf.WarnLevel).Infof("%s: %s: error: %s", v.issuer.GetObjectMeta().Name, messageVaultStatusVerificationFailed, err.Error()) + apiutil.SetIssuerCondition(v.issuer, v.issuer.GetGeneration(), v1.IssuerConditionReady, cmmeta.ConditionFalse, errorVault, messageVaultStatusVerificationFailed) + return fmt.Errorf(messageVaultStatusVerificationFailed) + } + logf.V(logf.WarnLevel).Infof("%s: %s", v.issuer.GetObjectMeta().Name, err.Error) + apiutil.SetIssuerCondition(v.issuer, v.issuer.GetGeneration(), v1.IssuerConditionReady, cmmeta.ConditionFalse, errorVault, err.Error()) + return err } logf.Log.V(logf.DebugLevel).Info(messageVaultVerified) diff --git a/pkg/issuer/vault/setup_test.go b/pkg/issuer/vault/setup_test.go index 9bcb99d1d..6fa891a17 100644 --- a/pkg/issuer/vault/setup_test.go +++ b/pkg/issuer/vault/setup_test.go @@ -368,6 +368,42 @@ func TestVault_Setup(t *testing.T) { }, expectCond: "Ready True: VaultVerified: Vault verified", }, + { + name: "server with invalid url should fail to setup", + givenIssuer: v1.IssuerConfig{ + Vault: &v1.VaultIssuer{ + Path: "pki_int", + Server: "https:/vault.example.com", + Auth: v1.VaultAuth{ + TokenSecretRef: &cmmeta.SecretKeySelector{ + LocalObjectReference: cmmeta.LocalObjectReference{ + Name: "cert-manager", + }, + Key: "", + }, + }, + }, + }, + expectErr: "host not found in vault server url: https:/vault.example.com", + }, + { + name: "server with leading whitespace should fail to parse", + givenIssuer: v1.IssuerConfig{ + Vault: &v1.VaultIssuer{ + Path: "pki_int", + Server: " https:///vault.example.com", + Auth: v1.VaultAuth{ + TokenSecretRef: &cmmeta.SecretKeySelector{ + LocalObjectReference: cmmeta.LocalObjectReference{ + Name: "cert-manager", + }, + Key: "", + }, + }, + }, + }, + expectErr: "error parsing vault url: parse \" https:///vault.example.com\": first path segment in URL cannot contain colon", + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { From ef6ef1f0dbe96d8695e3f4b3b6737133a54d87de Mon Sep 17 00:00:00 2001 From: Vinny Sabatini Date: Fri, 20 Oct 2023 16:33:27 -0500 Subject: [PATCH 2/3] additional improvements to vault issuer error messages When initializing a Vault issuer: * Create different error messages depending on if Vault is sealed or not initialized * Do not explicitly parse the Vault server URL (this is covered when trying to access health endpoint) Signed-off-by: Vinny Sabatini --- internal/vault/vault.go | 16 +++++++--------- pkg/issuer/vault/setup.go | 18 +++++------------- pkg/issuer/vault/setup_test.go | 6 +++--- 3 files changed, 15 insertions(+), 25 deletions(-) diff --git a/internal/vault/vault.go b/internal/vault/vault.go index 6c51ec8c0..6b11b919f 100644 --- a/internal/vault/vault.go +++ b/internal/vault/vault.go @@ -22,7 +22,6 @@ import ( "errors" "fmt" "net/http" - "net/url" "path" "path/filepath" "strings" @@ -226,14 +225,6 @@ func (v *Vault) newConfig() (*vault.Config, error) { cfg := vault.DefaultConfig() cfg.Address = v.issuer.GetSpec().Vault.Server - urlParse, err := url.Parse(cfg.Address) - if err != nil { - return nil, fmt.Errorf("error parsing vault url: %w", err) - } - if urlParse.Host == "" { - return nil, fmt.Errorf("host not found in vault server url: %s", cfg.Address) - } - caBundle, err := v.caBundle() if err != nil { return nil, fmt.Errorf("failed to load vault CA bundle: %w", err) @@ -506,15 +497,22 @@ func (v *Vault) IsVaultInitializedAndUnsealed() error { defer healthResp.Body.Close() } + // 200 = if initialized, unsealed, and active // 429 = if unsealed and standby // 472 = if disaster recovery mode replication secondary and active // 473 = if performance standby + // 501 = if not initialized + // 503 = if sealed if err != nil { switch { case healthResp == nil: return err case healthResp.StatusCode == 429, healthResp.StatusCode == 472, healthResp.StatusCode == 473: return nil + case healthResp.StatusCode == 501: + return fmt.Errorf("Vault is not initialized") + case healthResp.StatusCode == 503: + return fmt.Errorf("Vault is sealed") default: return fmt.Errorf("error calling Vault %s: %w", healthURL, err) } diff --git a/pkg/issuer/vault/setup.go b/pkg/issuer/vault/setup.go index 886c9ed79..1ffb8cfe5 100644 --- a/pkg/issuer/vault/setup.go +++ b/pkg/issuer/vault/setup.go @@ -18,8 +18,6 @@ package vault import ( "context" - "fmt" - "strings" vaultinternal "github.com/cert-manager/cert-manager/internal/vault" apiutil "github.com/cert-manager/cert-manager/pkg/api/util" @@ -34,12 +32,11 @@ const ( errorVault = "VaultError" - messageVaultClientInitFailed = "Failed to initialize Vault client: " - messageVaultStatusVerificationFailed = "Vault is not initialized or is sealed" - messageVaultConfigRequired = "Vault config cannot be empty" - messageServerAndPathRequired = "Vault server and path are required fields" - messageAuthFieldsRequired = "Vault tokenSecretRef, appRole, or kubernetes is required" - messageMultipleAuthFieldsSet = "Multiple auth methods cannot be set on the same Vault issuer" + messageVaultClientInitFailed = "Failed to initialize Vault client: " + messageVaultConfigRequired = "Vault config cannot be empty" + messageServerAndPathRequired = "Vault server and path are required fields" + messageAuthFieldsRequired = "Vault tokenSecretRef, appRole, or kubernetes is required" + messageMultipleAuthFieldsSet = "Multiple auth methods cannot be set on the same Vault issuer" messageKubeAuthRoleRequired = "Vault Kubernetes auth requires a role to be set" messageKubeAuthEitherRequired = "Vault Kubernetes auth requires either secretRef.name or serviceAccountRef.name to be set" @@ -136,11 +133,6 @@ func (v *Vault) Setup(ctx context.Context) error { } if err := client.IsVaultInitializedAndUnsealed(); err != nil { - if strings.Contains(err.Error(), "error calling Vault") { - logf.V(logf.WarnLevel).Infof("%s: %s: error: %s", v.issuer.GetObjectMeta().Name, messageVaultStatusVerificationFailed, err.Error()) - apiutil.SetIssuerCondition(v.issuer, v.issuer.GetGeneration(), v1.IssuerConditionReady, cmmeta.ConditionFalse, errorVault, messageVaultStatusVerificationFailed) - return fmt.Errorf(messageVaultStatusVerificationFailed) - } logf.V(logf.WarnLevel).Infof("%s: %s", v.issuer.GetObjectMeta().Name, err.Error) apiutil.SetIssuerCondition(v.issuer, v.issuer.GetGeneration(), v1.IssuerConditionReady, cmmeta.ConditionFalse, errorVault, err.Error()) return err diff --git a/pkg/issuer/vault/setup_test.go b/pkg/issuer/vault/setup_test.go index 6fa891a17..68302baa5 100644 --- a/pkg/issuer/vault/setup_test.go +++ b/pkg/issuer/vault/setup_test.go @@ -384,14 +384,14 @@ func TestVault_Setup(t *testing.T) { }, }, }, - expectErr: "host not found in vault server url: https:/vault.example.com", + expectErr: "Get \"https:///vault.example.com/v1/sys/health\": http: no Host in request URL", }, { name: "server with leading whitespace should fail to parse", givenIssuer: v1.IssuerConfig{ Vault: &v1.VaultIssuer{ Path: "pki_int", - Server: " https:///vault.example.com", + Server: " https://vault.example.com", Auth: v1.VaultAuth{ TokenSecretRef: &cmmeta.SecretKeySelector{ LocalObjectReference: cmmeta.LocalObjectReference{ @@ -402,7 +402,7 @@ func TestVault_Setup(t *testing.T) { }, }, }, - expectErr: "error parsing vault url: parse \" https:///vault.example.com\": first path segment in URL cannot contain colon", + expectErr: "error initializing Vault client: parse \" https://vault.example.com\": first path segment in URL cannot contain colon", }, } for _, tt := range tests { From d15e55a16cc2bdf0f6189ffff4222773be439686 Mon Sep 17 00:00:00 2001 From: Vinny Sabatini Date: Tue, 24 Oct 2023 09:52:52 -0500 Subject: [PATCH 3/3] Update pkg/issuer/vault/setup.go Co-authored-by: Tim Ramlot <42113979+inteon@users.noreply.github.com> Signed-off-by: Vinny Sabatini --- pkg/issuer/vault/setup.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/issuer/vault/setup.go b/pkg/issuer/vault/setup.go index 1ffb8cfe5..980134e52 100644 --- a/pkg/issuer/vault/setup.go +++ b/pkg/issuer/vault/setup.go @@ -133,7 +133,7 @@ func (v *Vault) Setup(ctx context.Context) error { } if err := client.IsVaultInitializedAndUnsealed(); err != nil { - logf.V(logf.WarnLevel).Infof("%s: %s", v.issuer.GetObjectMeta().Name, err.Error) + logf.V(logf.WarnLevel).Infof("%s: %s", v.issuer.GetObjectMeta().Name, err.Error()) apiutil.SetIssuerCondition(v.issuer, v.issuer.GetGeneration(), v1.IssuerConditionReady, cmmeta.ConditionFalse, errorVault, err.Error()) return err }