authorino icon indicating copy to clipboard operation
authorino copied to clipboard
verified publisher icon Kuadrant

feat: configurable API Key K8s secret key name

Open KevFan opened this issue 1 year ago • 4 comments

Description

Closes: https://github.com/Kuadrant/authorino/issues/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

KevFan avatar Sep 18 '24 09:09 KevFan

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 🙈 !

alexsnaps avatar Sep 23 '24 13:09 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!

alexsnaps avatar Feb 18 '25 16:02 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!

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 👍

KevFan avatar Feb 19 '25 12:02 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 👍

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.

guicassolato avatar Feb 24 '25 17:02 guicassolato