Support disabling the Accept header when requesting Json Web Key Sets. #201

hmadison · 2023-07-27T12:43:09Z

For certain servers which provide Json Web Key Sets, such as the Kubernetes API Server, if the Accept header sent by the client is present it is expected to be application/jwk-set+json. Based off of the discussion in the Strimzi slack channel, I've added an option which allows for users to skip sending the Accept header when making the JWKS request.

I've also updated the test documentation to cover how to use MiniKube and Helm to simplify the testing process and included an example of how to use this feature to authenticate into a Kafka cluster with a Service Account's token. To validate this locally, you can apply kafka-oauth-single-authz-service-accounts.yaml to a Kubernetes cluster with the operator installed and using a version of strimzi-kafka-oauth installed then:

Generate a JWT for the default service account in the default namespace with kubectl create token default.
Enter the Kafka pod via kubectl exec -it pod/test-cluster-kafka-0 -- /bin/bash.
Connect to the cluster via one of the included scripts usingenv OAUTH_ACCESS_TOKEN="<token>" bin/kafka-console-producer.sh --broker-list test-cluster-kafka-bootstrap.default:9092 --topic my-topic --producer-property 'security.protocol=SASL_PLAINTEXT' --producer-property 'sasl.mechanism=OAUTHBEARER' --producer-property 'sasl.jaas.config=org.apache.kafka.common.security.oauthbearer.OAuthBearerLoginModule required;' --producer-property 'sasl.login.callback.handler.class=io.strimzi.kafka.oauth.client.JaasClientOauthLoginCallbackHandler'

Signed-off-by: Hunter Madison <[email protected]>

When attemping to use Kubernetes' api-server as the source of JWK keys for JWT validation, it will only process requests which either do not have the "Accept" header set or requests which have the header set to "application/jwk-set+json". Signed-off-by: Hunter Madison <[email protected]>

Signed-off-by: Hunter Madison <[email protected]>

scholzj · 2023-07-27T21:08:09Z

examples/kubernetes/README.md

+* `kafka-oauth-single-authz-service-accounts.yaml`
+
+  A single node Kafka cluster using [service account tokens](https://kubernetes.io/docs/reference/access-authn-authz/authentication/#service-account-tokens) for authorization and the `simple` authorizer.
+  It requires that the `kube-root-ca.crt` be copied from its ConfigMap to a Secret:
+
+      kubectl get configmap/kube-root-ca.crt -o=json | jq -r '.data."ca.crt"' | kubectl create secret generic kube-root-ca --from-file=ca.crt=/dev/stdin


Is this really something what works with every single Kubernetes cluster based on Service accounts only? Or does it really on OpenID provider?

The Kubernetes api-server supports OpenID Connect to validate the credentials which (since version 1.22) get projected into a pod based off of it's service account. You don't need anything outside of a Kubernetes cluster to make use of this authentication.

Ok, interesting. I will have to try that.

examples/kubernetes/README.md

scholzj · 2023-07-27T21:09:02Z

examples/kubernetes/kafka-oauth-single-authz-service-accounts.yaml

+subjects:
+  - apiGroup: rbac.authorization.k8s.io
+    kind: User
+    name: "system:anonymous"


This should have the service account of the Kafka brokers I guess and not anonymous?

Since strimizi-kafka-oauth isn't setting its Bearer token to the one projected into the container, you need to allow system:anonymous to access the endpoints needed to retrieve the OpenID Connect settings and the Json Web Key Sets.

If that is not enabled for everyone by default, it means this is likely a security risk? Should it really be used here? Shouldn't you instead use the broker's service account?

I guess the same applies to system:serviceaccount:default:default -> maybe you should use some better example SA then the default one.

I'm also not sure I follow the relation ship of this example to the PR here. How do they relate? You do not seem to use any new flag in the example to disable the Accept header. So while the Kubernetes example is interesting, I do not follow the relationship here.

I'm setting the value of oauth.include.accept.header to false via spec.kafka.jvmOptions.javaSystemProperties on line 63.

From my perspective, the ODIC and JWKS endpdoints in a "normal" application are exposed over the public internet. If someone is in a position to make a web request from inside of the kubernetes cluster to these endpoints, they are also going to be able to access the token for the service account and make those requests with authentication. If we want to use the brokers service account, then we should back this example out of this pull request and I'll open a new one which includes a dedicated authenticator for Kubernetes service accounts.

We'll need to add the includeAcceptHeader config option to oauth listener authentication and keycloak authorization when we integrate the next OAuth release into the Strimzi Operator (another PR). We can then improve the example Kafka CR.

I'm setting the value of oauth.include.accept.header to false via spec.kafka.jvmOptions.javaSystemProperties on line 63.

I missed that part. As mentioned by Marko, this will need to be added tot he options after the next OAuth release. Until then it is a bit misleading since it will not work with OAuth 0.13.

From my perspective, the ODIC and JWKS endpdoints in a "normal" application are exposed over the public internet. If someone is in a position to make a web request from inside of the kubernetes cluster to these endpoints, they are also going to be able to access the token for the service account and make those requests with authentication. If we want to use the brokers service account, then we should back this example out of this pull request and I'll open a new one which includes a dedicated authenticator for Kubernetes service accounts.

It seems that from Kubernetes perspective it is not something what they thing should be public. Also, thing like this will discourage users from using it as it does not look secure. If this works on any Kubernetes cluster, I think this would be a great usecase. But I think we need to work out the details to make sure it uses the service account and does not connect over as anonymous. So my vote would be to move this to a separate PR and do this through the broker service account.

That's fine by me. I'll back this change out and start on a dedicated Kubernetes service account configuration which supports using the pod's service account token to authenticate with the api server.

oauth-server/src/main/java/io/strimzi/kafka/oauth/server/ServerConfig.java

mstruk

Thanks for the PR. As I mentioned in another comment I think this new option constant should really live in Config.java, and functionally be used not only in JWTSignatureValidator.java but also in OAuthIntrospectionValidator.java, and in KeycloakRBACAuthorizer.java and JaasClientOauthLoginCallbackHandler.

In KeycloakRBACAuthorizer.java the analogous option should also be introduced strimzi.authorization.include.accept.header that falls back to oauth.include.accept.header the same way some other options in there do.

hmadison · 2023-07-28T16:57:15Z

@mstruk I've made the requested changes. Do you have any idea why the s390x portion of the build is currently breaking?

Signed-off-by: Hunter Madison <[email protected]>

mstruk · 2023-07-29T12:24:41Z

@mstruk I've made the requested changes. Do you have any idea why the s390x portion of the build is currently breaking?

The issue is definitely unrelated to this PR. travis-ci.com has had some issues with JDK 11 on s390x platform last weeks. They apparently got things working to some degree but it seems JDK 11 is still partially broken. Also the .travis/build.sh script does some special build in s390x with Keycloak and it's that particular custom thing that now breaks. I'll see if I can fix it in a separate PR.

mstruk

Some minor issues, otherwise it looks good to me.

examples/kubernetes/kafka-oauth-single-authz-service-accounts.yaml

...-keycloak-authorizer/src/main/java/io/strimzi/kafka/oauth/server/authorizer/AuthzConfig.java

...-client/src/main/java/io/strimzi/kafka/oauth/client/JaasClientOauthLoginCallbackHandler.java

Signed-off-by: Hunter Madison <[email protected]>

mstruk · 2023-07-31T18:12:12Z

examples/kubernetes/kafka-oauth-single-authz-service-accounts.yaml

+metadata:
+  name: oidc-reader-binding
+  annotations:
+  kubernetes.io/description: |


Still not ok. It's this line (19) that should be indented in order to be a child of annotations. Currently it is a child of metadata which is incorrect.

Based off of @scholzj's feedback, I've backed this example out and will open up a subsequent PR with a better "out of the box" experience around service account usage.

mstruk · 2023-07-31T18:17:26Z

examples/kubernetes/kafka-oauth-single-authz-service-accounts.yaml

+subjects:
+  - apiGroup: rbac.authorization.k8s.io
+    kind: User
+    name: "system:anonymous"


We'll need to add the includeAcceptHeader config option to oauth listener authentication and keycloak authorization when we integrate the next OAuth release into the Strimzi Operator (another PR). We can then improve the example Kafka CR.

scholzj

One nit. LGTM otherwise.

examples/kubernetes/README.md

Signed-off-by: Hunter Madison <[email protected]>

Signed-off-by: Marko Strukelj <[email protected]>

mstruk · 2023-08-08T18:15:51Z

I added some tests to the testsuite and fixed some additional issues I found while writing tests: hmadison#1

Added configuration tests to the testsuite + fixed some issues

mstruk · 2023-08-09T07:51:29Z

@hmadison Thank you very much for the contribution!

hmadison added 3 commits July 27, 2023 08:44

Document how to test with Minikube and Helm

1e6154d

Signed-off-by: Hunter Madison <[email protected]>

Add an example making use of the new configuration option.

ce72e6b

Signed-off-by: Hunter Madison <[email protected]>

hmadison force-pushed the hm/jwks-skip-header branch from e217d8b to ce72e6b Compare July 27, 2023 12:45

Update JWKSKeyUseTest to match the new constructor definition.

d5a099e

Signed-off-by: Hunter Madison <[email protected]>

scholzj reviewed Jul 27, 2023

View reviewed changes

scholzj requested a review from mstruk July 27, 2023 21:10

mstruk reviewed Jul 28, 2023

View reviewed changes

hmadison force-pushed the hm/jwks-skip-header branch from 5c1b7fe to 2c41701 Compare July 28, 2023 16:35

hmadison requested a review from mstruk July 28, 2023 16:56

hmadison force-pushed the hm/jwks-skip-header branch 6 times, most recently from d9e321f to adfbf8e Compare July 28, 2023 19:47

Address comments from code review.

c55abba

Signed-off-by: Hunter Madison <[email protected]>

hmadison force-pushed the hm/jwks-skip-header branch from adfbf8e to c55abba Compare July 28, 2023 19:55

mstruk reviewed Jul 31, 2023

View reviewed changes

Address pull request feedback.

aa19ad1

Signed-off-by: Hunter Madison <[email protected]>

hmadison requested a review from mstruk July 31, 2023 13:55

mstruk reviewed Jul 31, 2023

View reviewed changes

mstruk added this to the 0.14.0 milestone Jul 31, 2023

scholzj reviewed Aug 1, 2023

View reviewed changes

examples/kubernetes/README.md Outdated Show resolved Hide resolved

Back out service account example.

b7f488d

Signed-off-by: Hunter Madison <[email protected]>

hmadison force-pushed the hm/jwks-skip-header branch from 33e8980 to b7f488d Compare August 1, 2023 16:53

scholzj approved these changes Aug 1, 2023

View reviewed changes

Added configuration tests to the testsuite + fixed some issues

61b47ad

Signed-off-by: Marko Strukelj <[email protected]>

Merge pull request #1 from mstruk/hm/jwks-skip-header

5d72a7c

Added configuration tests to the testsuite + fixed some issues

mstruk approved these changes Aug 9, 2023

View reviewed changes

mstruk merged commit 75044c8 into strimzi:main Aug 9, 2023

hmadison deleted the hm/jwks-skip-header branch August 9, 2023 09:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support disabling the Accept header when requesting Json Web Key Sets. #201

Support disabling the Accept header when requesting Json Web Key Sets. #201

hmadison commented Jul 27, 2023

scholzj Jul 27, 2023

hmadison Jul 28, 2023

scholzj Jul 28, 2023

scholzj Jul 27, 2023

hmadison Jul 28, 2023

scholzj Jul 28, 2023

scholzj Jul 28, 2023

hmadison Jul 29, 2023

mstruk Jul 31, 2023

scholzj Jul 31, 2023

hmadison Aug 1, 2023

mstruk left a comment

hmadison commented Jul 28, 2023

mstruk commented Jul 29, 2023 •

edited

Loading

mstruk left a comment

mstruk Jul 31, 2023

hmadison Aug 1, 2023

mstruk Jul 31, 2023

scholzj left a comment

mstruk commented Aug 8, 2023

mstruk commented Aug 9, 2023

Support disabling the Accept header when requesting Json Web Key Sets. #201

Support disabling the Accept header when requesting Json Web Key Sets. #201

Conversation

hmadison commented Jul 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mstruk left a comment

Choose a reason for hiding this comment

hmadison commented Jul 28, 2023

mstruk commented Jul 29, 2023 • edited Loading

mstruk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scholzj left a comment

Choose a reason for hiding this comment

mstruk commented Aug 8, 2023

mstruk commented Aug 9, 2023

mstruk commented Jul 29, 2023 •

edited

Loading