KAFKA-7462: Make token optional for OAuthBearerLoginModule by rajinisivaram · Pull Request #5733 · apache/kafka

rajinisivaram · 2018-10-03T13:19:10Z

OAuthBearerLoginModule is used both on the server-side and client-side (similar to login modules for other mechanisms). OAUTHBEARER tokens are client credentials used only on the client-side to authenticate with servers, but the current implementation requires tokens to be provided on the server-side even if OAUTHBEARER is not used for inter-broker communication. Tokens should be optional for server-side login context to allow brokers to be configured without a token when OAUTHBEARER is not used for inter-broker communication.

Committer Checklist (excluded from commit message)

Verify design and implementation
Verify test coverage and CI build status
Verify documentation (including upgrade notes)

rajinisivaram · 2018-10-03T13:20:00Z

@rondagostino Can you review, please? Thank you!

rondagostino · 2018-10-03T14:42:13Z

@rajinisivaram Before I review, is it possible simply to mark the LoginModule as OPTIONAL in the case where it is one of multiple login modules on the broker side but it is not used for inter-broker communication? The Javadoc (https://docs.oracle.com/javase/7/docs/api/javax/security/auth/spi/LoginModule.html#commit()) says a return of false means the module is ignored, and I confirmed in the debugger that it is in fact the case (see, for example, https://github.com/frohoff/jdk8u-dev-jdk/blob/master/src/share/classes/javax/security/auth/login/LoginContext.java#L779). See also https://docs.oracle.com/javase/7/docs/api/javax/security/auth/login/Configuration.html where it says:

Optional - The LoginModule is not required to succeed. If it succeeds or fails, authentication still continues to proceed down the LoginModule list.

rondagostino · 2018-10-03T16:12:25Z

@rajinisivaram Ah, wait, I think I see the issue. If we want to add a listener, then it may be the only LoginModule listed. Is that correct? In the meantime, I will review.

rajinisivaram · 2018-10-03T16:35:36Z

@rondagostino We need one login module that succeeds. With the sasl.jaas.config option, we only have one login module (because it is a per-mechanism config). With static JAAS config, we could have multiple modules if on the same listener, but I think OAuth requires a single login module (

kafka/clients/src/main/java/org/apache/kafka/common/security/oauthbearer/internals/unsecured/OAuthBearerUnsecuredValidatorCallbackHandler.java

Line 115 in 09f3205

    
           if (Objects.requireNonNull(jaasConfigEntries).size() != 1 || jaasConfigEntries.get(0) == null)

). And as you said, you could have just OAuth on a listener. Thanks for reviewing!

rondagostino

LGTM aside from the minor comments.

rondagostino · 2018-10-03T16:15:34Z

@@ -93,7 +93,7 @@ public String errorUri() {
     *            the mandatory token to set


Javadoc should say "optional" now instead of "mandatory"

rondagostino · 2018-10-03T16:22:49Z

 public class OAuthBearerLoginModule implements LoginModule {
+
+    private enum LoginState {
+        NOT_LOGGED_IN,


Would be good to comment that this implies logged-out or aborted since the state is null at the beginning (when we aren't logged in, either, but we don't set the state to NOT_LOGGED_IN). Either comment or rename to LOGGED_OUT_OR_ABORTED?

Added comment.

rondagostino · 2018-10-03T16:28:21Z

+            identifyExtensions();
+        } else {
+            log.info("Logged in without a token, this login cannot be used to establish client connections");
+        }


nit: style seems to be to not include braces when there is only one if or else statement

rondagostino · 2018-10-03T16:44:57Z

+            log.info("Done logging out my extensions");
+        } else {
+            log.info("No extensions to logout for this login");
+        }


nit: braces unneeded

rondagostino · 2018-10-03T16:45:49Z

+            log.info("Done committing my token; committed token count is now {}", committedTokenCount());
+        } else {
+            log.info("No tokens to commit, this login cannot be used to establish client connections");
+        }


nit: braces unneeded

rondagostino · 2018-10-03T16:49:27Z

-        extensionsRequiringCommit = null;
+        if (extensionsRequiringCommit != null) {
+            if (myCommittedToken == null)
+                throw new IllegalStateException("Extensions being committed without a token");


Is the myCommittedToken == null check unnecessary here since it can never be the case when there are extensions? I think we make sure of this since we only call identifyExtensions() when there is a token.

rajinisivaram · 2018-10-04T12:24:51Z

@rondagostino Thanks for the review, have addressed the comments.

rajinisivaram · 2018-10-05T08:39:57Z

retest this please

junrao

@rajinisivaram : Thanks for the patch. LGTM

junrao · 2018-10-05T22:08:20Z

        if (callback.token() != null)
            throw new IllegalArgumentException("Callback had a token already");
+        if (moduleOptions.isEmpty()) {
+            log.info("Token not provided, this login cannot be used to establish client connections");


Does this get logged on the broker side? Will that be too noisy?

@junrao Thanks for the review, I have changed all the new log entries to debug. We also have https://issues.apache.org/jira/browse/KAFKA-7478 to reduce the level of some of the existing log entries.

rajinisivaram · 2018-10-08T09:12:28Z

Merging to trunk and 2.1.

OAuthBearerLoginModule is used both on the server-side and client-side (similar to login modules for other mechanisms). OAUTHBEARER tokens are client credentials used only on the client-side to authenticate with servers, but the current implementation requires tokens to be provided on the server-side even if OAUTHBEARER is not used for inter-broker communication. This commit makes tokens optional for server-side login context to allow brokers to be configured without a token when OAUTHBEARER is not used for inter-broker communication. Reviewers: Ron Dagostino <rndgstn@gmail.com>, Jun Rao <junrao@gmail.com>

AntonSmolkov · 2023-09-04T10:36:07Z

@kirktrue, @rajinisivaram Hi!
Looks like the same fix is needed for the new secured org.apache.kafka.common.security.oauthbearer.OAuthBearerLoginCallbackHandler
It also requires clientId/clientSecret even if oidc listener is not user for inter-broker communication.

kirktrue · 2023-09-08T18:34:51Z

@AntonSmolkov would you mind filing a new Jira to track? Thanks!

KAFKA-7462: Make token optional for OAuthBearerLoginModule

23fd758

rondagostino reviewed Oct 3, 2018

View reviewed changes

Address review comments

90cf558

rajinisivaram requested a review from junrao October 4, 2018 12:25

junrao approved these changes Oct 5, 2018

View reviewed changes

Address review comments

f4f073d

rajinisivaram merged commit c26823b into apache:trunk Oct 8, 2018

		@@ -93,7 +93,7 @@ public String errorUri() {
		* the mandatory token to set

Uh oh!

Conversation

rajinisivaram commented Oct 3, 2018

Committer Checklist (excluded from commit message)

Uh oh!

rajinisivaram commented Oct 3, 2018

Uh oh!

rondagostino commented Oct 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rondagostino commented Oct 3, 2018

Uh oh!

rajinisivaram commented Oct 3, 2018

Uh oh!

rondagostino left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rajinisivaram commented Oct 4, 2018

Uh oh!

rajinisivaram commented Oct 5, 2018

Uh oh!

junrao left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rajinisivaram commented Oct 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AntonSmolkov commented Sep 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kirktrue commented Sep 8, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

rondagostino commented Oct 3, 2018 •

edited

Loading

rajinisivaram commented Oct 8, 2018 •

edited

Loading

AntonSmolkov commented Sep 4, 2023 •

edited

Loading