HBASE-23881 Ensure Netty client receives at least one response before… #1260
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
Member
Author
|
This was a bear to figure out. This is only for Netty -- NIO works fine. This PR is also only valid for master -- due to the ZK-less MasterRegistry implementation. I think earlier 2.x branches will need custom patches. I'll pick those up after we're good on master. |
joshelser
changed the title
|
💔 -1 overall
This message was automatically generated. |
Apache9
requested changes
Mar 10, 2020
hbase-client/src/main/java/org/apache/hadoop/hbase/ipc/NettyRpcConnection.java
Show resolved
Hide resolved
hbase-client/src/main/java/org/apache/hadoop/hbase/security/NettyHBaseSaslRpcClientHandler.java
Show resolved
Hide resolved
Contributor
|
OK, I checked the PlainClient, so the assumption here is that, client should have get at least one sasl response from server, even if it has reported as complete? But waht if later sasl client reports complete? Do we still need to wait for the server response? I'm a bit confused. |
|
🎊 +1 overall
This message was automatically generated. |
Contributor
|
Think again, the end condition here should be:
Let me check the code again. Found a bug through a roughly looking... Where EMPTY_TOKEN is a byte[0], but in upper layer. we will check null to determine whether we have an initial response... |
|
🎊 +1 overall
This message was automatically generated. |
Contributor
|
@joshelser If you do not mind, I plan to make a overall fix based on your patch and what's important, add more comments to say the rules here, to avoid further confusing of others. Thanks. |
Member
Author
Actually, I'd prefer to keep working on this, but am happy to work with you. I have a vested interest to make sure that, as we're looking at how HBase uses SASL, this is all translating well into the AuthenticationProvider stuff I've recently added into Master. My take is that the interfaces I added are lacking (given what we know about how other mechanisms work), but we could extend them and prevent other developers from making similar errors in the future. LMK. |
Member
Author
We could just make some AtomicBoolean, defaulting to false, which is changed to true after the first call to Do you think it's better to have an explicit check in the code rather than comments saying "don't re-add this"?
Will push a fix for this. Nice catch. I didn't notice that. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
bharathv
reviewed
Mar 12, 2020
Contributor
bharathv
left a comment
bharathv
left a comment
There was a problem hiding this comment.
Thanks for chasing this down @joshelser. I have a couple of nits about the test but looks like the core of the fix is in the right direction. +1 to making the flow clear. I'm trying to wrap my head around the flow of the sasl handlers in the Netty code.
...a/org/apache/hadoop/hbase/security/provider/example/TestShadeSaslAuthenticationProvider.java
Outdated
Show resolved
Hide resolved
...a/org/apache/hadoop/hbase/security/provider/example/TestShadeSaslAuthenticationProvider.java
Show resolved
Hide resolved
Apache9
requested changes
Mar 13, 2020
Contributor
Apache9
left a comment
Apache9
left a comment
There was a problem hiding this comment.
Please hold on, the implementation still has some problems. Will be back soon.
hbase-client/src/main/java/org/apache/hadoop/hbase/security/AbstractHBaseSaslRpcClient.java
Show resolved
Hide resolved
Contributor
|
Sorry a bit busy these days. Will try to get sometime this weekendto review the patch more carefully. |
Member
Author
|
It's ok. I still owe you a fix-up of the re-introduction of |
Apache9
approved these changes
Mar 21, 2020
Contributor
Apache9
left a comment
Apache9
left a comment
There was a problem hiding this comment.
+1 after changing the getInitialResponse to return null when there is no initial response.
And reviewed the code, I think the blocking rpc client implementation has the same problem but due to the recent master based registry implementation, the blocking rpc client is just broken and can not be used anyway as it does not implement the createHedgedRpcChannel method...
Let's file another issue for fixing it, or we just remove the implementation, as it is not a good idea to leave a broken implementation in our code base.
Member
Author
|
Thanks Duo! I'll make the change, run it through QA and try to get this resolved in master. Removing sync impl for master seems right to me. I'll work on pulling this back to other branches after that. |
… considering SASL negotiation complete The PLAIN mechanism test added in the Shade authentication example has different semantics than GSSAPI mechanism -- the client reports that the handshake is done after the original challenge is computed. The javadoc on SaslClient, however, tells us that we need to wait for a response from the server before proceeding. The client, best as I can see, does not receive any data from HBase; however the application semantics (e.g. throw an exception on auth'n error) do not work as we intend as a result of this bug.
joshelser
force-pushed
the
23881-sasl-auth-example
branch
from
a0d7a67 to
92e9886
Compare
Member
Author
|
Rebased and fixed the last round of requests/suggestions from Bharath and Duo. I plan to commit to master and 2.x branches that this comes back cleanly to on QA. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
bharathv
approved these changes
Mar 25, 2020
Contributor
bharathv
left a comment
bharathv
left a comment
There was a problem hiding this comment.
Sorry for the delay, I was busy in the past few weeks. I finally had a chance to spend some time on this and it makes sense to me.
hbase-client/src/main/java/org/apache/hadoop/hbase/security/NettyHBaseSaslRpcClientHandler.java
Show resolved
Hide resolved
joshelser
added a commit
to joshelser/hbase
that referenced
this pull request
Apr 13, 2020
… considering SASL negotiation complete The PLAIN mechanism test added in the Shade authentication example has different semantics than GSSAPI mechanism -- the client reports that the handshake is done after the original challenge is computed. The javadoc on SaslClient, however, tells us that we need to wait for a response from the server before proceeding. The client, best as I can see, does not receive any data from HBase; however the application semantics (e.g. throw an exception on auth'n error) do not work as we intend as a result of this bug. Extra trace logging was also added to debug this, should a similar error ever happen again with some other mechanism. Closes apache#1260 Signed-off-by: Duo Zhang <zhangduo@apache.org> Signed-off-by: Bharath Vissapragada <bharathv@apache.org>
thangTang
pushed a commit
to thangTang/hbase
that referenced
this pull request
Apr 16, 2020
… considering SASL negotiation complete The PLAIN mechanism test added in the Shade authentication example has different semantics than GSSAPI mechanism -- the client reports that the handshake is done after the original challenge is computed. The javadoc on SaslClient, however, tells us that we need to wait for a response from the server before proceeding. The client, best as I can see, does not receive any data from HBase; however the application semantics (e.g. throw an exception on auth'n error) do not work as we intend as a result of this bug. Extra trace logging was also added to debug this, should a similar error ever happen again with some other mechanism. Closes apache#1260 Signed-off-by: Duo Zhang <zhangduo@apache.org> Signed-off-by: Bharath Vissapragada <bharathv@apache.org>
thangTang
pushed a commit
to thangTang/hbase
that referenced
this pull request
Apr 16, 2020
… considering SASL negotiation complete The PLAIN mechanism test added in the Shade authentication example has different semantics than GSSAPI mechanism -- the client reports that the handshake is done after the original challenge is computed. The javadoc on SaslClient, however, tells us that we need to wait for a response from the server before proceeding. The client, best as I can see, does not receive any data from HBase; however the application semantics (e.g. throw an exception on auth'n error) do not work as we intend as a result of this bug. Extra trace logging was also added to debug this, should a similar error ever happen again with some other mechanism. Closes apache#1260 Signed-off-by: Duo Zhang <zhangduo@apache.org> Signed-off-by: Bharath Vissapragada <bharathv@apache.org>
thangTang
pushed a commit
to thangTang/hbase
that referenced
this pull request
Apr 16, 2020
… considering SASL negotiation complete The PLAIN mechanism test added in the Shade authentication example has different semantics than GSSAPI mechanism -- the client reports that the handshake is done after the original challenge is computed. The javadoc on SaslClient, however, tells us that we need to wait for a response from the server before proceeding. The client, best as I can see, does not receive any data from HBase; however the application semantics (e.g. throw an exception on auth'n error) do not work as we intend as a result of this bug. Extra trace logging was also added to debug this, should a similar error ever happen again with some other mechanism. Closes apache#1260 Signed-off-by: Duo Zhang <zhangduo@apache.org> Signed-off-by: Bharath Vissapragada <bharathv@apache.org>
asfgit
pushed a commit
that referenced
this pull request
Apr 26, 2020
… considering SASL negotiation complete The PLAIN mechanism test added in the Shade authentication example has different semantics than GSSAPI mechanism -- the client reports that the handshake is done after the original challenge is computed. The javadoc on SaslClient, however, tells us that we need to wait for a response from the server before proceeding. The client, best as I can see, does not receive any data from HBase; however the application semantics (e.g. throw an exception on auth'n error) do not work as we intend as a result of this bug. Extra trace logging was also added to debug this, should a similar error ever happen again with some other mechanism. Closes #1260 Signed-off-by: Duo Zhang <zhangduo@apache.org> Signed-off-by: Bharath Vissapragada <bharathv@apache.org>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 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.
… considering SASL negotiation complete
The PLAIN mechanism test added in the Shade authentication example has
different semantics than GSSAPI mechanism -- the client reports that the
handshake is done after the original challenge is computed. The javadoc
on SaslClient, however, tells us that we need to wait for a response
from the server before proceeding.
The client, best as I can see, does not receive any data from HBase;
however the application semantics (e.g. throw an exception on auth'n
error) do not work as we intend as a result of this bug.