Uh oh!
There was an error while loading. Please reload this page.
HBASE-26553 OAuth Bearer authentication mech plugin for SASL (initial commit) - #4019
Closed
anmolnar wants to merge 15 commits into
Closed
HBASE-26553 OAuth Bearer authentication mech plugin for SASL (initial commit)#4019anmolnar wants to merge 15 commits into
anmolnar wants to merge 15 commits into
Conversation
Apache-HBase
commented
Jan 10, 2022
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jan 10, 2022
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jan 10, 2022
💔 -1 overall
This message was automatically generated. |
joshelser
requested changes
Jan 10, 2022
joshelser
left a comment
Member
There was a problem hiding this comment.
Another round of review. Thanks for all of the cleanup you did already.
Moving ahead under the idea that we'll do some iteration on a feature branch -- I think this is pretty close. I would very much like to see an end to end unit test with some kind of embedded JWT authorization server before committing this. You mentioned offline that you were already doing this, so I'll wait patiently :) (I think you mentioned that nimbus might have some testing utility. Spring might also have something. Shout if you want me to dig in and look).
We definitely have some extra work to be done before this would be security-ready for others to consume, but it will be good to work through them separately.
| * This class has been copy-and-pasted from Kafka codebase. | ||
| */ | ||
| @InterfaceAudience.Public | ||
| public class OAuthBearerSaslClient implements SaslClient { |
Member
There was a problem hiding this comment.
I wonder if we just need to extend the HBase base SASL client in order to get the RPC encryption. That's probably why it didn't work OOTB.
ContributorAuthor
There was a problem hiding this comment.
HBaseSaslRpcClient uses to composition to extend the functionality of SaslClient and it's already being used in the OAuthBearer case. I don't think inheriting it would help here.
However I don't know how property negotiation works in SASL and need dig into Kafka's code again for our usecase. Maybe they haven't implemented that either. So, in my understanding the negotiated QoP indicates what transport level protection is active and we should require "auth-conf" here to get wire encryption.
ContributorAuthor
There was a problem hiding this comment.
Some further investigation: I need to set "hbase.rpc.protection" to "privacy" which will be translated to "auth-conf" QoP in Sasl properties. These properties will be set both in Sasl client and server. I think this is accurate so far.
But where is the negotiation part?
I mean this is certainly an explicit setting inside Hbase, where does it become "negotiated"?
Member
There was a problem hiding this comment.
I need to set "hbase.rpc.protection" to "privacy" which will be translated to "auth-conf" QoP in Sasl properties
This has been something I've never fully looked into, but, IIRC, the quality of protection (QOP) ultimately defers back to the mechanism in use to give that protection. I can't point to any documentation so the easiest thing is to try this out.
I had used Wireshark just to listen to the RegionServer traffic on my local host when running your standalone tool.
I don't think inheriting it would help here.
See the code here.
This is the commons-crypto logic I'm referring to. This wrapping and unwrapping will be encrypting the traffic after the SASL handshake is done.
Either way, let me try this out with setting the QOP.
But where is the negotiation part?
The negotiation is partially happening with the JDK itself and partially happening by the Callback implementation's you've written. SASL is just a series of challenge/response messages being passed between client and server.
| @Override | ||
| public String[] getMechanismNames(Map<String, ?> props) { | ||
| return OAuthBearerSaslClient.mechanismNamesCompatibleWithPolicy(props); |
Member
There was a problem hiding this comment.
If this code is being executed, it is because someone explicitly activated it via configuration. We should throw a RuntimeException if the configuration disallows us to use this implementation (e.g. if Sasl.POLICY_NOPLAINTEXT == true). We should fail in this manner until we have RPC encryption tested/working.
| } | ||
| public static void initialize() { | ||
| Security.addProvider(new OAuthBearerSaslClientProvider()); |
Member
There was a problem hiding this comment.
I don't think we actually need to register a provider with the JCA security provider framework because we don't actually use it. IIRC, Kafka relies on JAAS to automatically perform the logins for them, whereas HBase explicitly sets up the implementation to use (via the AuthenticationProviderSelector). I think we can drop this class entirely.
ContributorAuthor
There was a problem hiding this comment.
It doesn't work without it, because we don't instantiate the client and the server directly, but asking the Sasl framework to create them for us. In the client auth provider we call Sasl.createSaslClient() to create the client which doesn't work without the client registered for the mechanism.
Member
There was a problem hiding this comment.
In the client auth provider we call Sasl.createSaslClient() to create the client which doesn't work without the client registered for the mechanism.
Ah, ok. I remember that when we were messing around with other custom Providers. Thanks for clarifying!
Comment on lines
+34
to
+36
| public SaslAuthMethod getSaslAuthMethod() { | ||
| return SASL_AUTH_METHOD; | ||
| } |
Member
There was a problem hiding this comment.
For clarity, this SaslAuthMethod (HBase class) is how HBase's RPC classes are figuring out the correct client-side logic to use given a token (secret material).
Comment on lines
+107
to
+136
| if (privateCredentials.size() == 1) { | ||
| LOG.debug("Found 1 OAuthBearer token"); | ||
| callback.token(privateCredentials.iterator().next()); | ||
| } else { | ||
| /* | ||
| * There a very small window of time upon token refresh (on the order of milliseconds) | ||
| * where both an old and a new token appear on the Subject's private credentials. | ||
| * Rather than implement a lock to eliminate this window, we will deal with it by | ||
| * checking for the existence of multiple tokens and choosing the one that has the | ||
| * longest lifetime. It is also possible that a bug could cause multiple tokens to | ||
| * exist (e.g. KAFKA-7902), so dealing with the unlikely possibility that occurs | ||
| * during normal operation also allows us to deal more robustly with potential bugs. | ||
| */ | ||
| SortedSet<OAuthBearerToken> sortedByLifetime = | ||
| new TreeSet<>( | ||
| new Comparator<OAuthBearerToken>() { | ||
| @Override | ||
| public int compare(OAuthBearerToken o1, OAuthBearerToken o2) { | ||
| return Long.compare(o1.lifetimeMs(), o2.lifetimeMs()); | ||
| } | ||
| }); | ||
| sortedByLifetime.addAll(privateCredentials); | ||
| if (LOG.isWarnEnabled()) { | ||
| LOG.warn("Found {} OAuth Bearer tokens in Subject's private credentials; " + | ||
| "the oldest expires at {}, will use the newest, which expires at {}", | ||
| sortedByLifetime.size(), new Date(sortedByLifetime.first().lifetimeMs()), | ||
| new Date(sortedByLifetime.last().lifetimeMs())); | ||
| } | ||
| callback.token(sortedByLifetime.last()); | ||
| } |
Member
There was a problem hiding this comment.
It would be nice to move choosing the private credential to its own method and simplifying this to be:
callback.token(choosePrivateCredential(privateCredentials));
Uh oh!
There was an error while loading. Please reload this page.
| * This class has been copy-and-pasted from Kafka codebase. | ||
| */ | ||
| @InterfaceAudience.Public | ||
| public class OAuthBearerClientInitialResponse { |
Member
There was a problem hiding this comment.
I'm not a fan of the regex-based parsing happening here. I think we could just replace this with some protobuf code which is what the rest of HBase RPCs use consistently.
If we are going the feature branch route, I'm happy to defer making that change until after we have some known-good test cases to start with.
ContributorAuthor
There was a problem hiding this comment.
I'm happy to do the refactoring, but will deliver it in a separate patch.
| public byte[] evaluateResponse(byte[] response) | ||
| throws SaslException, SaslAuthenticationException { | ||
| try { | ||
| if (response.length == 1 && response[0] == OAuthBearerSaslClient.BYTE_CONTROL_A && |
Member
There was a problem hiding this comment.
Using protobuf will also have the benefit of being able to strongly represent state (rather than relying on special bytes, like this is doing).
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Apache-HBase
commented
Jan 10, 2022
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jan 11, 2022
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jan 11, 2022
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jan 11, 2022
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jan 12, 2022
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jan 12, 2022
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jan 12, 2022
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jan 12, 2022
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jan 12, 2022
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jan 13, 2022
🎊 +1 overall
This message was automatically generated. |
joshelser
approved these changes
Jan 13, 2022
joshelser
left a comment
Member
There was a problem hiding this comment.
After talking with Andor and breaking out some tasks into HBASE-26553, I'm OK to merge this into the feature-branch to get some iteration. +1 to merge to the feature branch, assuming precommit looks happy.
Apache-HBase
commented
Jan 13, 2022
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jan 14, 2022
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jan 19, 2022
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jan 19, 2022
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jan 20, 2022
🎊 +1 overall
This message was automatically generated. |
joshelser
commented
Jan 20, 2022
Member
Could you fix up the two automated check outputs, Andor? Then, I think we're good for the feature branch? |
Apache-HBase
commented
Jan 20, 2022
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jan 20, 2022
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jan 20, 2022
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jan 20, 2022
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jan 20, 2022
💔 -1 overall
This message was automatically generated. |
joshelser
commented
Jan 20, 2022
Member
Fixing the javadoc warning as I merge this. I think javadoc is wrong in its warning, but I'm just removing the |
joshelser
commented
Jan 20, 2022
Member
Merged into HBASE-26553 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
HBASE-26655 Initial commit with basic functionality and example code
Based on previous PR #3934