HBASE-26553 OAuth Bearer authentication mech plugin for SASL #3934
Conversation
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
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
taklwu
requested changes
Dec 17, 2021
...java/org/apache/hadoop/hbase/security/provider/OAuthBearerSaslClientCallbackHandlerTest.java
Outdated
Show resolved
Hide resolved
hbase-common/src/main/java/org/apache/hadoop/hbase/exceptions/IllegalSaslStateException.java
Outdated
Show resolved
Hide resolved
...mon/src/main/java/org/apache/hadoop/hbase/security/oauthbearer/OAuthBearerTokenCallback.java
Outdated
Show resolved
Hide resolved
.../hbase/security/oauthbearer/internals/knox/OAuthBearerSignedJwtValidatorCallbackHandler.java
Outdated
Show resolved
Hide resolved
hbase-common/src/main/java/org/apache/hadoop/hbase/security/oauthbearer/Utils.java
Outdated
Show resolved
Hide resolved
hbase-common/src/test/java/org/apache/hadoop/hbase/security/oauthbearer/JwtTestUtils.java
Outdated
Show resolved
Hide resolved
...test/java/org/apache/hadoop/hbase/security/oauthbearer/OAuthBearerValidatorCallbackTest.java
Outdated
Show resolved
Hide resolved
hbase-examples/src/main/java/org/apache/hadoop/hbase/jwt/client/example/JwtClientExample.java
Outdated
Show resolved
Hide resolved
hbase-examples/src/main/java/org/apache/hadoop/hbase/jwt/client/example/JwtClientExample.java
Outdated
Show resolved
Hide resolved
hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/ServerRpcConnection.java
Show resolved
Hide resolved
meszibalu
requested changes
Dec 21, 2021
...a/org/apache/hadoop/hbase/security/provider/OAuthBearerSaslClientAuthenticationProvider.java
Outdated
Show resolved
Hide resolved
...a/org/apache/hadoop/hbase/security/provider/OAuthBearerSaslClientAuthenticationProvider.java
Outdated
Show resolved
Hide resolved
...ava/org/apache/hadoop/hbase/security/oauthbearer/OAuthBearerExtensionsValidatorCallback.java
Outdated
Show resolved
Hide resolved
...mon/src/main/java/org/apache/hadoop/hbase/security/oauthbearer/OAuthBearerTokenCallback.java
Outdated
Show resolved
Hide resolved
...src/main/java/org/apache/hadoop/hbase/security/oauthbearer/OAuthBearerValidatorCallback.java
Outdated
Show resolved
Hide resolved
.../main/java/org/apache/hadoop/hbase/security/oauthbearer/internals/OAuthBearerSaslServer.java
Outdated
Show resolved
Hide resolved
...pache/hadoop/hbase/security/oauthbearer/internals/knox/OAuthBearerIllegalTokenException.java
Outdated
Show resolved
Hide resolved
...pache/hadoop/hbase/security/oauthbearer/internals/knox/OAuthBearerIllegalTokenException.java
Outdated
Show resolved
Hide resolved
...a/org/apache/hadoop/hbase/security/provider/OAuthBearerSaslServerAuthenticationProvider.java
Outdated
Show resolved
Hide resolved
...a/org/apache/hadoop/hbase/security/provider/OAuthBearerSaslServerAuthenticationProvider.java
Outdated
Show resolved
Hide resolved
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
taklwu
reviewed
Jan 5, 2022
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
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
joshelser
reviewed
Jan 5, 2022
| OAuthBearerTokenCallback callback = new OAuthBearerTokenCallback(); | ||
| handler.handle(new Callback[] {callback}); | ||
| assertEquals(num, callback.token().lifetimeMs()); | ||
| } |
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 { |
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.
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.
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.
hbase-common/src/main/java/org/apache/hadoop/hbase/exceptions/IllegalSaslStateException.java
Outdated
Show resolved
Hide resolved
hbase-common/src/main/java/org/apache/hadoop/hbase/exceptions/SaslAuthenticationException.java
Show resolved
Hide resolved
| /* | ||
| * Callback handler for SASL-based authentication | ||
| */ | ||
| @InterfaceAudience.Public |
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"
hbase-common/src/main/java/org/apache/hadoop/hbase/security/auth/SaslExtensionsCallback.java
Show resolved
Hide resolved
...mon/src/main/java/org/apache/hadoop/hbase/security/oauthbearer/OAuthBearerTokenCallback.java
Show resolved
Hide resolved
.../main/java/org/apache/hadoop/hbase/security/oauthbearer/internals/OAuthBearerSaslServer.java
Outdated
Show resolved
Hide resolved
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); | ||
| } |
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.
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
joshelser
reviewed
Jan 5, 2022
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.
...a/org/apache/hadoop/hbase/security/provider/OAuthBearerSaslServerAuthenticationProvider.java
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| public static String[] mechanismNamesCompatibleWithPolicy(Map<String, ?> props) { | ||
| return props != null && "true".equals(String.valueOf(props.get(Sasl.POLICY_NOPLAINTEXT))) |
There was a problem hiding this comment.
Boolean.valueOf(String.valueOf(props.get(Sasl.POLICY_NOPLAINTEXT))) is a little more succient.
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) { |
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; |
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) { |
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"; |
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.
| @@ -19,7 +19,6 @@ | |||
|
|
|||
| import static org.junit.Assert.assertTrue; | |||
| import static org.junit.Assert.fail; | |||
|
|
|||
There was a problem hiding this comment.
nit: In general, try to avoid making changes to unrelated files.
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
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. |
|
Closing this PR due to feature branch request. |
|
Follow-up PR #4019 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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