Uh oh!
There was an error while loading. Please reload this page.
HBASE-26553 OAuth Bearer authentication mech plugin for SASL - #3934
Closed
anmolnar wants to merge 7 commits into
Closed
HBASE-26553 OAuth Bearer authentication mech plugin for SASL#3934anmolnar wants to merge 7 commits into
anmolnar wants to merge 7 commits into
Conversation
Apache-HBase
commented
Dec 10, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Dec 10, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Dec 10, 2021
💔 -1 overall
This message was automatically generated. |
taklwu
requested changes
Dec 17, 2021
taklwu
left a comment
Contributor
There was a problem hiding this comment.
sorry that I didn't give much comment on the JWT integration, I will come back on after another review.
at the same time, can you fix those javac, javadoc, and checkstyle -0 ?
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
meszibalu
requested changes
Dec 21, 2021
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
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 4, 2022
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jan 4, 2022
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jan 4, 2022
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jan 5, 2022
🎊 +1 overall
This message was automatically generated. |
taklwu
reviewed
Jan 5, 2022
taklwu
left a comment
Contributor
There was a problem hiding this comment.
LGTM, please add the @ClassRule/CLASS_RULE and @Category annotations for new test classes, rerun the tests or see if the failures are related
Apache-HBase
commented
Jan 5, 2022
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jan 5, 2022
💔 -1 overall
This message was automatically generated. |
joshelser
left a comment
Member
There was a problem hiding this comment.
WIP review but wanted to get this out.
| OAuthBearerTokenCallback callback = new OAuthBearerTokenCallback(); | ||
| handler.handle(new Callback[] {callback}); | ||
| assertEquals(num, callback.token().lifetimeMs()); | ||
| } |
Member
There was a problem hiding this comment.
Would be nice to then add a new PrivateCredential with a lesser lifetimeMs, just to make sure that the sorting is working properly (since you test the life times generated in sorted order, 1, 2, 3, 4).
| * is used to connect to a SASL endpoint. | ||
| */ | ||
| @InterfaceAudience.Public | ||
| public class IllegalSaslStateException extends IllegalStateException { |
Member
There was a problem hiding this comment.
We typically use the DoNotRetryIOException through the public API to indicate an operation (especially those which might come from a Master/RegionServer) should not be intrinsically retried. Since this is a RuntimeException, it might be treated as something that is non-retriable already.
ContributorAuthor
There was a problem hiding this comment.
I see 2 problems with that:
DoNotRetryIOExceptionis in hbase-client module and cannot be used in hbase-common- It's not a RuntimeException so cannot be replaced easily.
Though this exception is only used at one place in the PR, so I'm happy with any suggestion to get rid of that.
ContributorAuthor
There was a problem hiding this comment.
It's thrown here.
The overriden method must throw SaslException, so we don't have to many options.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| /* | ||
| * Callback handler for SASL-based authentication | ||
| */ | ||
| @InterfaceAudience.Public |
Member
There was a problem hiding this comment.
Users would not be implementing this directly, would they? I think good to start this off as "Private"
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Comment on lines
+147
to
+161
| @Override | ||
| public byte[] unwrap(byte[] incoming, int offset, int len) { | ||
| if (!complete) { | ||
| throw new IllegalStateException("Authentication exchange has not completed"); | ||
| } | ||
| return Arrays.copyOfRange(incoming, offset, offset + len); | ||
| } | ||
| @Override | ||
| public byte[] wrap(byte[] outgoing, int offset, int len) { | ||
| if (!complete) { | ||
| throw new IllegalStateException("Authentication exchange has not completed"); | ||
| } | ||
| return Arrays.copyOfRange(outgoing, offset, offset + len); | ||
| } |
Member
There was a problem hiding this comment.
I thought SASL had some extra header on the data packet which unwrap and wrap were handling. Maybe it's only important when we have SASL QOP set to integrity or confidentiality? With the bearer token approach we'll have to make sure that the data on the wire is encrypted.
Apache-HBase
commented
Jan 5, 2022
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jan 5, 2022
🎊 +1 overall
This message was automatically generated. |
joshelser
left a comment
Member
There was a problem hiding this comment.
Lots of content here. I'm trying to unpack it all :)
- There's a lot of code around the SaslExtensions and the related Callbacks, but I'm struggling to understand if there's a good reason for us to keep these around. Are they just something that was in the Kafka implementation or are they critical to basic authentication via JWT working?
- I know that you mentioned in the Jira issue that you were leveraging Knox as the authorization server. I think it would go a very long way if we could provide a relatively simple authorization server such that other developers could try this out locally. I'm not seeing anything simple that nimbus-jose-jwt includes (but maybe it wouldn't be bad to just support Knox in a simple mode?)
- There's some checks you have around NO_PLAINTEXT sasl options. My understanding is that, for JWT based authentication to be secure (not subject to replay attacks), we need to have an encrypted channel (no plain text on the wire). Traditionally, we enabled wire encryption for HBase with SASL+GSSAPI/Kerberos by setting hbase.rpc.protection=privacy (which sets the SASL qop to be auth-conf). However, I believe this encryption depends on GSSAPI as the mechanism and would not be effective for this OAuthBearer solution. We also have commons-crypto based encryption as described in https://issues.apache.org/jira/browse/HBASE-16414. If this wire encryption works, I think we should just require it to be on to use the OauthBearer provider.
Uh oh!
There was an error while loading. Please reload this page.
| } | ||
| public static String[] mechanismNamesCompatibleWithPolicy(Map<String, ?> props) { | ||
| return props != null && "true".equals(String.valueOf(props.get(Sasl.POLICY_NOPLAINTEXT))) |
Member
There was a problem hiding this comment.
Boolean.valueOf(String.valueOf(props.get(Sasl.POLICY_NOPLAINTEXT))) is a little more succient.
Member
There was a problem hiding this comment.
I'm also tryign to make sure I understand this check -- if we disallow plaintext mechanism, we also disallow oauth bearer mechanism?
In the context of HBase, would we ever want to allow the user to run HBase with oauth-bearer authentication without wire encryption? For a production scenario, I think the answer would be "no".
| private String errorMessage = null; | ||
| private SaslExtensions extensions; | ||
| public OAuthBearerSaslServer(CallbackHandler callbackHandler) { |
Member
There was a problem hiding this comment.
I think it might be better to fail hard (throw exception) if we try to construct the SaslServer in an inherently insecure manner. Then we could get rid of all of the indirection in this class about compatibility with mechanism/policy.
| private JWTClaimsSet claims; | ||
| private long lifetime; | ||
| private int maxClockSkewSeconds = 0; |
Member
There was a problem hiding this comment.
Is the value of 0 disabling the clock skew validation?
| * @param jwkSet | ||
| * the key set which the signature of this JWT should be verified with | ||
| */ | ||
| public OAuthBearerSignedJwt(String compactSerialization, JWKSet jwkSet) { |
Member
There was a problem hiding this comment.
Unless there's a reason, could we construct this via one constructor with all of the options or make a Builder class? (which could automatically validate when we construct the Token)
| */ | ||
| @Test | ||
| public void testBuildClientResponseToBytes() throws Exception { | ||
| String expectedMesssage = "n,,\u0001auth=Bearer 123.345.567\u0001nineteen=42\u0001\u0001"; |
Member
There was a problem hiding this comment.
Are the \u0001 characters in here from the serialization done in OAuthBearerStringUtils? Would be nice to use the corresponding API to generate this String.
| import static org.junit.Assert.assertTrue; | ||
| import static org.junit.Assert.fail; | ||
Member
There was a problem hiding this comment.
nit: In general, try to avoid making changes to unrelated files.
Uh oh!
There was an error while loading. Please reload this page.
Apache-HBase
commented
Jan 6, 2022
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jan 6, 2022
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jan 6, 2022
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jan 6, 2022
💔 -1 overall
This message was automatically generated. |
joshelser
commented
Jan 7, 2022
Member
Alright! I was actually able to test this out today using Knox. I think there are a couple of high level things we need to figure out
Now, given how big this patch is already, I think I'd suggest we work through these on a feature branch rather than try to do them in a single commit. WDYT, Andor? I think this approach would let us do some iteration more easily. |
anmolnar
commented
Jan 10, 2022
ContributorAuthor
Closing this PR due to feature branch request. |
ContributorAuthor
Follow-up PR #4019 |
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
5 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.
Initial commit for JWT authentication.
Details are in the Jira: https://issues.apache.org/jira/browse/HBASE-26553