HBASE-23347 Allowable custom authentication methods for RPCs #884
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. |
wchevreuil
reviewed
Nov 28, 2019
Contributor
wchevreuil
left a comment
wchevreuil
left a comment
There was a problem hiding this comment.
Great work on making the RPC authentication "pluggable", @joshelser ! Kind of a big one to digest, but had made some small nit comments, together with some thoughts about kerberos specific references in some parts of client rpc code. Still got go through the tests.
hbase-client/src/main/java/org/apache/hadoop/hbase/ipc/RpcConnection.java
Outdated
Show resolved
Hide resolved
hbase-client/src/main/java/org/apache/hadoop/hbase/security/AbstractHBaseSaslRpcClient.java
Outdated
Show resolved
Hide resolved
| throw new IOException("Unknown authentication method " + method); | ||
| } | ||
|
|
||
| saslClient = provider.createClient(conf, serverPrincipal, token, fallbackAllowed, saslProps); |
Contributor
There was a problem hiding this comment.
This is great, much simpler, but do we still need a server principal along here? Couldn't that be something the kerberos provider defines internally, and sets accordingly on its created client?
Member
Author
There was a problem hiding this comment.
I agree with your line of thinking, but struggle to suggest a better way to do it. Right now, the client authentication provider will be used to create a new SaslClient every time we try to RPC to a remote service. That might be the Master, RS, Thrift server (or anything else in SecurityInfo).
We could push the SecurityInfo from RpcConnection down into the AuthenticationProvider -- maybe that's cleaner than the Kerberos principal itself? If we have the SecurityInfo, we could re-compute the kerberos name just inside the authnprovider where we need it to create teh SaslClient.
Let me try.
| */ | ||
| @InterfaceAudience.LimitedPrivate(HBaseInterfaceAudience.AUTHENTICATION) | ||
| @InterfaceStability.Evolving | ||
| public abstract class AbstractSaslClientAuthenticationProvider implements SaslClientAuthenticationProvider { |
Contributor
There was a problem hiding this comment.
Do we really need an extra layer of inheritance here, or could we just have the interface with default implementations?
hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/ServerRpcConnection.java
Outdated
Show resolved
Hide resolved
...c/main/java/org/apache/hadoop/hbase/security/provider/SaslServerAuthenticationProviders.java
Show resolved
Hide resolved
saintstack
reviewed
Nov 30, 2019
Contributor
saintstack
left a comment
saintstack
left a comment
There was a problem hiding this comment.
Reviewed bottom half only.
Nice write-up for reviewers. Helped.
How will compat be managed? You have a use-case?
...st/java/org/apache/hadoop/hbase/security/provider/TestSaslServerAuthenticationProviders.java
Show resolved
Hide resolved
...est/java/org/apache/hadoop/hbase/security/provider/TestCustomSaslAuthenticationProvider.java
Show resolved
Hide resolved
...META-INF/services/org.apache.hadoop.hbase.security.provider.SaslServerAuthenticationProvider
Outdated
Show resolved
Hide resolved
...n/java/org/apache/hadoop/hbase/security/provider/SimpleSaslServerAuthenticationProvider.java
Show resolved
Hide resolved
...n/java/org/apache/hadoop/hbase/security/provider/SimpleSaslServerAuthenticationProvider.java
Outdated
Show resolved
Hide resolved
...main/java/org/apache/hadoop/hbase/security/provider/GssSaslServerAuthenticationProvider.java
Outdated
Show resolved
Hide resolved
|
💔 -1 overall
This message was automatically generated. |
Member
Author
|
Slowly working through review comments. Keep 'em coming. |
Member
Author
Client fallbackIt looks like we had copied an idea from Hadoop where the client which was configured to auth'n over Kerberos, could fall back to trying to authenticate via SIMPLE. This seems silly to me and is off by default. I see one instance of someone poking at this https://issues.apache.org/jira/browse/HBASE-20993 but I'm missing the "why". I can't think of a reason that we would want to encourage this (not using the correct configuration to talk to HBase). Am I missing a use-case? How will compat be managed?Great question. I specifically avoided being prescriptive as to how compatibility should be maintained within some AuthenticationProvider. We have a couple ways that compatibility rears its head. RPC client/server <-> AuthenticationProvider interfaceI think this shouldn't be too bad to maintain since the interfaces are presently not tied to any RPC-related classes. Meaning, the RPC impl can do whatever processing it needs to do and just needs to extract the info to make the calls into the AuthProvider methods. ClientAuthnProvider v1 to ServerAuthnProvider v2 (etc)Clearly, each client/server AuthenticationProvider impl needs to have compatibility across versions to ensure that clients can continue to authenticate properly. I specifically avoided being prescriptive as to how implementations should account for this. The current interfaces don't leave much to "extrapolate" in the current interface -- I don't see much being able to change on the surface. Presently, I've only done stuff with Auth'nProviders which still rely on Hadoop Tokens. In theory, we could set up TLS in some other way, but that's a can of worms I didn't want to open. The other related piece here is that the You have a use-case?Wellington and I have been playing around with an implementation where we hook up HBase to Linux PAM. Client creates a new Token (ala TokenUtil) in which they put in their username/password, this is sent across the wire to the Master/RegionServer who asks PAM if the credentials are valid, allowing/disallowing the RPC per PAM's reply. This has been pretty slick in practice -- suddenly, HBase has username/password auth'n and we aren't in the business of storing/managing those passwords. The problem with this approach is that we have to handle a plaintext password going over HBase RPC. Best as I know, the RPC encryption we get from SASL only comes with the GSSAPI mechanism, not as a function of any SASL mechanism. PAM is (usually for real installations) going against some corporate auth'n database (e.g. active directory), and not just the local password db on each host. When we scale up&out, there's also going to be a worry about how well the backend auth'n database works (and is that going to kill HBase perf). So, it's a win to me, but it does bring some baggage with it. Hope that helps. |
|
💔 -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. |
|
💔 -1 overall
This message was automatically generated. |
wchevreuil
reviewed
Dec 6, 2019
Contributor
wchevreuil
left a comment
wchevreuil
left a comment
There was a problem hiding this comment.
LGTM. Just have some small remarks, not something that should prevent this from getting through.
dev-support/design-docs/HBASE-23347-pluggable-authentication.md
Outdated
Show resolved
Hide resolved
dev-support/design-docs/HBASE-23347-pluggable-authentication.md
Outdated
Show resolved
Hide resolved
...-client/src/main/java/org/apache/hadoop/hbase/security/provider/DefaultProviderSelector.java
Outdated
Show resolved
Hide resolved
hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/ServerRpcConnection.java
Outdated
Show resolved
Hide resolved
Member
Author
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
wchevreuil
reviewed
Dec 9, 2019
Contributor
wchevreuil
left a comment
wchevreuil
left a comment
There was a problem hiding this comment.
New round of reviews. Latest updates on the interfaces hierarchy looking great! Again, just minor remarks, now related to simple authentication and the fallback.
hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/ServerRpcConnection.java
Outdated
Show resolved
Hide resolved
hbase-server/src/main/java/org/apache/hadoop/hbase/security/HBaseSaslRpcServer.java
Outdated
Show resolved
Hide resolved
...n/java/org/apache/hadoop/hbase/security/provider/SimpleSaslServerAuthenticationProvider.java
Outdated
Show resolved
Hide resolved
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
wchevreuil
approved these changes
Dec 10, 2019
Contributor
wchevreuil
left a comment
wchevreuil
left a comment
There was a problem hiding this comment.
Just a typo and a minor nit left, but no other concerns regarding the code itself, so LGTM +1.
dev-support/design-docs/HBASE-23347-pluggable-authentication.md
Outdated
Show resolved
Hide resolved
hbase-client/src/main/java/org/apache/hadoop/hbase/security/AbstractHBaseSaslRpcClient.java
Outdated
Show resolved
Hide resolved
belugabehr
reviewed
Dec 11, 2019
hbase-client/src/main/java/org/apache/hadoop/hbase/ipc/BlockingRpcConnection.java
Outdated
Show resolved
Hide resolved
belugabehr
reviewed
Dec 11, 2019
hbase-client/src/main/java/org/apache/hadoop/hbase/ipc/BlockingRpcConnection.java
Outdated
Show resolved
Hide resolved
belugabehr
reviewed
Dec 11, 2019
hbase-client/src/main/java/org/apache/hadoop/hbase/ipc/BlockingRpcConnection.java
Outdated
Show resolved
Hide resolved
belugabehr
reviewed
Dec 11, 2019
hbase-client/src/main/java/org/apache/hadoop/hbase/ipc/BlockingRpcConnection.java
Outdated
Show resolved
Hide resolved
belugabehr
reviewed
Dec 11, 2019
hbase-client/src/main/java/org/apache/hadoop/hbase/ipc/BlockingRpcConnection.java
Show resolved
Hide resolved
belugabehr
reviewed
Dec 11, 2019
hbase-client/src/main/java/org/apache/hadoop/hbase/ipc/RpcConnection.java
Outdated
Show resolved
Hide resolved
Member
Author
|
@belugabehr thanks for taking a look. Can you do me a solid?
|
We provide a number of implementations of our LimitedPrivate interface but these are provided for user authentication, not as stable extension points for end-users. They may copy and modify this code, but should not rely on these implementations in their own code. Thanks to Duo for the suggestion.
These are kept separate because they are largely changing code that was otherwise unchanged by this changeset.
Also improve the javadoc on the new `getRealUser` method. Try to do a better job explaining why this is here.
joshelser
force-pushed
the
23347-pluggable-authentication
branch
from
f7f126b to
d546b8a
Compare
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
Adds a unit test to have some general confidence about this being called automatically. Cleans up the provided Providers a little (most don't need to do any initialization).
Member
Author
|
Progress update with what i've pushed since Monday.
|
Member
Author
|
Trying to parse Andrew's approval with a request for some changes as non-blocking (specifically, just the SASL renaming), I plan to commit this once QA comes back clean again. Please shout if I'm misinterpreting. Should this go back to branch-2 as well? I don't think our compatibility guarantees preclude this change from going to branch-2 and master. I'll re-read, but please shout if anyone feels strongly. (FWIW, this technically could hit 2.1 and 2.2, but I don't want to risk destabilization. Old clients should continue to function as they did -- nothing changed in the wire format) |
Member
Author
|
@Apache9 Github seems to have "lost" your review because of my rebase earlier this morning. I think it was just asking for an addition to hbase-examples (which is here now). At-mentioning because I might have it completely wrong and don't want to miss something. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
9 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.
This is a big one. Sorry ;)
Start here with a one-page writeup: https://github.com/joshelser/hbase/blob/23347-pluggable-authentication/dev-support/design-docs/HBASE-23347-pluggable-authentication.md
Next, look at the client side interfaces: https://github.com/joshelser/hbase/tree/23347-pluggable-authentication/hbase-client/src/main/java/org/apache/hadoop/hbase/security/provider
Then, the server side interfaces: https://github.com/joshelser/hbase/tree/23347-pluggable-authentication/hbase-server/src/main/java/org/apache/hadoop/hbase/security/provider
Finally, an end-to-end example of how you can use this: https://github.com/joshelser/hbase/blob/23347-pluggable-authentication/hbase-server/src/test/java/org/apache/hadoop/hbase/security/provider/TestCustomSaslAuthenticationProvider.java
This is very-much a first draft. This is "new art" for HBase and I expect some discussion on how we want to safely do this. I'm fully expecting lots of revisions.
Relevant tests should be passing, but this is also partially for me to let QA-bot run. Everything I did was in attempts to retain backwards compatibility with older clients, as well as RPC-impl compatibility. This should all be implementation-details inside of HBase.