HBASE-27280 Add mutual authentication support to TLS #4724
Closed
Conversation
This file contains 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. |
bbeaudreault
commented
Aug 24, 2022
| * revision</a> | ||
| */ | ||
| @InterfaceAudience.Private | ||
| final class HBaseHostnameVerifier implements HostnameVerifier { |
There was a problem hiding this comment.
this file was copied verbatim
bbeaudreault
commented
Aug 24, 2022
| * revision</a> | ||
| */ | ||
| @InterfaceAudience.Private | ||
| public class HBaseTrustManager extends X509ExtendedTrustManager { |
There was a problem hiding this comment.
this file was copied verbatim
bbeaudreault
commented
Aug 24, 2022
| * "https://github.com/apache/zookeeper/blob/5820d10d9dc58c8e12d2e25386fdf92acb360359/zookeeper-server/src/test/java/org/apache/zookeeper/common/ZKHostnameVerifierTest.java">Base | ||
| * revision</a> | ||
| */ | ||
| @Category({ MiscTests.class, SmallTests.class }) |
There was a problem hiding this comment.
the test cases in this class were copied from zk, but there they use pre-generated certificate strings. Here I decided to use our existing X509TestHelpers to generate the certs at test time. I verified that the generated certs here match the expectation of the pre-generated ones in zk.
bbeaudreault
commented
Aug 24, 2022
| * revision</a> | ||
| */ | ||
| @Category({ MiscTests.class, SmallTests.class }) | ||
| public class TestHBaseTrustManager { |
There was a problem hiding this comment.
this class was copied verbatim
bbeaudreault
commented
Aug 24, 2022
| @@ -142,6 +142,38 @@ public void cleanUp() { | |||
| Security.setProperty("com.sun.security.enableCRLDP", Boolean.FALSE.toString()); | |||
| } | |||
|
|
|||
| @Test | |||
| public void testCreateSSLContextWithClientAuthDefault() throws Exception { | |||
There was a problem hiding this comment.
I add tests for ClientAuth passthrough here, but it's not really possible (as far as i can tell) to access the TrustManager of the underlying SslContext/SSLEngine. So I can't easily add tests here for the TrustManager hostname verification arg passthrough. So instead we have the comprehensive end-to-end tests in TestMutualTlsClientSide/ServerSide
|
I've added comprehensive tests (about 250 test cases covering all permutations). I also marked with comments the files which are copied from ZK unchanged. |
|
@anmolnar I can't assign you as a reviewer, but would appreciate a look when you get a chance. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
@Apache9 any chance you can take a look at this? It's just a backport from zk like the other recent TLS patches, mostly copied except for the new end-to-end tests I added. |
anmolnar
approved these changes
Sep 7, 2022
hbase-common/src/main/java/org/apache/hadoop/hbase/io/crypto/tls/HBaseTrustManager.java
Outdated
Show resolved
Hide resolved
hbase-common/src/main/java/org/apache/hadoop/hbase/io/crypto/tls/X509Util.java
Outdated
Show resolved
Hide resolved
hbase-common/src/main/java/org/apache/hadoop/hbase/io/crypto/tls/X509Util.java
Outdated
Show resolved
Hide resolved
|
@bbeaudreault One more thing before I forget: I think you don't need to include the documentation in this patch, but I suggest to create a separate ticket for it, rather than overloading my pending docs PR. Thanks. |
|
Yea, I was wondering we should add it to yours. I had started submitting a suggestion comment. But actually I think it might need a larger refactor to make it make sense relative to the "Secure Access" section. So I agree about creating a separate jira. |
bbeaudreault
force-pushed
the
HBASE-27280
branch
from
4254782
to
4ef95b0
Compare
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
Yesterday I fixed the review feedback. Just pushed a commit to fix the checkstyle/errorprone failures -- I cant seem to run them locally, so we'll see. The spotbugs failure seems to both be wrong and unrelated to my change. |
|
💔 -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. |
|
@Apache9 @meszibalu any chance I could get a quick review on this? |
|
Sorry, I brought my children to a tourist attraction and just got back. And I googled what does 'mutual' mean(sorry for why bad English...)... Now I know what is going on here. Let me take a look soon. |
Apache9
reviewed
Sep 18, 2022
hbase-common/src/main/java/org/apache/hadoop/hbase/io/crypto/tls/HBaseHostnameVerifier.java
Outdated
Show resolved
Hide resolved
hbase-common/src/main/java/org/apache/hadoop/hbase/io/crypto/tls/HBaseHostnameVerifier.java
Outdated
Show resolved
Hide resolved
hbase-common/src/main/java/org/apache/hadoop/hbase/io/crypto/tls/HBaseHostnameVerifier.java
Show resolved
Hide resolved
hbase-common/src/main/java/org/apache/hadoop/hbase/io/crypto/tls/X509Util.java
Show resolved
Hide resolved
hbase-common/src/main/java/org/apache/hadoop/hbase/io/crypto/tls/HBaseTrustManager.java
Show resolved
Hide resolved
Apache9
reviewed
Sep 19, 2022
hbase-common/src/main/java/org/apache/hadoop/hbase/io/crypto/tls/HBaseTrustManager.java
Outdated
Show resolved
Hide resolved
hbase-common/src/main/java/org/apache/hadoop/hbase/io/crypto/tls/HBaseHostnameVerifier.java
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. |
Apache9
reviewed
Sep 20, 2022
hbase-common/src/main/java/org/apache/hadoop/hbase/io/crypto/tls/HBaseHostnameVerifier.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. |
|
Test failures are real but I ran out of time today. Will fix them up tomorrow morning |
|
🎊 +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. |
|
💔 -1 overall
This message was automatically generated. |
Apache9
reviewed
Sep 22, 2022
| if (type != null) { | ||
| if (type == SubjectName.DNS || type == SubjectName.IP) { | ||
| final Object o = entry.get(1); | ||
| // TODO ASN.1 DER encoded form when instanceof byte[] |
There was a problem hiding this comment.
Will this be a follow on issue?
There was a problem hiding this comment.
This todo existed in the file we pulled from ZK. I don't have plans to actually do that work unless we came up with some reason. I'll delete it
| hostAddress = inetAddress.getHostAddress(); | ||
| hostnameVerifier.verify(hostAddress, certificate); | ||
| } catch (SSLException addressVerificationException) { | ||
| if (!allowReverseDnsLookup) { |
There was a problem hiding this comment.
For server side verification, if the InetAddress is created with a host name, we do not need to do a reverse dns lookup and usually we need to verify it through host name instead of ip address? So I prefer here we should try to find out whethter we already have a host name in the InetAddress, if so, just check it, otherwise we try to use reverse dns lookup, and the flag will take effect.
There was a problem hiding this comment.
After googling, a possible way to check whether host name is present is to check the toString result, a bit hacky but could work.
/**
* Converts this IP address to a {@code String}. The
* string returned is of the form: hostname / literal IP
* address.
*
* If the host name is unresolved, no reverse name service lookup
* is performed. The hostname part will be represented by an empty string.
*
* @return a string representation of this IP address.
*/
public String toString() {
String hostName = holder().getHostName();
return ((hostName != null) ? hostName : "")
+ "/" + getHostAddress();
}
So we could check whether the toString result of the InetAddress starts with '/', if so, there is no host name provided, so if we want to check hostname, we need a reverse dns lookup, otherwise not.
|
We are very close now, thanks @bbeaudreault for your patient! |
Apache9
approved these changes
Sep 22, 2022
|
Thanks for all the guidance and review here, Duo! |
bbeaudreault
force-pushed
the
HBASE-27280
branch
from
c4726a9
to
4f8a369
Compare
bbeaudreault
force-pushed
the
HBASE-27280
branch
from
4f8a369
to
de8ee7b
Compare
|
Pre-commit is weirdly failing trying to checkout from git. I just squashed, rebased, and force pushed. No code changes. |
|
@Apache9 sorry, but any idea why this keeps continually failing?
I tried running that command locally and it works. The sha has changed every time i push, and the sha look correct. I wonder if i need to re-open this? |
|
Not sure what's the problem. Let's close this PR and open a new one to see if it could fix the strange problem. |
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.
As with the rest of the netty tls stuff, this is largely pulled and adapted from ZK. This adds support for setting netty's ClientAuth mode (none, need, want) which will handle the cert verification from trust store. We additionally extend the trust manager to enable verification of hostnames.
When TLS is enabled, the default will be all hostnames verified and ClientAuth.NEED. User can tune it down from there if desired.
Comes with comprehensive test of all possible modes from client and server side.
Note: There is follow-up work to be done in another issue which we can use the X509Certificate DN (distinguished name) to validate the ConnectionHeader's username and groups. This first pass simply plugs in the barebones mTLS support.
cc @anmolnar