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

Jump to bottom

feat: configurable API Key K8s secret key name #488

Draft
wants to merge 1 commit 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
1 commit
Select commit
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
4 changes: 2 additions & 2 deletions api/v1beta1/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.

6 changes: 6 additions & 0 deletions api/v1beta2/auth_config_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -343,6 +343,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
9 changes: 7 additions & 2 deletions api/v1beta2/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 @@ -249,7 +249,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 @@ -2261,6 +2261,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 @@ -2541,6 +2541,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
37 changes: 22 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),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Minor suggestion:

Suggested change
KeySelectors: append(keySelectors, apiKeySelector),
KeySelectors: append(keySelectors, defaultAPIKeySelector),

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good 👍

On a related note, what's your thoughts on instead of doing this append, use the kubebuilder default annotation in the API struct instead? The append works but this default value is hidden from the CRD, but not sure is this want we want in general

	// +kubebuilder:default:={"api_key"}

secrets: make(map[string]k8s.Secret),
k8sClient: k8sClient,
}
Expand Down Expand Up @@ -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 {
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)
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should flag this and the appendK8sSecretBasedIdentity function as non-thread safe. Today the API Key reconciler won't process two workloads at a time, but still long-term risky.

logger.V(1).Info("api key updated")
} else {
logger.V(1).Info("api key unchanged")
}
return
}
return
}
}

Expand Down Expand Up @@ -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
}
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