feat: configurable API Key K8s secret key name

[KevFan]

Description

Closes: #360

Adds Key Selectors support for API Key k8s secret

Verification

• Create local cluster

make local-setup

• Apply secret

kubectl apply -f -<<EOF
apiVersion: v1
kind: Secret
metadata:
  name: api-key-1
  labels:
    authorino.kuadrant.io/managed-by: authorino
    group: friends
stringData:
  api_key:  key_1
  api_key_2: key_2
  key3: key_3
EOF

• Apply AuthConfig

kubectl apply -f -<<EOF                                                           
apiVersion: authorino.kuadrant.io/v1beta3
kind: AuthConfig   
metadata:
  name: talker-api-protection
spec:
  hosts:
  - talker-api.127.0.0.1.nip.io
  authentication:
    "friends":
      apiKey:
        selector:
          matchLabels:
            group: friends
      credentials:
        authorizationHeader:
          prefix: APIKEY
EOF

• Port forward envoy

kubectl port-forward deployment/envoy 8000:8000 2>&1 >/dev/null &

• Curl using api_key key (default key as not key selector was defined) should succeed

curl -H 'Authorization: APIKEY key_1' http://talker-api.127.0.0.1.nip.io:8000/hello
# HTTP/1.1 200 OK

• Curl using api_key_2 and key 3 key should fail with 401 Unauthorized

curl -H 'Authorization: APIKEY key_2' http://talker-api.127.0.0.1.nip.io:8000/hello -v
curl -H 'Authorization: APIKEY key_3' http://talker-api.127.0.0.1.nip.io:8000/hello -v

• Update AuthConfig to only selector for api_key_2

kubectl apply -f -<<EOF                                                           
apiVersion: authorino.kuadrant.io/v1beta3
kind: AuthConfig   
metadata:
  name: talker-api-protection
spec:
  hosts:
  - talker-api.127.0.0.1.nip.io
  authentication:
    "friends":
      apiKey:
        selector:
          matchLabels:
            group: friends
        keySelector: "['api_key_2']"
      credentials:
        authorizationHeader:
          prefix: APIKEY
EOF

• Curling with api_key_2 should succeed

curl -H 'Authorization: APIKEY key_2' http://talker-api.127.0.0.1.nip.io:8000/hello -v
# HTTP/1.1 200 OK

• Curl using api_key and key 3 key should fail with 401 Unauthorized

curl -H 'Authorization: APIKEY key_1' http://talker-api.127.0.0.1.nip.io:8000/hello -v
curl -H 'Authorization: APIKEY key_3' http://talker-api.127.0.0.1.nip.io:8000/hello -v

• Update AuthConfig to selector for keys matching api_key*

kubectl apply -f -<<EOF                                                           
apiVersion: authorino.kuadrant.io/v1beta3
kind: AuthConfig   
metadata:
  name: talker-api-protection
spec:
  hosts:
  - talker-api.127.0.0.1.nip.io
  authentication:
    "friends":
      apiKey:
        selector:
          matchLabels:
            group: friends
        keySelector: "keys.filter(k, k.startsWith('api_key'))"
      credentials:
        authorizationHeader:
          prefix: APIKEY
EOF

• Curling with api_key and api_key_2 should succeed

curl -H 'Authorization: APIKEY key_1' http://talker-api.127.0.0.1.nip.io:8000/hello 
curl -H 'Authorization: APIKEY key_2' http://talker-api.127.0.0.1.nip.io:8000/hello 

• Curling with key_3 key should fail with 401 Unauthorized

curl -H 'Authorization: APIKEY key_3' http://talker-api.127.0.0.1.nip.io:8000/hello -v

• Update secret to remove api_key_2

kubectl patch secret api-key-1 -p '{"data":{"api_key_2": null}}'

• Curling with api_key_2 should fail

curl -H 'Authorization: APIKEY key_2' http://talker-api.127.0.0.1.nip.io:8000/hello -v
# HTTP/1.1 401 Unauthorized

• Curling with api_key should still succeed

curl -H 'Authorization: APIKEY key_1' http://talker-api.127.0.0.1.nip.io:8000/hello
# HTTP/1.1 200 OK

• Create second secret

kubectl apply -f -<<EOF
apiVersion: v1
kind: Secret
metadata:
  name: api-key-2
  labels:
    authorino.kuadrant.io/managed-by: authorino
    group: friends
stringData:
  api_key_2: another_2
EOF

• Curling with api_key_2 from second secret should succeed

curl -H 'Authorization: APIKEY another_2' http://talker-api.127.0.0.1.nip.io:8000/hello 
# HTTP/1.1 200 OK

• Delete first secet

kubectl delete secret api-key-1

• Curling with api_key, api_key_2 and api_key_3 should fail

curl -H 'Authorization: APIKEY key_1' http://talker-api.127.0.0.1.nip.io:8000/hello 
curl -H 'Authorization: APIKEY key_2' http://talker-api.127.0.0.1.nip.io:8000/hello 
curl -H 'Authorization: APIKEY key_3' http://talker-api.127.0.0.1.nip.io:8000/hello 

• Update second secret with new key

kubectl apply -f -<<EOF
apiVersion: v1
kind: Secret
metadata:
  name: api-key-2
  labels:
    authorino.kuadrant.io/managed-by: authorino
    group: friends
stringData:
  api_key_2: another_2
  api_key_3: another_3
EOF

• Curling with api_key_2 and api_key_3 should succeed

curl -H 'Authorization: APIKEY another_2' http://talker-api.127.0.0.1.nip.io:8000/hello 
curl -H 'Authorization: APIKEY another_3' http://talker-api.127.0.0.1.nip.io:8000/hello 
# HTTP/1.1 200 OK

[guicassolato]

Thank you so much for taking the time to address this issue, @KevFan!

I've left a few comments. Nothing that invalidates the change, but mainly to double check on the design we want for the feature and some implementation detail.

One general wondering I have though is whether we want to allow this change into v1beta2 or v1beta3 (addressing #480 first). Although the keySelectors field not being backwards compatible with v1beta1 is not much of a concern (since #483 was merged), it feels like breaking a promise (internal agreement maybe?) that all changes to the API now would go into v1beta3.

Do you have an opinion on this, @alexsnaps?

[alexsnaps]

all changes to the API now would go into v1beta3

So... @guicassolato, you'd bump the version with this change, is that it? I'm growing concerned I won't get the CEL stuff done by the EOW tho... So the question becomes more of when would v1beta3 be releasable? If we miss the mark of next week's Kuadrant release, can we keep authorino at v0.18 for another iteration? I think we could, hoping no need for a v0.18.1 🙈 !

[KevFan]

Should no longer return early since the change now allows for the possibility of multiple keys pointing to the same secret

[KevFan]

Do we want to define the kubebuilder default here also? 🤔

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

Do we want to set a maximum?

+kubebuilder:validation:MaxItems

[guicassolato]

This is a good question. I guess the only reason I can think of to do that is to help users not to shoot themselves in the foot when tempted to add hundreds, thousands of valid keys as a workaround to store multiple valid API keys inside of a same Kubernetes secret.

If we ignore performance (and let users figure that out by themselves), arguably storing multiple valid identities in the same Secret may not be necessarily such a bad idea－even though it isn't how this feature was originally designed to work and users would lose capabilities in the process (e.g. storing one identity's metadata in the secret's metadata).

Say we do want to let users store multiple API keys in a Secret (now that it will be technically possible anyway). Probably, having to come up with a different key name for each identity would be a PITA. I can see requests of extending the key selector to support things like regexes, globs, etc coming up.

So I wonder, should we embrace that we make the key selector to be a CEL expression?

cc @alexsnaps @maleck13

[KevFan]

I think that is a good point, moving to CEL expression would make a more consistent experience while giving users more capabilities to how they want to select the secret keys 👍

[KevFan]

I've a working implementation based on how it's currently done for the AuthJSON and implementing

authorino/pkg/expressions/types.go

Lines 3 to 5 in b2bde91

	type Value interface {
	ResolveFor(jsonData string) (interface{}, error)
	}

Interested in @alexsnaps opinions on this

[guicassolato]

Verification steps work.

nit: we may want to fix perhaps only where it says:

• Curling with api_key_2 should succeed

curl -H 'Authorization: APIKEY key_2' http://talker-api.127.0.0.1.nip.io:8000/hello -v
# HTTP/1.1 401 Unauthorized

Last line (commented) should probably read # HTTP/1.1 200 OK.

Let's just make sure we're all in agreement regarding https://github.com/Kuadrant/authorino/pull/488/files#r1958525812 before merging this (or making the proper adjustments.)

Nice job, @KevFan!

[alexsnaps]

I wondered if binding the keys root to the set of keys could have become an issue. Looking at the API tho, there is not much we'd miss from the whole secret. Most importantly, afaict if I read this properly, it's always a unique set of keys (whether the secret defines stringData or data... is both possible?). So I think there isn't much issues with "only" exposing keys for now and possibly expose more (through an additional root) when needed, e.g. type or immutable, or even secret entirely when needed.
Thanks for the PR!

[didierofrivia]

<3

[KevFan]

I wondered if binding the keys root to the set of keys could have become an issue. Looking at the API tho, there is not much we'd miss from the whole secret. Most importantly, afaict if I read this properly, it's always a unique set of keys (whether the secret defines stringData or data... is both possible?). So I think there isn't much issues with "only" exposing keys for now and possibly expose more (through an additional root) when needed, e.g. type or immutable, or even secret entirely when needed. Thanks for the PR!

Thanks @alexsnaps for raising this. Yes, I believe its always a unique set of keys also. Agreed, I'm in favour of possibly exposing more of the secret as needed in any future work 👍

[guicassolato]

I wondered if binding the keys root to the set of keys could have become an issue. Looking at the API tho, there is not much we'd miss from the whole secret. Most importantly, afaict if I read this properly, it's always a unique set of keys (whether the secret defines stringData or data... is both possible?). So I think there isn't much issues with "only" exposing keys for now and possibly expose more (through an additional root) when needed, e.g. type or immutable, or even secret entirely when needed. Thanks for the PR!

Thanks @alexsnaps for raising this. Yes, I believe its always a unique set of keys also. Agreed, I'm in favour of possibly exposing more of the secret as needed in any future work 👍

Let me start by saying that API key authentication, without proper guardrails set by a cluster admin beforehand, can easily be the least secure feature of Authorino.

On one hand, giving access to the entire secret by binding to the root of the object instead of keys would enable extra functionality, not yet contemplated in this PR. E.g., one could store the valid key names for any given API key secret in the annotations of the secret itself; then use this root binding to read the annotation, cast its value to the expected format (list of strings), thus achieving dynamic key names on a per secret basis.

Only this change could make label selectors obsolete (if it wasn't for the optimisation aspect involved when querying the API server of course).

On the other hand, such change marginally increases an existing known vulnerability of unsafe Authorino deployments, that allow malicious users to elevate privileges and get access to Kubernetes secrets otherwise out of reach.

While checking the listed secrets for valid API keys stored in it, this line could be exploited to leak sensitive data to the logs, such as other values stored in the secret that are unrelated to API key authn. Combined with allNamespaces: true and some rather loose labels selectors, this can be a recipe for disaster in a cluster without proper RBAC rules set for the Authorino instance.

I say "marginally", however, because arguably such vulnerability already exists. It exists in 2 levels.

First, because any secret deemed to contain a valid API key secret already gets a root binding at auth.identity. This happens at request time, in the data plane, instead of control plane, but one who happens to know what's stored in an api_key entry of a valid API key secret can read the entire secret today already. This is by design and meant to let applications store user info in the secret's metadata, store other secrets in association with the user identity (that are later used in calls to fetch metadata from external HTTP services, e.g.), etc.

The second level is enabled by this PR. One who can guess the value of any key stored in a secret－it doesn't have to be necessarily the value of the api_key entry only－could write an AuthConfig that adds the known key to the list of valid key names, and use that to get access to the rest of the secret.