Add OAuth support for Kafka client in native mode #12732

dejanb · 2020-10-15T10:58:19Z

No description provided.

dejanb · 2020-10-15T11:04:19Z

This is a first step toward full OAUTH support for the Kafka extension in native mode. These changes get the communication with Keycloak going and obtain a proper token. However, there's an issue with the Kafka client trying to connect to the broker with that token.

I tracked it down to this issue

oracle/graal#2745

Basically,

Subject subject = Subject.getSubject(AccessController.getContext());

call return null in the native mode.

I tried to add some classes to the jni configuration, but as with the above reporter that didn't help.

Anyone experienced something like this before? Any pointers on where to look next would be helpful.

Note that the PR in the current state is usable/mergeable, but it doesn't get us to the end.

...kafka-client/deployment/src/main/java/io/quarkus/kafka/client/deployment/KafkaProcessor.java

cescoffier · 2020-10-15T11:42:04Z

About Subject, we had a similar issue with Jass and Kafka. We need to check why the lookup fails. IT generally because of a missing class or module.

sberyozkin · 2020-10-15T12:01:32Z

@dejanb Can quarkus-oidc help here ? Why to wire the Keycloak specific code into the Kafka extension ?

dejanb · 2020-10-15T12:08:50Z

About Subject, we had a similar issue with Jass and Kafka. We need to check why the lookup fails. IT generally because of a missing class or module.

@cescoffier If you have any pointers on what to try and how to more info please let me know. I can try to provide substitutions for some of these classes and see if that would help.

dejanb · 2020-10-15T12:11:08Z

@dejanb Can quarkus-oidc help here ? Why to wire the Keycloak specific code into the Kafka extension ?

@sberyozkin these are used by strimzi oauth client, so this PR is intended to provide a better support for that case. I'd need to give oidc a look, but I assume that the client would need first changed to use it.

sberyozkin · 2020-10-15T12:36:05Z

@dejanb I see, so in your PR it is only about getting a token by directly requesting it from Keycloak. So quarkus-oidc won't help at the moment, I've started working on OidcClient (quarkus-oidc-client) which will work not only with Keycloak, but it is not ready yet.
Thanks

dejanb · 2020-10-15T12:53:34Z

@sberyozkin Great. We can take a look of moving strimzi oauth to it later on.

cescoffier · 2020-10-26T14:38:44Z

@dejanb Sorry, missed the notifications.

I would go with simple substitutions adding traces to check where it fails. Unfortunately, this is going to be a lot of fails and retries cycles.

dejanb · 2020-10-26T16:46:31Z

@cescoffier no worries, I was on other stuff as well. I pretty much pin-pointed the problematic places already, I'll try to play with substitutions to come up with a fix. Hopefully, I'll have something to show this week. Thanks for all your help so far.

cescoffier · 2020-10-26T18:27:32Z

Can you link the place you think it fails?

dejanb · 2020-10-27T12:29:39Z

Can you link the place you think it fails?
@cescoffier

So what I think is going on is that SaslClientAuthenticator do Subject.doAs() call with the proper subject it wants to use

https://github.com/apache/kafka/blob/trunk/clients/src/main/java/org/apache/kafka/common/security/authenticator/SaslClientAuthenticator.java#L524

This subject later on should be available with the call to Subject.getSubject(AccessController.getContext()), but this call returns null in the callback handler.

https://github.com/apache/kafka/blob/aa287acb2eed07cf6d75c10e71f051f538a57872/clients/src/main/java/org/apache/kafka/common/security/oauthbearer/internals/OAuthBearerSaslClientCallbackHandler.java#L99

That's where it fails.

Let me know if this helpful or do you need any more info from my side.

cescoffier · 2020-10-27T16:23:52Z

It seems close to the SASL issue we had with kafka a few months ago.

dejanb · 2020-11-03T11:13:03Z

@cescoffier unfortunately SASL fix wasn't applicable here. I ended up substituting Subject class which should help with all these doAs() -> getSubject() usage patterns until graalvm issue is fixed. It might not be ideal but all other solutions would need much more substitutions to avoid Subject usage at all (which is even less ideal). This now works nicely. Let me know what you think.

cescoffier · 2020-11-03T13:45:55Z

Thanks @dejanb.
@galderz WDYT about the substitutions?

cescoffier

Nice trick.
I've done a few suggestions around SubstitutionsUtils.

There is a potential memory leak if there is lots of call to doAs breaking before the call to getSubject- the map size could grow unexpectedly.
I guess it's fine here, as we won't have such a scenario.

cescoffier · 2020-11-03T13:48:17Z

...ka-client/runtime/src/main/java/io/quarkus/kafka/client/runtime/graal/SubstitutionUtils.java

+
+import javax.security.auth.Subject;
+
+public class SubstitutionUtils {


Not sure about the name.
Maybe SubjectHolder ?

cescoffier · 2020-11-03T13:48:45Z

...ka-client/runtime/src/main/java/io/quarkus/kafka/client/runtime/graal/SubstitutionUtils.java

+
+public class SubstitutionUtils {
+
+    public static HashMap<AccessControlContext, Subject> subjects = new HashMap<>();


Should it be final?

Should this field be moved to the Subject class?

@cescoffier The field has to be kept outside otherwise IIRC you get an error. The only fields allowed in substituted classes are aliases of existing fields.

I'm not an expert on this java security APIs, but a couple of things to bear in mind would be:

Should be it be concurrent?

Is there a chance entries could be stored and not removed? Maybe you need some kind of expiration of entries to avoid a leak?

ah, thanks for reminding me of this @galderz! I always forgot.

Handling expiration might be good as it would avoid the potential growth I mentioned in my review.

dejanb · 2020-11-03T14:25:38Z

Nice trick.
I've done a few suggestions around SubstitutionsUtils.

There is a potential memory leak if there is lots of call to doAs breaking before the call to getSubject- the map size could grow unexpectedly.
I guess it's fine here, as we won't have such a scenario.

@cescoffier thanks, great suggestions. I'll incorporate them in next.

dejanb · 2020-11-04T11:16:00Z

@cescoffier @galderz Thanks for the feedback. I pushed some more changes. As Galder mentioned, the static field cannot be added to the target class. But I moved the SubjectHolder class to the target class and that works and looks a bit better.

In terms of actual map, I made it synchronized and converted to WeakHashMap() which removes entries when the key is no longer in use. I think this should help with potential leaks.

I can use full blown cache (with expiring entries) here but I'll need to add a dependency on Infinispan/Guava for that. Let me know if you want me to explore that route.

galderz · 2020-11-05T17:02:06Z

@dejanb Rather than WeakHashMap, I'd probably use Caffeine for this which is a library already in use within Quarkus in different places (e.g. to power caching for quarkus end user apps). I'm not expert on WeakHashMap but you can find some details here on the gotchas of using them as a cache. Also, I don't think WeakHashMap is designed for concurrent access.

dejanb · 2020-11-09T11:47:31Z

@galderz Makes sense. I pushed an implementation using Caffeine cache. I used some defaults for sizes and expiry times we use on other projects and it seems to me they should work well here as well. Let me know if you have any other suggestions.

extensions/kafka-client/runtime/pom.xml

cescoffier · 2020-12-08T14:19:25Z

Re-running the CI before merging.

cescoffier · 2020-12-08T14:20:02Z

@dejanb Sorry for the delay. It should be fine to merge it, but I re-run the CI to be sure.

machi1990 · 2020-12-08T20:51:13Z

This errored again with

Error: Unable to process command '::set-env name=JAVA_HOME_11_x64::/opt/hostedtoolcache/AdoptOpenJDK/1.0.0-ga-11-jdk-hotspot-linux-x64-normal-latest/x64' successfully.
Error: The `set-env` command is disabled. Please upgrade to using Environment Files or opt into unsecure command execution by setting the `ACTIONS_ALLOW_UNSECURE_COMMANDS` environment variable to `true`. For more information see:

I've this these error before (a month or so ago) and they were fixed by @loicmathieu or @gsmet. I think a rebase will do and it will re-trigger a clean build. Do you want to take a look? @dejanb

dejanb · 2020-12-09T10:41:13Z

@cescoffier @machi1990 Done. Hopefully it'll do it.

cescoffier · 2021-01-04T14:18:18Z

@dejanb It's IN! What a great way to start the year!

dejanb · 2021-01-04T16:30:02Z

@cescoffier awesome ... To many more in this year :)

dejanb marked this pull request as draft October 15, 2020 10:58

cescoffier reviewed Oct 15, 2020

View reviewed changes

...kafka-client/deployment/src/main/java/io/quarkus/kafka/client/deployment/KafkaProcessor.java Outdated Show resolved Hide resolved

...kafka-client/deployment/src/main/java/io/quarkus/kafka/client/deployment/KafkaProcessor.java Show resolved Hide resolved

dejanb force-pushed the kafka-oauth branch from 51264a4 to 0122ea9 Compare October 15, 2020 12:39

dejanb force-pushed the kafka-oauth branch from 0122ea9 to 9e7e5b8 Compare November 3, 2020 11:09

dejanb marked this pull request as ready for review November 3, 2020 11:09

dejanb requested a review from cescoffier November 3, 2020 11:13

cescoffier requested changes Nov 3, 2020

View reviewed changes

cescoffier requested a review from galderz November 3, 2020 13:53

dejanb force-pushed the kafka-oauth branch from 9e7e5b8 to 25f5178 Compare November 4, 2020 11:07

dejanb force-pushed the kafka-oauth branch from 25f5178 to 530bcf7 Compare November 9, 2020 11:43

machi1990 reviewed Nov 9, 2020

View reviewed changes

extensions/kafka-client/runtime/pom.xml Outdated Show resolved Hide resolved

dejanb force-pushed the kafka-oauth branch from 530bcf7 to f0ecda8 Compare November 9, 2020 12:58

machi1990 requested a review from cescoffier November 14, 2020 16:52

cescoffier approved these changes Nov 15, 2020

View reviewed changes

Add OAuth support for Kafka client in native mode

dbdccd1

dejanb force-pushed the kafka-oauth branch from f0ecda8 to dbdccd1 Compare December 9, 2020 10:40

ghost added the area/kafka label Dec 9, 2020

machi1990 approved these changes Dec 9, 2020

View reviewed changes

cescoffier merged commit d71f941 into quarkusio:master Jan 4, 2021

ghost added this to the 1.11 - master milestone Jan 4, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add OAuth support for Kafka client in native mode #12732

Add OAuth support for Kafka client in native mode #12732

dejanb commented Oct 15, 2020

dejanb commented Oct 15, 2020

cescoffier commented Oct 15, 2020

sberyozkin commented Oct 15, 2020

dejanb commented Oct 15, 2020 •

edited

Loading

dejanb commented Oct 15, 2020

sberyozkin commented Oct 15, 2020 •

edited

Loading

dejanb commented Oct 15, 2020

cescoffier commented Oct 26, 2020 •

edited

Loading

dejanb commented Oct 26, 2020

cescoffier commented Oct 26, 2020

dejanb commented Oct 27, 2020

cescoffier commented Oct 27, 2020 •

edited

Loading

dejanb commented Nov 3, 2020 •

edited

Loading

cescoffier commented Nov 3, 2020

cescoffier left a comment

cescoffier Nov 3, 2020

cescoffier Nov 3, 2020

cescoffier Nov 3, 2020

galderz Nov 4, 2020

cescoffier Nov 4, 2020

dejanb commented Nov 3, 2020

dejanb commented Nov 4, 2020

galderz commented Nov 5, 2020

dejanb commented Nov 9, 2020

cescoffier commented Dec 8, 2020

cescoffier commented Dec 8, 2020

machi1990 commented Dec 8, 2020

dejanb commented Dec 9, 2020

cescoffier commented Jan 4, 2021

dejanb commented Jan 4, 2021


		import javax.security.auth.Subject;

		public class SubstitutionUtils {


		public class SubstitutionUtils {

		public static HashMap<AccessControlContext, Subject> subjects = new HashMap<>();

Add OAuth support for Kafka client in native mode #12732

Add OAuth support for Kafka client in native mode #12732

Conversation

dejanb commented Oct 15, 2020

dejanb commented Oct 15, 2020

cescoffier commented Oct 15, 2020

sberyozkin commented Oct 15, 2020

dejanb commented Oct 15, 2020 • edited Loading

dejanb commented Oct 15, 2020

sberyozkin commented Oct 15, 2020 • edited Loading

dejanb commented Oct 15, 2020

cescoffier commented Oct 26, 2020 • edited Loading

dejanb commented Oct 26, 2020

cescoffier commented Oct 26, 2020

dejanb commented Oct 27, 2020

cescoffier commented Oct 27, 2020 • edited Loading

dejanb commented Nov 3, 2020 • edited Loading

cescoffier commented Nov 3, 2020

cescoffier left a comment

Choose a reason for hiding this comment

cescoffier Nov 3, 2020

Choose a reason for hiding this comment

cescoffier Nov 3, 2020

Choose a reason for hiding this comment

cescoffier Nov 3, 2020

Choose a reason for hiding this comment

galderz Nov 4, 2020

Choose a reason for hiding this comment

cescoffier Nov 4, 2020

Choose a reason for hiding this comment

dejanb commented Nov 3, 2020

dejanb commented Nov 4, 2020

galderz commented Nov 5, 2020

dejanb commented Nov 9, 2020

cescoffier commented Dec 8, 2020

cescoffier commented Dec 8, 2020

machi1990 commented Dec 8, 2020

dejanb commented Dec 9, 2020

cescoffier commented Jan 4, 2021

dejanb commented Jan 4, 2021

dejanb commented Oct 15, 2020 •

edited

Loading

sberyozkin commented Oct 15, 2020 •

edited

Loading

cescoffier commented Oct 26, 2020 •

edited

Loading

cescoffier commented Oct 27, 2020 •

edited

Loading

dejanb commented Nov 3, 2020 •

edited

Loading