From e9f5e8241355a5b3ae2fb6b78579061808dd359e Mon Sep 17 00:00:00 2001 From: KevFan <chfan@redhat.com> Date: Wed, 18 Sep 2024 10:27:10 +0100 Subject: [PATCH 01/12] feat: configurable API Key K8s secret key name Signed-off-by: KevFan <chfan@redhat.com> --- api/v1beta3/auth_config_types.go | 6 +++ api/v1beta3/zz_generated.deepcopy.go | 5 +++ controllers/auth_config_controller.go | 2 +- controllers/secret_controller_test.go | 2 +- .../authorino.kuadrant.io_authconfigs.yaml | 8 ++++ install/manifests.yaml | 8 ++++ pkg/evaluators/identity/api_key.go | 37 +++++++++------- pkg/evaluators/identity/api_key_test.go | 44 +++++++++++++++---- 8 files changed, 86 insertions(+), 26 deletions(-) diff --git a/api/v1beta3/auth_config_types.go b/api/v1beta3/auth_config_types.go index 4abe3857..75050779 100644 --- a/api/v1beta3/auth_config_types.go +++ b/api/v1beta3/auth_config_types.go @@ -355,6 +355,12 @@ type ApiKeyAuthenticationSpec struct { // +optional // +kubebuilder:default:=false AllNamespaces bool `json:"allNamespaces,omitempty"` + + // List of keys within the selected Kubernetes secret that contain valid API credentials. + // Authorino will attempt to authenticate using the first key that matches. + // If no match is found, authentication will fail. + // +optional + KeySelectors []string `json:"keySelectors,omitempty"` } // Settings to fetch the JSON Web Key Set (JWKS) for the JWT authentication. diff --git a/api/v1beta3/zz_generated.deepcopy.go b/api/v1beta3/zz_generated.deepcopy.go index 382a3ad7..84cb1a70 100644 --- a/api/v1beta3/zz_generated.deepcopy.go +++ b/api/v1beta3/zz_generated.deepcopy.go @@ -49,6 +49,11 @@ func (in *ApiKeyAuthenticationSpec) DeepCopyInto(out *ApiKeyAuthenticationSpec) *out = new(v1.LabelSelector) (*in).DeepCopyInto(*out) } + if in.KeySelectors != nil { + in, out := &in.KeySelectors, &out.KeySelectors + *out = make([]string, len(*in)) + copy(*out, *in) + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new ApiKeyAuthenticationSpec. diff --git a/controllers/auth_config_controller.go b/controllers/auth_config_controller.go index d1c7d5ea..85c59dfc 100644 --- a/controllers/auth_config_controller.go +++ b/controllers/auth_config_controller.go @@ -261,7 +261,7 @@ func (r *AuthConfigReconciler) translateAuthConfig(ctx context.Context, authConf if err != nil { return nil, err } - translatedIdentity.APIKey = identity_evaluators.NewApiKeyIdentity(identityCfgName, selector, namespace, authCred, r.Client, ctxWithLogger) + translatedIdentity.APIKey = identity_evaluators.NewApiKeyIdentity(identityCfgName, selector, namespace, identity.ApiKey.KeySelectors, authCred, r.Client, ctxWithLogger) // MTLS case api.X509ClientCertificateAuthentication: diff --git a/controllers/secret_controller_test.go b/controllers/secret_controller_test.go index c14d2db6..05351186 100644 --- a/controllers/secret_controller_test.go +++ b/controllers/secret_controller_test.go @@ -94,7 +94,7 @@ func newSecretReconcilerTest(mockCtrl *gomock.Controller, secretLabels map[strin indexedAuthConfig := &evaluators.AuthConfig{ Labels: map[string]string{"namespace": "authorino", "name": "api-protection"}, IdentityConfigs: []auth.AuthConfigEvaluator{&fakeAPIKeyIdentityConfig{ - evaluator: identity_evaluators.NewApiKeyIdentity("api-key", apiKeyLabelSelectors, "", auth.NewAuthCredential("", ""), fakeK8sClient, context.TODO()), + evaluator: identity_evaluators.NewApiKeyIdentity("api-key", apiKeyLabelSelectors, "", []string{}, auth.NewAuthCredential("", ""), fakeK8sClient, context.TODO()), }}, } indexMock := mock_index.NewMockIndex(mockCtrl) diff --git a/install/crd/authorino.kuadrant.io_authconfigs.yaml b/install/crd/authorino.kuadrant.io_authconfigs.yaml index 4f963ca4..731111f1 100644 --- a/install/crd/authorino.kuadrant.io_authconfigs.yaml +++ b/install/crd/authorino.kuadrant.io_authconfigs.yaml @@ -2393,6 +2393,14 @@ spec: Whether Authorino should look for API key secrets in all namespaces or only in the same namespace as the AuthConfig. Enabling this option in namespaced Authorino instances has no effect. type: boolean + keySelectors: + description: |- + List of keys within the selected Kubernetes secret that contain valid API credentials. + Authorino will attempt to authenticate using the first key that matches. + If no match is found, authentication will fail. + items: + type: string + type: array selector: description: Label selector used by Authorino to match secrets from the cluster storing valid credentials to authenticate diff --git a/install/manifests.yaml b/install/manifests.yaml index cd566bda..dd27a1d7 100644 --- a/install/manifests.yaml +++ b/install/manifests.yaml @@ -2660,6 +2660,14 @@ spec: Whether Authorino should look for API key secrets in all namespaces or only in the same namespace as the AuthConfig. Enabling this option in namespaced Authorino instances has no effect. type: boolean + keySelectors: + description: |- + List of keys within the selected Kubernetes secret that contain valid API credentials. + Authorino will attempt to authenticate using the first key that matches. + If no match is found, authentication will fail. + items: + type: string + type: array selector: description: Label selector used by Authorino to match secrets from the cluster storing valid credentials to authenticate diff --git a/pkg/evaluators/identity/api_key.go b/pkg/evaluators/identity/api_key.go index ab2a0236..885a41e8 100644 --- a/pkg/evaluators/identity/api_key.go +++ b/pkg/evaluators/identity/api_key.go @@ -26,18 +26,20 @@ type APIKey struct { Name string `yaml:"name"` LabelSelectors k8s_labels.Selector `yaml:"labelSelectors"` Namespace string `yaml:"namespace"` + KeySelectors []string `yaml:"keySelectors"` secrets map[string]k8s.Secret mutex sync.RWMutex k8sClient k8s_client.Reader } -func NewApiKeyIdentity(name string, labelSelectors k8s_labels.Selector, namespace string, authCred auth.AuthCredentials, k8sClient k8s_client.Reader, ctx context.Context) *APIKey { +func NewApiKeyIdentity(name string, labelSelectors k8s_labels.Selector, namespace string, keySelectors []string, authCred auth.AuthCredentials, k8sClient k8s_client.Reader, ctx context.Context) *APIKey { apiKey := &APIKey{ AuthCredentials: authCred, Name: name, LabelSelectors: labelSelectors, Namespace: namespace, + KeySelectors: append(keySelectors, apiKeySelector), secrets: make(map[string]k8s.Secret), k8sClient: k8sClient, } @@ -103,17 +105,19 @@ func (a *APIKey) AddK8sSecretBasedIdentity(ctx context.Context, new k8s.Secret) logger := log.FromContext(ctx).WithName("apikey") // updating existing - newAPIKeyValue := string(new.Data[apiKeySelector]) - for oldAPIKeyValue, current := range a.secrets { - if current.GetNamespace() == new.GetNamespace() && current.GetName() == new.GetName() { - if oldAPIKeyValue != newAPIKeyValue { - a.appendK8sSecretBasedIdentity(new) - delete(a.secrets, oldAPIKeyValue) - logger.V(1).Info("api key updated") - } else { - logger.V(1).Info("api key unchanged") + for _, key := range a.KeySelectors { + newAPIKeyValue := string(new.Data[key]) + for oldAPIKeyValue, current := range a.secrets { + if current.GetNamespace() == new.GetNamespace() && current.GetName() == new.GetName() { + if oldAPIKeyValue != newAPIKeyValue { + a.appendK8sSecretBasedIdentity(new) + delete(a.secrets, oldAPIKeyValue) + logger.V(1).Info("api key updated") + } else { + logger.V(1).Info("api key unchanged") + } + return } - return } } @@ -146,10 +150,13 @@ func (a *APIKey) withinScope(namespace string) bool { // Appends the K8s Secret to the cache of API keys // Caution! This function is not thread-safe. Make sure to acquire a lock before calling it. func (a *APIKey) appendK8sSecretBasedIdentity(secret k8s.Secret) bool { - value, isAPIKeySecret := secret.Data[apiKeySelector] - if isAPIKeySecret && len(value) > 0 { - a.secrets[string(value)] = secret - return true + for _, key := range a.KeySelectors { + value, isAPIKeySecret := secret.Data[key] + if isAPIKeySecret && len(value) > 0 { + a.secrets[string(value)] = secret + return true + } } + return false } diff --git a/pkg/evaluators/identity/api_key_test.go b/pkg/evaluators/identity/api_key_test.go index 8c569280..2d50f50b 100644 --- a/pkg/evaluators/identity/api_key_test.go +++ b/pkg/evaluators/identity/api_key_test.go @@ -11,7 +11,7 @@ import ( k8s_meta "k8s.io/apimachinery/pkg/apis/meta/v1" k8s_labels "k8s.io/apimachinery/pkg/labels" - gomock "github.com/golang/mock/gomock" + "github.com/golang/mock/gomock" "gotest.tools/assert" ) @@ -32,11 +32,13 @@ func TestNewApiKeyIdentityAllNamespaces(t *testing.T) { defer ctrl.Finish() selector, _ := k8s_labels.Parse("planet=coruscant") - apiKey := NewApiKeyIdentity("jedi", selector, "", mock_auth.NewMockAuthCredentials(ctrl), testAPIKeyK8sClient, context.TODO()) + apiKey := NewApiKeyIdentity("jedi", selector, "", []string{}, mock_auth.NewMockAuthCredentials(ctrl), testAPIKeyK8sClient, context.TODO()) assert.Equal(t, apiKey.Name, "jedi") assert.Equal(t, apiKey.LabelSelectors.String(), "planet=coruscant") assert.Equal(t, apiKey.Namespace, "") + assert.Equal(t, len(apiKey.KeySelectors), 1) + assert.Equal(t, apiKey.KeySelectors[0], apiKeySelector) assert.Equal(t, len(apiKey.secrets), 2) _, exists := apiKey.secrets["ObiWanKenobiLightSaber"] assert.Check(t, exists) @@ -51,11 +53,35 @@ func TestNewApiKeyIdentitySingleNamespace(t *testing.T) { defer ctrl.Finish() selector, _ := k8s_labels.Parse("planet=coruscant") - apiKey := NewApiKeyIdentity("jedi", selector, "ns1", mock_auth.NewMockAuthCredentials(ctrl), testAPIKeyK8sClient, context.TODO()) + apiKey := NewApiKeyIdentity("jedi", selector, "ns1", []string{}, mock_auth.NewMockAuthCredentials(ctrl), testAPIKeyK8sClient, context.TODO()) assert.Equal(t, apiKey.Name, "jedi") assert.Equal(t, apiKey.LabelSelectors.String(), "planet=coruscant") assert.Equal(t, apiKey.Namespace, "ns1") + assert.Equal(t, len(apiKey.KeySelectors), 1) + assert.Equal(t, apiKey.KeySelectors[0], apiKeySelector) + assert.Equal(t, len(apiKey.secrets), 1) + _, exists := apiKey.secrets["ObiWanKenobiLightSaber"] + assert.Check(t, exists) + _, exists = apiKey.secrets["MasterYodaLightSaber"] + assert.Check(t, !exists) + _, exists = apiKey.secrets["AnakinSkywalkerLightSaber"] + assert.Check(t, !exists) +} + +func TestNewApiKeyIdentityMultipleKeySelectors(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + selector, _ := k8s_labels.Parse("planet=coruscant") + apiKey := NewApiKeyIdentity("jedi", selector, "ns1", []string{"no_op"}, mock_auth.NewMockAuthCredentials(ctrl), testAPIKeyK8sClient, context.TODO()) + + assert.Equal(t, apiKey.Name, "jedi") + assert.Equal(t, apiKey.LabelSelectors.String(), "planet=coruscant") + assert.Equal(t, apiKey.Namespace, "ns1") + assert.Equal(t, len(apiKey.KeySelectors), 2) + assert.Equal(t, apiKey.KeySelectors[0], "no_op") + assert.Equal(t, apiKey.KeySelectors[1], apiKeySelector) assert.Equal(t, len(apiKey.secrets), 1) _, exists := apiKey.secrets["ObiWanKenobiLightSaber"] assert.Check(t, exists) @@ -74,7 +100,7 @@ func TestCallSuccess(t *testing.T) { authCredMock.EXPECT().GetCredentialsFromReq(gomock.Any()).Return("ObiWanKenobiLightSaber", nil) selector, _ := k8s_labels.Parse("planet=coruscant") - apiKey := NewApiKeyIdentity("jedi", selector, "", authCredMock, testAPIKeyK8sClient, context.TODO()) + apiKey := NewApiKeyIdentity("jedi", selector, "", []string{}, authCredMock, testAPIKeyK8sClient, context.TODO()) auth, err := apiKey.Call(pipelineMock, context.TODO()) assert.NilError(t, err) @@ -90,7 +116,7 @@ func TestCallNoApiKeyFail(t *testing.T) { authCredMock.EXPECT().GetCredentialsFromReq(gomock.Any()).Return("", fmt.Errorf("something went wrong getting the API Key")) selector, _ := k8s_labels.Parse("planet=coruscant") - apiKey := NewApiKeyIdentity("jedi", selector, "", authCredMock, testAPIKeyK8sClient, context.TODO()) + apiKey := NewApiKeyIdentity("jedi", selector, "", []string{}, authCredMock, testAPIKeyK8sClient, context.TODO()) _, err := apiKey.Call(pipelineMock, context.TODO()) @@ -106,7 +132,7 @@ func TestCallInvalidApiKeyFail(t *testing.T) { authCredMock.EXPECT().GetCredentialsFromReq(gomock.Any()).Return("ASithLightSaber", nil) selector, _ := k8s_labels.Parse("planet=coruscant") - apiKey := NewApiKeyIdentity("jedi", selector, "", authCredMock, testAPIKeyK8sClient, context.TODO()) + apiKey := NewApiKeyIdentity("jedi", selector, "", []string{}, authCredMock, testAPIKeyK8sClient, context.TODO()) _, err := apiKey.Call(pipelineMock, context.TODO()) assert.Error(t, err, "the API Key provided is invalid") @@ -114,7 +140,7 @@ func TestCallInvalidApiKeyFail(t *testing.T) { func TestLoadSecretsSuccess(t *testing.T) { selector, _ := k8s_labels.Parse("planet=coruscant") - apiKey := NewApiKeyIdentity("X-API-KEY", selector, "", nil, testAPIKeyK8sClient, nil) + apiKey := NewApiKeyIdentity("X-API-KEY", selector, "", []string{}, nil, testAPIKeyK8sClient, nil) err := apiKey.loadSecrets(context.TODO()) assert.NilError(t, err) @@ -131,7 +157,7 @@ func TestLoadSecretsSuccess(t *testing.T) { func TestLoadSecretsFail(t *testing.T) { selector, _ := k8s_labels.Parse("planet=coruscant") - apiKey := NewApiKeyIdentity("X-API-KEY", selector, "", nil, &flawedAPIkeyK8sClient{}, context.TODO()) + apiKey := NewApiKeyIdentity("X-API-KEY", selector, "", []string{}, nil, &flawedAPIkeyK8sClient{}, context.TODO()) err := apiKey.loadSecrets(context.TODO()) assert.Error(t, err, "something terribly wrong happened") @@ -145,7 +171,7 @@ func BenchmarkAPIKeyAuthn(b *testing.B) { authCredMock := mock_auth.NewMockAuthCredentials(ctrl) authCredMock.EXPECT().GetCredentialsFromReq(gomock.Any()).Return("ObiWanKenobiLightSaber", nil).MinTimes(1) selector, _ := k8s_labels.Parse("planet=coruscant") - apiKey := NewApiKeyIdentity("jedi", selector, "", authCredMock, testAPIKeyK8sClient, context.TODO()) + apiKey := NewApiKeyIdentity("jedi", selector, "", []string{}, authCredMock, testAPIKeyK8sClient, context.TODO()) var err error b.ResetTimer() From 1d533224107ee2192a992f8f66d4cb81453a241b Mon Sep 17 00:00:00 2001 From: KevFan <chfan@redhat.com> Date: Wed, 22 Jan 2025 14:41:10 +0000 Subject: [PATCH 02/12] fixup: add non-thread safe comment to AddK8sSecretBasedIdentity Signed-off-by: KevFan <chfan@redhat.com> --- pkg/evaluators/identity/api_key.go | 1 + 1 file changed, 1 insertion(+) diff --git a/pkg/evaluators/identity/api_key.go b/pkg/evaluators/identity/api_key.go index 885a41e8..9bdb4070 100644 --- a/pkg/evaluators/identity/api_key.go +++ b/pkg/evaluators/identity/api_key.go @@ -94,6 +94,7 @@ func (a *APIKey) GetK8sSecretLabelSelectors() k8s_labels.Selector { return a.LabelSelectors } +// Caution! This function is not thread-safe. Make sure to acquire a lock before calling it. func (a *APIKey) AddK8sSecretBasedIdentity(ctx context.Context, new k8s.Secret) { if !a.withinScope(new.GetNamespace()) { return From 9b8990c8d5cf6b4b4feb1d89be3476a5d9b63d85 Mon Sep 17 00:00:00 2001 From: KevFan <chfan@redhat.com> Date: Fri, 31 Jan 2025 10:07:10 +0000 Subject: [PATCH 03/12] fixup: rename apiKeySelector -> defaultAPIKeySelector Signed-off-by: KevFan <chfan@redhat.com> --- pkg/evaluators/identity/api_key.go | 4 ++-- pkg/evaluators/identity/api_key_test.go | 8 ++++---- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/pkg/evaluators/identity/api_key.go b/pkg/evaluators/identity/api_key.go index 9bdb4070..10d53a0e 100644 --- a/pkg/evaluators/identity/api_key.go +++ b/pkg/evaluators/identity/api_key.go @@ -15,7 +15,7 @@ import ( ) const ( - apiKeySelector = "api_key" + defaultAPIKeySelector = "api_key" invalidApiKeyMsg = "the API Key provided is invalid" credentialsFetchingErrorMsg = "Something went wrong fetching the authorized credentials" ) @@ -39,7 +39,7 @@ func NewApiKeyIdentity(name string, labelSelectors k8s_labels.Selector, namespac Name: name, LabelSelectors: labelSelectors, Namespace: namespace, - KeySelectors: append(keySelectors, apiKeySelector), + KeySelectors: append(keySelectors, defaultAPIKeySelector), secrets: make(map[string]k8s.Secret), k8sClient: k8sClient, } diff --git a/pkg/evaluators/identity/api_key_test.go b/pkg/evaluators/identity/api_key_test.go index 2d50f50b..66cfc0ee 100644 --- a/pkg/evaluators/identity/api_key_test.go +++ b/pkg/evaluators/identity/api_key_test.go @@ -23,7 +23,7 @@ var ( ) func TestConstants(t *testing.T) { - assert.Equal(t, apiKeySelector, "api_key") + assert.Equal(t, defaultAPIKeySelector, "api_key") assert.Equal(t, invalidApiKeyMsg, "the API Key provided is invalid") } @@ -38,7 +38,7 @@ func TestNewApiKeyIdentityAllNamespaces(t *testing.T) { assert.Equal(t, apiKey.LabelSelectors.String(), "planet=coruscant") assert.Equal(t, apiKey.Namespace, "") assert.Equal(t, len(apiKey.KeySelectors), 1) - assert.Equal(t, apiKey.KeySelectors[0], apiKeySelector) + assert.Equal(t, apiKey.KeySelectors[0], defaultAPIKeySelector) assert.Equal(t, len(apiKey.secrets), 2) _, exists := apiKey.secrets["ObiWanKenobiLightSaber"] assert.Check(t, exists) @@ -59,7 +59,7 @@ func TestNewApiKeyIdentitySingleNamespace(t *testing.T) { assert.Equal(t, apiKey.LabelSelectors.String(), "planet=coruscant") assert.Equal(t, apiKey.Namespace, "ns1") assert.Equal(t, len(apiKey.KeySelectors), 1) - assert.Equal(t, apiKey.KeySelectors[0], apiKeySelector) + assert.Equal(t, apiKey.KeySelectors[0], defaultAPIKeySelector) assert.Equal(t, len(apiKey.secrets), 1) _, exists := apiKey.secrets["ObiWanKenobiLightSaber"] assert.Check(t, exists) @@ -81,7 +81,7 @@ func TestNewApiKeyIdentityMultipleKeySelectors(t *testing.T) { assert.Equal(t, apiKey.Namespace, "ns1") assert.Equal(t, len(apiKey.KeySelectors), 2) assert.Equal(t, apiKey.KeySelectors[0], "no_op") - assert.Equal(t, apiKey.KeySelectors[1], apiKeySelector) + assert.Equal(t, apiKey.KeySelectors[1], defaultAPIKeySelector) assert.Equal(t, len(apiKey.secrets), 1) _, exists := apiKey.secrets["ObiWanKenobiLightSaber"] assert.Check(t, exists) From 07048ae75f1a8d745f7e8d431c98044b46c79904 Mon Sep 17 00:00:00 2001 From: KevFan <chfan@redhat.com> Date: Tue, 4 Feb 2025 15:05:11 +0000 Subject: [PATCH 04/12] fixup: store all api keys for all defined key selectors Signed-off-by: KevFan <chfan@redhat.com> --- pkg/evaluators/identity/api_key.go | 29 ++++++++++++----------------- 1 file changed, 12 insertions(+), 17 deletions(-) diff --git a/pkg/evaluators/identity/api_key.go b/pkg/evaluators/identity/api_key.go index 10d53a0e..025c70ba 100644 --- a/pkg/evaluators/identity/api_key.go +++ b/pkg/evaluators/identity/api_key.go @@ -28,6 +28,7 @@ type APIKey struct { Namespace string `yaml:"namespace"` KeySelectors []string `yaml:"keySelectors"` + // Map of API Key value to secret secrets map[string]k8s.Secret mutex sync.RWMutex k8sClient k8s_client.Reader @@ -94,7 +95,6 @@ func (a *APIKey) GetK8sSecretLabelSelectors() k8s_labels.Selector { return a.LabelSelectors } -// Caution! This function is not thread-safe. Make sure to acquire a lock before calling it. func (a *APIKey) AddK8sSecretBasedIdentity(ctx context.Context, new k8s.Secret) { if !a.withinScope(new.GetNamespace()) { return @@ -105,23 +105,16 @@ func (a *APIKey) AddK8sSecretBasedIdentity(ctx context.Context, new k8s.Secret) logger := log.FromContext(ctx).WithName("apikey") - // updating existing - for _, key := range a.KeySelectors { - newAPIKeyValue := string(new.Data[key]) - for oldAPIKeyValue, current := range a.secrets { - if current.GetNamespace() == new.GetNamespace() && current.GetName() == new.GetName() { - if oldAPIKeyValue != newAPIKeyValue { - a.appendK8sSecretBasedIdentity(new) - delete(a.secrets, oldAPIKeyValue) - logger.V(1).Info("api key updated") - } else { - logger.V(1).Info("api key unchanged") - } - return - } + // Remove existing entries that match namespace/name, regardless of key value. + // This ensures old key values are removed. + for oldAPIKeyValue, current := range a.secrets { + if current.GetNamespace() == new.GetNamespace() && current.GetName() == new.GetName() { + delete(a.secrets, oldAPIKeyValue) + logger.V(1).Info("api key removed (replaced or updated)") } } + // Add the new secret. if a.appendK8sSecretBasedIdentity(new) { logger.V(1).Info("api key added") } @@ -151,13 +144,15 @@ func (a *APIKey) withinScope(namespace string) bool { // Appends the K8s Secret to the cache of API keys // Caution! This function is not thread-safe. Make sure to acquire a lock before calling it. func (a *APIKey) appendK8sSecretBasedIdentity(secret k8s.Secret) bool { + appended := false for _, key := range a.KeySelectors { value, isAPIKeySecret := secret.Data[key] + if isAPIKeySecret && len(value) > 0 { a.secrets[string(value)] = secret - return true + appended = true } } - return false + return appended } From 44925628f3753bf15504da04c756a25adb26f6e2 Mon Sep 17 00:00:00 2001 From: KevFan <chfan@redhat.com> Date: Wed, 5 Feb 2025 10:50:23 +0000 Subject: [PATCH 05/12] refactor: loading all matched api key values Signed-off-by: KevFan <chfan@redhat.com> --- go.mod | 3 +- go.sum | 6 ++-- pkg/evaluators/identity/api_key.go | 49 +++++++++++++++++++++--------- 3 files changed, 41 insertions(+), 17 deletions(-) diff --git a/go.mod b/go.mod index 3b9d59d5..0a4d2d50 100644 --- a/go.mod +++ b/go.mod @@ -19,6 +19,7 @@ require ( github.com/hashicorp/go-multierror v1.1.1 github.com/open-policy-agent/opa v0.68.0 github.com/prometheus/client_golang v1.20.2 + github.com/samber/lo v1.49.1 github.com/spf13/cobra v1.8.1 github.com/spf13/pflag v1.0.5 github.com/tidwall/gjson v1.14.0 @@ -115,7 +116,7 @@ require ( github.com/rcrowley/go-metrics v0.0.0-20200313005456-10cdbea86bc0 // indirect github.com/sirupsen/logrus v1.9.3 // indirect github.com/spf13/cast v1.6.0 // indirect - github.com/stretchr/testify v1.9.0 + github.com/stretchr/testify v1.10.0 github.com/tidwall/match v1.1.1 // indirect github.com/tidwall/pretty v1.2.0 // indirect github.com/xeipuuv/gojsonpointer v0.0.0-20190905194746-02993c407bfb // indirect diff --git a/go.sum b/go.sum index 97e247af..8b33233e 100644 --- a/go.sum +++ b/go.sum @@ -483,6 +483,8 @@ github.com/rogpeppe/go-internal v1.12.0/go.mod h1:E+RYuTGaKKdloAfM02xzb0FW3Paa99 github.com/russross/blackfriday/v2 v2.0.1/go.mod h1:+Rmxgy9KzJVeS9/2gXHxylqXiyQDYRxCVz55jmeOWTM= github.com/russross/blackfriday/v2 v2.1.0/go.mod h1:+Rmxgy9KzJVeS9/2gXHxylqXiyQDYRxCVz55jmeOWTM= github.com/ryanuber/columnize v0.0.0-20160712163229-9b3edd62028f/go.mod h1:sm1tb6uqfes/u+d4ooFouqFdy9/2g9QGwK3SQygK0Ts= +github.com/samber/lo v1.49.1 h1:4BIFyVfuQSEpluc7Fua+j1NolZHiEHEpaSEKdsH0tew= +github.com/samber/lo v1.49.1/go.mod h1:dO6KHFzUKXgP8LDhU0oI8d2hekjXnGOu0DB8Jecxd6o= github.com/samuel/go-zookeeper v0.0.0-20190923202752-2cc03de413da/go.mod h1:gi+0XIa01GRL2eRQVjQkKGqKF3SF9vZR/HnPullcV2E= github.com/sean-/seed v0.0.0-20170313163322-e2103e2c3529/go.mod h1:DxrIzT+xaE7yg65j358z/aeFdxmN0P9QXhEzd20vsDc= github.com/shurcooL/sanitized_anchor_name v1.0.0/go.mod h1:1NzhyTcUVG4SuEtjjoZeVRXNmyL/1OwPU0+IJeTBvfc= @@ -529,8 +531,8 @@ github.com/stretchr/testify v1.7.0/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/ github.com/stretchr/testify v1.7.1/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg= github.com/stretchr/testify v1.8.0/go.mod h1:yNjHg4UonilssWZ8iaSj1OCr/vHnekPRkoO+kdMU+MU= github.com/stretchr/testify v1.8.1/go.mod h1:w2LPCIKwWwSfY2zedu0+kehJoqGctiVI29o6fzry7u4= -github.com/stretchr/testify v1.9.0 h1:HtqpIVDClZ4nwg75+f6Lvsy/wHu+3BoSGCbBAcpTsTg= -github.com/stretchr/testify v1.9.0/go.mod h1:r2ic/lqez/lEtzL7wO/rwa5dbSLXVDPFyf8C91i36aY= +github.com/stretchr/testify v1.10.0 h1:Xv5erBjTwe/5IxqUQTdXv5kgmIvbHo3QQyRwhJsOfJA= +github.com/stretchr/testify v1.10.0/go.mod h1:r2ic/lqez/lEtzL7wO/rwa5dbSLXVDPFyf8C91i36aY= github.com/tchap/go-patricia/v2 v2.3.1 h1:6rQp39lgIYZ+MHmdEq4xzuk1t7OdC35z/xm0BGhTkes= github.com/tchap/go-patricia/v2 v2.3.1/go.mod h1:VZRHKAb53DLaG+nA9EaYYiaEx6YztwDlLElMsnSHD4k= github.com/tidwall/gjson v1.14.0 h1:6aeJ0bzojgWLa82gDQHcx3S0Lr/O51I9bJ5nv6JFx5w= diff --git a/pkg/evaluators/identity/api_key.go b/pkg/evaluators/identity/api_key.go index 025c70ba..91f78ffc 100644 --- a/pkg/evaluators/identity/api_key.go +++ b/pkg/evaluators/identity/api_key.go @@ -5,6 +5,8 @@ import ( "fmt" "sync" + "github.com/samber/lo" + "github.com/kuadrant/authorino/pkg/auth" "github.com/kuadrant/authorino/pkg/log" @@ -105,18 +107,28 @@ func (a *APIKey) AddK8sSecretBasedIdentity(ctx context.Context, new k8s.Secret) logger := log.FromContext(ctx).WithName("apikey") - // Remove existing entries that match namespace/name, regardless of key value. - // This ensures old key values are removed. - for oldAPIKeyValue, current := range a.secrets { - if current.GetNamespace() == new.GetNamespace() && current.GetName() == new.GetName() { - delete(a.secrets, oldAPIKeyValue) - logger.V(1).Info("api key removed (replaced or updated)") + // Get all current keys in the map that match the new secret name and namespace + currentKeysSecret := lo.PickBy(a.secrets, func(key string, current k8s.Secret) bool { + return current.GetNamespace() == new.GetNamespace() && current.GetName() == new.GetName() + }) + + // get api keys from new secret + newAPIKeys := a.getValuesFromSecret(new) + + for _, newKey := range newAPIKeys { + a.secrets[newKey] = new + if _, ok := currentKeysSecret[newKey]; !ok { + logger.V(1).Info("api key added") + } else { + logger.V(1).Info("api key secret updated") } } - // Add the new secret. - if a.appendK8sSecretBasedIdentity(new) { - logger.V(1).Info("api key added") + // get difference between new and the old + staleKeys, _ := lo.Difference(lo.Keys(currentKeysSecret), newAPIKeys) + for _, newKey := range staleKeys { + delete(a.secrets, newKey) + logger.V(1).Info("stale api key deleted") } } @@ -132,7 +144,6 @@ func (a *APIKey) RevokeK8sSecretBasedIdentity(ctx context.Context, deleted k8s_t if secret.GetNamespace() == deleted.Namespace && secret.GetName() == deleted.Name { delete(a.secrets, key) log.FromContext(ctx).WithName("apikey").V(1).Info("api key deleted") - return } } } @@ -144,15 +155,25 @@ func (a *APIKey) withinScope(namespace string) bool { // Appends the K8s Secret to the cache of API keys // Caution! This function is not thread-safe. Make sure to acquire a lock before calling it. func (a *APIKey) appendK8sSecretBasedIdentity(secret k8s.Secret) bool { - appended := false + values := a.getValuesFromSecret(secret) + for _, value := range values { + a.secrets[value] = secret + } + + // Was appended if length is greater than zero + return len(values) != 0 +} + +// getValuesFromSecret extracts the values from the secret based on APIKey KeySelectors +func (a *APIKey) getValuesFromSecret(secret k8s.Secret) []string { + var keys []string for _, key := range a.KeySelectors { value, isAPIKeySecret := secret.Data[key] if isAPIKeySecret && len(value) > 0 { - a.secrets[string(value)] = secret - appended = true + keys = append(keys, string(value)) } } - return appended + return keys } From 071259c017d92428164ae2748373d9383c5e68c7 Mon Sep 17 00:00:00 2001 From: KevFan <chfan@redhat.com> Date: Wed, 5 Feb 2025 11:09:05 +0000 Subject: [PATCH 06/12] fixup: KeySelectors description Signed-off-by: KevFan <chfan@redhat.com> --- api/v1beta3/auth_config_types.go | 4 ++-- install/crd/authorino.kuadrant.io_authconfigs.yaml | 4 ++-- install/manifests.yaml | 4 ++-- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/api/v1beta3/auth_config_types.go b/api/v1beta3/auth_config_types.go index 75050779..210e51bf 100644 --- a/api/v1beta3/auth_config_types.go +++ b/api/v1beta3/auth_config_types.go @@ -357,8 +357,8 @@ type ApiKeyAuthenticationSpec struct { AllNamespaces bool `json:"allNamespaces,omitempty"` // List of keys within the selected Kubernetes secret that contain valid API credentials. - // Authorino will attempt to authenticate using the first key that matches. - // If no match is found, authentication will fail. + // Authorino will attempt to authenticate using any matching key, including "api-key". + // If no match is found, the Kubernetes secret is not considered a valid Authorino API Key secret and is ignored. // +optional KeySelectors []string `json:"keySelectors,omitempty"` } diff --git a/install/crd/authorino.kuadrant.io_authconfigs.yaml b/install/crd/authorino.kuadrant.io_authconfigs.yaml index 731111f1..28667ce4 100644 --- a/install/crd/authorino.kuadrant.io_authconfigs.yaml +++ b/install/crd/authorino.kuadrant.io_authconfigs.yaml @@ -2396,8 +2396,8 @@ spec: keySelectors: description: |- List of keys within the selected Kubernetes secret that contain valid API credentials. - Authorino will attempt to authenticate using the first key that matches. - If no match is found, authentication will fail. + Authorino will attempt to authenticate using any matching key, including "api-key". + If no match is found, the Kubernetes secret is not considered a valid Authorino API Key secret and is ignored. items: type: string type: array diff --git a/install/manifests.yaml b/install/manifests.yaml index dd27a1d7..3b72cfb0 100644 --- a/install/manifests.yaml +++ b/install/manifests.yaml @@ -2663,8 +2663,8 @@ spec: keySelectors: description: |- List of keys within the selected Kubernetes secret that contain valid API credentials. - Authorino will attempt to authenticate using the first key that matches. - If no match is found, authentication will fail. + Authorino will attempt to authenticate using any matching key, including "api-key". + If no match is found, the Kubernetes secret is not considered a valid Authorino API Key secret and is ignored. items: type: string type: array From 62b9d94b6fde686760a427ee3c24cc1a626143ea Mon Sep 17 00:00:00 2001 From: KevFan <chfan@redhat.com> Date: Wed, 5 Feb 2025 12:21:57 +0000 Subject: [PATCH 07/12] test: improve unit test Signed-off-by: KevFan <chfan@redhat.com> --- pkg/evaluators/identity/api_key_test.go | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/pkg/evaluators/identity/api_key_test.go b/pkg/evaluators/identity/api_key_test.go index 66cfc0ee..6d71611b 100644 --- a/pkg/evaluators/identity/api_key_test.go +++ b/pkg/evaluators/identity/api_key_test.go @@ -16,7 +16,7 @@ import ( ) var ( - testAPIKeyK8sSecret1 = &k8s.Secret{ObjectMeta: k8s_meta.ObjectMeta{Name: "obi-wan", Namespace: "ns1", Labels: map[string]string{"planet": "coruscant"}}, Data: map[string][]byte{"api_key": []byte("ObiWanKenobiLightSaber")}} + testAPIKeyK8sSecret1 = &k8s.Secret{ObjectMeta: k8s_meta.ObjectMeta{Name: "obi-wan", Namespace: "ns1", Labels: map[string]string{"planet": "coruscant"}}, Data: map[string][]byte{"api_key": []byte("ObiWanKenobiLightSaber"), "api_key_2": []byte("TeraSinubeLightSaber")}} testAPIKeyK8sSecret2 = &k8s.Secret{ObjectMeta: k8s_meta.ObjectMeta{Name: "yoda", Namespace: "ns2", Labels: map[string]string{"planet": "coruscant"}}, Data: map[string][]byte{"api_key": []byte("MasterYodaLightSaber")}} testAPIKeyK8sSecret3 = &k8s.Secret{ObjectMeta: k8s_meta.ObjectMeta{Name: "anakin", Namespace: "ns2", Labels: map[string]string{"planet": "tatooine"}}, Data: map[string][]byte{"api_key": []byte("AnakinSkywalkerLightSaber")}} testAPIKeyK8sClient = mockK8sClient(testAPIKeyK8sSecret1, testAPIKeyK8sSecret2, testAPIKeyK8sSecret3) @@ -74,17 +74,20 @@ func TestNewApiKeyIdentityMultipleKeySelectors(t *testing.T) { defer ctrl.Finish() selector, _ := k8s_labels.Parse("planet=coruscant") - apiKey := NewApiKeyIdentity("jedi", selector, "ns1", []string{"no_op"}, mock_auth.NewMockAuthCredentials(ctrl), testAPIKeyK8sClient, context.TODO()) + apiKey := NewApiKeyIdentity("jedi", selector, "ns1", []string{"no_op", "api_key_2"}, mock_auth.NewMockAuthCredentials(ctrl), testAPIKeyK8sClient, context.TODO()) assert.Equal(t, apiKey.Name, "jedi") assert.Equal(t, apiKey.LabelSelectors.String(), "planet=coruscant") assert.Equal(t, apiKey.Namespace, "ns1") - assert.Equal(t, len(apiKey.KeySelectors), 2) + assert.Equal(t, len(apiKey.KeySelectors), 3) assert.Equal(t, apiKey.KeySelectors[0], "no_op") - assert.Equal(t, apiKey.KeySelectors[1], defaultAPIKeySelector) - assert.Equal(t, len(apiKey.secrets), 1) + assert.Equal(t, apiKey.KeySelectors[1], "api_key_2") + assert.Equal(t, apiKey.KeySelectors[2], defaultAPIKeySelector) + assert.Equal(t, len(apiKey.secrets), 2) _, exists := apiKey.secrets["ObiWanKenobiLightSaber"] assert.Check(t, exists) + _, exists = apiKey.secrets["TeraSinubeLightSaber"] + assert.Check(t, exists) _, exists = apiKey.secrets["MasterYodaLightSaber"] assert.Check(t, !exists) _, exists = apiKey.secrets["AnakinSkywalkerLightSaber"] From 5e7120bfbe05170958fa45dc1e91453c9670844d Mon Sep 17 00:00:00 2001 From: KevFan <chfan@redhat.com> Date: Wed, 5 Feb 2025 14:55:56 +0000 Subject: [PATCH 08/12] refactor: appended default key selector only when list is not defined Signed-off-by: KevFan <chfan@redhat.com> --- api/v1beta3/auth_config_types.go | 2 +- install/crd/authorino.kuadrant.io_authconfigs.yaml | 2 +- install/manifests.yaml | 2 +- pkg/evaluators/identity/api_key.go | 5 ++++- pkg/evaluators/identity/api_key_test.go | 7 +++---- 5 files changed, 10 insertions(+), 8 deletions(-) diff --git a/api/v1beta3/auth_config_types.go b/api/v1beta3/auth_config_types.go index 210e51bf..b1725a2a 100644 --- a/api/v1beta3/auth_config_types.go +++ b/api/v1beta3/auth_config_types.go @@ -357,7 +357,7 @@ type ApiKeyAuthenticationSpec struct { AllNamespaces bool `json:"allNamespaces,omitempty"` // List of keys within the selected Kubernetes secret that contain valid API credentials. - // Authorino will attempt to authenticate using any matching key, including "api-key". + // Authorino will attempt to authenticate using any matching key. If no keys are defined, the default "api-key" will be used. // If no match is found, the Kubernetes secret is not considered a valid Authorino API Key secret and is ignored. // +optional KeySelectors []string `json:"keySelectors,omitempty"` diff --git a/install/crd/authorino.kuadrant.io_authconfigs.yaml b/install/crd/authorino.kuadrant.io_authconfigs.yaml index 28667ce4..4ff05c49 100644 --- a/install/crd/authorino.kuadrant.io_authconfigs.yaml +++ b/install/crd/authorino.kuadrant.io_authconfigs.yaml @@ -2396,7 +2396,7 @@ spec: keySelectors: description: |- List of keys within the selected Kubernetes secret that contain valid API credentials. - Authorino will attempt to authenticate using any matching key, including "api-key". + Authorino will attempt to authenticate using any matching key. If no keys are defined, the default "api-key" will be used. If no match is found, the Kubernetes secret is not considered a valid Authorino API Key secret and is ignored. items: type: string diff --git a/install/manifests.yaml b/install/manifests.yaml index 3b72cfb0..d249fec3 100644 --- a/install/manifests.yaml +++ b/install/manifests.yaml @@ -2663,7 +2663,7 @@ spec: keySelectors: description: |- List of keys within the selected Kubernetes secret that contain valid API credentials. - Authorino will attempt to authenticate using any matching key, including "api-key". + Authorino will attempt to authenticate using any matching key. If no keys are defined, the default "api-key" will be used. If no match is found, the Kubernetes secret is not considered a valid Authorino API Key secret and is ignored. items: type: string diff --git a/pkg/evaluators/identity/api_key.go b/pkg/evaluators/identity/api_key.go index 91f78ffc..61472c15 100644 --- a/pkg/evaluators/identity/api_key.go +++ b/pkg/evaluators/identity/api_key.go @@ -37,12 +37,15 @@ type APIKey struct { } func NewApiKeyIdentity(name string, labelSelectors k8s_labels.Selector, namespace string, keySelectors []string, authCred auth.AuthCredentials, k8sClient k8s_client.Reader, ctx context.Context) *APIKey { + if len(keySelectors) == 0 { + keySelectors = append(keySelectors, defaultAPIKeySelector) + } apiKey := &APIKey{ AuthCredentials: authCred, Name: name, LabelSelectors: labelSelectors, Namespace: namespace, - KeySelectors: append(keySelectors, defaultAPIKeySelector), + KeySelectors: keySelectors, secrets: make(map[string]k8s.Secret), k8sClient: k8sClient, } diff --git a/pkg/evaluators/identity/api_key_test.go b/pkg/evaluators/identity/api_key_test.go index 6d71611b..ca2ada32 100644 --- a/pkg/evaluators/identity/api_key_test.go +++ b/pkg/evaluators/identity/api_key_test.go @@ -79,13 +79,12 @@ func TestNewApiKeyIdentityMultipleKeySelectors(t *testing.T) { assert.Equal(t, apiKey.Name, "jedi") assert.Equal(t, apiKey.LabelSelectors.String(), "planet=coruscant") assert.Equal(t, apiKey.Namespace, "ns1") - assert.Equal(t, len(apiKey.KeySelectors), 3) + assert.Equal(t, len(apiKey.KeySelectors), 2) assert.Equal(t, apiKey.KeySelectors[0], "no_op") assert.Equal(t, apiKey.KeySelectors[1], "api_key_2") - assert.Equal(t, apiKey.KeySelectors[2], defaultAPIKeySelector) - assert.Equal(t, len(apiKey.secrets), 2) + assert.Equal(t, len(apiKey.secrets), 1) _, exists := apiKey.secrets["ObiWanKenobiLightSaber"] - assert.Check(t, exists) + assert.Check(t, !exists) _, exists = apiKey.secrets["TeraSinubeLightSaber"] assert.Check(t, exists) _, exists = apiKey.secrets["MasterYodaLightSaber"] From ec85c100a9f4733c7c2a2bc25fad96e9c2b104d3 Mon Sep 17 00:00:00 2001 From: KevFan <chfan@redhat.com> Date: Fri, 7 Feb 2025 09:08:15 +0000 Subject: [PATCH 09/12] refactor: CEL expression for keySelector Signed-off-by: KevFan <chfan@redhat.com> --- api/v1beta3/auth_config_types.go | 7 +- api/v1beta3/zz_generated.deepcopy.go | 5 - controllers/auth_config_controller.go | 3 +- controllers/secret_controller_test.go | 2 +- .../authorino.kuadrant.io_authconfigs.yaml | 11 +- install/manifests.yaml | 11 +- pkg/evaluators/identity/api_key.go | 125 +++++++++++++----- pkg/evaluators/identity/api_key_test.go | 27 ++-- pkg/expressions/cel/expressions.go | 51 ++++++- 9 files changed, 168 insertions(+), 74 deletions(-) diff --git a/api/v1beta3/auth_config_types.go b/api/v1beta3/auth_config_types.go index b1725a2a..cb5eba2f 100644 --- a/api/v1beta3/auth_config_types.go +++ b/api/v1beta3/auth_config_types.go @@ -356,11 +356,14 @@ type ApiKeyAuthenticationSpec struct { // +kubebuilder:default:=false AllNamespaces bool `json:"allNamespaces,omitempty"` - // List of keys within the selected Kubernetes secret that contain valid API credentials. + // A Common Expression Language (CEL) expression that evaluates to a list of string keys within the selected Kubernetes + // secret that contain valid API credentials. The keys of the selected Kubernetes secret are available for evaluation + // in the following structure: `{"keys": ["key1", "key2"]}`. // Authorino will attempt to authenticate using any matching key. If no keys are defined, the default "api-key" will be used. // If no match is found, the Kubernetes secret is not considered a valid Authorino API Key secret and is ignored. + // String expressions are supported (https://pkg.go.dev/github.com/google/cel-go/ext#Strings). // +optional - KeySelectors []string `json:"keySelectors,omitempty"` + KeySelector CelExpression `json:"keySelector,omitempty"` } // Settings to fetch the JSON Web Key Set (JWKS) for the JWT authentication. diff --git a/api/v1beta3/zz_generated.deepcopy.go b/api/v1beta3/zz_generated.deepcopy.go index 84cb1a70..382a3ad7 100644 --- a/api/v1beta3/zz_generated.deepcopy.go +++ b/api/v1beta3/zz_generated.deepcopy.go @@ -49,11 +49,6 @@ func (in *ApiKeyAuthenticationSpec) DeepCopyInto(out *ApiKeyAuthenticationSpec) *out = new(v1.LabelSelector) (*in).DeepCopyInto(*out) } - if in.KeySelectors != nil { - in, out := &in.KeySelectors, &out.KeySelectors - *out = make([]string, len(*in)) - copy(*out, *in) - } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new ApiKeyAuthenticationSpec. diff --git a/controllers/auth_config_controller.go b/controllers/auth_config_controller.go index 85c59dfc..ecfed6ae 100644 --- a/controllers/auth_config_controller.go +++ b/controllers/auth_config_controller.go @@ -261,7 +261,8 @@ func (r *AuthConfigReconciler) translateAuthConfig(ctx context.Context, authConf if err != nil { return nil, err } - translatedIdentity.APIKey = identity_evaluators.NewApiKeyIdentity(identityCfgName, selector, namespace, identity.ApiKey.KeySelectors, authCred, r.Client, ctxWithLogger) + + translatedIdentity.APIKey = identity_evaluators.NewApiKeyIdentity(identityCfgName, selector, namespace, string(identity.ApiKey.KeySelector), authCred, r.Client, ctxWithLogger) // MTLS case api.X509ClientCertificateAuthentication: diff --git a/controllers/secret_controller_test.go b/controllers/secret_controller_test.go index 05351186..3f5904f4 100644 --- a/controllers/secret_controller_test.go +++ b/controllers/secret_controller_test.go @@ -94,7 +94,7 @@ func newSecretReconcilerTest(mockCtrl *gomock.Controller, secretLabels map[strin indexedAuthConfig := &evaluators.AuthConfig{ Labels: map[string]string{"namespace": "authorino", "name": "api-protection"}, IdentityConfigs: []auth.AuthConfigEvaluator{&fakeAPIKeyIdentityConfig{ - evaluator: identity_evaluators.NewApiKeyIdentity("api-key", apiKeyLabelSelectors, "", []string{}, auth.NewAuthCredential("", ""), fakeK8sClient, context.TODO()), + evaluator: identity_evaluators.NewApiKeyIdentity("api-key", apiKeyLabelSelectors, "", "", auth.NewAuthCredential("", ""), fakeK8sClient, context.TODO()), }}, } indexMock := mock_index.NewMockIndex(mockCtrl) diff --git a/install/crd/authorino.kuadrant.io_authconfigs.yaml b/install/crd/authorino.kuadrant.io_authconfigs.yaml index 4ff05c49..b409f58f 100644 --- a/install/crd/authorino.kuadrant.io_authconfigs.yaml +++ b/install/crd/authorino.kuadrant.io_authconfigs.yaml @@ -2393,14 +2393,15 @@ spec: Whether Authorino should look for API key secrets in all namespaces or only in the same namespace as the AuthConfig. Enabling this option in namespaced Authorino instances has no effect. type: boolean - keySelectors: + keySelector: description: |- - List of keys within the selected Kubernetes secret that contain valid API credentials. + A Common Expression Language (CEL) expression that evaluates to a list of string keys within the selected Kubernetes + secret that contain valid API credentials. The keys of the selected Kubernetes secret are available for evaluation + in the following structure: `{"keys": ["key1", "key2"]}`. Authorino will attempt to authenticate using any matching key. If no keys are defined, the default "api-key" will be used. If no match is found, the Kubernetes secret is not considered a valid Authorino API Key secret and is ignored. - items: - type: string - type: array + String expressions are supported (https://pkg.go.dev/github.com/google/cel-go/ext#Strings). + type: string selector: description: Label selector used by Authorino to match secrets from the cluster storing valid credentials to authenticate diff --git a/install/manifests.yaml b/install/manifests.yaml index d249fec3..9b657f4a 100644 --- a/install/manifests.yaml +++ b/install/manifests.yaml @@ -2660,14 +2660,15 @@ spec: Whether Authorino should look for API key secrets in all namespaces or only in the same namespace as the AuthConfig. Enabling this option in namespaced Authorino instances has no effect. type: boolean - keySelectors: + keySelector: description: |- - List of keys within the selected Kubernetes secret that contain valid API credentials. + A Common Expression Language (CEL) expression that evaluates to a list of string keys within the selected Kubernetes + secret that contain valid API credentials. The keys of the selected Kubernetes secret are available for evaluation + in the following structure: `{"keys": ["key1", "key2"]}`. Authorino will attempt to authenticate using any matching key. If no keys are defined, the default "api-key" will be used. If no match is found, the Kubernetes secret is not considered a valid Authorino API Key secret and is ignored. - items: - type: string - type: array + String expressions are supported (https://pkg.go.dev/github.com/google/cel-go/ext#Strings). + type: string selector: description: Label selector used by Authorino to match secrets from the cluster storing valid credentials to authenticate diff --git a/pkg/evaluators/identity/api_key.go b/pkg/evaluators/identity/api_key.go index 61472c15..13de4c77 100644 --- a/pkg/evaluators/identity/api_key.go +++ b/pkg/evaluators/identity/api_key.go @@ -2,12 +2,17 @@ package identity import ( "context" + "encoding/json" "fmt" "sync" + "github.com/google/cel-go/common/types" + "github.com/google/cel-go/common/types/ref" "github.com/samber/lo" "github.com/kuadrant/authorino/pkg/auth" + "github.com/kuadrant/authorino/pkg/expressions" + "github.com/kuadrant/authorino/pkg/expressions/cel" "github.com/kuadrant/authorino/pkg/log" k8s "k8s.io/api/core/v1" @@ -17,18 +22,18 @@ import ( ) const ( - defaultAPIKeySelector = "api_key" - invalidApiKeyMsg = "the API Key provided is invalid" - credentialsFetchingErrorMsg = "Something went wrong fetching the authorized credentials" + defaultKeySelectorExpression = `['api_key']` + invalidApiKeyMsg = "the API Key provided is invalid" + credentialsFetchingErrorMsg = "Something went wrong fetching the authorized credentials" ) type APIKey struct { auth.AuthCredentials - Name string `yaml:"name"` - LabelSelectors k8s_labels.Selector `yaml:"labelSelectors"` - Namespace string `yaml:"namespace"` - KeySelectors []string `yaml:"keySelectors"` + Name string `yaml:"name"` + LabelSelectors k8s_labels.Selector `yaml:"labelSelectors"` + Namespace string `yaml:"namespace"` + KeySelectorExpression expressions.Value `yaml:"keySelector"` // Map of API Key value to secret secrets map[string]k8s.Secret @@ -36,21 +41,30 @@ type APIKey struct { k8sClient k8s_client.Reader } -func NewApiKeyIdentity(name string, labelSelectors k8s_labels.Selector, namespace string, keySelectors []string, authCred auth.AuthCredentials, k8sClient k8s_client.Reader, ctx context.Context) *APIKey { - if len(keySelectors) == 0 { - keySelectors = append(keySelectors, defaultAPIKeySelector) +func NewApiKeyIdentity(name string, labelSelectors k8s_labels.Selector, namespace string, keySelectorExpression string, authCred auth.AuthCredentials, k8sClient k8s_client.Reader, ctx context.Context) *APIKey { + if keySelectorExpression == "" { + keySelectorExpression = defaultKeySelectorExpression } + + logger := log.FromContext(ctx).WithName("apikey") + + expr, err := cel.NewKeySelectorExpression(keySelectorExpression) + if err != nil { + logger.Error(err, "failed to create key selector expression") + return nil + } + apiKey := &APIKey{ - AuthCredentials: authCred, - Name: name, - LabelSelectors: labelSelectors, - Namespace: namespace, - KeySelectors: keySelectors, - secrets: make(map[string]k8s.Secret), - k8sClient: k8sClient, + AuthCredentials: authCred, + Name: name, + LabelSelectors: labelSelectors, + Namespace: namespace, + KeySelectorExpression: expr, + secrets: make(map[string]k8s.Secret), + k8sClient: k8sClient, } if err := apiKey.loadSecrets(context.TODO()); err != nil { - log.FromContext(ctx).WithName("apikey").Error(err, credentialsFetchingErrorMsg) + logger.Error(err, credentialsFetchingErrorMsg) } return apiKey } @@ -70,7 +84,7 @@ func (a *APIKey) loadSecrets(ctx context.Context) error { defer a.mutex.Unlock() for _, secret := range secretList.Items { - a.appendK8sSecretBasedIdentity(secret) + a.appendK8sSecretBasedIdentity(ctx, secret) } return nil @@ -105,9 +119,6 @@ func (a *APIKey) AddK8sSecretBasedIdentity(ctx context.Context, new k8s.Secret) return } - a.mutex.Lock() - defer a.mutex.Unlock() - logger := log.FromContext(ctx).WithName("apikey") // Get all current keys in the map that match the new secret name and namespace @@ -116,7 +127,10 @@ func (a *APIKey) AddK8sSecretBasedIdentity(ctx context.Context, new k8s.Secret) }) // get api keys from new secret - newAPIKeys := a.getValuesFromSecret(new) + newAPIKeys := a.getValuesFromSecret(ctx, new) + + a.mutex.Lock() + defer a.mutex.Unlock() for _, newKey := range newAPIKeys { a.secrets[newKey] = new @@ -157,8 +171,8 @@ func (a *APIKey) withinScope(namespace string) bool { // Appends the K8s Secret to the cache of API keys // Caution! This function is not thread-safe. Make sure to acquire a lock before calling it. -func (a *APIKey) appendK8sSecretBasedIdentity(secret k8s.Secret) bool { - values := a.getValuesFromSecret(secret) +func (a *APIKey) appendK8sSecretBasedIdentity(ctx context.Context, secret k8s.Secret) bool { + values := a.getValuesFromSecret(ctx, secret) for _, value := range values { a.secrets[value] = secret } @@ -167,16 +181,61 @@ func (a *APIKey) appendK8sSecretBasedIdentity(secret k8s.Secret) bool { return len(values) != 0 } -// getValuesFromSecret extracts the values from the secret based on APIKey KeySelectors -func (a *APIKey) getValuesFromSecret(secret k8s.Secret) []string { - var keys []string - for _, key := range a.KeySelectors { - value, isAPIKeySecret := secret.Data[key] +// getValuesFromSecret extracts the values from the secret based on APIKey KeySelector expression +func (a *APIKey) getValuesFromSecret(ctx context.Context, secret k8s.Secret) []string { + logger := log.FromContext(ctx).WithName("apikey") + + // Extract secret keys + secretKeys := lo.Keys(secret.Data) - if isAPIKeySecret && len(value) > 0 { - keys = append(keys, string(value)) + // Prepare JSON for CEL evaluation + jsonBytes, err := json.Marshal(map[string][]string{cel.RootSecretKeysBinding: secretKeys}) + if err != nil { + logger.Error(err, "failed to marshal secret keys to JSON") + return nil + } + + // Evaluate CEL expression + evaluated, err := a.KeySelectorExpression.ResolveFor(string(jsonBytes)) + if err != nil { + logger.Error(err, "failed to resolve key selector expression") + return nil + } + + // Convert evaluated result to a slice of strings + selectedKeys, ok := convertToStringSlice(evaluated) + if !ok { + logger.Error(fmt.Errorf("unexpected type for resolved key"), "expected string or []string", "value", evaluated) + return nil + } + + // Extract values for the selected keys + values := make([]string, 0, len(selectedKeys)) + for _, key := range selectedKeys { + if v, exists := secret.Data[key]; exists && len(v) > 0 { + values = append(values, string(v)) + } + } + + return values +} + +// Helper function to safely convert an interface{} of type []ref.Val to []string +func convertToStringSlice(value any) ([]string, bool) { + items, ok := value.([]ref.Val) + if !ok { + return nil, false + } + + out := make([]string, len(items)) + for i, item := range items { + if item.Type() == types.StringType { + out[i] = item.Value().(string) + } else { + // unexpected type + return nil, false } } - return keys + return out, true } diff --git a/pkg/evaluators/identity/api_key_test.go b/pkg/evaluators/identity/api_key_test.go index ca2ada32..629d7a46 100644 --- a/pkg/evaluators/identity/api_key_test.go +++ b/pkg/evaluators/identity/api_key_test.go @@ -23,7 +23,7 @@ var ( ) func TestConstants(t *testing.T) { - assert.Equal(t, defaultAPIKeySelector, "api_key") + assert.Equal(t, defaultKeySelectorExpression, `['api_key']`) assert.Equal(t, invalidApiKeyMsg, "the API Key provided is invalid") } @@ -32,13 +32,11 @@ func TestNewApiKeyIdentityAllNamespaces(t *testing.T) { defer ctrl.Finish() selector, _ := k8s_labels.Parse("planet=coruscant") - apiKey := NewApiKeyIdentity("jedi", selector, "", []string{}, mock_auth.NewMockAuthCredentials(ctrl), testAPIKeyK8sClient, context.TODO()) + apiKey := NewApiKeyIdentity("jedi", selector, "", "", mock_auth.NewMockAuthCredentials(ctrl), testAPIKeyK8sClient, context.TODO()) assert.Equal(t, apiKey.Name, "jedi") assert.Equal(t, apiKey.LabelSelectors.String(), "planet=coruscant") assert.Equal(t, apiKey.Namespace, "") - assert.Equal(t, len(apiKey.KeySelectors), 1) - assert.Equal(t, apiKey.KeySelectors[0], defaultAPIKeySelector) assert.Equal(t, len(apiKey.secrets), 2) _, exists := apiKey.secrets["ObiWanKenobiLightSaber"] assert.Check(t, exists) @@ -53,13 +51,11 @@ func TestNewApiKeyIdentitySingleNamespace(t *testing.T) { defer ctrl.Finish() selector, _ := k8s_labels.Parse("planet=coruscant") - apiKey := NewApiKeyIdentity("jedi", selector, "ns1", []string{}, mock_auth.NewMockAuthCredentials(ctrl), testAPIKeyK8sClient, context.TODO()) + apiKey := NewApiKeyIdentity("jedi", selector, "ns1", "", mock_auth.NewMockAuthCredentials(ctrl), testAPIKeyK8sClient, context.TODO()) assert.Equal(t, apiKey.Name, "jedi") assert.Equal(t, apiKey.LabelSelectors.String(), "planet=coruscant") assert.Equal(t, apiKey.Namespace, "ns1") - assert.Equal(t, len(apiKey.KeySelectors), 1) - assert.Equal(t, apiKey.KeySelectors[0], defaultAPIKeySelector) assert.Equal(t, len(apiKey.secrets), 1) _, exists := apiKey.secrets["ObiWanKenobiLightSaber"] assert.Check(t, exists) @@ -74,14 +70,11 @@ func TestNewApiKeyIdentityMultipleKeySelectors(t *testing.T) { defer ctrl.Finish() selector, _ := k8s_labels.Parse("planet=coruscant") - apiKey := NewApiKeyIdentity("jedi", selector, "ns1", []string{"no_op", "api_key_2"}, mock_auth.NewMockAuthCredentials(ctrl), testAPIKeyK8sClient, context.TODO()) + apiKey := NewApiKeyIdentity("jedi", selector, "ns1", "['no_op','api_key_2']", mock_auth.NewMockAuthCredentials(ctrl), testAPIKeyK8sClient, context.TODO()) assert.Equal(t, apiKey.Name, "jedi") assert.Equal(t, apiKey.LabelSelectors.String(), "planet=coruscant") assert.Equal(t, apiKey.Namespace, "ns1") - assert.Equal(t, len(apiKey.KeySelectors), 2) - assert.Equal(t, apiKey.KeySelectors[0], "no_op") - assert.Equal(t, apiKey.KeySelectors[1], "api_key_2") assert.Equal(t, len(apiKey.secrets), 1) _, exists := apiKey.secrets["ObiWanKenobiLightSaber"] assert.Check(t, !exists) @@ -102,7 +95,7 @@ func TestCallSuccess(t *testing.T) { authCredMock.EXPECT().GetCredentialsFromReq(gomock.Any()).Return("ObiWanKenobiLightSaber", nil) selector, _ := k8s_labels.Parse("planet=coruscant") - apiKey := NewApiKeyIdentity("jedi", selector, "", []string{}, authCredMock, testAPIKeyK8sClient, context.TODO()) + apiKey := NewApiKeyIdentity("jedi", selector, "", "", authCredMock, testAPIKeyK8sClient, context.TODO()) auth, err := apiKey.Call(pipelineMock, context.TODO()) assert.NilError(t, err) @@ -118,7 +111,7 @@ func TestCallNoApiKeyFail(t *testing.T) { authCredMock.EXPECT().GetCredentialsFromReq(gomock.Any()).Return("", fmt.Errorf("something went wrong getting the API Key")) selector, _ := k8s_labels.Parse("planet=coruscant") - apiKey := NewApiKeyIdentity("jedi", selector, "", []string{}, authCredMock, testAPIKeyK8sClient, context.TODO()) + apiKey := NewApiKeyIdentity("jedi", selector, "", "", authCredMock, testAPIKeyK8sClient, context.TODO()) _, err := apiKey.Call(pipelineMock, context.TODO()) @@ -134,7 +127,7 @@ func TestCallInvalidApiKeyFail(t *testing.T) { authCredMock.EXPECT().GetCredentialsFromReq(gomock.Any()).Return("ASithLightSaber", nil) selector, _ := k8s_labels.Parse("planet=coruscant") - apiKey := NewApiKeyIdentity("jedi", selector, "", []string{}, authCredMock, testAPIKeyK8sClient, context.TODO()) + apiKey := NewApiKeyIdentity("jedi", selector, "", "", authCredMock, testAPIKeyK8sClient, context.TODO()) _, err := apiKey.Call(pipelineMock, context.TODO()) assert.Error(t, err, "the API Key provided is invalid") @@ -142,7 +135,7 @@ func TestCallInvalidApiKeyFail(t *testing.T) { func TestLoadSecretsSuccess(t *testing.T) { selector, _ := k8s_labels.Parse("planet=coruscant") - apiKey := NewApiKeyIdentity("X-API-KEY", selector, "", []string{}, nil, testAPIKeyK8sClient, nil) + apiKey := NewApiKeyIdentity("X-API-KEY", selector, "", "", nil, testAPIKeyK8sClient, nil) err := apiKey.loadSecrets(context.TODO()) assert.NilError(t, err) @@ -159,7 +152,7 @@ func TestLoadSecretsSuccess(t *testing.T) { func TestLoadSecretsFail(t *testing.T) { selector, _ := k8s_labels.Parse("planet=coruscant") - apiKey := NewApiKeyIdentity("X-API-KEY", selector, "", []string{}, nil, &flawedAPIkeyK8sClient{}, context.TODO()) + apiKey := NewApiKeyIdentity("X-API-KEY", selector, "", "", nil, &flawedAPIkeyK8sClient{}, context.TODO()) err := apiKey.loadSecrets(context.TODO()) assert.Error(t, err, "something terribly wrong happened") @@ -173,7 +166,7 @@ func BenchmarkAPIKeyAuthn(b *testing.B) { authCredMock := mock_auth.NewMockAuthCredentials(ctrl) authCredMock.EXPECT().GetCredentialsFromReq(gomock.Any()).Return("ObiWanKenobiLightSaber", nil).MinTimes(1) selector, _ := k8s_labels.Parse("planet=coruscant") - apiKey := NewApiKeyIdentity("jedi", selector, "", []string{}, authCredMock, testAPIKeyK8sClient, context.TODO()) + apiKey := NewApiKeyIdentity("jedi", selector, "", "", authCredMock, testAPIKeyK8sClient, context.TODO()) var err error b.ResetTimer() diff --git a/pkg/expressions/cel/expressions.go b/pkg/expressions/cel/expressions.go index ca3e78da..de0a2c4e 100644 --- a/pkg/expressions/cel/expressions.go +++ b/pkg/expressions/cel/expressions.go @@ -21,6 +21,7 @@ const RootRequestBinding = "request" const RootSourceBinding = "source" const RootDestinationBinding = "destination" const RootAuthBinding = "auth" +const RootSecretKeysBinding = "keys" type Predicate struct { program cel.Program @@ -100,6 +101,7 @@ func Compile(expression string, expectedType *cel.Type, opts ...cel.EnvOption) ( decls.NewConst(RootSourceBinding, decls.NewObjectType("google.protobuf.Struct"), nil), decls.NewConst(RootDestinationBinding, decls.NewObjectType("google.protobuf.Struct"), nil), decls.NewConst(RootAuthBinding, decls.NewObjectType("google.protobuf.Struct"), nil), + decls.NewConst(RootSecretKeysBinding, decls.NewListType(decls.String), nil), )}, opts...) envOpts = append(envOpts, ext.Strings()) env, env_err := cel.NewEnv(envOpts...) @@ -149,11 +151,14 @@ func AuthJsonToCel(json string) (map[string]interface{}, error) { if err := jsonpb.Unmarshal(strings.NewReader(json), &data); err != nil { return nil, err } - metadata := data.GetFields()[RootMetadataBinding] - request := data.GetFields()[RootRequestBinding] - source := data.GetFields()[RootSourceBinding] - destination := data.GetFields()[RootDestinationBinding] - auth := data.GetFields()[RootAuthBinding] + + fields := data.GetFields() + + metadata := fields[RootMetadataBinding] + request := fields[RootRequestBinding] + source := fields[RootSourceBinding] + destination := fields[RootDestinationBinding] + auth := fields[RootAuthBinding] input := map[string]interface{}{ RootMetadataBinding: metadata, @@ -164,3 +169,39 @@ func AuthJsonToCel(json string) (map[string]interface{}, error) { } return input, nil } + +type KeySelectorExpression struct { + program cel.Program + source string +} + +func NewKeySelectorExpression(source string) (*KeySelectorExpression, error) { + program, err := Compile(source, cel.ListType(cel.StringType)) + if err != nil { + return nil, err + } + return &KeySelectorExpression{ + program: program, + source: source, + }, nil +} + +func (e *KeySelectorExpression) ResolveFor(jsonData string) (any, error) { + data := structpb.Struct{} + if err := jsonpb.Unmarshal(strings.NewReader(jsonData), &data); err != nil { + return nil, err + } + + fields := data.GetFields() + + input := map[string]any{ + RootSecretKeysBinding: fields[RootSecretKeysBinding], + } + + result, _, err := e.program.Eval(input) + if err != nil { + return nil, err + } + + return result.Value(), nil +} From dedc10ac48d0344ec5e8a0591511b1017d3e5843 Mon Sep 17 00:00:00 2001 From: KevFan <chfan@redhat.com> Date: Wed, 12 Feb 2025 13:06:35 +0000 Subject: [PATCH 10/12] fixup: typo Signed-off-by: KevFan <chfan@redhat.com> --- api/v1beta3/auth_config_types.go | 2 +- install/crd/authorino.kuadrant.io_authconfigs.yaml | 2 +- install/manifests.yaml | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/api/v1beta3/auth_config_types.go b/api/v1beta3/auth_config_types.go index cb5eba2f..b498a662 100644 --- a/api/v1beta3/auth_config_types.go +++ b/api/v1beta3/auth_config_types.go @@ -359,7 +359,7 @@ type ApiKeyAuthenticationSpec struct { // A Common Expression Language (CEL) expression that evaluates to a list of string keys within the selected Kubernetes // secret that contain valid API credentials. The keys of the selected Kubernetes secret are available for evaluation // in the following structure: `{"keys": ["key1", "key2"]}`. - // Authorino will attempt to authenticate using any matching key. If no keys are defined, the default "api-key" will be used. + // Authorino will attempt to authenticate using any matching key. If no keys are defined, the default "api_key" will be used. // If no match is found, the Kubernetes secret is not considered a valid Authorino API Key secret and is ignored. // String expressions are supported (https://pkg.go.dev/github.com/google/cel-go/ext#Strings). // +optional diff --git a/install/crd/authorino.kuadrant.io_authconfigs.yaml b/install/crd/authorino.kuadrant.io_authconfigs.yaml index b409f58f..ff1e8429 100644 --- a/install/crd/authorino.kuadrant.io_authconfigs.yaml +++ b/install/crd/authorino.kuadrant.io_authconfigs.yaml @@ -2398,7 +2398,7 @@ spec: A Common Expression Language (CEL) expression that evaluates to a list of string keys within the selected Kubernetes secret that contain valid API credentials. The keys of the selected Kubernetes secret are available for evaluation in the following structure: `{"keys": ["key1", "key2"]}`. - Authorino will attempt to authenticate using any matching key. If no keys are defined, the default "api-key" will be used. + Authorino will attempt to authenticate using any matching key. If no keys are defined, the default "api_key" will be used. If no match is found, the Kubernetes secret is not considered a valid Authorino API Key secret and is ignored. String expressions are supported (https://pkg.go.dev/github.com/google/cel-go/ext#Strings). type: string diff --git a/install/manifests.yaml b/install/manifests.yaml index 9b657f4a..bac1404c 100644 --- a/install/manifests.yaml +++ b/install/manifests.yaml @@ -2665,7 +2665,7 @@ spec: A Common Expression Language (CEL) expression that evaluates to a list of string keys within the selected Kubernetes secret that contain valid API credentials. The keys of the selected Kubernetes secret are available for evaluation in the following structure: `{"keys": ["key1", "key2"]}`. - Authorino will attempt to authenticate using any matching key. If no keys are defined, the default "api-key" will be used. + Authorino will attempt to authenticate using any matching key. If no keys are defined, the default "api_key" will be used. If no match is found, the Kubernetes secret is not considered a valid Authorino API Key secret and is ignored. String expressions are supported (https://pkg.go.dev/github.com/google/cel-go/ext#Strings). type: string From 01a81eb499e81660a58f410ed8ceaee27ac47846 Mon Sep 17 00:00:00 2001 From: KevFan <chfan@redhat.com> Date: Wed, 19 Feb 2025 11:30:54 +0000 Subject: [PATCH 11/12] fixup: descriptions Signed-off-by: KevFan <chfan@redhat.com> --- api/v1beta3/auth_config_types.go | 20 +++++++++++----- .../authorino.kuadrant.io_authconfigs.yaml | 23 ++++++++++++++----- install/manifests.yaml | 23 ++++++++++++++----- pkg/evaluators/identity/api_key.go | 2 +- 4 files changed, 49 insertions(+), 19 deletions(-) diff --git a/api/v1beta3/auth_config_types.go b/api/v1beta3/auth_config_types.go index b498a662..7ec574a2 100644 --- a/api/v1beta3/auth_config_types.go +++ b/api/v1beta3/auth_config_types.go @@ -356,12 +356,20 @@ type ApiKeyAuthenticationSpec struct { // +kubebuilder:default:=false AllNamespaces bool `json:"allNamespaces,omitempty"` - // A Common Expression Language (CEL) expression that evaluates to a list of string keys within the selected Kubernetes - // secret that contain valid API credentials. The keys of the selected Kubernetes secret are available for evaluation - // in the following structure: `{"keys": ["key1", "key2"]}`. - // Authorino will attempt to authenticate using any matching key. If no keys are defined, the default "api_key" will be used. - // If no match is found, the Kubernetes secret is not considered a valid Authorino API Key secret and is ignored. - // String expressions are supported (https://pkg.go.dev/github.com/google/cel-go/ext#Strings). + // A Common Expression Language (CEL) expression that evaluates to a list of string keys, such as `["custom_key1", "custom_key2"]`, + // within the selected Kubernetes secret that contains valid API credentials. + // + // The keys of the selected Kubernetes secret are available for evaluation in the following structure: + // `{"keys": ["api_key", "custom_key1", "custom_key2"]}`. + // + // For example, to select keys that start with "custom", use the following CEL expression: + // `"keys.filter(k, k.startsWith('custom'))"` + // + // Authorino will attempt to authenticate using any matching key. If this field is omitted or empty, the default `["api_key"]` will be used. + // If no match is found, the Kubernetes secret is not considered a valid Authorino API Key secret and will be ignored. + // + // String expressions are supported: https://pkg.go.dev/github.com/google/cel-go/ext#Strings + // // +optional KeySelector CelExpression `json:"keySelector,omitempty"` } diff --git a/install/crd/authorino.kuadrant.io_authconfigs.yaml b/install/crd/authorino.kuadrant.io_authconfigs.yaml index ff1e8429..5b47f73b 100644 --- a/install/crd/authorino.kuadrant.io_authconfigs.yaml +++ b/install/crd/authorino.kuadrant.io_authconfigs.yaml @@ -2395,12 +2395,23 @@ spec: type: boolean keySelector: description: |- - A Common Expression Language (CEL) expression that evaluates to a list of string keys within the selected Kubernetes - secret that contain valid API credentials. The keys of the selected Kubernetes secret are available for evaluation - in the following structure: `{"keys": ["key1", "key2"]}`. - Authorino will attempt to authenticate using any matching key. If no keys are defined, the default "api_key" will be used. - If no match is found, the Kubernetes secret is not considered a valid Authorino API Key secret and is ignored. - String expressions are supported (https://pkg.go.dev/github.com/google/cel-go/ext#Strings). + A Common Expression Language (CEL) expression that evaluates to a list of string keys, such as `["custom_key1", "custom_key2"]`, + within the selected Kubernetes secret that contains valid API credentials. + + + The keys of the selected Kubernetes secret are available for evaluation in the following structure: + `{"keys": ["api_key", "custom_key1", "custom_key2"]}`. + + + For example, to select keys that start with "custom", use the following CEL expression: + `"keys.filter(k, k.startsWith('custom'))"` + + + Authorino will attempt to authenticate using any matching key. If this field is omitted or empty, the default `["api_key"]` will be used. + If no match is found, the Kubernetes secret is not considered a valid Authorino API Key secret and will be ignored. + + + String expressions are supported: https://pkg.go.dev/github.com/google/cel-go/ext#Strings type: string selector: description: Label selector used by Authorino to match secrets diff --git a/install/manifests.yaml b/install/manifests.yaml index bac1404c..f9cbe24a 100644 --- a/install/manifests.yaml +++ b/install/manifests.yaml @@ -2662,12 +2662,23 @@ spec: type: boolean keySelector: description: |- - A Common Expression Language (CEL) expression that evaluates to a list of string keys within the selected Kubernetes - secret that contain valid API credentials. The keys of the selected Kubernetes secret are available for evaluation - in the following structure: `{"keys": ["key1", "key2"]}`. - Authorino will attempt to authenticate using any matching key. If no keys are defined, the default "api_key" will be used. - If no match is found, the Kubernetes secret is not considered a valid Authorino API Key secret and is ignored. - String expressions are supported (https://pkg.go.dev/github.com/google/cel-go/ext#Strings). + A Common Expression Language (CEL) expression that evaluates to a list of string keys, such as `["custom_key1", "custom_key2"]`, + within the selected Kubernetes secret that contains valid API credentials. + + + The keys of the selected Kubernetes secret are available for evaluation in the following structure: + `{"keys": ["api_key", "custom_key1", "custom_key2"]}`. + + + For example, to select keys that start with "custom", use the following CEL expression: + `"keys.filter(k, k.startsWith('custom'))"` + + + Authorino will attempt to authenticate using any matching key. If this field is omitted or empty, the default `["api_key"]` will be used. + If no match is found, the Kubernetes secret is not considered a valid Authorino API Key secret and will be ignored. + + + String expressions are supported: https://pkg.go.dev/github.com/google/cel-go/ext#Strings type: string selector: description: Label selector used by Authorino to match secrets diff --git a/pkg/evaluators/identity/api_key.go b/pkg/evaluators/identity/api_key.go index 13de4c77..a9e7bddb 100644 --- a/pkg/evaluators/identity/api_key.go +++ b/pkg/evaluators/identity/api_key.go @@ -205,7 +205,7 @@ func (a *APIKey) getValuesFromSecret(ctx context.Context, secret k8s.Secret) []s // Convert evaluated result to a slice of strings selectedKeys, ok := convertToStringSlice(evaluated) if !ok { - logger.Error(fmt.Errorf("unexpected type for resolved key"), "expected string or []string", "value", evaluated) + logger.Error(fmt.Errorf("unexpected type for resolved key"), "expected []string", "value", evaluated) return nil } From 51a230aacb0616dbac8bd4cc212956d6c57351e4 Mon Sep 17 00:00:00 2001 From: KevFan <chfan@redhat.com> Date: Tue, 25 Feb 2025 10:55:56 +0000 Subject: [PATCH 12/12] fixup: return err if failing to compile KeySelector CEL expression Signed-off-by: KevFan <chfan@redhat.com> --- controllers/auth_config_controller.go | 6 +++- controllers/secret_controller_test.go | 3 +- pkg/evaluators/identity/api_key.go | 6 ++-- pkg/evaluators/identity/api_key_test.go | 42 ++++++++++++++++--------- 4 files changed, 38 insertions(+), 19 deletions(-) diff --git a/controllers/auth_config_controller.go b/controllers/auth_config_controller.go index ecfed6ae..7cd2a564 100644 --- a/controllers/auth_config_controller.go +++ b/controllers/auth_config_controller.go @@ -262,7 +262,11 @@ func (r *AuthConfigReconciler) translateAuthConfig(ctx context.Context, authConf return nil, err } - translatedIdentity.APIKey = identity_evaluators.NewApiKeyIdentity(identityCfgName, selector, namespace, string(identity.ApiKey.KeySelector), authCred, r.Client, ctxWithLogger) + if apiKeyIdentity, err := identity_evaluators.NewApiKeyIdentity(identityCfgName, selector, namespace, string(identity.ApiKey.KeySelector), authCred, r.Client, ctxWithLogger); err != nil { + return nil, err + } else { + translatedIdentity.APIKey = apiKeyIdentity + } // MTLS case api.X509ClientCertificateAuthentication: diff --git a/controllers/secret_controller_test.go b/controllers/secret_controller_test.go index 3f5904f4..65f00cb3 100644 --- a/controllers/secret_controller_test.go +++ b/controllers/secret_controller_test.go @@ -91,10 +91,11 @@ func newSecretReconcilerTest(mockCtrl *gomock.Controller, secretLabels map[strin fakeK8sClient := fake.NewClientBuilder().WithScheme(scheme).WithRuntimeObjects(&secret).Build() apiKeyLabelSelectors, _ := labels.Parse("target=echo-api") + apiKeyIdentity, _ := identity_evaluators.NewApiKeyIdentity("api-key", apiKeyLabelSelectors, "", "", auth.NewAuthCredential("", ""), fakeK8sClient, context.TODO()) indexedAuthConfig := &evaluators.AuthConfig{ Labels: map[string]string{"namespace": "authorino", "name": "api-protection"}, IdentityConfigs: []auth.AuthConfigEvaluator{&fakeAPIKeyIdentityConfig{ - evaluator: identity_evaluators.NewApiKeyIdentity("api-key", apiKeyLabelSelectors, "", "", auth.NewAuthCredential("", ""), fakeK8sClient, context.TODO()), + evaluator: apiKeyIdentity, }}, } indexMock := mock_index.NewMockIndex(mockCtrl) diff --git a/pkg/evaluators/identity/api_key.go b/pkg/evaluators/identity/api_key.go index a9e7bddb..eff57d01 100644 --- a/pkg/evaluators/identity/api_key.go +++ b/pkg/evaluators/identity/api_key.go @@ -41,7 +41,7 @@ type APIKey struct { k8sClient k8s_client.Reader } -func NewApiKeyIdentity(name string, labelSelectors k8s_labels.Selector, namespace string, keySelectorExpression string, authCred auth.AuthCredentials, k8sClient k8s_client.Reader, ctx context.Context) *APIKey { +func NewApiKeyIdentity(name string, labelSelectors k8s_labels.Selector, namespace string, keySelectorExpression string, authCred auth.AuthCredentials, k8sClient k8s_client.Reader, ctx context.Context) (*APIKey, error) { if keySelectorExpression == "" { keySelectorExpression = defaultKeySelectorExpression } @@ -51,7 +51,7 @@ func NewApiKeyIdentity(name string, labelSelectors k8s_labels.Selector, namespac expr, err := cel.NewKeySelectorExpression(keySelectorExpression) if err != nil { logger.Error(err, "failed to create key selector expression") - return nil + return nil, err } apiKey := &APIKey{ @@ -66,7 +66,7 @@ func NewApiKeyIdentity(name string, labelSelectors k8s_labels.Selector, namespac if err := apiKey.loadSecrets(context.TODO()); err != nil { logger.Error(err, credentialsFetchingErrorMsg) } - return apiKey + return apiKey, nil } // loadSecrets will load the matching k8s secrets from the cluster to the cache of trusted API keys diff --git a/pkg/evaluators/identity/api_key_test.go b/pkg/evaluators/identity/api_key_test.go index 629d7a46..307b5092 100644 --- a/pkg/evaluators/identity/api_key_test.go +++ b/pkg/evaluators/identity/api_key_test.go @@ -32,8 +32,9 @@ func TestNewApiKeyIdentityAllNamespaces(t *testing.T) { defer ctrl.Finish() selector, _ := k8s_labels.Parse("planet=coruscant") - apiKey := NewApiKeyIdentity("jedi", selector, "", "", mock_auth.NewMockAuthCredentials(ctrl), testAPIKeyK8sClient, context.TODO()) + apiKey, err := NewApiKeyIdentity("jedi", selector, "", "", mock_auth.NewMockAuthCredentials(ctrl), testAPIKeyK8sClient, context.TODO()) + assert.NilError(t, err) assert.Equal(t, apiKey.Name, "jedi") assert.Equal(t, apiKey.LabelSelectors.String(), "planet=coruscant") assert.Equal(t, apiKey.Namespace, "") @@ -51,8 +52,9 @@ func TestNewApiKeyIdentitySingleNamespace(t *testing.T) { defer ctrl.Finish() selector, _ := k8s_labels.Parse("planet=coruscant") - apiKey := NewApiKeyIdentity("jedi", selector, "ns1", "", mock_auth.NewMockAuthCredentials(ctrl), testAPIKeyK8sClient, context.TODO()) + apiKey, err := NewApiKeyIdentity("jedi", selector, "ns1", "", mock_auth.NewMockAuthCredentials(ctrl), testAPIKeyK8sClient, context.TODO()) + assert.NilError(t, err) assert.Equal(t, apiKey.Name, "jedi") assert.Equal(t, apiKey.LabelSelectors.String(), "planet=coruscant") assert.Equal(t, apiKey.Namespace, "ns1") @@ -70,8 +72,9 @@ func TestNewApiKeyIdentityMultipleKeySelectors(t *testing.T) { defer ctrl.Finish() selector, _ := k8s_labels.Parse("planet=coruscant") - apiKey := NewApiKeyIdentity("jedi", selector, "ns1", "['no_op','api_key_2']", mock_auth.NewMockAuthCredentials(ctrl), testAPIKeyK8sClient, context.TODO()) + apiKey, err := NewApiKeyIdentity("jedi", selector, "ns1", "['no_op','api_key_2']", mock_auth.NewMockAuthCredentials(ctrl), testAPIKeyK8sClient, context.TODO()) + assert.NilError(t, err) assert.Equal(t, apiKey.Name, "jedi") assert.Equal(t, apiKey.LabelSelectors.String(), "planet=coruscant") assert.Equal(t, apiKey.Namespace, "ns1") @@ -95,7 +98,8 @@ func TestCallSuccess(t *testing.T) { authCredMock.EXPECT().GetCredentialsFromReq(gomock.Any()).Return("ObiWanKenobiLightSaber", nil) selector, _ := k8s_labels.Parse("planet=coruscant") - apiKey := NewApiKeyIdentity("jedi", selector, "", "", authCredMock, testAPIKeyK8sClient, context.TODO()) + apiKey, err := NewApiKeyIdentity("jedi", selector, "", "", authCredMock, testAPIKeyK8sClient, context.TODO()) + assert.NilError(t, err) auth, err := apiKey.Call(pipelineMock, context.TODO()) assert.NilError(t, err) @@ -111,9 +115,10 @@ func TestCallNoApiKeyFail(t *testing.T) { authCredMock.EXPECT().GetCredentialsFromReq(gomock.Any()).Return("", fmt.Errorf("something went wrong getting the API Key")) selector, _ := k8s_labels.Parse("planet=coruscant") - apiKey := NewApiKeyIdentity("jedi", selector, "", "", authCredMock, testAPIKeyK8sClient, context.TODO()) + apiKey, err := NewApiKeyIdentity("jedi", selector, "", "", authCredMock, testAPIKeyK8sClient, context.TODO()) + assert.NilError(t, err) - _, err := apiKey.Call(pipelineMock, context.TODO()) + _, err = apiKey.Call(pipelineMock, context.TODO()) assert.Error(t, err, "something went wrong getting the API Key") } @@ -127,17 +132,19 @@ func TestCallInvalidApiKeyFail(t *testing.T) { authCredMock.EXPECT().GetCredentialsFromReq(gomock.Any()).Return("ASithLightSaber", nil) selector, _ := k8s_labels.Parse("planet=coruscant") - apiKey := NewApiKeyIdentity("jedi", selector, "", "", authCredMock, testAPIKeyK8sClient, context.TODO()) - _, err := apiKey.Call(pipelineMock, context.TODO()) + apiKey, err := NewApiKeyIdentity("jedi", selector, "", "", authCredMock, testAPIKeyK8sClient, context.TODO()) + assert.NilError(t, err) + _, err = apiKey.Call(pipelineMock, context.TODO()) assert.Error(t, err, "the API Key provided is invalid") } func TestLoadSecretsSuccess(t *testing.T) { selector, _ := k8s_labels.Parse("planet=coruscant") - apiKey := NewApiKeyIdentity("X-API-KEY", selector, "", "", nil, testAPIKeyK8sClient, nil) + apiKey, err := NewApiKeyIdentity("X-API-KEY", selector, "", "", nil, testAPIKeyK8sClient, nil) + assert.NilError(t, err) - err := apiKey.loadSecrets(context.TODO()) + err = apiKey.loadSecrets(context.TODO()) assert.NilError(t, err) assert.Equal(t, len(apiKey.secrets), 2) @@ -152,12 +159,19 @@ func TestLoadSecretsSuccess(t *testing.T) { func TestLoadSecretsFail(t *testing.T) { selector, _ := k8s_labels.Parse("planet=coruscant") - apiKey := NewApiKeyIdentity("X-API-KEY", selector, "", "", nil, &flawedAPIkeyK8sClient{}, context.TODO()) + apiKey, err := NewApiKeyIdentity("X-API-KEY", selector, "", "", nil, &flawedAPIkeyK8sClient{}, context.TODO()) + assert.NilError(t, err) - err := apiKey.loadSecrets(context.TODO()) + err = apiKey.loadSecrets(context.TODO()) assert.Error(t, err, "something terribly wrong happened") } +func TestCELCompileFail(t *testing.T) { + selector, _ := k8s_labels.Parse("planet=coruscant") + _, err := NewApiKeyIdentity("X-API-KEY", selector, "", "stringLiteral", nil, testAPIKeyK8sClient, context.TODO()) + assert.Assert(t, err != nil) +} + func BenchmarkAPIKeyAuthn(b *testing.B) { ctrl := gomock.NewController(b) defer ctrl.Finish() @@ -166,9 +180,9 @@ func BenchmarkAPIKeyAuthn(b *testing.B) { authCredMock := mock_auth.NewMockAuthCredentials(ctrl) authCredMock.EXPECT().GetCredentialsFromReq(gomock.Any()).Return("ObiWanKenobiLightSaber", nil).MinTimes(1) selector, _ := k8s_labels.Parse("planet=coruscant") - apiKey := NewApiKeyIdentity("jedi", selector, "", "", authCredMock, testAPIKeyK8sClient, context.TODO()) + apiKey, err := NewApiKeyIdentity("jedi", selector, "", "", authCredMock, testAPIKeyK8sClient, context.TODO()) + assert.NilError(b, err) - var err error b.ResetTimer() for i := 0; i < b.N; i++ { _, err = apiKey.Call(pipelineMock, context.TODO())