From 99e8b44d112cde6f329befb4c48bd067fcc9c2b3 Mon Sep 17 00:00:00 2001 From: Andika Ahmad Ramadhan Date: Wed, 22 Nov 2023 07:07:24 +0700 Subject: [PATCH] fix: safely parse renewable field from vault response (#5193) * fix: safely parse renewable field from vault response Signed-off-by: Andika Ahmad Ramadhan * fix: typos Signed-off-by: Andika Ahmad Ramadhan --------- Signed-off-by: Andika Ahmad Ramadhan Signed-off-by: Yoon Park --- CHANGELOG.md | 1 + .../resolver/hashicorpvault_handler.go | 3 +- .../resolver/hashicorpvault_handler_test.go | 59 +++++++++++++++---- 3 files changed, 51 insertions(+), 12 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4372aea58f4..1139d80b882 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -65,6 +65,7 @@ Here is an overview of all new **experimental** features: - **General**: Support TriggerAuthentication properties from ConfigMap ([#4830](https://github.com/kedacore/keda/issues/4830)) - **Hashicorp Vault**: Add support to get secret that needs write operation (e.g. pki) ([#5067](https://github.com/kedacore/keda/issues/5067)) - **Hashicorp Vault**: Fix operator panic when spec.hashiCorpVault.credential.serviceAccount is not set ([#4964](https://github.com/kedacore/keda/issues/4964)) +- **Hashicorp Vault**: Fix operator panic when using root token to authenticate to vault server ([#5192](https://github.com/kedacore/keda/issues/5192)) - **Kafka Scaler**: Ability to set upper bound to the number of partitions with lag ([#3997](https://github.com/kedacore/keda/issues/3997)) - **Kafka Scaler**: Add more logging to check Sarama DescribeTopics method ([#5102](https://github.com/kedacore/keda/issues/5102)) - **Kafka Scaler**: Add support for Kerberos authentication (SASL / GSSAPI) ([#4836](https://github.com/kedacore/keda/issues/4836)) diff --git a/pkg/scaling/resolver/hashicorpvault_handler.go b/pkg/scaling/resolver/hashicorpvault_handler.go index b2c7f94f6e5..119ec9e62bd 100644 --- a/pkg/scaling/resolver/hashicorpvault_handler.go +++ b/pkg/scaling/resolver/hashicorpvault_handler.go @@ -75,8 +75,7 @@ func (vh *HashicorpVaultHandler) Initialize(logger logr.Logger) error { return err } - renew := lookup.Data["renewable"].(bool) - if renew { + if renew, ok := lookup.Data["renewable"].(bool); ok && renew { vh.stopCh = make(chan struct{}) go vh.renewToken(logger) } diff --git a/pkg/scaling/resolver/hashicorpvault_handler_test.go b/pkg/scaling/resolver/hashicorpvault_handler_test.go index 9faf1e3c5c7..41cab9c4017 100644 --- a/pkg/scaling/resolver/hashicorpvault_handler_test.go +++ b/pkg/scaling/resolver/hashicorpvault_handler_test.go @@ -117,13 +117,16 @@ func TestGetPkiRequest(t *testing.T) { } } -func mockVault(t *testing.T) *httptest.Server { +func mockVault(t *testing.T, useRootToken bool) *httptest.Server { server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { var data map[string]interface{} switch r.URL.Path { case "/v1/auth/token/lookup-self": - data = vaultTokenSelf + if useRootToken { + // remove renewable field + delete(data, "renewable") + } case "/v1/kv_v2/data/keda": //todo: more generic data = kvV2SecretDataKeda case "/v1/kv/keda": //todo: more generic @@ -170,7 +173,7 @@ func mockVault(t *testing.T) *httptest.Server { } func TestHashicorpVaultHandler_getSecretValue_specify_secret_type(t *testing.T) { - server := mockVault(t) + server := mockVault(t, false) defer server.Close() vault := kedav1alpha1.HashiCorpVault{ @@ -191,7 +194,7 @@ func TestHashicorpVaultHandler_getSecretValue_specify_secret_type(t *testing.T) }} assert.Equalf(t, kedav1alpha1.VaultSecretTypeGeneric, secrets[0].Type, "Expected secret to not have a vlue") secrets, _ = vaultHandler.ResolveSecrets(secrets) - assert.Len(t, secrets, 1, "Supposed to got back one secret") + assert.Len(t, secrets, 1, "Supposed to get back one secret") secret := secrets[0] assert.Equalf(t, kedav1alpha1.VaultSecretTypeSecretV2, secret.Type, "Expexted secret type be %s", kedav1alpha1.VaultSecretTypeSecretV2) assert.Equalf(t, kedaSecretValue, secret.Value, "Expexted secret to be %s", kedaSecretValue) @@ -202,7 +205,7 @@ func TestHashicorpVaultHandler_getSecretValue_specify_secret_type(t *testing.T) }} assert.Equalf(t, kedav1alpha1.VaultSecretTypeGeneric, secrets[0].Type, "Expected secret to not have a vlue") secrets, _ = vaultHandler.ResolveSecrets(secrets) - assert.Len(t, secrets, 1, "Supposed to got back one secret") + assert.Len(t, secrets, 1, "Supposed to get back one secret") secret = secrets[0] assert.Equalf(t, kedav1alpha1.VaultSecretTypeSecret, secret.Type, "Expexted secret type be %s", kedav1alpha1.VaultSecretTypeSecret) assert.Equalf(t, kedaSecretValue, secret.Value, "Expexted secret to be %s", kedaSecretValue) @@ -310,7 +313,43 @@ var resolveRequestTestDataSet = []resolveRequestTestData{ } func TestHashicorpVaultHandler_ResolveSecret(t *testing.T) { - server := mockVault(t) + server := mockVault(t, false) + defer server.Close() + + vault := kedav1alpha1.HashiCorpVault{ + Address: server.URL, + Authentication: kedav1alpha1.VaultAuthenticationToken, + Credential: &kedav1alpha1.Credential{ + Token: vaultTestToken, + }, + } + vaultHandler := NewHashicorpVaultHandler(&vault) + err := vaultHandler.Initialize(logf.Log.WithName("test")) + defer vaultHandler.Stop() + assert.Nil(t, err) + + for _, testData := range resolveRequestTestDataSet { + secrets := []kedav1alpha1.VaultSecret{{ + Parameter: "test", + Path: testData.path, + Key: testData.key, + Type: testData.secretType, + PkiData: testData.pkiData, + }} + secrets, err := vaultHandler.ResolveSecrets(secrets) + assert.Len(t, secrets, 1, "Supposed to get back one secret") + secret := secrets[0] + if testData.isError { + assert.NotNilf(t, err, "test %s: expected error but got success, testData - %+v", testData.name, testData) + continue + } + assert.Nilf(t, err, "test %s: expected success but got error - %s", testData.name, err) + assert.Equalf(t, testData.expectedValue, secret.Value, "test %s: expected data does not match given secret", testData.name) + } +} + +func TestHashicorpVaultHandler_ResolveSecret_UsingRootToken(t *testing.T) { + server := mockVault(t, true) defer server.Close() vault := kedav1alpha1.HashiCorpVault{ @@ -334,7 +373,7 @@ func TestHashicorpVaultHandler_ResolveSecret(t *testing.T) { PkiData: testData.pkiData, }} secrets, err := vaultHandler.ResolveSecrets(secrets) - assert.Len(t, secrets, 1, "Supposed to got back one secret") + assert.Len(t, secrets, 1, "Supposed to get back one secret") secret := secrets[0] if testData.isError { assert.NotNilf(t, err, "test %s: expected error but got success, testData - %+v", testData.name, testData) @@ -347,7 +386,7 @@ func TestHashicorpVaultHandler_ResolveSecret(t *testing.T) { func TestHashicorpVaultHandler_DefaultKubernetesVaultRole(t *testing.T) { defaultServiceAccountPath := "/var/run/secrets/kubernetes.io/serviceaccount/token" - server := mockVault(t) + server := mockVault(t, false) defer server.Close() vault := kedav1alpha1.HashiCorpVault{ @@ -365,7 +404,7 @@ func TestHashicorpVaultHandler_DefaultKubernetesVaultRole(t *testing.T) { } func TestHashicorpVaultHandler_ResolveSecrets_SameCertAndKey(t *testing.T) { - server := mockVault(t) + server := mockVault(t, false) defer server.Close() vault := kedav1alpha1.HashiCorpVault{ @@ -393,6 +432,6 @@ func TestHashicorpVaultHandler_ResolveSecrets_SameCertAndKey(t *testing.T) { PkiData: kedav1alpha1.VaultPkiData{CommonName: "test"}, }} secrets, _ = vaultHandler.ResolveSecrets(secrets) - assert.Len(t, secrets, 2, "Supposed to got back two secret") + assert.Len(t, secrets, 2, "Supposed to get back two secrets") assert.Equalf(t, secrets[0].Value, secrets[1].Value, "Refetching same path should yield same value") }