Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: configurable API Key K8s secret key name #488

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

feat: configurable API Key K8s secret key name #488

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
00b7b87
feat: configurable API Key K8s secret key name
KevFan Sep 18, 2024
692c2b4
fixup: add non-thread safe comment to AddK8sSecretBasedIdentity
KevFan Jan 22, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions api/v1beta3/auth_config_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
5 changes: 5 additions & 0 deletions api/v1beta3/zz_generated.deepcopy.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion controllers/auth_config_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
2 changes: 1 addition & 1 deletion controllers/secret_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
8 changes: 8 additions & 0 deletions install/crd/authorino.kuadrant.io_authconfigs.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
8 changes: 8 additions & 0 deletions install/manifests.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
38 changes: 23 additions & 15 deletions pkg/evaluators/identity/api_key.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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),
KevFan marked this conversation as resolved.
Show resolved Hide resolved
secrets: make(map[string]k8s.Secret),
k8sClient: k8sClient,
}
Expand Down Expand Up @@ -92,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
Expand All @@ -103,17 +106,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 {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if the struct shouldn't store 2 maps – api key value → k8s secret and k8s secret namespaced name → api key value. This would make this function as well as appendK8sSecretBasedIdentity both slightly more efficient. On the other hand, updating both maps might be an atomic operation.

WDYT?

if current.GetNamespace() == new.GetNamespace() && current.GetName() == new.GetName() {
if oldAPIKeyValue != newAPIKeyValue {
a.appendK8sSecretBasedIdentity(new)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the issue I said:

Authorino would try in order when reading the secret value of the API key (stopping when the first valid key name is found within the Kubernetes Secret), this change would also make it easier to implement key rotation […]

But I guess the part of making it easier to implement key rotation is not really true, is it? In order to properly support rotation, all keys in a Secret must be accepted, otherwise at the moment a new valid key is added to the secret, the old one is automatically deleted and stop being accepted.

Should we add to the APIKey.secrets map all API key values whose key match any of the APIKey.KeySelectors?

delete(a.secrets, oldAPIKeyValue)
KevFan marked this conversation as resolved.
Show resolved Hide resolved
logger.V(1).Info("api key updated")
} else {
logger.V(1).Info("api key unchanged")
}
return
}
return
}
}

Expand Down Expand Up @@ -146,10 +151,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
}
44 changes: 35 additions & 9 deletions pkg/evaluators/identity/api_key_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand All @@ -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)
Expand All @@ -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)
Expand All @@ -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)
Expand All @@ -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())

Expand All @@ -106,15 +132,15 @@ 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")
}

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)
Expand All @@ -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")
Expand All @@ -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()
Expand Down
Loading