Oauth client assertion support #211

robbertvanwaveren · 2023-10-30T12:33:34Z

This PR adds support for Kafka clients to obtain an access token by authenticating to the authorization server with client assertion as specified by https://www.rfc-editor.org/rfc/rfc7523 and https://www.rfc-editor.org/rfc/rfc7521. The assertion can be provided by an external mechanism and available as a file on the file system or it can be explicitly set through OAuth configuration before running the Kafka client.

In addition, the static access token or refresh token can now be provided as a file on the local filesystem.

The following new configuration options have been added by this PR:

oauth.client.assertion
oauth.client.assertion.location
oauth.client.assertion.type
oauth.refresh.token.location
oauth.access.token.location

Signed-off-by: Robbert van Waveren <robbert1@gmail.com>

…ProviderTest.java Co-authored-by: Marko Strukelj <marko.strukelj@gmail.com> Signed-off-by: Robbert van Waveren <robbert1@gmail.com>

Signed-off-by: Robbert van Waveren <robbert1@gmail.com>

robbertvanwaveren · 2023-10-30T12:34:59Z

this PR now has DCO signoff and grants maintainer edit rights

…on-support' into oauth-client-assertion-support

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

scholzj

Can we have the description and the PR name reflect what it does?

mstruk · 2023-11-08T11:30:15Z

I added a better description.

scholzj · 2023-11-08T16:59:20Z

@mstruk Same as the other PR. The Travis tests do not seem to run here.

tombentley

We shouldn't assume that people trying to use this understand what they're doing. Even if they do it's easy to become focussed on "making it work" and forget about all the steps needed to actually make it secure. so I think:

the README should recommend the use of filesystem permissions to protect tokens on disk.
we should consider, when the OS is unix, checking the file permissions and failing if they token files are readable by all, or the group. Obviously this doesn't provide any protection on non-unix, but it's better than nothing in my opinion.

README.md

tombentley · 2023-11-19T21:34:56Z

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

@@ -193,11 +215,20 @@ private long getHttpRetryPauseMillis(ClientConfig config, int retries) {
        return retryPauseMillis;
    }

+    private TokenProvider createProvider(final String token, final String tokenLocation) {
+        if (tokenLocation != null) {


Do we want to accept the case where both parameters are non-null? It's ambiguous what the user's intent is in this case.

I aded a logic that if both are set, the location (file) is used rather than the token, and warning is logged.

tombentley · 2023-11-19T21:42:12Z

oauth-common/src/main/java/io/strimzi/kafka/oauth/common/OAuthAuthenticator.java

+     * @throws IllegalStateException If the response from the authorization server could not be handled
+     */
+    @SuppressWarnings("checkstyle:ParameterNumber")
+    public static TokenInfo loginWithClientAssertion(URI tokenEndpointUrl, SSLSocketFactory socketFactory,


Can we put each parameter on a separate line.

tombentley · 2023-11-19T21:42:35Z

oauth-common/src/main/java/io/strimzi/kafka/oauth/common/OAuthAuthenticator.java

+     * @throws IllegalStateException If the response from the authorization server could not be handled
+     */
+    @SuppressWarnings("checkstyle:ParameterNumber")
+    public static TokenInfo loginWithClientAssertion(URI tokenEndpointUrl, SSLSocketFactory socketFactory,


Can we put each parameter on a separate line.

I aded logic that if both are set, the location (file) is used rather than the token, and warning is logged.

mstruk · 2023-11-20T11:00:20Z

the README should recommend the use of filesystem permissions to protect tokens on disk.

Good point, I'll add some docs to address this.

we should consider, when the OS is unix, checking the file permissions and failing if they token files are readable by all, or the group. Obviously this doesn't provide any protection on non-unix, but it's better than nothing in my opinion.

Failing is a harsh reaction to this. Especially if an external mechanism takes care of mounting a filesystem so it's all automatic and possibly out of your control. I would prefer to log a warning, but not fail. WDYT?

Co-authored-by: Tom Bentley <tombentley@users.noreply.github.com> Signed-off-by: Marko Strukelj <marko.strukelj@gmail.com>

tombentley · 2023-11-20T22:09:38Z

Failing is a harsh reaction to this. Especially if an external mechanism takes care of mounting a filesystem so it's all automatic and possibly out of your control. I would prefer to log a warning, but not fail. WDYT?

The 'possibly out of your control point' is a good one. So I guess logging a warning is reasonable.

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

mstruk · 2023-11-30T11:15:18Z

The 'env:ENV_VARIABLE_NAME' feature doesn't seem to add any capability that couldn't be performed before. Also rather than conflating such a feature with the current PR it is better to introduce it separately if at all.

Also, there is a possibility of unexpected behaviour, for example using oauth.client.secret=\"env:Hfs67vfv.M8a\" would automatically trigger a lookup of Hfs67vfv.M8a env variable which in this case was not the intention.

It is already the case that when a configuration option is resolved the resolution tries several different override possibilities in a specific order as implemented here and documented here.

First, if the system property exists with the same name it overrides the JAAS configured attribute. Second, if the env variable with the same name exists that is used. Third, if the env variable exists with the name equal to configuration key converted to all caps and underscores then that one is used. Only if there are no overrides via System properties and env vars the config parameter as configured via JAAS is used.

According to these rules using oauth.client.assertion.location="env:TOKEN_FILE" can be achieved by setting the following env variable in the client's execution environment: OAUTH_CLIENT_ASSERTION_LOCATION="$TOKEN_FILE" rather than set as a JAAS parameter.

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

robbertvanwaveren · 2023-11-30T12:55:07Z

I don’t see how the logic you refer to performs the substitution of the TOKEN_FILE env variable for its actual content.

The original intent of this PR is support for MS Azure Workload Identity which injects certain env vars into the execution environment. Which is the main (only) practical use-case for this PR.

Note that this means that these env var names are not configurable so these should be piped towards this libs configs parama/vars.

mstruk · 2023-11-30T13:17:00Z

@robbertvanwaveren Can you give an example or provide a failing test?

Substitution happens in your kafka-client.yaml (or whatever you call the yaml file that you use) when setting environment vars for the pod.
Or in your bash startup script used as an entry point for your client.

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

mstruk · 2024-01-04T10:42:41Z

I'm thinking of renaming the oauth.*.location config options to oauth.*.path. The reasoning is that location is too generic, it can mean a file on the local filesystem or a url or something completely different as one of the values from a set of supported values. By saying path it makes it super clear (at least to me) that it is the path to the file on the local filesystem.

I wonder what others think?

mstruk · 2024-01-09T11:08:54Z

Actually that won't work as there are existing previous options that refer to files and are called oauth.*.location (e.g. oauth.ssl.truststore.location) and I prefer to keep consistency in the naming.

mstruk · 2024-01-11T08:23:43Z

@tombentley Do you think this needs another review (specifically, the file permissions check)?

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

mstruk · 2024-01-22T08:03:58Z

Travis CI integration not working again. My local run of the full testsuite has been passing on this one for a long time. If some Travis CI issues appear during release of the next version we'll fix it then.

This PR has been a long time in the making. Many thanks to everyone who contributed, and especially @robbertvanwaveren for the initial development and continued engagement.

robbertvanwaveren and others added 3 commits October 30, 2023 13:24

support client_credentials with client_assertion

2554553

Signed-off-by: Robbert van Waveren <robbert1@gmail.com>

Update oauth-common/src/test/java/io/strimzi/kafka/oauth/common/Token…

8843e81

…ProviderTest.java Co-authored-by: Marko Strukelj <marko.strukelj@gmail.com> Signed-off-by: Robbert van Waveren <robbert1@gmail.com>

update README and example and make 'env:' dereference safer

56bea18

Signed-off-by: Robbert van Waveren <robbert1@gmail.com>

robbertvanwaveren mentioned this pull request Oct 30, 2023

support client_credentials with client_assertion #167

Closed

Merge remote-tracking branch 'remotes/vanwaveren/oauth-client-asserti…

f6fffde

…on-support' into oauth-client-assertion-support

mstruk added this to the 0.15.0 milestone Nov 2, 2023

mstruk self-requested a review November 2, 2023 11:21

mstruk marked this pull request as draft November 2, 2023 11:21

Fix javadoc and styling issues

bf14af3

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

mstruk marked this pull request as ready for review November 7, 2023 15:17

mstruk requested review from scholzj and removed request for mstruk November 7, 2023 15:17

scholzj requested a review from tombentley November 7, 2023 15:25

scholzj reviewed Nov 7, 2023

View reviewed changes

scholzj approved these changes Nov 8, 2023

View reviewed changes

tombentley reviewed Nov 19, 2023

View reviewed changes

Apply suggestions from code review

e14bb5e

Co-authored-by: Tom Bentley <tombentley@users.noreply.github.com> Signed-off-by: Marko Strukelj <marko.strukelj@gmail.com>

mstruk added 2 commits November 27, 2023 13:47

Fix ClientAssertionAuthTest failing

7f93599

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

Add file permissions checks + additional tests + refactor some code

bfafe87

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

Remove the env:ENV_VAR_NAME lookup feature

7072726

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

mstruk added 2 commits December 6, 2023 13:15

Add to README.md the comments about file permissions

e2243a2

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

Add support for file permissions check on Windows

f9c939f

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

Add file permissions test when symlinks are used

b5e468d

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

mstruk requested a review from tombentley December 17, 2023 21:05

Fix README

9c7cc2b

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

mstruk merged commit b8076c7 into strimzi:main Jan 22, 2024
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Oauth client assertion support #211

Oauth client assertion support #211

robbertvanwaveren commented Oct 30, 2023 •

edited by mstruk

Loading

robbertvanwaveren commented Oct 30, 2023

scholzj left a comment

mstruk commented Nov 8, 2023

scholzj commented Nov 8, 2023

tombentley left a comment

tombentley Nov 19, 2023

mstruk Dec 17, 2023

tombentley Nov 19, 2023

tombentley Nov 19, 2023

mstruk Dec 17, 2023

mstruk commented Nov 20, 2023

tombentley commented Nov 20, 2023

mstruk commented Nov 30, 2023

robbertvanwaveren commented Nov 30, 2023 •

edited

Loading

mstruk commented Nov 30, 2023

mstruk commented Jan 4, 2024

mstruk commented Jan 9, 2024

mstruk commented Jan 11, 2024

mstruk commented Jan 22, 2024

Oauth client assertion support #211

Oauth client assertion support #211

Conversation

robbertvanwaveren commented Oct 30, 2023 • edited by mstruk Loading

robbertvanwaveren commented Oct 30, 2023

scholzj left a comment

Choose a reason for hiding this comment

mstruk commented Nov 8, 2023

scholzj commented Nov 8, 2023

tombentley left a comment

Choose a reason for hiding this comment

tombentley Nov 19, 2023

Choose a reason for hiding this comment

mstruk Dec 17, 2023

Choose a reason for hiding this comment

tombentley Nov 19, 2023

Choose a reason for hiding this comment

tombentley Nov 19, 2023

Choose a reason for hiding this comment

mstruk Dec 17, 2023

Choose a reason for hiding this comment

mstruk commented Nov 20, 2023

tombentley commented Nov 20, 2023

mstruk commented Nov 30, 2023

robbertvanwaveren commented Nov 30, 2023 • edited Loading

mstruk commented Nov 30, 2023

mstruk commented Jan 4, 2024

mstruk commented Jan 9, 2024

mstruk commented Jan 11, 2024

mstruk commented Jan 22, 2024

robbertvanwaveren commented Oct 30, 2023 •

edited by mstruk

Loading

robbertvanwaveren commented Nov 30, 2023 •

edited

Loading