Uh oh!
There was an error while loading. Please reload this page.
HBASE-26667 Integrate user-experience for hbase-client - #4064
Merged
Conversation
Apache-HBase
commented
Jan 25, 2022
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jan 25, 2022
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jan 25, 2022
🎊 +1 overall
This message was automatically generated. |
joshelser
self-requested a review
February 11, 2022 18:31
joshelser
requested changes
Feb 14, 2022
joshelser
left a comment
Member
There was a problem hiding this comment.
A couple of nit-picky things, but looks good!
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| } | ||
| /** | ||
| * Check whether an OAuth Beaerer token is provided in environment variable HADOOP_JWT. |
Member
There was a problem hiding this comment.
Suggested change
| * CheckwhetheranOAuthBeaerertokenisprovidedinenvironmentvariableHADOOP_JWT. | |
| * CheckwhetheranOAuthBearertokenisprovidedinenvironmentvariableHBASE_JWT. |
| ZonedDateTime lifetime = ZonedDateTime.parse(tokens[1]); | ||
| lifetimeMs = lifetime.toInstant().toEpochMilli(); | ||
| } catch (DateTimeParseException e) { | ||
| LOG.warn("Unable to parse JWT expiry: {}", tokens[1]); |
Member
There was a problem hiding this comment.
Reading addTokenForUser(String, Token, long), it seems like we don't need to have a lifetime, which matches the warning (not failure) here. Lazy question: do we have an "expectation" on how it should work? Like, does Knox always give us a lifetime? What about the nimbus test server?
If the jwt server doesn't give us a lifetime, do we have any kind of loss of functionality on the HBase client side?
ContributorAuthor
There was a problem hiding this comment.
Lifetime is generally needed for the overlap of token renewal when you're having 2 tokens at the same time in your subject credentials. In this case the client have to choose the most recent one for the authentication. This logic is already implemented, but lifetimeMs is a must for this to work.
Yes, in the Knox response you'll get the base64 encoded token and the lifetimeMs in the JSON. Theoretically it's also possible to parse the expiry field from the JWT itself, but the question here is do you want to do that without signature validation? If not, the client will also have to be capable of JWT validation like the server.
Member
There was a problem hiding this comment.
Theoretically it's also possible to parse the expiry field from the JWT itself, but the question here is do you want to do that without signature validation? If not, the client will also have to be capable of JWT validation like the server.
Yeah, I would not suggest that we push the validation logic into the client (at least without a very good reason)
but lifetimeMs is a must for this to work.
If lifetimeMs is going to be important for the token selection to work, I think we should just throw an Exception if we can't parse the timestamp. Otherwise, we'll get questions later about why renewal/expiration/selection logic didn't work correctly.
If we're going to focus on Knox as the authorization server, let's build the implementation as to how Knox works. As we want to support other authz servers, it will be easier to codify "supported features" of different authorization servers.
| @Test | ||
| public void testAddTokenFromEnvVar() { | ||
| // Arrange | ||
| User user = User.createUserForTesting(new HBaseConfiguration(), "testuser", new String[] {}); |
Member
There was a problem hiding this comment.
Suggested change
| Useruser = User.createUserForTesting(newHBaseConfiguration(), "testuser", newString[] {}); | |
| Useruser = User.createUserForTesting(HBaseConfiguration.create(), "testuser", newString[] {}); |
| long numberOfTokens = user.getTokens().stream() | ||
| .filter((t) -> new Text(TOKEN_KIND).equals(t.getKind())) | ||
| .count(); | ||
| assertEquals("Invalid number of tokens on User",1, numberOfTokens); |
Member
There was a problem hiding this comment.
Suggested change
| assertEquals("Invalid number of tokens on User",1, numberOfTokens); | |
| assertEquals("Invalid number of tokens on User",1, numberOfTokens); |
Uh oh!
There was an error while loading. Please reload this page.
Apache-HBase
commented
Mar 2, 2022
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Mar 2, 2022
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Mar 2, 2022
🎊 +1 overall
This message was automatically generated. |
anmolnar
commented
Mar 2, 2022
ContributorAuthor
@joshelser The builds are finally green on this patch and I addressed your review comments. PTAL. |
joshelser
requested changes
Mar 20, 2022
joshelser
left a comment
Member
There was a problem hiding this comment.
Unless you think strongly otherwise, let's just throw an exception for missing lifetime or expiration instead of logging a debug message. Otherwise, looks good.
| ZonedDateTime lifetime = ZonedDateTime.parse(tokens[1]); | ||
| lifetimeMs = lifetime.toInstant().toEpochMilli(); | ||
| } catch (DateTimeParseException e) { | ||
| LOG.warn("Unable to parse JWT expiry: {}", tokens[1]); |
Member
There was a problem hiding this comment.
Theoretically it's also possible to parse the expiry field from the JWT itself, but the question here is do you want to do that without signature validation? If not, the client will also have to be capable of JWT validation like the server.
Yeah, I would not suggest that we push the validation logic into the client (at least without a very good reason)
but lifetimeMs is a must for this to work.
If lifetimeMs is going to be important for the token selection to work, I think we should just throw an Exception if we can't parse the timestamp. Otherwise, we'll get questions later about why renewal/expiration/selection logic didn't work correctly.
If we're going to focus on Knox as the authorization server, let's build the implementation as to how Knox works. As we want to support other authz servers, it will be easier to codify "supported features" of different authorization servers.
Apache-HBase
commented
Mar 25, 2022
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Mar 25, 2022
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Mar 25, 2022
🎊 +1 overall
This message was automatically generated. |
joshelser
approved these changes
Mar 25, 2022
anmolnar added a commit
to anmolnar/hbase
that referenced
this pull request
Aug 23, 2022
Signed-off-by: Josh Elser <elserj@apache.org>
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.
Implemented the environment variable based approach for detecting available JWT tokens.
ConnectionFactorywill check for the presence of "HADOOP_JWT" environment variable and adds it to user's credentials. Format of value is<base64_encoded_jwt>,<expiry in DateTimeFormatter.ISO_ZONED_DATE_TIME format>Expiry info is optional.