Uh oh!
There was an error while loading. Please reload this page.
HBASE-26666 Add native TLS encryption support to RPC server/client - #4666
Merged
Conversation
Apache-HBase
commented
Jul 29, 2022
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jul 29, 2022
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jul 29, 2022
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jul 29, 2022
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jul 29, 2022
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jul 29, 2022
💔 -1 overall
This message was automatically generated. |
Apache9
reviewed
Jul 31, 2022
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| String keyStoreType = config.get(TLS_CONFIG_KEYSTORE_TYPE, ""); | ||
| if (keyStoreLocation.isEmpty()) { | ||
| LOG.warn("{} not specified", TLS_CONFIG_KEYSTORE_LOCATION); |
Contributor
There was a problem hiding this comment.
TLS_CONFIG_KEYSTORE_LOCATION is a constant? Then just concat it? It will be convert to a String literal at compile time so no performance issue.
ContributorAuthor
There was a problem hiding this comment.
I always do it this way by habit. No need to think about the perf impact.. Do you know about any downside?
Contributor
There was a problem hiding this comment.
It will lead to a String replacement at runtime, while the string constant can be computed at compile time.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| boolean sslOcspEnabled = config.getBoolean(TLS_CONFIG_OCSP, false); | ||
| if (trustStoreLocation.isEmpty()) { | ||
| LOG.warn("{} not specified", TLS_CONFIG_TRUSTSTORE_LOCATION); |
Contributor
There was a problem hiding this comment.
Will this cause the later sslContextBuilder.build() to fail?
ContributorAuthor
There was a problem hiding this comment.
Nope. I added 2 new tests to cover that.
Contributor
There was a problem hiding this comment.
Then what is real effect if we do not have trust store location specified? It will try to locate the default location on the OS?
Contributor
There was a problem hiding this comment.
This is an important problem. My concern is that, if this is misconfigured, i.e, no trust store location is specified, what is the actual effect? The user will have an insecure connection instead?
ContributorAuthor
There was a problem hiding this comment.
It always loads the default truststore from the OS. This config is just an addition for certificates that are not part of the official store, for instance because they're self-signed.
In a real production system one doesn't need to specify it, because the certificates are generated by a certified provider.
ContributorAuthor
There was a problem hiding this comment.
The user will have an insecure connection instead?
This is not possible. SSLHandler will never in any case accept an untrusted cerificate.
Apache-HBase
commented
Aug 1, 2022
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 1, 2022
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 1, 2022
💔 -1 overall
This message was automatically generated. |
Apache9
reviewed
Aug 1, 2022
Apache9
left a comment
Contributor
There was a problem hiding this comment.
Much better now. I think we are close to merge.
Another thing is that, the ssl configs can only work with NettyRpcServer/NettyRpcClient, besides documentation, we'd better also change the implementation for SimpleRpcServer/BlockingRpcClient to check whether the ssl config is specified? If so, we should fail the initialization to tell users that the configs will not take effect.
| protected void initChannel(Channel ch) throws Exception { | ||
| if (conf.getBoolean(X509Util.HBASE_CLIENT_NETTY_TLS_ENABLED, false)) { | ||
| SslContext sslContext = rpcClient.getSslContext(); | ||
| SslHandler sslHandler = sslContext.newHandler(byteBufAllocator, |
Contributor
There was a problem hiding this comment.
Here you could just use ch.alloc(). so you do not need to store a byteBufAllocator in NettyRpcConnection.
| public NettyRpcClient(Configuration configuration, String clusterId, SocketAddress localAddress, | ||
| MetricsConnection metrics) { | ||
| MetricsConnection metrics) throws SSLContextException, IOException { |
Contributor
There was a problem hiding this comment.
I do not think we will throw these exceptions after refactoring, so just remove the throws here?
| return result; | ||
| } | ||
| private ByteBufAllocator getByteBufAllocator(Configuration conf) throws IOException { |
Contributor
There was a problem hiding this comment.
As another comment points out, we do not need to pass a ByteBufAllocator to NettyRpcConnection, so let's not include this change in this PR. We can file another issue to land this change.
ContributorAuthor
There was a problem hiding this comment.
I remove it from this patch, but do you think it still makes sense to do it separately?
Contributor
There was a problem hiding this comment.
Can do it separately. And also about whether to enable epoll on client side.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| SslContextBuilder sslContextBuilder = SslContextBuilder.forClient(); | ||
| String keyStoreLocation = config.get(TLS_CONFIG_KEYSTORE_LOCATION, ""); | ||
| String keyStorePassword = config.get(TLS_CONFIG_KEYSTORE_PASSWORD, ""); |
Contributor
There was a problem hiding this comment.
Is it a good practise to store password in hbase-site.xml? Not a blocker, just asking, for me I just do not have a good idea on how to do this in an open source project...
ContributorAuthor
There was a problem hiding this comment.
This hasn't been raised against ZooKeeper so far, but I can think of 2 other approaches which might safer:
- Separate file which can be protected differently.
- Env var.
Contributor
There was a problem hiding this comment.
Oh, this one is also a long term improment.
| "TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA" }; | ||
| } | ||
| private static String[] concatArrays(String[] left, String[] right) { |
Contributor
There was a problem hiding this comment.
Just use Guava's ObjectArrays.concat, so we can save several lines.
ContributorAuthor
There was a problem hiding this comment.
This has been reverted, because Guava's method has some glitch with the cert validation logic, I'm still working on it.
| Configuration conf, RpcScheduler scheduler) throws IOException; | ||
| protected abstract AbstractRpcClient<?> createRpcClientNoCodec(Configuration conf); | ||
| protected abstract AbstractRpcClient<?> createRpcClientNoCodec(Configuration conf) |
Contributor
There was a problem hiding this comment.
After removing the throws declaration of NettyRpcClient, I think we do not need to change these lines then.
Apache-HBase
commented
Aug 1, 2022
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 1, 2022
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 1, 2022
💔 -1 overall
This message was automatically generated. |
anmolnar
commented
Aug 1, 2022
ContributorAuthor
So good to hear that. ;)
I need some more cycles to address this. The rest is hopefully done. |
Apache-HBase
commented
Aug 1, 2022
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 2, 2022
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 2, 2022
💔 -1 overall
This message was automatically generated. |
Apache9
reviewed
Aug 2, 2022
| @BeforeClass | ||
| public static void setUp() throws IOException { | ||
| public static void setUp() throws IOException, SSLContextException { |
ContributorAuthor
There was a problem hiding this comment.
I reverted the entire file to master.
| } | ||
| protected AbstractRpcClient<?> createRpcClient(Configuration conf, boolean isSyncClient) { | ||
| protected AbstractRpcClient<?> createRpcClient(Configuration conf, boolean isSyncClient) |
Contributor
There was a problem hiding this comment.
Do we still need to throw SSLContextException here?
| @Test | ||
| public void testRpcWithWriteThread() throws IOException, InterruptedException { | ||
| public void testRpcWithWriteThread() | ||
| throws IOException, InterruptedException, SSLContextException { |
Uh oh!
There was an error while loading. Please reload this page.
Comment on lines
+245
to
+253
| SslContext result = sslContextForServer.get(); | ||
| if (result == null) { | ||
| result = X509Util.createSslContextForServer(conf); | ||
| if (!sslContextForServer.compareAndSet(null, result)) { | ||
| // lost the race, another thread already set the value | ||
| result = sslContextForServer.get(); | ||
| } | ||
| } | ||
| return result; |
Contributor
There was a problem hiding this comment.
If you create the context in initSsl then you don't have to lock, because it is called from the constructor.
ContributorAuthor
There was a problem hiding this comment.
This is done. I removed the entire lazy-init logic from the server.
Apache-HBase
commented
Aug 2, 2022
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 2, 2022
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 2, 2022
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 3, 2022
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 3, 2022
💔 -1 overall
This message was automatically generated. |
anmolnar
commented
Aug 3, 2022
ContributorAuthor
I'm working on fixing the unit tests, but can't figure out why |
ContributorAuthor
Eating the flush() call in |
Apache9
commented
Aug 3, 2022
Contributor
Ouch, let me take a look too... |
Contributor
Ah it is about the order... The original implementation is incorrect... It should place itself in front of BufferCallBeforeInitHandler... Change the saslNegotiate like this can solve the problem(also done a simple refactoring to give ReadTimeoutHandler a name as this is a general handler in netty, use class may have other side effects if we have other handlers with the same type in the pipeline) |
Apache9
commented
Aug 3, 2022
Contributor
Or I could open another PR for fixing this... |
Apache9
commented
Aug 5, 2022
Contributor
I think the second one has already been implemented? The first one can be a follow on issue. PTAL at the concerns raised by @wchevreuil . I think we could change TestTlsWithKerberos to test more sasl qops. Thanks. |
anmolnar
commented
Aug 5, 2022
ContributorAuthor
I've already tried that, it doesn't work. Neither privacy, nor integrity. I didn't dig too much into that, because TLS already provides both security and integrity, and in reality it doesn't make sense to me to use together with Kerberos' similar features. |
Apache-HBase
commented
Aug 5, 2022
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 5, 2022
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 5, 2022
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 6, 2022
💔 -1 overall
This message was automatically generated. |
anmolnar
commented
Aug 6, 2022
ContributorAuthor
@Apache9 I hopefully finished the final touches. As mentioned in the previous comment Kerberos privacy and integrity cannot work together with this TLS implementation. I receive the "Not a SSL/TLS packet" exception when any of these Krb features is enabled. We could disable this by raising an invalid configuration error in a later patch. The following items are outstanding for further PRs:
I can add the Kerberos config check to this.
Is there anything else I miss? |
Apache-HBase
commented
Aug 6, 2022
💔 -1 overall
This message was automatically generated. |
Apache9
commented
Aug 6, 2022
Contributor
I tried locally based on your patch, I could make the kerberos wrap/unwrap work with SSL, can work on a follow on issue to make it work, though this is not recommanded as it is useless and usually SSL could have a much better performance. The others are all good. Let me take a final look on the PR. Thanks. |
Apache9
commented
Aug 6, 2022
Contributor
Oh, when trying to add tests, I think we could also improve the tests, by extending the existing IPC UTs in HBase. Could also be a follow on issue. |
Apache-HBase
commented
Aug 6, 2022
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 6, 2022
🎊 +1 overall
This message was automatically generated. |
Apache9
approved these changes
Aug 6, 2022
| SslContextBuilder sslContextBuilder = SslContextBuilder.forClient(); | ||
| String keyStoreLocation = config.get(TLS_CONFIG_KEYSTORE_LOCATION, ""); | ||
| String keyStorePassword = config.get(TLS_CONFIG_KEYSTORE_PASSWORD, ""); |
Contributor
There was a problem hiding this comment.
Oh, this one is also a long term improment.
| boolean sslOcspEnabled = config.getBoolean(TLS_CONFIG_OCSP, false); | ||
| if (trustStoreLocation.isEmpty()) { | ||
| LOG.warn(TLS_CONFIG_TRUSTSTORE_LOCATION + " not specified"); |
Contributor
There was a problem hiding this comment.
Oh, IIRC I've asked this before. What will happen if we do not specify the location? The problem will go to the OS default location? Maybe you have already answered but I can not find the comments.
Anyway, can also be a follow on issue.
ContributorAuthor
There was a problem hiding this comment.
Yeah, I already commented the same. Truststore takes precedence, but otherwise OS certificate store will be checked just like in a browser. I need to verify this to be on the safe side.
bbeaudreault
approved these changes
Aug 6, 2022
bbeaudreault
left a comment
Contributor
There was a problem hiding this comment.
Looks great! Thank you both for all the work here. @anmolnar if you're done with this I can merge once pre-commit finishes. Let me know if that works for you
Apache-HBase
commented
Aug 6, 2022
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 6, 2022
🎊 +1 overall
This message was automatically generated. |
ContributorAuthor
@bbeaudreault Yeah, I'm done, feel free to merge the patch. Thanks everybody for the help!
and...
What do you mean exactly? |
bbeaudreault pushed a commit
that referenced
this pull request
Aug 6, 2022
…4666) Signed-off-by: Duo Zhang <zhangduo@apache.org> Signed-off-by: Bryan Beaudreault <bbeaudreault@apache.org> (cherry picked from commit f8dcf07) Conflicts: hbase-client/src/main/java/org/apache/hadoop/hbase/ipc/NettyRpcClient.java hbase-client/src/main/java/org/apache/hadoop/hbase/ipc/NettyRpcConnection.java hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/NettyRpcServer.java
Apache9
commented
Aug 7, 2022
Contributor
I could give it a try, un how to improve the tests. Let open an issue and create a PR. |
anmolnar
commented
Aug 7, 2022
ContributorAuthor
Thanks @bbeaudreault and @Apache9 for the help. I'll continue with the leftover items and documentation. |
bbeaudreault pushed a commit
to HubSpot/hbase
that referenced
this pull request
Oct 4, 2022
…pache#4666) Signed-off-by: Duo Zhang <zhangduo@apache.org> Signed-off-by: Bryan Beaudreault <bbeaudreault@apache.org> (cherry picked from commit f8dcf07) Conflicts: hbase-client/src/main/java/org/apache/hadoop/hbase/ipc/NettyRpcClient.java hbase-client/src/main/java/org/apache/hadoop/hbase/ipc/NettyRpcConnection.java hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/NettyRpcServer.java
bbeaudreault pushed a commit
to HubSpot/hbase
that referenced
this pull request
Apr 2, 2023
…pache#4666) Signed-off-by: Duo Zhang <zhangduo@apache.org> Signed-off-by: Bryan Beaudreault <bbeaudreault@apache.org> (cherry picked from commit f8dcf07) Conflicts: hbase-client/src/main/java/org/apache/hadoop/hbase/ipc/NettyRpcClient.java hbase-client/src/main/java/org/apache/hadoop/hbase/ipc/NettyRpcConnection.java hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/NettyRpcServer.java
bbeaudreault pushed a commit
to HubSpot/hbase
that referenced
this pull request
Apr 4, 2023
…pache#4666) Signed-off-by: Duo Zhang <zhangduo@apache.org> Signed-off-by: Bryan Beaudreault <bbeaudreault@apache.org> (cherry picked from commit f8dcf07) Conflicts: hbase-client/src/main/java/org/apache/hadoop/hbase/ipc/NettyRpcClient.java hbase-client/src/main/java/org/apache/hadoop/hbase/ipc/NettyRpcConnection.java hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/NettyRpcServer.java
bbeaudreault added a commit
to HubSpot/hbase
that referenced
this pull request
Apr 7, 2023
* HBASE-27185 Rewrite NettyRpcServer to decode rpc request with netty handler (apache#4624) * HBASE-27185 Addendum fix TestShadeSaslAuthenticationProvider * HBASE-27271 BufferCallBeforeInitHandler should ignore the flush request (apache#4676) * HBASE-26666 Add native TLS encryption support to RPC server/client (apache#4666) * HBASE-27278 Improve TestTlsIPC to reuse existing IPC test code (apache#4682) * HBASE-27279 Make SslHandler work with SaslWrapHandler/SaslUnwrapHandler (apache#4705) * HBASE-27342 Use Hadoop Credentials API to retrieve passwords of TLS key/trust stores (apache#4751) * HBASE-27346 Autodetect key/truststore file type from file extension (apache#4757) * HBASE-27280 Add mutual authentication support to TLS (apache#4796) * HBASE-27673 Fix mTLS client hostname verification (apache#5066) * HBASE-27347 Port FileWatcher from ZK to autodetect keystore/truststore changes in TLS connections (branch-2) (apache#4897) * HBASE-27779 Make X509Util config constants public * HBASE-27578 Upgrade hbase-thirdparty to 4.1.4 (apache#4985)
vinayakphegde pushed a commit
to vinayakphegde/hbase
that referenced
this pull request
Apr 4, 2024
…pache#4666) Change[1/4] for: Backporting the changes related to HBASE-26666. This commit does not contain secrets. Signed-off-by: Duo Zhang <zhangduo@apache.org> Signed-off-by: Bryan Beaudreault <bbeaudreault@apache.org> (cherry picked from commit f8dcf07) Change-Id: I6782126306c0c6bd91242285ef3b401288f040cc
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
6 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.
Based on @Apache9 's suggestions and the previous PR #4125 the following changes have been incorporated:
flush()call handling is fixed inBufferCallBeforeInitHandler, so it will not block the SSL handshake,SSLContextandSSLEngineare now based on Netty'sSslContextBuilderclass. In order to fully take advantage of that I had to add ByteBufAllocator config to the client side like we already have in the server,SSLContextis cached inX509Utilfor client and server. This is because I'd like to add FileWatchers for keystore/truststore in order to easily renew certificates in a running cluster.cc @bbeaudreault@meszibalu@joshelser