Additional authentication type 'k8s-oidc' that extends 'oauth' #9657

mstruk · 2024-02-07T20:40:08Z

Type of change

Enhancement / new feature

Description

TODO: This PR uses an unreleased SNAPSHOT version of OAuth library.

Make it easy to configure the usage of Kubernetes internal OIDC server. The new authentication type 'k8s-oidc' is an extension of 'oauth' type for listeners and various managed Kafka clients (KafkaConnector, KafkaBridge, KafkaMirrorMaker2, ...).

Checklist

Please go through this checklist and make sure all applicable tasks have been done

Write tests
Make sure all tests pass
Update documentation
Check RBAC rights for Kubernetes / OpenShift roles
Try your changes from Pod inside your Kubernetes and OpenShift cluster, not just locally
Reference relevant issue(s) and close them after merging
Update CHANGELOG.md
Supply screenshots for visual changes, such as Grafana dashboards

scholzj · 2024-02-13T11:42:17Z

...rc/main/java/io/strimzi/api/kafka/model/common/authentication/KafkaClientAuthentication.java

@@ -27,6 +27,7 @@
    @JsonSubTypes.Type(name = KafkaClientAuthenticationScramSha512.TYPE_SCRAM_SHA_512, value = KafkaClientAuthenticationScramSha512.class),
    @JsonSubTypes.Type(name = KafkaClientAuthenticationPlain.TYPE_PLAIN, value = KafkaClientAuthenticationPlain.class),
    @JsonSubTypes.Type(name = KafkaClientAuthenticationOAuth.TYPE_OAUTH, value = KafkaClientAuthenticationOAuth.class),
+    @JsonSubTypes.Type(name = KafkaClientAuthenticationK8sOIDC.TYPE_K8S_OIDC, value = KafkaClientAuthenticationK8sOIDC.class),


Should we call it kubernetes? Or serviceaccount?

Sure, if it makes it clearer, it can be either. Both names make sense. Do you have a preference?

I wonder if we should have a proposal for this to clarify details like this. @strimzi/maintainers Any thoughts on the type name?

How about kubernetes-internal / KUBERNETES_INTERNAL / K8S_INTERNAL ? From the discussion during community call it would make sense to me.

What about TYPE_KUBERNETES_OPEN_ID_CONNECT_AUTH?

That's may actually not be a good idea because searching for it through documentation or search engines will not produce good results.

So based on the community call from this week let's move this a little bit. In the comments there were the following options more or less:

kubernetes-openid

kubernetes-auth

kubernetes

kubernetes-oidc

kubernetes-internal

kubernetes-sa maybe (not listed above)?

serviceaccount

serviceacount-oidc

serviceacount-oauth

Any other options?

edit: added serviceaccount, serviceacount-oidc, serviceacount-oauth

serviceaccount or some variant on it? serviceacount-oidc or serviceaccount-oauth?

ye I was thinking about serviceaccount as well, but alone it sounds little bit strange to me. serviceacount-oidc or serviceacount-oauth sounds better I think. I will put it into the list and I guess we could do some vote soon.

Discussed on the community call of 16.5.2024: We decided to go with type: serviceaccount-oauth.

Signed-off-by: Marko Strukelj <marko.strukelj@gmail.com>

scholzj · 2024-05-22T21:21:08Z

...rc/main/java/io/strimzi/api/kafka/model/common/authentication/KafkaClientAuthentication.java

            "`scram-sha-256` and `scram-sha-512` types use SASL SCRAM-SHA-256 and SASL SCRAM-SHA-512 Authentication, respectively. " +
            "`plain` type uses SASL PLAIN Authentication. " +
            "`oauth` type uses SASL OAUTHBEARER Authentication. " +
+            "`serviceaccount-oauth` type uses SASL OAUTHBEARER Authentication. " +


Maybe we could add something like uses SASL OAUTHBEARER Authentication in combination with Kubernetes ServiceAccounts?

Yes, sounds good.

...in/java/io/strimzi/api/kafka/model/common/authentication/KafkaClientAuthenticationOAuth.java

scholzj · 2024-05-22T21:22:42Z

api/src/main/java/io/strimzi/api/kafka/model/kafka/listener/KafkaListenerAuthentication.java

@@ -41,6 +42,7 @@ public abstract class KafkaListenerAuthentication implements UnknownPropertyPres

    @Description("Authentication type. " +
            "`oauth` type uses SASL OAUTHBEARER Authentication. " +
+            "`serviceaccount-oauth` type uses SASL OAUTHBEARER Authentication. " +


Same as earlier -> maybe you can explain how it differs from oauth.

Your proposed string would work here as well. In order to be slightly more descriptive maybe we could say something like: 'uses SASL OAUTHBEARER Authentication pre-configured with Kubernetes ServiceAccount token file location'

Yeah, I think what you suggest would work fine here.

...rc/main/java/io/strimzi/api/kafka/model/kafka/listener/KafkaListenerAuthenticationOAuth.java

scholzj · 2024-05-22T21:28:19Z

...r-operator/src/test/java/io/strimzi/operator/cluster/model/KafkaMirrorMaker2ClusterTest.java

@@ -1635,6 +1636,31 @@ public void testPodSetWithOAuthWithAccessToken() {
        });
    }

+    @ParallelTest
+    public void testPodSetWithServiceAccountOAuth() {


Should this test also when it is used for the source clusters?

I added the test by pattern-matching other OAuth tests, and they only test with target cluster. I guess the reasoning here was that we really test KafkaClientAuthenticationOAuthBuilder here, which is used both for the target and for the source cluster, thus if it works for one why would it fail for the other?

The env vars work differently for source clusters as there might be many of them. That is why I think it needs to be tested as well. It is possible it was forgotten for the previous part, but you can probably add it at least for this feature.

scholzj · 2024-05-22T21:28:55Z

...er-operator/src/test/java/io/strimzi/operator/cluster/model/KafkaMirrorMakerClusterTest.java

@@ -1062,6 +1063,27 @@ public void testGenerateDeploymentWithConsumerOAuthWithAccessToken() {
        assertThat(cont.getEnv().stream().filter(var -> KafkaMirrorMakerCluster.ENV_VAR_KAFKA_MIRRORMAKER_OAUTH_CONFIG_CONSUMER.equals(var.getName())).findFirst().orElseThrow().getValue().isEmpty(), is(true));
    }

+    @ParallelTest
+    public void testGenerateDeploymentWithConsumerServiceAccountOAuth() {


Should we have test for producer as well? (or cover it in this test)

That's a good catch. There's is a whole series of tests with consumer and with producer there. I missed the producer one.

scholzj

One more thing - this should also have some CHANGELOG record.

mstruk added this to the 0.40.0 milestone Feb 7, 2024

scholzj reviewed Feb 13, 2024

View reviewed changes

scholzj modified the milestones: 0.40.0, 0.41.0 Mar 8, 2024

mstruk force-pushed the k8s-oidc-auth branch from b63a6d3 to 043fe9b Compare April 15, 2024 09:17

mstruk marked this pull request as ready for review April 15, 2024 13:34

mstruk requested review from tombentley and ppatierno April 15, 2024 13:36

scholzj modified the milestones: 0.41.0, 0.42.0 May 7, 2024

mstruk added 8 commits May 20, 2024 12:55

Add support for serverBearerTokenLocation to oauth authentication

e4b8c73

Signed-off-by: Marko Strukelj <marko.strukelj@gmail.com>

Add 'k8s-oidc' type authentication

902ce3e

Signed-off-by: Marko Strukelj <marko.strukelj@gmail.com>

Add 'k8s-oidc' type authentication on clients

3d5ad4d

Signed-off-by: Marko Strukelj <marko.strukelj@gmail.com>

Fix equals() and serialVersionUID of k8s-oidc model api classes

bbcfb31

Signed-off-by: Marko Strukelj <marko.strukelj@gmail.com>

Fix crd-annotations and rebuild appendix-crds.adoc

57a73e2

Signed-off-by: Marko Strukelj <marko.strukelj@gmail.com>

Add serverBearerTokenLocation option to the configuration test

4330100

Signed-off-by: Marko Strukelj <marko.strukelj@gmail.com>

Rename k8s-oidc to serviceaccount-oauth

ce1c887

Signed-off-by: Marko Strukelj <marko.strukelj@gmail.com>

Fix appendix_crds.adoc after rebase

7138c29

Signed-off-by: Marko Strukelj <marko.strukelj@gmail.com>

mstruk force-pushed the k8s-oidc-auth branch from 70165b2 to 7138c29 Compare May 20, 2024 11:07

scholzj reviewed May 22, 2024

View reviewed changes

scholzj reviewed May 23, 2024

View reviewed changes

scholzj mentioned this pull request May 23, 2024

[Enhancement]: Ability to fully configured CONTROLLER and REPLICATION listeners, then being able to support PLAINTEXT and SASL_PLAINTEXT. #10153

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Additional authentication type 'k8s-oidc' that extends 'oauth' #9657

Additional authentication type 'k8s-oidc' that extends 'oauth' #9657

mstruk commented Feb 7, 2024

scholzj Feb 13, 2024

mstruk Feb 13, 2024

scholzj Feb 13, 2024

Frawless Apr 18, 2024

see-quick Apr 18, 2024

mstruk Apr 25, 2024

Frawless May 3, 2024 •

edited

scholzj May 3, 2024

Frawless May 3, 2024

scholzj May 16, 2024

scholzj May 22, 2024

mstruk May 23, 2024

scholzj May 22, 2024

mstruk May 23, 2024

scholzj May 23, 2024

scholzj May 22, 2024

mstruk May 23, 2024

scholzj May 23, 2024

scholzj May 22, 2024

mstruk May 23, 2024

scholzj left a comment

Additional authentication type 'k8s-oidc' that extends 'oauth' #9657

Are you sure you want to change the base?

Additional authentication type 'k8s-oidc' that extends 'oauth' #9657

Conversation

mstruk commented Feb 7, 2024

Type of change

Description

Checklist

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

Frawless May 3, 2024 • edited

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

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

Frawless May 3, 2024 •

edited