You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@hbase.apache.org by GitBox <gi...@apache.org> on 2022/08/23 22:04:51 UTC
[GitHub] [hbase] bbeaudreault opened a new pull request, #4724: HBASE-27280 Add mutual authentication support to TLS
bbeaudreault opened a new pull request, #4724:
URL: https://github.com/apache/hbase/pull/4724
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.
I still need to add end-to-end tests of the functionality, but wanted to get this up.
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache9 commented on pull request #4724: HBASE-27280 Add mutual authentication support to TLS
Posted by GitBox <gi...@apache.org>.
Apache9 commented on PR #4724:
URL: https://github.com/apache/hbase/pull/4724#issuecomment-1254434888
We are very close now, thanks @bbeaudreault for your patient!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #4724: HBASE-27280 Add mutual authentication support to TLS
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on PR #4724:
URL: https://github.com/apache/hbase/pull/4724#issuecomment-1224959295
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 0m 34s | Docker mode activated. |
| -0 :warning: | yetus | 0m 3s | Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck |
||| _ Prechecks _ |
||| _ master Compile Tests _ |
| +1 :green_heart: | mvninstall | 2m 28s | master passed |
| +1 :green_heart: | compile | 0m 14s | master passed |
| +1 :green_heart: | shadedjars | 4m 5s | branch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 0m 14s | master passed |
||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 2m 8s | the patch passed |
| +1 :green_heart: | compile | 0m 14s | the patch passed |
| +1 :green_heart: | javac | 0m 14s | the patch passed |
| +1 :green_heart: | shadedjars | 4m 3s | patch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 0m 12s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | unit | 1m 30s | hbase-common in the patch passed. |
| | | 16m 43s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4724/1/artifact/yetus-jdk8-hadoop3-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/4724 |
| Optional Tests | javac javadoc unit shadedjars compile |
| uname | Linux 4b10cf3f5bf3 5.4.0-1081-aws #88~18.04.1-Ubuntu SMP Thu Jun 23 16:29:17 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / 950ad8dd3e |
| Default Java | AdoptOpenJDK-1.8.0_282-b08 |
| Test Results | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4724/1/testReport/ |
| Max. process+thread count | 160 (vs. ulimit of 30000) |
| modules | C: hbase-common U: hbase-common |
| Console output | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4724/1/console |
| versions | git=2.17.1 maven=3.6.3 |
| Powered by | Apache Yetus 0.12.0 https://yetus.apache.org |
This message was automatically generated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #4724: HBASE-27280 Add mutual authentication support to TLS
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on PR #4724:
URL: https://github.com/apache/hbase/pull/4724#issuecomment-1241438867
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 0m 37s | Docker mode activated. |
| -0 :warning: | yetus | 0m 3s | Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck |
||| _ Prechecks _ |
||| _ master Compile Tests _ |
| +0 :ok: | mvndep | 0m 13s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 2m 23s | master passed |
| +1 :green_heart: | compile | 0m 48s | master passed |
| +1 :green_heart: | shadedjars | 4m 4s | branch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 0m 35s | master passed |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 11s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 2m 10s | the patch passed |
| +1 :green_heart: | compile | 0m 50s | the patch passed |
| +1 :green_heart: | javac | 0m 50s | the patch passed |
| +1 :green_heart: | shadedjars | 4m 6s | patch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 0m 33s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | unit | 1m 40s | hbase-common in the patch passed. |
| +1 :green_heart: | unit | 197m 51s | hbase-server in the patch passed. |
| | | 217m 50s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4724/4/artifact/yetus-jdk8-hadoop3-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/4724 |
| Optional Tests | javac javadoc unit shadedjars compile |
| uname | Linux aa50b3195d30 5.4.0-1081-aws #88~18.04.1-Ubuntu SMP Thu Jun 23 16:29:17 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / 793f020e13 |
| Default Java | AdoptOpenJDK-1.8.0_282-b08 |
| Test Results | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4724/4/testReport/ |
| Max. process+thread count | 2614 (vs. ulimit of 30000) |
| modules | C: hbase-common hbase-server U: . |
| Console output | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4724/4/console |
| versions | git=2.17.1 maven=3.6.3 |
| Powered by | Apache Yetus 0.12.0 https://yetus.apache.org |
This message was automatically generated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #4724: HBASE-27280 Add mutual authentication support to TLS
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on PR #4724:
URL: https://github.com/apache/hbase/pull/4724#issuecomment-1251566260
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 1m 5s | Docker mode activated. |
||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 0s | No case conflicting files found. |
| +1 :green_heart: | hbaseanti | 0m 0s | Patch does not have any anti-patterns. |
| +1 :green_heart: | @author | 0m 0s | The patch does not contain any @author tags. |
||| _ master Compile Tests _ |
| +0 :ok: | mvndep | 0m 11s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 2m 22s | master passed |
| +1 :green_heart: | compile | 2m 47s | master passed |
| +1 :green_heart: | checkstyle | 0m 45s | master passed |
| +1 :green_heart: | spotless | 0m 41s | branch has no errors when running spotless:check. |
| +1 :green_heart: | spotbugs | 1m 50s | master passed |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 10s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 2m 12s | the patch passed |
| +1 :green_heart: | compile | 2m 47s | the patch passed |
| -0 :warning: | javac | 2m 14s | hbase-server generated 1 new + 192 unchanged - 1 fixed = 193 total (was 193) |
| +1 :green_heart: | checkstyle | 0m 44s | the patch passed |
| +1 :green_heart: | whitespace | 0m 0s | The patch has no whitespace issues. |
| +1 :green_heart: | hadoopcheck | 7m 57s | Patch does not cause any errors with Hadoop 3.2.4 3.3.4. |
| +1 :green_heart: | spotless | 0m 39s | patch has no errors when running spotless:check. |
| +1 :green_heart: | spotbugs | 1m 57s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | asflicense | 0m 19s | The patch does not generate ASF License warnings. |
| | | 32m 4s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4724/7/artifact/yetus-general-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/4724 |
| Optional Tests | dupname asflicense javac spotbugs hadoopcheck hbaseanti spotless checkstyle compile |
| uname | Linux 7ce192c54d81 5.4.0-124-generic #140-Ubuntu SMP Thu Aug 4 02:23:37 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / 3f5c0a505a |
| Default Java | Temurin-1.8.0_345-b01 |
| javac | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4724/7/artifact/yetus-general-check/output/diff-compile-javac-hbase-server.txt |
| Max. process+thread count | 69 (vs. ulimit of 30000) |
| modules | C: hbase-common hbase-server U: . |
| Console output | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4724/7/console |
| versions | git=2.17.1 maven=3.6.3 spotbugs=4.7.2 |
| Powered by | Apache Yetus 0.12.0 https://yetus.apache.org |
This message was automatically generated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #4724: HBASE-27280 Add mutual authentication support to TLS
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on PR #4724:
URL: https://github.com/apache/hbase/pull/4724#issuecomment-1252737613
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 1m 3s | Docker mode activated. |
||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 0s | No case conflicting files found. |
| +1 :green_heart: | hbaseanti | 0m 0s | Patch does not have any anti-patterns. |
| +1 :green_heart: | @author | 0m 0s | The patch does not contain any @author tags. |
||| _ master Compile Tests _ |
| +0 :ok: | mvndep | 0m 15s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 2m 15s | master passed |
| +1 :green_heart: | compile | 2m 44s | master passed |
| +1 :green_heart: | checkstyle | 0m 45s | master passed |
| +1 :green_heart: | spotless | 0m 39s | branch has no errors when running spotless:check. |
| +1 :green_heart: | spotbugs | 1m 51s | master passed |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 10s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 2m 12s | the patch passed |
| +1 :green_heart: | compile | 2m 47s | the patch passed |
| +1 :green_heart: | javac | 2m 47s | the patch passed |
| -0 :warning: | checkstyle | 0m 15s | hbase-common: The patch generated 1 new + 0 unchanged - 0 fixed = 1 total (was 0) |
| +1 :green_heart: | whitespace | 0m 1s | The patch has no whitespace issues. |
| +1 :green_heart: | hadoopcheck | 8m 1s | Patch does not cause any errors with Hadoop 3.2.4 3.3.4. |
| -1 :x: | spotless | 0m 14s | patch has 24 errors when running spotless:check, run spotless:apply to fix. |
| +1 :green_heart: | spotbugs | 1m 59s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | asflicense | 0m 19s | The patch does not generate ASF License warnings. |
| | | 31m 6s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4724/8/artifact/yetus-general-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/4724 |
| Optional Tests | dupname asflicense javac spotbugs hadoopcheck hbaseanti spotless checkstyle compile |
| uname | Linux 998252923d62 5.4.0-124-generic #140-Ubuntu SMP Thu Aug 4 02:23:37 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / 8bc6a5954a |
| Default Java | Temurin-1.8.0_345-b01 |
| checkstyle | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4724/8/artifact/yetus-general-check/output/diff-checkstyle-hbase-common.txt |
| spotless | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4724/8/artifact/yetus-general-check/output/patch-spotless.txt |
| Max. process+thread count | 64 (vs. ulimit of 30000) |
| modules | C: hbase-common hbase-server U: . |
| Console output | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4724/8/console |
| versions | git=2.17.1 maven=3.6.3 spotbugs=4.7.2 |
| Powered by | Apache Yetus 0.12.0 https://yetus.apache.org |
This message was automatically generated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] bbeaudreault commented on a diff in pull request #4724: HBASE-27280 Add mutual authentication support to TLS
Posted by GitBox <gi...@apache.org>.
bbeaudreault commented on code in PR #4724:
URL: https://github.com/apache/hbase/pull/4724#discussion_r974346146
##########
hbase-common/src/main/java/org/apache/hadoop/hbase/io/crypto/tls/HBaseTrustManager.java:
##########
@@ -0,0 +1,160 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hbase.io.crypto.tls;
+
+import java.net.InetAddress;
+import java.net.Socket;
+import java.net.UnknownHostException;
+import java.security.cert.CertificateException;
+import java.security.cert.X509Certificate;
+import javax.net.ssl.SSLEngine;
+import javax.net.ssl.SSLException;
+import javax.net.ssl.X509ExtendedTrustManager;
+import org.apache.yetus.audience.InterfaceAudience;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * A custom TrustManager that supports hostname verification We attempt to perform verification
Review Comment:
Sorry for the incomplete message. I had been trying various ways of phrasing it and forgot to clean it up :)
Here's a link with an example of how we might provide the identification algo to netty: https://codetinkering.com/enable-hostname-verification-netty/. You probably know better than me though if there's a better alternative.
Thanks for looking
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] bbeaudreault commented on pull request #4724: HBASE-27280 Add mutual authentication support to TLS
Posted by GitBox <gi...@apache.org>.
bbeaudreault commented on PR #4724:
URL: https://github.com/apache/hbase/pull/4724#issuecomment-1241946893
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] bbeaudreault commented on pull request #4724: HBASE-27280 Add mutual authentication support to TLS
Posted by GitBox <gi...@apache.org>.
bbeaudreault commented on PR #4724:
URL: https://github.com/apache/hbase/pull/4724#issuecomment-1226030954
@anmolnar I can't assign you as a reviewer, but would appreciate a look when you get a chance.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #4724: HBASE-27280 Add mutual authentication support to TLS
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on PR #4724:
URL: https://github.com/apache/hbase/pull/4724#issuecomment-1242468023
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 0m 35s | Docker mode activated. |
| -0 :warning: | yetus | 0m 3s | Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck |
||| _ Prechecks _ |
||| _ master Compile Tests _ |
| +0 :ok: | mvndep | 0m 14s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 2m 4s | master passed |
| +1 :green_heart: | compile | 0m 48s | master passed |
| +1 :green_heart: | shadedjars | 4m 6s | branch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 0m 34s | master passed |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 11s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 2m 8s | the patch passed |
| +1 :green_heart: | compile | 0m 49s | the patch passed |
| +1 :green_heart: | javac | 0m 49s | the patch passed |
| +1 :green_heart: | shadedjars | 4m 7s | patch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 0m 33s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | unit | 1m 41s | hbase-common in the patch passed. |
| +1 :green_heart: | unit | 199m 53s | hbase-server in the patch passed. |
| | | 219m 22s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4724/6/artifact/yetus-jdk8-hadoop3-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/4724 |
| Optional Tests | javac javadoc unit shadedjars compile |
| uname | Linux f0fddfab03c2 5.4.0-1081-aws #88~18.04.1-Ubuntu SMP Thu Jun 23 16:29:17 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / f3f88ff6c1 |
| Default Java | AdoptOpenJDK-1.8.0_282-b08 |
| Test Results | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4724/6/testReport/ |
| Max. process+thread count | 2987 (vs. ulimit of 30000) |
| modules | C: hbase-common hbase-server U: . |
| Console output | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4724/6/console |
| versions | git=2.17.1 maven=3.6.3 |
| Powered by | Apache Yetus 0.12.0 https://yetus.apache.org |
This message was automatically generated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #4724: HBASE-27280 Add mutual authentication support to TLS
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on PR #4724:
URL: https://github.com/apache/hbase/pull/4724#issuecomment-1241980615
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 1m 9s | Docker mode activated. |
||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 0s | No case conflicting files found. |
| +1 :green_heart: | hbaseanti | 0m 0s | Patch does not have any anti-patterns. |
| +1 :green_heart: | @author | 0m 0s | The patch does not contain any @author tags. |
||| _ master Compile Tests _ |
| +0 :ok: | mvndep | 0m 17s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 2m 15s | master passed |
| +1 :green_heart: | compile | 2m 45s | master passed |
| +1 :green_heart: | checkstyle | 0m 46s | master passed |
| +1 :green_heart: | spotless | 0m 39s | branch has no errors when running spotless:check. |
| -1 :x: | spotbugs | 1m 16s | hbase-server in master has 1 extant spotbugs warnings. |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 12s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 2m 7s | the patch passed |
| +1 :green_heart: | compile | 2m 47s | the patch passed |
| -0 :warning: | javac | 0m 32s | hbase-common generated 2 new + 2 unchanged - 0 fixed = 4 total (was 2) |
| +1 :green_heart: | checkstyle | 0m 45s | the patch passed |
| +1 :green_heart: | whitespace | 0m 0s | The patch has no whitespace issues. |
| +1 :green_heart: | hadoopcheck | 7m 54s | Patch does not cause any errors with Hadoop 3.2.4 3.3.4. |
| +1 :green_heart: | spotless | 0m 39s | patch has no errors when running spotless:check. |
| +1 :green_heart: | spotbugs | 0m 34s | hbase-common in the patch passed. |
| +1 :green_heart: | spotbugs | 1m 23s | hbase-server generated 0 new + 0 unchanged - 1 fixed = 0 total (was 1) |
||| _ Other Tests _ |
| +1 :green_heart: | asflicense | 0m 18s | The patch does not generate ASF License warnings. |
| | | 31m 44s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4724/5/artifact/yetus-general-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/4724 |
| Optional Tests | dupname asflicense javac spotbugs hadoopcheck hbaseanti spotless checkstyle compile |
| uname | Linux 825ed5f96bff 5.4.0-124-generic #140-Ubuntu SMP Thu Aug 4 02:23:37 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / 793f020e13 |
| Default Java | AdoptOpenJDK-1.8.0_282-b08 |
| spotbugs | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4724/5/artifact/yetus-general-check/output/branch-spotbugs-hbase-server-warnings.html |
| javac | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4724/5/artifact/yetus-general-check/output/diff-compile-javac-hbase-common.txt |
| Max. process+thread count | 64 (vs. ulimit of 30000) |
| modules | C: hbase-common hbase-server U: . |
| Console output | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4724/5/console |
| versions | git=2.17.1 maven=3.6.3 spotbugs=4.2.2 |
| Powered by | Apache Yetus 0.12.0 https://yetus.apache.org |
This message was automatically generated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] bbeaudreault commented on pull request #4724: HBASE-27280 Add mutual authentication support to TLS
Posted by GitBox <gi...@apache.org>.
bbeaudreault commented on PR #4724:
URL: https://github.com/apache/hbase/pull/4724#issuecomment-1255094102
Thanks for all the guidance and review here, Duo!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #4724: HBASE-27280 Add mutual authentication support to TLS
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on PR #4724:
URL: https://github.com/apache/hbase/pull/4724#issuecomment-1253955830
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 1m 0s | Docker mode activated. |
||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 0s | No case conflicting files found. |
| +1 :green_heart: | hbaseanti | 0m 0s | Patch does not have any anti-patterns. |
| +1 :green_heart: | @author | 0m 0s | The patch does not contain any @author tags. |
||| _ master Compile Tests _ |
| +0 :ok: | mvndep | 0m 13s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 2m 9s | master passed |
| +1 :green_heart: | compile | 2m 47s | master passed |
| +1 :green_heart: | checkstyle | 0m 44s | master passed |
| +1 :green_heart: | spotless | 0m 39s | branch has no errors when running spotless:check. |
| +1 :green_heart: | spotbugs | 1m 45s | master passed |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 11s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 2m 11s | the patch passed |
| +1 :green_heart: | compile | 2m 45s | the patch passed |
| -0 :warning: | javac | 2m 13s | hbase-server generated 1 new + 192 unchanged - 1 fixed = 193 total (was 193) |
| +1 :green_heart: | checkstyle | 0m 44s | the patch passed |
| +1 :green_heart: | whitespace | 0m 0s | The patch has no whitespace issues. |
| +1 :green_heart: | hadoopcheck | 7m 59s | Patch does not cause any errors with Hadoop 3.2.4 3.3.4. |
| +1 :green_heart: | spotless | 0m 39s | patch has no errors when running spotless:check. |
| +1 :green_heart: | spotbugs | 1m 59s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | asflicense | 0m 19s | The patch does not generate ASF License warnings. |
| | | 31m 20s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4724/10/artifact/yetus-general-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/4724 |
| Optional Tests | dupname asflicense javac spotbugs hadoopcheck hbaseanti spotless checkstyle compile |
| uname | Linux 99e1f6546893 5.4.0-124-generic #140-Ubuntu SMP Thu Aug 4 02:23:37 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / de127bde84 |
| Default Java | Temurin-1.8.0_345-b01 |
| javac | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4724/10/artifact/yetus-general-check/output/diff-compile-javac-hbase-server.txt |
| Max. process+thread count | 64 (vs. ulimit of 30000) |
| modules | C: hbase-common hbase-server U: . |
| Console output | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4724/10/console |
| versions | git=2.17.1 maven=3.6.3 spotbugs=4.7.2 |
| Powered by | Apache Yetus 0.12.0 https://yetus.apache.org |
This message was automatically generated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache9 commented on a diff in pull request #4724: HBASE-27280 Add mutual authentication support to TLS
Posted by GitBox <gi...@apache.org>.
Apache9 commented on code in PR #4724:
URL: https://github.com/apache/hbase/pull/4724#discussion_r975459148
##########
hbase-common/src/main/java/org/apache/hadoop/hbase/io/crypto/tls/HBaseHostnameVerifier.java:
##########
@@ -0,0 +1,274 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hbase.io.crypto.tls;
+
+import java.net.InetAddress;
+import java.security.cert.Certificate;
+import java.security.cert.CertificateParsingException;
+import java.security.cert.X509Certificate;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.List;
+import java.util.Locale;
+import java.util.NoSuchElementException;
+import java.util.Objects;
+import java.util.Optional;
+import javax.naming.InvalidNameException;
+import javax.naming.NamingException;
+import javax.naming.directory.Attribute;
+import javax.naming.directory.Attributes;
+import javax.naming.ldap.LdapName;
+import javax.naming.ldap.Rdn;
+import javax.net.ssl.HostnameVerifier;
+import javax.net.ssl.SSLException;
+import javax.net.ssl.SSLPeerUnverifiedException;
+import javax.net.ssl.SSLSession;
+import javax.security.auth.x500.X500Principal;
+import org.apache.yetus.audience.InterfaceAudience;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.apache.hbase.thirdparty.com.google.common.net.InetAddresses;
+
+/**
+ * When enabled in {@link X509Util}, handles verifying that the hostname of a peer matches the
+ * certificate it presents.
+ * <p/>
+ * This file has been copied from the Apache ZooKeeper project.
+ * @see <a href=
+ * "https://github.com/apache/zookeeper/blob/5820d10d9dc58c8e12d2e25386fdf92acb360359/zookeeper-server/src/main/java/org/apache/zookeeper/common/ZKHostnameVerifier.java">Base
+ * revision</a>
+ */
+@InterfaceAudience.Private
+final class HBaseHostnameVerifier implements HostnameVerifier {
+
+ private final Logger LOG = LoggerFactory.getLogger(HBaseHostnameVerifier.class);
+
+ /**
+ * Note: copied from Apache httpclient with some minor modifications. We want host verification,
+ * but depending on the httpclient jar caused unexplained performance regressions (even when the
+ * code was not used).
+ */
+ private static final class SubjectName {
+
+ static final int DNS = 2;
+ static final int IP = 7;
+
+ private final String value;
+ private final int type;
+
+ SubjectName(final String value, final int type) {
+ if (type != DNS && type != IP) {
+ throw new IllegalArgumentException("Invalid type: " + type);
+ }
+ this.value = Objects.requireNonNull(value);
+ this.type = type;
+ }
+
+ public int getType() {
+ return type;
+ }
+
+ public String getValue() {
+ return value;
+ }
+
+ @Override
+ public String toString() {
+ return value;
+ }
+
+ }
+
+ @Override
+ public boolean verify(final String host, final SSLSession session) {
+ try {
+ final Certificate[] certs = session.getPeerCertificates();
+ final X509Certificate x509 = (X509Certificate) certs[0];
+ verify(host, x509);
+ return true;
+ } catch (final SSLException ex) {
+ LOG.debug("Unexpected exception", ex);
+ return false;
+ }
+ }
+
+ void verify(final String host, final X509Certificate cert) throws SSLException {
+ final List<SubjectName> subjectAlts = getSubjectAltNames(cert);
+ if (subjectAlts != null && !subjectAlts.isEmpty()) {
+ Optional<InetAddress> inetAddress = parseIpAddress(host);
+ if (inetAddress.isPresent()) {
+ matchIPAddress(host, inetAddress.get(), subjectAlts);
+ } else {
+ matchDNSName(host, subjectAlts);
+ }
+ } else {
+ // CN matching has been deprecated by rfc2818 and can be used
+ // as fallback only when no subjectAlts are available
+ final X500Principal subjectPrincipal = cert.getSubjectX500Principal();
+ final String cn = extractCN(subjectPrincipal.getName(X500Principal.RFC2253));
+ if (cn == null) {
+ throw new SSLException("Certificate subject for <" + host + "> doesn't contain "
+ + "a common name and does not have alternative names");
+ }
+ matchCN(host, cn);
+ }
+ }
+
+ private static void matchIPAddress(final String host, final InetAddress inetAddress,
+ final List<SubjectName> subjectAlts) throws SSLException {
+ for (final SubjectName subjectAlt : subjectAlts) {
+ if (subjectAlt.getType() == SubjectName.IP) {
+ Optional<InetAddress> parsed = parseIpAddress(subjectAlt.getValue());
+ if (parsed.filter(altAddr -> altAddr.equals(inetAddress)).isPresent()) {
+ return;
+ }
+ }
+ }
+ throw new SSLPeerUnverifiedException("Certificate for <" + host + "> doesn't match any "
+ + "of the subject alternative names: " + subjectAlts);
+ }
+
+ private static void matchDNSName(final String host, final List<SubjectName> subjectAlts)
+ throws SSLException {
+ final String normalizedHost = host.toLowerCase(Locale.ROOT);
+ for (final SubjectName subjectAlt : subjectAlts) {
+ if (subjectAlt.getType() == SubjectName.DNS) {
+ final String normalizedSubjectAlt = subjectAlt.getValue().toLowerCase(Locale.ROOT);
+ if (matchIdentityStrict(normalizedHost, normalizedSubjectAlt)) {
+ return;
+ }
+ }
+ }
+ throw new SSLPeerUnverifiedException("Certificate for <" + host + "> doesn't match any "
+ + "of the subject alternative names: " + subjectAlts);
+ }
+
+ private static void matchCN(final String host, final String cn) throws SSLException {
+ final String normalizedHost = host.toLowerCase(Locale.ROOT);
+ final String normalizedCn = cn.toLowerCase(Locale.ROOT);
+ if (!matchIdentityStrict(normalizedHost, normalizedCn)) {
+ throw new SSLPeerUnverifiedException("Certificate for <" + host + "> doesn't match "
+ + "common name of the certificate subject: " + cn);
+ }
+ }
+
+ private static boolean matchIdentity(final String host, final String identity,
+ final boolean strict) {
+ // RFC 2818, 3.1. Server Identity
+ // "...Names may contain the wildcard
+ // character * which is considered to match any single domain name
+ // component or component fragment..."
+ // Based on this statement presuming only singular wildcard is legal
+ final int asteriskIdx = identity.indexOf('*');
+ if (asteriskIdx != -1) {
+ final String prefix = identity.substring(0, asteriskIdx);
+ final String suffix = identity.substring(asteriskIdx + 1);
+ if (!prefix.isEmpty() && !host.startsWith(prefix)) {
+ return false;
+ }
+ if (!suffix.isEmpty() && !host.endsWith(suffix)) {
+ return false;
+ }
+ // Additional sanity checks on content selected by wildcard can be done here
+ if (strict) {
+ final String remainder = host.substring(prefix.length(), host.length() - suffix.length());
+ return !remainder.contains(".");
+ }
+ return true;
+ }
+ return host.equalsIgnoreCase(identity);
+ }
+
+ private static boolean matchIdentityStrict(final String host, final String identity) {
+ return matchIdentity(host, identity, true);
+ }
+
+ private static String extractCN(final String subjectPrincipal) throws SSLException {
+ if (subjectPrincipal == null) {
+ return null;
+ }
+ try {
+ final LdapName subjectDN = new LdapName(subjectPrincipal);
+ final List<Rdn> rdns = subjectDN.getRdns();
+ for (int i = rdns.size() - 1; i >= 0; i--) {
+ final Rdn rds = rdns.get(i);
+ final Attributes attributes = rds.toAttributes();
+ final Attribute cn = attributes.get("cn");
+ if (cn != null) {
+ try {
+ final Object value = cn.get();
+ if (value != null) {
+ return value.toString();
+ }
+ } catch (final NoSuchElementException ignore) {
+ // ignore exception
+ } catch (final NamingException ignore) {
+ // ignore exception
+ }
+ }
+ }
+ return null;
+ } catch (final InvalidNameException e) {
+ throw new SSLException(subjectPrincipal + " is not a valid X500 distinguished name");
+ }
+ }
+
+ private static Optional<InetAddress> parseIpAddress(String host) {
+ try {
+ host = host.trim();
+ // Uri strings only work for ipv6 and are wrapped with brackets
+ // Unfortunately InetAddresses can't handle a mixed input, so we
+ // check here and choose which parse method to use.
+ if (host.startsWith("[") && host.endsWith("]")) {
+ return Optional.of(InetAddresses.forUriString(host));
+ } else {
+ return Optional.of(InetAddresses.forString(host));
+ }
+ } catch (Exception e) {
Review Comment:
For this case, usually it is recommended to add checks instead of throwing exceptions :)
https://www.baeldung.com/java-check-string-number
You can see the micro benchmark in 6.2 section, only 5% percentage of illegal input can cause a big performance impact if we choose to use NumberFormatException to test whether the input is valid.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] bbeaudreault commented on a diff in pull request #4724: HBASE-27280 Add mutual authentication support to TLS
Posted by GitBox <gi...@apache.org>.
bbeaudreault commented on code in PR #4724:
URL: https://github.com/apache/hbase/pull/4724#discussion_r974453294
##########
hbase-common/src/main/java/org/apache/hadoop/hbase/io/crypto/tls/HBaseHostnameVerifier.java:
##########
@@ -0,0 +1,368 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hbase.io.crypto.tls;
+
+import java.net.InetAddress;
+import java.net.UnknownHostException;
+import java.security.cert.Certificate;
+import java.security.cert.CertificateParsingException;
+import java.security.cert.X509Certificate;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.List;
+import java.util.Locale;
+import java.util.NoSuchElementException;
+import java.util.Objects;
+import java.util.regex.Pattern;
+import javax.naming.InvalidNameException;
+import javax.naming.NamingException;
+import javax.naming.directory.Attribute;
+import javax.naming.directory.Attributes;
+import javax.naming.ldap.LdapName;
+import javax.naming.ldap.Rdn;
+import javax.net.ssl.HostnameVerifier;
+import javax.net.ssl.SSLException;
+import javax.net.ssl.SSLPeerUnverifiedException;
+import javax.net.ssl.SSLSession;
+import javax.security.auth.x500.X500Principal;
+import org.apache.yetus.audience.InterfaceAudience;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * When enabled in {@link X509Util}, handles verifying that the hostname of a peer matches the
+ * certificate it presents.
+ * <p/>
+ * This file has been copied from the Apache ZooKeeper project.
+ * @see <a href=
Review Comment:
Yea, there's a comment below (copied from ZK):
```
/**
* Note: copied from Apache httpclient with some minor modifications. We want host verification,
* but depending on the httpclient jar caused unexplained performance regressions (even when the
* code was not used).
*/
```
Looks like we pull in httpcomponents `httpclient` in hbase-http, but not elsewhere. Do we want to try including it here instead?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] bbeaudreault commented on a diff in pull request #4724: HBASE-27280 Add mutual authentication support to TLS
Posted by GitBox <gi...@apache.org>.
bbeaudreault commented on code in PR #4724:
URL: https://github.com/apache/hbase/pull/4724#discussion_r974216103
##########
hbase-common/src/main/java/org/apache/hadoop/hbase/io/crypto/tls/HBaseTrustManager.java:
##########
@@ -0,0 +1,160 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hbase.io.crypto.tls;
+
+import java.net.InetAddress;
+import java.net.Socket;
+import java.net.UnknownHostException;
+import java.security.cert.CertificateException;
+import java.security.cert.X509Certificate;
+import javax.net.ssl.SSLEngine;
+import javax.net.ssl.SSLException;
+import javax.net.ssl.X509ExtendedTrustManager;
+import org.apache.yetus.audience.InterfaceAudience;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * A custom TrustManager that supports hostname verification We attempt to perform verification
Review Comment:
> This is not the default behavior of java's TrustManager implementation?
There is some basic support for hostname verification in the default TrustManager. You enable it by calling `sslParameters.setEndpointIdentificationAlgorithm("HTTPS")`. It only supports HTTPS and LDAP algorithms, which enforces some specific rules on wildcard matching and other things.
The tricky thing with that is Netty does not expose the SSLParameters to us, so we can't pass that in. We could potentially get around this by creating our own wrapper HBaseSslContext which wraps netty's SslContext and overloads `newEngine`, etc methods. This would allow us to set the aboe method onto the SSLParameters passed into the created engine.... but it's a wrapper either way, and this one seems more simple and to the point. This also gives us more control over the verification itself (i.e. DNS/IP instead of HTTPS/LDAP)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #4724: HBASE-27280 Add mutual authentication support to TLS
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on PR #4724:
URL: https://github.com/apache/hbase/pull/4724#issuecomment-1251707181
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 2m 12s | Docker mode activated. |
| -0 :warning: | yetus | 0m 2s | Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck |
||| _ Prechecks _ |
||| _ master Compile Tests _ |
| +0 :ok: | mvndep | 0m 16s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 2m 23s | master passed |
| +1 :green_heart: | compile | 0m 57s | master passed |
| +1 :green_heart: | shadedjars | 3m 47s | branch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 0m 41s | master passed |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 14s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 2m 12s | the patch passed |
| +1 :green_heart: | compile | 0m 58s | the patch passed |
| +1 :green_heart: | javac | 0m 58s | the patch passed |
| +1 :green_heart: | shadedjars | 3m 47s | patch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 0m 39s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | unit | 1m 45s | hbase-common in the patch passed. |
| +1 :green_heart: | unit | 207m 52s | hbase-server in the patch passed. |
| | | 229m 53s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4724/7/artifact/yetus-jdk8-hadoop3-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/4724 |
| Optional Tests | javac javadoc unit shadedjars compile |
| uname | Linux 5052ed47e636 5.4.0-122-generic #138-Ubuntu SMP Wed Jun 22 15:00:31 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / 3f5c0a505a |
| Default Java | Temurin-1.8.0_345-b01 |
| Test Results | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4724/7/testReport/ |
| Max. process+thread count | 2402 (vs. ulimit of 30000) |
| modules | C: hbase-common hbase-server U: . |
| Console output | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4724/7/console |
| versions | git=2.17.1 maven=3.6.3 |
| Powered by | Apache Yetus 0.12.0 https://yetus.apache.org |
This message was automatically generated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] bbeaudreault commented on a diff in pull request #4724: HBASE-27280 Add mutual authentication support to TLS
Posted by GitBox <gi...@apache.org>.
bbeaudreault commented on code in PR #4724:
URL: https://github.com/apache/hbase/pull/4724#discussion_r954068720
##########
hbase-common/src/test/java/org/apache/hadoop/hbase/io/crypto/tls/TestHBaseHostnameVerifier.java:
##########
@@ -0,0 +1,230 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hbase.io.crypto.tls;
+
+import static org.junit.Assert.fail;
+
+import java.io.ByteArrayInputStream;
+import java.io.InputStream;
+import java.lang.invoke.MethodHandles;
+import java.security.KeyPair;
+import java.security.Security;
+import java.security.cert.CertificateFactory;
+import java.security.cert.X509Certificate;
+import javax.net.ssl.SSLException;
+import org.apache.hadoop.hbase.HBaseClassTestRule;
+import org.apache.hadoop.hbase.testclassification.MiscTests;
+import org.apache.hadoop.hbase.testclassification.SmallTests;
+import org.bouncycastle.asn1.x500.X500Name;
+import org.bouncycastle.asn1.x500.X500NameBuilder;
+import org.bouncycastle.asn1.x500.style.BCStyle;
+import org.bouncycastle.asn1.x509.GeneralName;
+import org.bouncycastle.asn1.x509.GeneralNames;
+import org.bouncycastle.jce.provider.BouncyCastleProvider;
+import org.junit.Before;
+import org.junit.BeforeClass;
+import org.junit.ClassRule;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+
+/**
+ * Test cases taken and adapted from Apache ZooKeeper Project
+ * @see <a href=
+ * "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 })
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] bbeaudreault commented on a diff in pull request #4724: HBASE-27280 Add mutual authentication support to TLS
Posted by GitBox <gi...@apache.org>.
bbeaudreault commented on code in PR #4724:
URL: https://github.com/apache/hbase/pull/4724#discussion_r954065571
##########
hbase-common/src/main/java/org/apache/hadoop/hbase/io/crypto/tls/HBaseHostnameVerifier.java:
##########
@@ -0,0 +1,367 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hbase.io.crypto.tls;
+
+import java.net.InetAddress;
+import java.net.UnknownHostException;
+import java.security.cert.Certificate;
+import java.security.cert.CertificateParsingException;
+import java.security.cert.X509Certificate;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.List;
+import java.util.Locale;
+import java.util.NoSuchElementException;
+import java.util.Objects;
+import java.util.regex.Pattern;
+import javax.naming.InvalidNameException;
+import javax.naming.NamingException;
+import javax.naming.directory.Attribute;
+import javax.naming.directory.Attributes;
+import javax.naming.ldap.LdapName;
+import javax.naming.ldap.Rdn;
+import javax.net.ssl.HostnameVerifier;
+import javax.net.ssl.SSLException;
+import javax.net.ssl.SSLPeerUnverifiedException;
+import javax.net.ssl.SSLSession;
+import javax.security.auth.x500.X500Principal;
+import org.apache.yetus.audience.InterfaceAudience;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * When enabled in {@link X509Util}, handles verifying that the hostname of a peer matches the
+ * certificate it presents.
+ * <p/>
+ * This file has been copied from the Apache ZooKeeper project.
+ * @see <a href=
+ * "https://github.com/apache/zookeeper/blob/5820d10d9dc58c8e12d2e25386fdf92acb360359/zookeeper-server/src/main/java/org/apache/zookeeper/common/ZKHostnameVerifier.java">Base
+ * revision</a>
+ */
+@InterfaceAudience.Private
+final class HBaseHostnameVerifier implements HostnameVerifier {
Review Comment:
this file was copied verbatim
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] bbeaudreault commented on a diff in pull request #4724: HBASE-27280 Add mutual authentication support to TLS
Posted by GitBox <gi...@apache.org>.
bbeaudreault commented on code in PR #4724:
URL: https://github.com/apache/hbase/pull/4724#discussion_r954066160
##########
hbase-common/src/main/java/org/apache/hadoop/hbase/io/crypto/tls/HBaseTrustManager.java:
##########
@@ -0,0 +1,160 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hbase.io.crypto.tls;
+
+import java.net.InetAddress;
+import java.net.Socket;
+import java.net.UnknownHostException;
+import java.security.cert.CertificateException;
+import java.security.cert.X509Certificate;
+import javax.net.ssl.SSLEngine;
+import javax.net.ssl.SSLException;
+import javax.net.ssl.X509ExtendedTrustManager;
+import org.apache.yetus.audience.InterfaceAudience;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * A custom TrustManager that supports hostname verification We attempt to perform verification
+ * using just the IP address first and if that fails will attempt to perform a reverse DNS lookup
+ * and verify using the hostname. This file has been copied from the Apache ZooKeeper project.
+ * @see <a href=
+ * "https://github.com/apache/zookeeper/blob/c74658d398cdc1d207aa296cb6e20de00faec03e/zookeeper-server/src/main/java/org/apache/zookeeper/common/ZKTrustManager.java">Base
+ * revision</a>
+ */
+@InterfaceAudience.Private
+public class HBaseTrustManager extends X509ExtendedTrustManager {
Review Comment:
this file was copied verbatim
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] bbeaudreault commented on pull request #4724: HBASE-27280 Add mutual authentication support to TLS
Posted by GitBox <gi...@apache.org>.
bbeaudreault commented on PR #4724:
URL: https://github.com/apache/hbase/pull/4724#issuecomment-1236902293
@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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #4724: HBASE-27280 Add mutual authentication support to TLS
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on PR #4724:
URL: https://github.com/apache/hbase/pull/4724#issuecomment-1242297182
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 1m 1s | Docker mode activated. |
||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 0s | No case conflicting files found. |
| +1 :green_heart: | hbaseanti | 0m 0s | Patch does not have any anti-patterns. |
| +1 :green_heart: | @author | 0m 0s | The patch does not contain any @author tags. |
||| _ master Compile Tests _ |
| +0 :ok: | mvndep | 0m 12s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 2m 26s | master passed |
| +1 :green_heart: | compile | 2m 50s | master passed |
| +1 :green_heart: | checkstyle | 0m 46s | master passed |
| +1 :green_heart: | spotless | 0m 42s | branch has no errors when running spotless:check. |
| -1 :x: | spotbugs | 1m 16s | hbase-server in master has 1 extant spotbugs warnings. |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 12s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 2m 10s | the patch passed |
| +1 :green_heart: | compile | 2m 45s | the patch passed |
| +1 :green_heart: | javac | 2m 45s | the patch passed |
| +1 :green_heart: | checkstyle | 0m 45s | the patch passed |
| +1 :green_heart: | whitespace | 0m 0s | The patch has no whitespace issues. |
| +1 :green_heart: | hadoopcheck | 7m 57s | Patch does not cause any errors with Hadoop 3.2.4 3.3.4. |
| +1 :green_heart: | spotless | 0m 38s | patch has no errors when running spotless:check. |
| +1 :green_heart: | spotbugs | 0m 34s | hbase-common in the patch passed. |
| +1 :green_heart: | spotbugs | 1m 25s | hbase-server generated 0 new + 0 unchanged - 1 fixed = 0 total (was 1) |
||| _ Other Tests _ |
| +1 :green_heart: | asflicense | 0m 20s | The patch does not generate ASF License warnings. |
| | | 32m 0s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4724/6/artifact/yetus-general-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/4724 |
| Optional Tests | dupname asflicense javac spotbugs hadoopcheck hbaseanti spotless checkstyle compile |
| uname | Linux 7bc49357fbec 5.4.0-122-generic #138-Ubuntu SMP Wed Jun 22 15:00:31 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / f3f88ff6c1 |
| Default Java | AdoptOpenJDK-1.8.0_282-b08 |
| spotbugs | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4724/6/artifact/yetus-general-check/output/branch-spotbugs-hbase-server-warnings.html |
| Max. process+thread count | 60 (vs. ulimit of 30000) |
| modules | C: hbase-common hbase-server U: . |
| Console output | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4724/6/console |
| versions | git=2.17.1 maven=3.6.3 spotbugs=4.2.2 |
| Powered by | Apache Yetus 0.12.0 https://yetus.apache.org |
This message was automatically generated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] bbeaudreault commented on a diff in pull request #4724: HBASE-27280 Add mutual authentication support to TLS
Posted by GitBox <gi...@apache.org>.
bbeaudreault commented on code in PR #4724:
URL: https://github.com/apache/hbase/pull/4724#discussion_r974216103
##########
hbase-common/src/main/java/org/apache/hadoop/hbase/io/crypto/tls/HBaseTrustManager.java:
##########
@@ -0,0 +1,160 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hbase.io.crypto.tls;
+
+import java.net.InetAddress;
+import java.net.Socket;
+import java.net.UnknownHostException;
+import java.security.cert.CertificateException;
+import java.security.cert.X509Certificate;
+import javax.net.ssl.SSLEngine;
+import javax.net.ssl.SSLException;
+import javax.net.ssl.X509ExtendedTrustManager;
+import org.apache.yetus.audience.InterfaceAudience;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * A custom TrustManager that supports hostname verification We attempt to perform verification
Review Comment:
> This is not the default behavior of java's TrustManager implementation?
There is some basic support for hostname verification in the default TrustManager. You enable it by calling `sslParameters.setEndpointIdentificationAlgorithm("HTTPS")`. It only supports HTTPS and LDAP algorithms, which enforces some specific rules on wildcard matching and other things.
The tricky thing with that is Netty does not expose the SSLParameters to us, so we can't pass that in. We could potentially get around this by creating our own wrapper HBaseSslContext which wraps netty's SslContext and overloads `newEngine`, etc methods. This would allow us to set the aboe method onto the SSLParameters passed into the created engine.... but it's a wrapper either way, and this one seems more simple and to the point. This also gives us more control over the verification itself (i.e. DNS/IP instead of HTTPS/LDAP)
And the HTTPS/LDAP requirement might be restrictive. We could get arou
It's not enabled by default and can only be enabled via SSLParameters. Netty doesn't expose this on the usual SslContextBuilder, we'd need to The other problem is, it can only be enabled
No, and this is verified via the new tests I added -- when verify.hostname = false (on client or server), if default TrustManager had this default behavior, we'd still expect the VERIFIABLE_CERT_WITH_BAD_HOST test case to throw an exception or otherwise fail the handshake. But it only throws the exp
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #4724: HBASE-27280 Add mutual authentication support to TLS
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on PR #4724:
URL: https://github.com/apache/hbase/pull/4724#issuecomment-1253102239
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 0m 37s | Docker mode activated. |
| -0 :warning: | yetus | 0m 3s | Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck |
||| _ Prechecks _ |
||| _ master Compile Tests _ |
| +0 :ok: | mvndep | 0m 10s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 2m 6s | master passed |
| +1 :green_heart: | compile | 0m 49s | master passed |
| +1 :green_heart: | shadedjars | 4m 4s | branch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 0m 34s | master passed |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 12s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 2m 7s | the patch passed |
| +1 :green_heart: | compile | 0m 49s | the patch passed |
| +1 :green_heart: | javac | 0m 49s | the patch passed |
| +1 :green_heart: | shadedjars | 4m 7s | patch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 0m 34s | the patch passed |
||| _ Other Tests _ |
| -1 :x: | unit | 1m 9s | hbase-common in the patch failed. |
| +1 :green_heart: | unit | 201m 38s | hbase-server in the patch passed. |
| | | 220m 35s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4724/9/artifact/yetus-jdk8-hadoop3-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/4724 |
| Optional Tests | javac javadoc unit shadedjars compile |
| uname | Linux d107bc3eacfa 5.4.0-1081-aws #88~18.04.1-Ubuntu SMP Thu Jun 23 16:29:17 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / 8bc6a5954a |
| Default Java | Temurin-1.8.0_345-b01 |
| unit | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4724/9/artifact/yetus-jdk8-hadoop3-check/output/patch-unit-hbase-common.txt |
| Test Results | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4724/9/testReport/ |
| Max. process+thread count | 2444 (vs. ulimit of 30000) |
| modules | C: hbase-common hbase-server U: . |
| Console output | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4724/9/console |
| versions | git=2.17.1 maven=3.6.3 |
| Powered by | Apache Yetus 0.12.0 https://yetus.apache.org |
This message was automatically generated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #4724: HBASE-27280 Add mutual authentication support to TLS
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on PR #4724:
URL: https://github.com/apache/hbase/pull/4724#issuecomment-1254147425
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 0m 41s | Docker mode activated. |
| -0 :warning: | yetus | 0m 3s | Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck |
||| _ Prechecks _ |
||| _ master Compile Tests _ |
| +0 :ok: | mvndep | 0m 15s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 2m 44s | master passed |
| +1 :green_heart: | compile | 0m 58s | master passed |
| +1 :green_heart: | shadedjars | 3m 54s | branch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 0m 37s | master passed |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 11s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 2m 25s | the patch passed |
| +1 :green_heart: | compile | 0m 56s | the patch passed |
| +1 :green_heart: | javac | 0m 56s | the patch passed |
| +1 :green_heart: | shadedjars | 3m 52s | patch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 0m 36s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | unit | 2m 0s | hbase-common in the patch passed. |
| +1 :green_heart: | unit | 193m 20s | hbase-server in the patch passed. |
| | | 214m 57s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4724/10/artifact/yetus-jdk11-hadoop3-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/4724 |
| Optional Tests | javac javadoc unit shadedjars compile |
| uname | Linux 68ee3c5cf7ad 5.4.0-1071-aws #76~18.04.1-Ubuntu SMP Mon Mar 28 17:49:57 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / de127bde84 |
| Default Java | Eclipse Adoptium-11.0.16.1+1 |
| Test Results | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4724/10/testReport/ |
| Max. process+thread count | 2683 (vs. ulimit of 30000) |
| modules | C: hbase-common hbase-server U: . |
| Console output | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4724/10/console |
| versions | git=2.17.1 maven=3.6.3 |
| Powered by | Apache Yetus 0.12.0 https://yetus.apache.org |
This message was automatically generated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] bbeaudreault commented on a diff in pull request #4724: HBASE-27280 Add mutual authentication support to TLS
Posted by GitBox <gi...@apache.org>.
bbeaudreault commented on code in PR #4724:
URL: https://github.com/apache/hbase/pull/4724#discussion_r974474811
##########
hbase-common/src/main/java/org/apache/hadoop/hbase/io/crypto/tls/HBaseTrustManager.java:
##########
@@ -0,0 +1,160 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hbase.io.crypto.tls;
+
+import java.net.InetAddress;
+import java.net.Socket;
+import java.net.UnknownHostException;
+import java.security.cert.CertificateException;
+import java.security.cert.X509Certificate;
+import javax.net.ssl.SSLEngine;
+import javax.net.ssl.SSLException;
+import javax.net.ssl.X509ExtendedTrustManager;
+import org.apache.yetus.audience.InterfaceAudience;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * A custom TrustManager that supports hostname verification We attempt to perform verification
+ * using just the IP address first and if that fails will attempt to perform a reverse DNS lookup
+ * and verify using the hostname. This file has been copied from the Apache ZooKeeper project.
+ * @see <a href=
+ * "https://github.com/apache/zookeeper/blob/c74658d398cdc1d207aa296cb6e20de00faec03e/zookeeper-server/src/main/java/org/apache/zookeeper/common/ZKTrustManager.java">Base
+ * revision</a>
+ */
+@InterfaceAudience.Private
+public class HBaseTrustManager extends X509ExtendedTrustManager {
+
+ private static final Logger LOG = LoggerFactory.getLogger(HBaseTrustManager.class);
+
+ private final X509ExtendedTrustManager x509ExtendedTrustManager;
+ private final boolean serverHostnameVerificationEnabled;
+ private final boolean clientHostnameVerificationEnabled;
+
+ private final HBaseHostnameVerifier hostnameVerifier;
+
+ /**
+ * Instantiate a new HBaseTrustManager.
+ * @param x509ExtendedTrustManager The trustmanager to use for
+ * checkClientTrusted/checkServerTrusted logic
+ * @param serverHostnameVerificationEnabled If true, this TrustManager should verify hostnames of
+ * servers that this instance connects to.
+ * @param clientHostnameVerificationEnabled If true, the hostname of a client connecting to this
+ * machine will be verified.
+ */
+ HBaseTrustManager(X509ExtendedTrustManager x509ExtendedTrustManager,
+ boolean serverHostnameVerificationEnabled, boolean clientHostnameVerificationEnabled) {
+ this.x509ExtendedTrustManager = x509ExtendedTrustManager;
+ this.serverHostnameVerificationEnabled = serverHostnameVerificationEnabled;
+ this.clientHostnameVerificationEnabled = clientHostnameVerificationEnabled;
+ this.hostnameVerifier = new HBaseHostnameVerifier();
+ }
+
+ @Override
+ public X509Certificate[] getAcceptedIssuers() {
+ return x509ExtendedTrustManager.getAcceptedIssuers();
+ }
+
+ @Override
+ public void checkClientTrusted(X509Certificate[] chain, String authType, Socket socket)
+ throws CertificateException {
+ x509ExtendedTrustManager.checkClientTrusted(chain, authType, socket);
+ if (clientHostnameVerificationEnabled) {
+ performHostVerification(socket.getInetAddress(), chain[0]);
+ }
+ }
+
+ @Override
+ public void checkServerTrusted(X509Certificate[] chain, String authType, Socket socket)
+ throws CertificateException {
+ x509ExtendedTrustManager.checkServerTrusted(chain, authType, socket);
+ if (serverHostnameVerificationEnabled) {
+ performHostVerification(socket.getInetAddress(), chain[0]);
+ }
+ }
+
+ @Override
+ public void checkClientTrusted(X509Certificate[] chain, String authType, SSLEngine engine)
+ throws CertificateException {
+ x509ExtendedTrustManager.checkClientTrusted(chain, authType, engine);
+ if (clientHostnameVerificationEnabled) {
+ try {
+ performHostVerification(InetAddress.getByName(engine.getPeerHost()), chain[0]);
+ } catch (UnknownHostException e) {
+ throw new CertificateException("Failed to verify host", e);
+ }
+ }
+ }
+
+ @Override
+ public void checkServerTrusted(X509Certificate[] chain, String authType, SSLEngine engine)
+ throws CertificateException {
+ x509ExtendedTrustManager.checkServerTrusted(chain, authType, engine);
+ if (serverHostnameVerificationEnabled) {
+ try {
+ performHostVerification(InetAddress.getByName(engine.getPeerHost()), chain[0]);
+ } catch (UnknownHostException e) {
+ throw new CertificateException("Failed to verify host", e);
+ }
+ }
+ }
+
+ @Override
+ public void checkClientTrusted(X509Certificate[] chain, String authType)
+ throws CertificateException {
+ x509ExtendedTrustManager.checkClientTrusted(chain, authType);
+ }
+
+ @Override
+ public void checkServerTrusted(X509Certificate[] chain, String authType)
+ throws CertificateException {
+ x509ExtendedTrustManager.checkServerTrusted(chain, authType);
+ }
+
+ /**
+ * Compares peer's hostname with the one stored in the provided client certificate. Performs
+ * verification with the help of provided HostnameVerifier.
+ * @param inetAddress Peer's inet address.
+ * @param certificate Peer's certificate
+ * @throws CertificateException Thrown if the provided certificate doesn't match the peer
+ * hostname.
+ */
+ private void performHostVerification(InetAddress inetAddress, X509Certificate certificate)
+ throws CertificateException {
+ String hostAddress = "";
+ String hostName = "";
+ try {
+ hostAddress = inetAddress.getHostAddress();
+ hostnameVerifier.verify(hostAddress, certificate);
+ } catch (SSLException addressVerificationException) {
+ try {
Review Comment:
Sorry @Apache9, I'm not entirely following your question/suggestion.
Are you suggesting that I change to use inetAddress.getHostName() here which uses reverse lookup? Or are you suggesting that I swap the order of the try/catch here so we try the reverse dns below first?
Since we are in the TrustManager layer, we only have what's available in this layer. So it's possible this method will be called in one of two ways:
- Two of the callers above (one for client, one for server), they pass the `socket.getInetAddress()`. This will probably be an ip address I guess.
- The other two callers above pass the parsed `engine.getPeerHost()` which according to the javadoc is the un-verified hostname.
The internals of SSL are hard to follow; I don't know in what circumstances each caller will be used. But both should work, because we have this 2 layer check (one direct check, one reverse lookup).
I'm happy to optimize this by changing the order if that's what you're asking. Please clarify if possible.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #4724: HBASE-27280 Add mutual authentication support to TLS
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on PR #4724:
URL: https://github.com/apache/hbase/pull/4724#issuecomment-1226050782
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 6m 36s | Docker mode activated. |
||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 0s | No case conflicting files found. |
| +1 :green_heart: | hbaseanti | 0m 0s | Patch does not have any anti-patterns. |
| +1 :green_heart: | @author | 0m 0s | The patch does not contain any @author tags. |
||| _ master Compile Tests _ |
| +0 :ok: | mvndep | 0m 12s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 2m 27s | master passed |
| +1 :green_heart: | compile | 2m 48s | master passed |
| +1 :green_heart: | checkstyle | 0m 46s | master passed |
| +1 :green_heart: | spotless | 0m 41s | branch has no errors when running spotless:check. |
| +1 :green_heart: | spotbugs | 1m 47s | master passed |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 11s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 2m 12s | the patch passed |
| +1 :green_heart: | compile | 2m 46s | the patch passed |
| -0 :warning: | javac | 0m 32s | hbase-common generated 9 new + 0 unchanged - 0 fixed = 9 total (was 0) |
| -0 :warning: | javac | 2m 14s | hbase-server generated 1 new + 192 unchanged - 1 fixed = 193 total (was 193) |
| -0 :warning: | checkstyle | 0m 13s | hbase-common: The patch generated 5 new + 0 unchanged - 0 fixed = 5 total (was 0) |
| -0 :warning: | checkstyle | 0m 31s | hbase-server: The patch generated 2 new + 0 unchanged - 0 fixed = 2 total (was 0) |
| +1 :green_heart: | whitespace | 0m 0s | The patch has no whitespace issues. |
| +1 :green_heart: | hadoopcheck | 8m 3s | Patch does not cause any errors with Hadoop 3.2.4 3.3.4. |
| +1 :green_heart: | spotless | 0m 39s | patch has no errors when running spotless:check. |
| +1 :green_heart: | spotbugs | 1m 58s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | asflicense | 0m 19s | The patch does not generate ASF License warnings. |
| | | 37m 36s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4724/2/artifact/yetus-general-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/4724 |
| Optional Tests | dupname asflicense javac spotbugs hadoopcheck hbaseanti spotless checkstyle compile |
| uname | Linux c8f3da05dee7 5.4.0-124-generic #140-Ubuntu SMP Thu Aug 4 02:23:37 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / b44bfc52cc |
| Default Java | AdoptOpenJDK-1.8.0_282-b08 |
| javac | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4724/2/artifact/yetus-general-check/output/diff-compile-javac-hbase-common.txt |
| javac | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4724/2/artifact/yetus-general-check/output/diff-compile-javac-hbase-server.txt |
| checkstyle | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4724/2/artifact/yetus-general-check/output/diff-checkstyle-hbase-common.txt |
| checkstyle | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4724/2/artifact/yetus-general-check/output/diff-checkstyle-hbase-server.txt |
| Max. process+thread count | 64 (vs. ulimit of 30000) |
| modules | C: hbase-common hbase-server U: . |
| Console output | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4724/2/console |
| versions | git=2.17.1 maven=3.6.3 spotbugs=4.2.2 |
| Powered by | Apache Yetus 0.12.0 https://yetus.apache.org |
This message was automatically generated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] bbeaudreault commented on pull request #4724: HBASE-27280 Add mutual authentication support to TLS
Posted by GitBox <gi...@apache.org>.
bbeaudreault commented on PR #4724:
URL: https://github.com/apache/hbase/pull/4724#issuecomment-1226019986
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #4724: HBASE-27280 Add mutual authentication support to TLS
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on PR #4724:
URL: https://github.com/apache/hbase/pull/4724#issuecomment-1241344359
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 0m 40s | Docker mode activated. |
||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 0s | No case conflicting files found. |
| +1 :green_heart: | hbaseanti | 0m 0s | Patch does not have any anti-patterns. |
| +1 :green_heart: | @author | 0m 0s | The patch does not contain any @author tags. |
||| _ master Compile Tests _ |
| +0 :ok: | mvndep | 0m 15s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 4m 23s | master passed |
| +1 :green_heart: | compile | 2m 48s | master passed |
| +1 :green_heart: | checkstyle | 0m 40s | master passed |
| +1 :green_heart: | spotless | 0m 40s | branch has no errors when running spotless:check. |
| -1 :x: | spotbugs | 1m 14s | hbase-server in master has 1 extant spotbugs warnings. |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 10s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 2m 4s | the patch passed |
| +1 :green_heart: | compile | 2m 47s | the patch passed |
| -0 :warning: | javac | 0m 30s | hbase-common generated 10 new + 2 unchanged - 0 fixed = 12 total (was 2) |
| -0 :warning: | checkstyle | 0m 11s | hbase-common: The patch generated 5 new + 0 unchanged - 0 fixed = 5 total (was 0) |
| -0 :warning: | checkstyle | 0m 28s | hbase-server: The patch generated 2 new + 0 unchanged - 0 fixed = 2 total (was 0) |
| +1 :green_heart: | whitespace | 0m 0s | The patch has no whitespace issues. |
| +1 :green_heart: | hadoopcheck | 8m 36s | Patch does not cause any errors with Hadoop 3.2.4 3.3.4. |
| +1 :green_heart: | spotless | 0m 37s | patch has no errors when running spotless:check. |
| +1 :green_heart: | spotbugs | 0m 31s | hbase-common in the patch passed. |
| +1 :green_heart: | spotbugs | 1m 21s | hbase-server generated 0 new + 0 unchanged - 1 fixed = 0 total (was 1) |
||| _ Other Tests _ |
| +1 :green_heart: | asflicense | 0m 15s | The patch does not generate ASF License warnings. |
| | | 33m 47s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4724/4/artifact/yetus-general-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/4724 |
| Optional Tests | dupname asflicense javac spotbugs hadoopcheck hbaseanti spotless checkstyle compile |
| uname | Linux b9ce672097de 5.4.0-1081-aws #88~18.04.1-Ubuntu SMP Thu Jun 23 16:29:17 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / 793f020e13 |
| Default Java | AdoptOpenJDK-1.8.0_282-b08 |
| spotbugs | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4724/4/artifact/yetus-general-check/output/branch-spotbugs-hbase-server-warnings.html |
| javac | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4724/4/artifact/yetus-general-check/output/diff-compile-javac-hbase-common.txt |
| checkstyle | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4724/4/artifact/yetus-general-check/output/diff-checkstyle-hbase-common.txt |
| checkstyle | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4724/4/artifact/yetus-general-check/output/diff-checkstyle-hbase-server.txt |
| Max. process+thread count | 64 (vs. ulimit of 30000) |
| modules | C: hbase-common hbase-server U: . |
| Console output | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4724/4/console |
| versions | git=2.17.1 maven=3.6.3 spotbugs=4.2.2 |
| Powered by | Apache Yetus 0.12.0 https://yetus.apache.org |
This message was automatically generated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] bbeaudreault commented on a diff in pull request #4724: HBASE-27280 Add mutual authentication support to TLS
Posted by GitBox <gi...@apache.org>.
bbeaudreault commented on code in PR #4724:
URL: https://github.com/apache/hbase/pull/4724#discussion_r975397199
##########
hbase-common/src/main/java/org/apache/hadoop/hbase/io/crypto/tls/HBaseHostnameVerifier.java:
##########
@@ -0,0 +1,274 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hbase.io.crypto.tls;
+
+import java.net.InetAddress;
+import java.security.cert.Certificate;
+import java.security.cert.CertificateParsingException;
+import java.security.cert.X509Certificate;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.List;
+import java.util.Locale;
+import java.util.NoSuchElementException;
+import java.util.Objects;
+import java.util.Optional;
+import javax.naming.InvalidNameException;
+import javax.naming.NamingException;
+import javax.naming.directory.Attribute;
+import javax.naming.directory.Attributes;
+import javax.naming.ldap.LdapName;
+import javax.naming.ldap.Rdn;
+import javax.net.ssl.HostnameVerifier;
+import javax.net.ssl.SSLException;
+import javax.net.ssl.SSLPeerUnverifiedException;
+import javax.net.ssl.SSLSession;
+import javax.security.auth.x500.X500Principal;
+import org.apache.yetus.audience.InterfaceAudience;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.apache.hbase.thirdparty.com.google.common.net.InetAddresses;
+
+/**
+ * When enabled in {@link X509Util}, handles verifying that the hostname of a peer matches the
+ * certificate it presents.
+ * <p/>
+ * This file has been copied from the Apache ZooKeeper project.
+ * @see <a href=
+ * "https://github.com/apache/zookeeper/blob/5820d10d9dc58c8e12d2e25386fdf92acb360359/zookeeper-server/src/main/java/org/apache/zookeeper/common/ZKHostnameVerifier.java">Base
+ * revision</a>
+ */
+@InterfaceAudience.Private
+final class HBaseHostnameVerifier implements HostnameVerifier {
+
+ private final Logger LOG = LoggerFactory.getLogger(HBaseHostnameVerifier.class);
+
+ /**
+ * Note: copied from Apache httpclient with some minor modifications. We want host verification,
+ * but depending on the httpclient jar caused unexplained performance regressions (even when the
+ * code was not used).
+ */
+ private static final class SubjectName {
+
+ static final int DNS = 2;
+ static final int IP = 7;
+
+ private final String value;
+ private final int type;
+
+ SubjectName(final String value, final int type) {
+ if (type != DNS && type != IP) {
+ throw new IllegalArgumentException("Invalid type: " + type);
+ }
+ this.value = Objects.requireNonNull(value);
+ this.type = type;
+ }
+
+ public int getType() {
+ return type;
+ }
+
+ public String getValue() {
+ return value;
+ }
+
+ @Override
+ public String toString() {
+ return value;
+ }
+
+ }
+
+ @Override
+ public boolean verify(final String host, final SSLSession session) {
+ try {
+ final Certificate[] certs = session.getPeerCertificates();
+ final X509Certificate x509 = (X509Certificate) certs[0];
+ verify(host, x509);
+ return true;
+ } catch (final SSLException ex) {
+ LOG.debug("Unexpected exception", ex);
+ return false;
+ }
+ }
+
+ void verify(final String host, final X509Certificate cert) throws SSLException {
+ final List<SubjectName> subjectAlts = getSubjectAltNames(cert);
+ if (subjectAlts != null && !subjectAlts.isEmpty()) {
+ Optional<InetAddress> inetAddress = parseIpAddress(host);
+ if (inetAddress.isPresent()) {
+ matchIPAddress(host, inetAddress.get(), subjectAlts);
+ } else {
+ matchDNSName(host, subjectAlts);
+ }
+ } else {
+ // CN matching has been deprecated by rfc2818 and can be used
+ // as fallback only when no subjectAlts are available
+ final X500Principal subjectPrincipal = cert.getSubjectX500Principal();
+ final String cn = extractCN(subjectPrincipal.getName(X500Principal.RFC2253));
+ if (cn == null) {
+ throw new SSLException("Certificate subject for <" + host + "> doesn't contain "
+ + "a common name and does not have alternative names");
+ }
+ matchCN(host, cn);
+ }
+ }
+
+ private static void matchIPAddress(final String host, final InetAddress inetAddress,
+ final List<SubjectName> subjectAlts) throws SSLException {
+ for (final SubjectName subjectAlt : subjectAlts) {
+ if (subjectAlt.getType() == SubjectName.IP) {
+ Optional<InetAddress> parsed = parseIpAddress(subjectAlt.getValue());
+ if (parsed.filter(altAddr -> altAddr.equals(inetAddress)).isPresent()) {
+ return;
+ }
+ }
+ }
+ throw new SSLPeerUnverifiedException("Certificate for <" + host + "> doesn't match any "
+ + "of the subject alternative names: " + subjectAlts);
+ }
+
+ private static void matchDNSName(final String host, final List<SubjectName> subjectAlts)
+ throws SSLException {
+ final String normalizedHost = host.toLowerCase(Locale.ROOT);
+ for (final SubjectName subjectAlt : subjectAlts) {
+ if (subjectAlt.getType() == SubjectName.DNS) {
+ final String normalizedSubjectAlt = subjectAlt.getValue().toLowerCase(Locale.ROOT);
+ if (matchIdentityStrict(normalizedHost, normalizedSubjectAlt)) {
+ return;
+ }
+ }
+ }
+ throw new SSLPeerUnverifiedException("Certificate for <" + host + "> doesn't match any "
+ + "of the subject alternative names: " + subjectAlts);
+ }
+
+ private static void matchCN(final String host, final String cn) throws SSLException {
+ final String normalizedHost = host.toLowerCase(Locale.ROOT);
+ final String normalizedCn = cn.toLowerCase(Locale.ROOT);
+ if (!matchIdentityStrict(normalizedHost, normalizedCn)) {
+ throw new SSLPeerUnverifiedException("Certificate for <" + host + "> doesn't match "
+ + "common name of the certificate subject: " + cn);
+ }
+ }
+
+ private static boolean matchIdentity(final String host, final String identity,
+ final boolean strict) {
+ // RFC 2818, 3.1. Server Identity
+ // "...Names may contain the wildcard
+ // character * which is considered to match any single domain name
+ // component or component fragment..."
+ // Based on this statement presuming only singular wildcard is legal
+ final int asteriskIdx = identity.indexOf('*');
+ if (asteriskIdx != -1) {
+ final String prefix = identity.substring(0, asteriskIdx);
+ final String suffix = identity.substring(asteriskIdx + 1);
+ if (!prefix.isEmpty() && !host.startsWith(prefix)) {
+ return false;
+ }
+ if (!suffix.isEmpty() && !host.endsWith(suffix)) {
+ return false;
+ }
+ // Additional sanity checks on content selected by wildcard can be done here
+ if (strict) {
+ final String remainder = host.substring(prefix.length(), host.length() - suffix.length());
+ return !remainder.contains(".");
+ }
+ return true;
+ }
+ return host.equalsIgnoreCase(identity);
+ }
+
+ private static boolean matchIdentityStrict(final String host, final String identity) {
+ return matchIdentity(host, identity, true);
+ }
+
+ private static String extractCN(final String subjectPrincipal) throws SSLException {
+ if (subjectPrincipal == null) {
+ return null;
+ }
+ try {
+ final LdapName subjectDN = new LdapName(subjectPrincipal);
+ final List<Rdn> rdns = subjectDN.getRdns();
+ for (int i = rdns.size() - 1; i >= 0; i--) {
+ final Rdn rds = rdns.get(i);
+ final Attributes attributes = rds.toAttributes();
+ final Attribute cn = attributes.get("cn");
+ if (cn != null) {
+ try {
+ final Object value = cn.get();
+ if (value != null) {
+ return value.toString();
+ }
+ } catch (final NoSuchElementException ignore) {
+ // ignore exception
+ } catch (final NamingException ignore) {
+ // ignore exception
+ }
+ }
+ }
+ return null;
+ } catch (final InvalidNameException e) {
+ throw new SSLException(subjectPrincipal + " is not a valid X500 distinguished name");
+ }
+ }
+
+ private static Optional<InetAddress> parseIpAddress(String host) {
+ try {
+ host = host.trim();
+ // Uri strings only work for ipv6 and are wrapped with brackets
+ // Unfortunately InetAddresses can't handle a mixed input, so we
+ // check here and choose which parse method to use.
+ if (host.startsWith("[") && host.endsWith("]")) {
+ return Optional.of(InetAddresses.forUriString(host));
+ } else {
+ return Optional.of(InetAddresses.forString(host));
+ }
+ } catch (Exception e) {
Review Comment:
You're correct that this method will be called for both hostnames and ip addresses. So it may be common to hit the exception case if someone's deployment most often has hostnames passed around instead of ips.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #4724: HBASE-27280 Add mutual authentication support to TLS
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on PR #4724:
URL: https://github.com/apache/hbase/pull/4724#issuecomment-1252932662
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 0m 39s | Docker mode activated. |
| -0 :warning: | yetus | 0m 2s | Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck |
||| _ Prechecks _ |
||| _ master Compile Tests _ |
| +0 :ok: | mvndep | 0m 38s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 2m 15s | master passed |
| +1 :green_heart: | compile | 0m 48s | master passed |
| +1 :green_heart: | shadedjars | 4m 6s | branch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 0m 35s | master passed |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 11s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 2m 7s | the patch passed |
| +1 :green_heart: | compile | 0m 49s | the patch passed |
| +1 :green_heart: | javac | 0m 49s | the patch passed |
| +1 :green_heart: | shadedjars | 4m 5s | patch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 0m 35s | the patch passed |
||| _ Other Tests _ |
| -1 :x: | unit | 1m 11s | hbase-common in the patch failed. |
| +1 :green_heart: | unit | 202m 17s | hbase-server in the patch passed. |
| | | 221m 51s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4724/8/artifact/yetus-jdk8-hadoop3-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/4724 |
| Optional Tests | javac javadoc unit shadedjars compile |
| uname | Linux 1d7343260148 5.4.0-1081-aws #88~18.04.1-Ubuntu SMP Thu Jun 23 16:29:17 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / 8bc6a5954a |
| Default Java | Temurin-1.8.0_345-b01 |
| unit | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4724/8/artifact/yetus-jdk8-hadoop3-check/output/patch-unit-hbase-common.txt |
| Test Results | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4724/8/testReport/ |
| Max. process+thread count | 2588 (vs. ulimit of 30000) |
| modules | C: hbase-common hbase-server U: . |
| Console output | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4724/8/console |
| versions | git=2.17.1 maven=3.6.3 |
| Powered by | Apache Yetus 0.12.0 https://yetus.apache.org |
This message was automatically generated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] bbeaudreault commented on pull request #4724: HBASE-27280 Add mutual authentication support to TLS
Posted by GitBox <gi...@apache.org>.
bbeaudreault commented on PR #4724:
URL: https://github.com/apache/hbase/pull/4724#issuecomment-1252976475
Test failures are real but I ran out of time today. Will fix them up tomorrow morning
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache9 commented on a diff in pull request #4724: HBASE-27280 Add mutual authentication support to TLS
Posted by GitBox <gi...@apache.org>.
Apache9 commented on code in PR #4724:
URL: https://github.com/apache/hbase/pull/4724#discussion_r975074601
##########
hbase-common/src/main/java/org/apache/hadoop/hbase/io/crypto/tls/HBaseTrustManager.java:
##########
@@ -0,0 +1,160 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hbase.io.crypto.tls;
+
+import java.net.InetAddress;
+import java.net.Socket;
+import java.net.UnknownHostException;
+import java.security.cert.CertificateException;
+import java.security.cert.X509Certificate;
+import javax.net.ssl.SSLEngine;
+import javax.net.ssl.SSLException;
+import javax.net.ssl.X509ExtendedTrustManager;
+import org.apache.yetus.audience.InterfaceAudience;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * A custom TrustManager that supports hostname verification We attempt to perform verification
+ * using just the IP address first and if that fails will attempt to perform a reverse DNS lookup
+ * and verify using the hostname. This file has been copied from the Apache ZooKeeper project.
+ * @see <a href=
+ * "https://github.com/apache/zookeeper/blob/c74658d398cdc1d207aa296cb6e20de00faec03e/zookeeper-server/src/main/java/org/apache/zookeeper/common/ZKTrustManager.java">Base
+ * revision</a>
+ */
+@InterfaceAudience.Private
+public class HBaseTrustManager extends X509ExtendedTrustManager {
+
+ private static final Logger LOG = LoggerFactory.getLogger(HBaseTrustManager.class);
+
+ private final X509ExtendedTrustManager x509ExtendedTrustManager;
+ private final boolean serverHostnameVerificationEnabled;
+ private final boolean clientHostnameVerificationEnabled;
+
+ private final HBaseHostnameVerifier hostnameVerifier;
+
+ /**
+ * Instantiate a new HBaseTrustManager.
+ * @param x509ExtendedTrustManager The trustmanager to use for
+ * checkClientTrusted/checkServerTrusted logic
+ * @param serverHostnameVerificationEnabled If true, this TrustManager should verify hostnames of
+ * servers that this instance connects to.
+ * @param clientHostnameVerificationEnabled If true, the hostname of a client connecting to this
+ * machine will be verified.
+ */
+ HBaseTrustManager(X509ExtendedTrustManager x509ExtendedTrustManager,
+ boolean serverHostnameVerificationEnabled, boolean clientHostnameVerificationEnabled) {
+ this.x509ExtendedTrustManager = x509ExtendedTrustManager;
+ this.serverHostnameVerificationEnabled = serverHostnameVerificationEnabled;
+ this.clientHostnameVerificationEnabled = clientHostnameVerificationEnabled;
+ this.hostnameVerifier = new HBaseHostnameVerifier();
+ }
+
+ @Override
+ public X509Certificate[] getAcceptedIssuers() {
+ return x509ExtendedTrustManager.getAcceptedIssuers();
+ }
+
+ @Override
+ public void checkClientTrusted(X509Certificate[] chain, String authType, Socket socket)
+ throws CertificateException {
+ x509ExtendedTrustManager.checkClientTrusted(chain, authType, socket);
+ if (clientHostnameVerificationEnabled) {
+ performHostVerification(socket.getInetAddress(), chain[0]);
+ }
+ }
+
+ @Override
+ public void checkServerTrusted(X509Certificate[] chain, String authType, Socket socket)
+ throws CertificateException {
+ x509ExtendedTrustManager.checkServerTrusted(chain, authType, socket);
+ if (serverHostnameVerificationEnabled) {
+ performHostVerification(socket.getInetAddress(), chain[0]);
+ }
+ }
+
+ @Override
+ public void checkClientTrusted(X509Certificate[] chain, String authType, SSLEngine engine)
+ throws CertificateException {
+ x509ExtendedTrustManager.checkClientTrusted(chain, authType, engine);
+ if (clientHostnameVerificationEnabled) {
+ try {
+ performHostVerification(InetAddress.getByName(engine.getPeerHost()), chain[0]);
+ } catch (UnknownHostException e) {
+ throw new CertificateException("Failed to verify host", e);
+ }
+ }
+ }
+
+ @Override
+ public void checkServerTrusted(X509Certificate[] chain, String authType, SSLEngine engine)
+ throws CertificateException {
+ x509ExtendedTrustManager.checkServerTrusted(chain, authType, engine);
+ if (serverHostnameVerificationEnabled) {
+ try {
+ performHostVerification(InetAddress.getByName(engine.getPeerHost()), chain[0]);
+ } catch (UnknownHostException e) {
+ throw new CertificateException("Failed to verify host", e);
+ }
+ }
+ }
+
+ @Override
+ public void checkClientTrusted(X509Certificate[] chain, String authType)
+ throws CertificateException {
+ x509ExtendedTrustManager.checkClientTrusted(chain, authType);
+ }
+
+ @Override
+ public void checkServerTrusted(X509Certificate[] chain, String authType)
+ throws CertificateException {
+ x509ExtendedTrustManager.checkServerTrusted(chain, authType);
+ }
+
+ /**
+ * Compares peer's hostname with the one stored in the provided client certificate. Performs
+ * verification with the help of provided HostnameVerifier.
+ * @param inetAddress Peer's inet address.
+ * @param certificate Peer's certificate
+ * @throws CertificateException Thrown if the provided certificate doesn't match the peer
+ * hostname.
+ */
+ private void performHostVerification(InetAddress inetAddress, X509Certificate certificate)
+ throws CertificateException {
+ String hostAddress = "";
+ String hostName = "";
+ try {
+ hostAddress = inetAddress.getHostAddress();
+ hostnameVerifier.verify(hostAddress, certificate);
+ } catch (SSLException addressVerificationException) {
+ try {
Review Comment:
I mean for verifying a server, usually we should check host name first, before checking the ip, as checking ip requires a dns lookup. But since we will finally connect to the server, a dns lookup is always needed, so this should be fine.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] bbeaudreault commented on a diff in pull request #4724: HBASE-27280 Add mutual authentication support to TLS
Posted by GitBox <gi...@apache.org>.
bbeaudreault commented on code in PR #4724:
URL: https://github.com/apache/hbase/pull/4724#discussion_r975396014
##########
hbase-common/src/main/java/org/apache/hadoop/hbase/io/crypto/tls/HBaseHostnameVerifier.java:
##########
@@ -0,0 +1,274 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hbase.io.crypto.tls;
+
+import java.net.InetAddress;
+import java.security.cert.Certificate;
+import java.security.cert.CertificateParsingException;
+import java.security.cert.X509Certificate;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.List;
+import java.util.Locale;
+import java.util.NoSuchElementException;
+import java.util.Objects;
+import java.util.Optional;
+import javax.naming.InvalidNameException;
+import javax.naming.NamingException;
+import javax.naming.directory.Attribute;
+import javax.naming.directory.Attributes;
+import javax.naming.ldap.LdapName;
+import javax.naming.ldap.Rdn;
+import javax.net.ssl.HostnameVerifier;
+import javax.net.ssl.SSLException;
+import javax.net.ssl.SSLPeerUnverifiedException;
+import javax.net.ssl.SSLSession;
+import javax.security.auth.x500.X500Principal;
+import org.apache.yetus.audience.InterfaceAudience;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.apache.hbase.thirdparty.com.google.common.net.InetAddresses;
+
+/**
+ * When enabled in {@link X509Util}, handles verifying that the hostname of a peer matches the
+ * certificate it presents.
+ * <p/>
+ * This file has been copied from the Apache ZooKeeper project.
+ * @see <a href=
+ * "https://github.com/apache/zookeeper/blob/5820d10d9dc58c8e12d2e25386fdf92acb360359/zookeeper-server/src/main/java/org/apache/zookeeper/common/ZKHostnameVerifier.java">Base
+ * revision</a>
+ */
+@InterfaceAudience.Private
+final class HBaseHostnameVerifier implements HostnameVerifier {
+
+ private final Logger LOG = LoggerFactory.getLogger(HBaseHostnameVerifier.class);
+
+ /**
+ * Note: copied from Apache httpclient with some minor modifications. We want host verification,
+ * but depending on the httpclient jar caused unexplained performance regressions (even when the
+ * code was not used).
+ */
+ private static final class SubjectName {
+
+ static final int DNS = 2;
+ static final int IP = 7;
+
+ private final String value;
+ private final int type;
+
+ SubjectName(final String value, final int type) {
+ if (type != DNS && type != IP) {
+ throw new IllegalArgumentException("Invalid type: " + type);
+ }
+ this.value = Objects.requireNonNull(value);
+ this.type = type;
+ }
+
+ public int getType() {
+ return type;
+ }
+
+ public String getValue() {
+ return value;
+ }
+
+ @Override
+ public String toString() {
+ return value;
+ }
+
+ }
+
+ @Override
+ public boolean verify(final String host, final SSLSession session) {
+ try {
+ final Certificate[] certs = session.getPeerCertificates();
+ final X509Certificate x509 = (X509Certificate) certs[0];
+ verify(host, x509);
+ return true;
+ } catch (final SSLException ex) {
+ LOG.debug("Unexpected exception", ex);
+ return false;
+ }
+ }
+
+ void verify(final String host, final X509Certificate cert) throws SSLException {
+ final List<SubjectName> subjectAlts = getSubjectAltNames(cert);
+ if (subjectAlts != null && !subjectAlts.isEmpty()) {
+ Optional<InetAddress> inetAddress = parseIpAddress(host);
+ if (inetAddress.isPresent()) {
+ matchIPAddress(host, inetAddress.get(), subjectAlts);
+ } else {
+ matchDNSName(host, subjectAlts);
+ }
+ } else {
+ // CN matching has been deprecated by rfc2818 and can be used
+ // as fallback only when no subjectAlts are available
+ final X500Principal subjectPrincipal = cert.getSubjectX500Principal();
+ final String cn = extractCN(subjectPrincipal.getName(X500Principal.RFC2253));
+ if (cn == null) {
+ throw new SSLException("Certificate subject for <" + host + "> doesn't contain "
+ + "a common name and does not have alternative names");
+ }
+ matchCN(host, cn);
+ }
+ }
+
+ private static void matchIPAddress(final String host, final InetAddress inetAddress,
+ final List<SubjectName> subjectAlts) throws SSLException {
+ for (final SubjectName subjectAlt : subjectAlts) {
+ if (subjectAlt.getType() == SubjectName.IP) {
+ Optional<InetAddress> parsed = parseIpAddress(subjectAlt.getValue());
+ if (parsed.filter(altAddr -> altAddr.equals(inetAddress)).isPresent()) {
+ return;
+ }
+ }
+ }
+ throw new SSLPeerUnverifiedException("Certificate for <" + host + "> doesn't match any "
+ + "of the subject alternative names: " + subjectAlts);
+ }
+
+ private static void matchDNSName(final String host, final List<SubjectName> subjectAlts)
+ throws SSLException {
+ final String normalizedHost = host.toLowerCase(Locale.ROOT);
+ for (final SubjectName subjectAlt : subjectAlts) {
+ if (subjectAlt.getType() == SubjectName.DNS) {
+ final String normalizedSubjectAlt = subjectAlt.getValue().toLowerCase(Locale.ROOT);
+ if (matchIdentityStrict(normalizedHost, normalizedSubjectAlt)) {
+ return;
+ }
+ }
+ }
+ throw new SSLPeerUnverifiedException("Certificate for <" + host + "> doesn't match any "
+ + "of the subject alternative names: " + subjectAlts);
+ }
+
+ private static void matchCN(final String host, final String cn) throws SSLException {
+ final String normalizedHost = host.toLowerCase(Locale.ROOT);
+ final String normalizedCn = cn.toLowerCase(Locale.ROOT);
+ if (!matchIdentityStrict(normalizedHost, normalizedCn)) {
+ throw new SSLPeerUnverifiedException("Certificate for <" + host + "> doesn't match "
+ + "common name of the certificate subject: " + cn);
+ }
+ }
+
+ private static boolean matchIdentity(final String host, final String identity,
+ final boolean strict) {
+ // RFC 2818, 3.1. Server Identity
+ // "...Names may contain the wildcard
+ // character * which is considered to match any single domain name
+ // component or component fragment..."
+ // Based on this statement presuming only singular wildcard is legal
+ final int asteriskIdx = identity.indexOf('*');
+ if (asteriskIdx != -1) {
+ final String prefix = identity.substring(0, asteriskIdx);
+ final String suffix = identity.substring(asteriskIdx + 1);
+ if (!prefix.isEmpty() && !host.startsWith(prefix)) {
+ return false;
+ }
+ if (!suffix.isEmpty() && !host.endsWith(suffix)) {
+ return false;
+ }
+ // Additional sanity checks on content selected by wildcard can be done here
+ if (strict) {
+ final String remainder = host.substring(prefix.length(), host.length() - suffix.length());
+ return !remainder.contains(".");
+ }
+ return true;
+ }
+ return host.equalsIgnoreCase(identity);
+ }
+
+ private static boolean matchIdentityStrict(final String host, final String identity) {
+ return matchIdentity(host, identity, true);
+ }
+
+ private static String extractCN(final String subjectPrincipal) throws SSLException {
+ if (subjectPrincipal == null) {
+ return null;
+ }
+ try {
+ final LdapName subjectDN = new LdapName(subjectPrincipal);
+ final List<Rdn> rdns = subjectDN.getRdns();
+ for (int i = rdns.size() - 1; i >= 0; i--) {
+ final Rdn rds = rdns.get(i);
+ final Attributes attributes = rds.toAttributes();
+ final Attribute cn = attributes.get("cn");
+ if (cn != null) {
+ try {
+ final Object value = cn.get();
+ if (value != null) {
+ return value.toString();
+ }
+ } catch (final NoSuchElementException ignore) {
+ // ignore exception
+ } catch (final NamingException ignore) {
+ // ignore exception
+ }
+ }
+ }
+ return null;
+ } catch (final InvalidNameException e) {
+ throw new SSLException(subjectPrincipal + " is not a valid X500 distinguished name");
+ }
+ }
+
+ private static Optional<InetAddress> parseIpAddress(String host) {
+ try {
+ host = host.trim();
+ // Uri strings only work for ipv6 and are wrapped with brackets
+ // Unfortunately InetAddresses can't handle a mixed input, so we
+ // check here and choose which parse method to use.
+ if (host.startsWith("[") && host.endsWith("]")) {
+ return Optional.of(InetAddresses.forUriString(host));
+ } else {
+ return Optional.of(InetAddresses.forString(host));
+ }
+ } catch (Exception e) {
Review Comment:
The reason I did it this way is when I looked at the impl of `isInetAddress` and `forString`, they both do the exact same parsing. `isInetAddress` is `return ipStringToBytes(ipString) != null`, while `forString` calls the same method and throws an exception if null.
I'm not sure of the relative costs of throwing an exception vs having to parse the ip address/hostname twice. I went for trying to avoid parsing twice, but I could do the `isInetAddress` check instead.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] bbeaudreault commented on pull request #4724: HBASE-27280 Add mutual authentication support to TLS
Posted by GitBox <gi...@apache.org>.
bbeaudreault commented on PR #4724:
URL: https://github.com/apache/hbase/pull/4724#issuecomment-1250140413
@Apache9 @meszibalu any chance I could get a quick review on this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] bbeaudreault commented on a diff in pull request #4724: HBASE-27280 Add mutual authentication support to TLS
Posted by GitBox <gi...@apache.org>.
bbeaudreault commented on code in PR #4724:
URL: https://github.com/apache/hbase/pull/4724#discussion_r977603386
##########
hbase-common/src/main/java/org/apache/hadoop/hbase/io/crypto/tls/HBaseHostnameVerifier.java:
##########
@@ -0,0 +1,284 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hbase.io.crypto.tls;
+
+import java.net.InetAddress;
+import java.security.cert.Certificate;
+import java.security.cert.CertificateParsingException;
+import java.security.cert.X509Certificate;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.List;
+import java.util.Locale;
+import java.util.NoSuchElementException;
+import java.util.Objects;
+import java.util.Optional;
+import javax.naming.InvalidNameException;
+import javax.naming.NamingException;
+import javax.naming.directory.Attribute;
+import javax.naming.directory.Attributes;
+import javax.naming.ldap.LdapName;
+import javax.naming.ldap.Rdn;
+import javax.net.ssl.HostnameVerifier;
+import javax.net.ssl.SSLException;
+import javax.net.ssl.SSLPeerUnverifiedException;
+import javax.net.ssl.SSLSession;
+import javax.security.auth.x500.X500Principal;
+import org.apache.yetus.audience.InterfaceAudience;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.apache.hbase.thirdparty.com.google.common.net.InetAddresses;
+
+/**
+ * When enabled in {@link X509Util}, handles verifying that the hostname of a peer matches the
+ * certificate it presents.
+ * <p/>
+ * This file has been copied from the Apache ZooKeeper project.
+ * @see <a href=
+ * "https://github.com/apache/zookeeper/blob/5820d10d9dc58c8e12d2e25386fdf92acb360359/zookeeper-server/src/main/java/org/apache/zookeeper/common/ZKHostnameVerifier.java">Base
+ * revision</a>
+ */
+@InterfaceAudience.Private
+final class HBaseHostnameVerifier implements HostnameVerifier {
+
+ private final Logger LOG = LoggerFactory.getLogger(HBaseHostnameVerifier.class);
+
+ /**
+ * Note: copied from Apache httpclient with some minor modifications. We want host verification,
+ * but depending on the httpclient jar caused unexplained performance regressions (even when the
+ * code was not used).
+ */
+ private static final class SubjectName {
+
+ static final int DNS = 2;
+ static final int IP = 7;
+
+ private final String value;
+ private final int type;
+
+ SubjectName(final String value, final int type) {
+ if (type != DNS && type != IP) {
+ throw new IllegalArgumentException("Invalid type: " + type);
+ }
+ this.value = Objects.requireNonNull(value);
+ this.type = type;
+ }
+
+ public int getType() {
+ return type;
+ }
+
+ public String getValue() {
+ return value;
+ }
+
+ @Override
+ public String toString() {
+ return value;
+ }
+
+ }
+
+ @Override
+ public boolean verify(final String host, final SSLSession session) {
+ try {
+ final Certificate[] certs = session.getPeerCertificates();
+ final X509Certificate x509 = (X509Certificate) certs[0];
+ verify(host, x509);
+ return true;
+ } catch (final SSLException ex) {
+ LOG.debug("Unexpected exception", ex);
+ return false;
+ }
+ }
+
+ void verify(final String host, final X509Certificate cert) throws SSLException {
+ final List<SubjectName> subjectAlts = getSubjectAltNames(cert);
+ if (subjectAlts != null && !subjectAlts.isEmpty()) {
+ Optional<InetAddress> inetAddress = parseIpAddress(host);
+ if (inetAddress.isPresent()) {
+ matchIPAddress(host, inetAddress.get(), subjectAlts);
+ } else {
+ matchDNSName(host, subjectAlts);
+ }
+ } else {
+ // CN matching has been deprecated by rfc2818 and can be used
+ // as fallback only when no subjectAlts are available
+ final X500Principal subjectPrincipal = cert.getSubjectX500Principal();
+ final String cn = extractCN(subjectPrincipal.getName(X500Principal.RFC2253));
+ if (cn == null) {
+ throw new SSLException("Certificate subject for <" + host + "> doesn't contain "
+ + "a common name and does not have alternative names");
+ }
+ matchCN(host, cn);
+ }
+ }
+
+ private static void matchIPAddress(final String host, final InetAddress inetAddress,
+ final List<SubjectName> subjectAlts) throws SSLException {
+ for (final SubjectName subjectAlt : subjectAlts) {
+ if (subjectAlt.getType() == SubjectName.IP) {
+ Optional<InetAddress> parsed = parseIpAddress(subjectAlt.getValue());
+ if (parsed.filter(altAddr -> altAddr.equals(inetAddress)).isPresent()) {
+ return;
+ }
+ }
+ }
+ throw new SSLPeerUnverifiedException("Certificate for <" + host + "> doesn't match any "
+ + "of the subject alternative names: " + subjectAlts);
+ }
+
+ private static void matchDNSName(final String host, final List<SubjectName> subjectAlts)
+ throws SSLException {
+ final String normalizedHost = host.toLowerCase(Locale.ROOT);
+ for (final SubjectName subjectAlt : subjectAlts) {
+ if (subjectAlt.getType() == SubjectName.DNS) {
+ final String normalizedSubjectAlt = subjectAlt.getValue().toLowerCase(Locale.ROOT);
+ if (matchIdentityStrict(normalizedHost, normalizedSubjectAlt)) {
+ return;
+ }
+ }
+ }
+ throw new SSLPeerUnverifiedException("Certificate for <" + host + "> doesn't match any "
+ + "of the subject alternative names: " + subjectAlts);
+ }
+
+ private static void matchCN(final String host, final String cn) throws SSLException {
+ final String normalizedHost = host.toLowerCase(Locale.ROOT);
+ final String normalizedCn = cn.toLowerCase(Locale.ROOT);
+ if (!matchIdentityStrict(normalizedHost, normalizedCn)) {
+ throw new SSLPeerUnverifiedException("Certificate for <" + host + "> doesn't match "
+ + "common name of the certificate subject: " + cn);
+ }
+ }
+
+ private static boolean matchIdentity(final String host, final String identity,
+ final boolean strict) {
+ // RFC 2818, 3.1. Server Identity
+ // "...Names may contain the wildcard
+ // character * which is considered to match any single domain name
+ // component or component fragment..."
+ // Based on this statement presuming only singular wildcard is legal
+ final int asteriskIdx = identity.indexOf('*');
+ if (asteriskIdx != -1) {
+ final String prefix = identity.substring(0, asteriskIdx);
+ final String suffix = identity.substring(asteriskIdx + 1);
+ if (!prefix.isEmpty() && !host.startsWith(prefix)) {
+ return false;
+ }
+ if (!suffix.isEmpty() && !host.endsWith(suffix)) {
+ return false;
+ }
+ // Additional sanity checks on content selected by wildcard can be done here
+ if (strict) {
+ final String remainder = host.substring(prefix.length(), host.length() - suffix.length());
+ return !remainder.contains(".");
+ }
+ return true;
+ }
+ return host.equalsIgnoreCase(identity);
+ }
+
+ private static boolean matchIdentityStrict(final String host, final String identity) {
+ return matchIdentity(host, identity, true);
+ }
+
+ private static String extractCN(final String subjectPrincipal) throws SSLException {
+ if (subjectPrincipal == null) {
+ return null;
+ }
+ try {
+ final LdapName subjectDN = new LdapName(subjectPrincipal);
+ final List<Rdn> rdns = subjectDN.getRdns();
+ for (int i = rdns.size() - 1; i >= 0; i--) {
+ final Rdn rds = rdns.get(i);
+ final Attributes attributes = rds.toAttributes();
+ final Attribute cn = attributes.get("cn");
+ if (cn != null) {
+ try {
+ final Object value = cn.get();
+ if (value != null) {
+ return value.toString();
+ }
+ } catch (final NoSuchElementException ignore) {
+ // ignore exception
+ } catch (final NamingException ignore) {
+ // ignore exception
+ }
+ }
+ }
+ return null;
+ } catch (final InvalidNameException e) {
+ throw new SSLException(subjectPrincipal + " is not a valid X500 distinguished name");
+ }
+ }
+
+ private static Optional<InetAddress> parseIpAddress(String host) {
+ host = host.trim();
+ // Uri strings only work for ipv6 and are wrapped with brackets
+ // Unfortunately InetAddresses can't handle a mixed input, so we
+ // check here and choose which parse method to use.
+ if (host.startsWith("[") && host.endsWith("]")) {
+ return parseIpAddressUriString(host);
+ } else {
+ return parseIpAddressString(host);
+ }
+ }
+
+ private static Optional<InetAddress> parseIpAddressUriString(String host) {
+ if (InetAddresses.isUriInetAddress(host)) {
+ return Optional.of(InetAddresses.forUriString(host));
+ }
+ return Optional.empty();
+ }
+
+ private static Optional<InetAddress> parseIpAddressString(String host) {
+ if (InetAddresses.isInetAddress(host)) {
+ return Optional.of(InetAddresses.forString(host));
+ }
+ return Optional.empty();
+ }
+
+ @SuppressWarnings("MixedMutabilityReturnType")
+ private static List<SubjectName> getSubjectAltNames(final X509Certificate cert) {
+ try {
+ final Collection<List<?>> entries = cert.getSubjectAlternativeNames();
+ if (entries == null) {
+ return Collections.emptyList();
+ }
+ final List<SubjectName> result = new ArrayList<SubjectName>();
+ for (List<?> entry : entries) {
+ final Integer type = entry.size() >= 2 ? (Integer) entry.get(0) : null;
+ 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[]
Review 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] bbeaudreault commented on pull request #4724: HBASE-27280 Add mutual authentication support to TLS
Posted by GitBox <gi...@apache.org>.
bbeaudreault commented on PR #4724:
URL: https://github.com/apache/hbase/pull/4724#issuecomment-1239591155
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] anmolnar commented on a diff in pull request #4724: HBASE-27280 Add mutual authentication support to TLS
Posted by GitBox <gi...@apache.org>.
anmolnar commented on code in PR #4724:
URL: https://github.com/apache/hbase/pull/4724#discussion_r964585597
##########
hbase-common/src/main/java/org/apache/hadoop/hbase/io/crypto/tls/X509Util.java:
##########
@@ -80,6 +80,13 @@ public final class X509Util {
private static final String TLS_ENABLED_PROTOCOLS = CONFIG_PREFIX + "enabledProtocols";
private static final String TLS_CIPHER_SUITES = CONFIG_PREFIX + "ciphersuites";
+ public static final String TLS_CONFIG_VERIFY_CLIENT_HOSTNAMES =
+ CONFIG_PREFIX + "verify.client.hostnames";
+ public static final String TLS_CONFIG_VERIFY_SERVER_HOSTNAMES =
+ CONFIG_PREFIX + "verify.server.hostnames";
+
+ public static final String TLS_CONFIG_CLIENT_AUTH_MODE = CONFIG_PREFIX + "client.auth.mode";
Review Comment:
nit: the "CONFIG" word is not part of other property constants, so it's not consistent atm.
##########
hbase-common/src/main/java/org/apache/hadoop/hbase/io/crypto/tls/HBaseTrustManager.java:
##########
@@ -0,0 +1,160 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hbase.io.crypto.tls;
+
+import java.net.InetAddress;
+import java.net.Socket;
+import java.net.UnknownHostException;
+import java.security.cert.CertificateException;
+import java.security.cert.X509Certificate;
+import javax.net.ssl.SSLEngine;
+import javax.net.ssl.SSLException;
+import javax.net.ssl.X509ExtendedTrustManager;
+import org.apache.yetus.audience.InterfaceAudience;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * A custom TrustManager that supports hostname verification We attempt to perform verification
+ * using just the IP address first and if that fails will attempt to perform a reverse DNS lookup
+ * and verify using the hostname. This file has been copied from the Apache ZooKeeper project.
+ * @see <a href=
+ * "https://github.com/apache/zookeeper/blob/c74658d398cdc1d207aa296cb6e20de00faec03e/zookeeper-server/src/main/java/org/apache/zookeeper/common/ZKTrustManager.java">Base
+ * revision</a>
+ */
+@InterfaceAudience.Private
+public class HBaseTrustManager extends X509ExtendedTrustManager {
+
+ private static final Logger LOG = LoggerFactory.getLogger(HBaseTrustManager.class);
+
+ private final X509ExtendedTrustManager x509ExtendedTrustManager;
+ private final boolean serverHostnameVerificationEnabled;
+ private final boolean clientHostnameVerificationEnabled;
+
+ private final HBaseHostnameVerifier hostnameVerifier;
+
+ /**
+ * Instantiate a new ZKTrustManager.
Review Comment:
`HBaseTrustManager`
##########
hbase-common/src/main/java/org/apache/hadoop/hbase/io/crypto/tls/X509Util.java:
##########
@@ -80,6 +80,13 @@ public final class X509Util {
private static final String TLS_ENABLED_PROTOCOLS = CONFIG_PREFIX + "enabledProtocols";
private static final String TLS_CIPHER_SUITES = CONFIG_PREFIX + "ciphersuites";
+ public static final String TLS_CONFIG_VERIFY_CLIENT_HOSTNAMES =
+ CONFIG_PREFIX + "verify.client.hostnames";
+ public static final String TLS_CONFIG_VERIFY_SERVER_HOSTNAMES =
+ CONFIG_PREFIX + "verify.server.hostnames";
Review Comment:
nit: I think these properties would be better in singular, not plural. It's possible to add multiple subject alternative names to certificates, but the verification effectively happens against a single, resolved hostname/ip address.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] anmolnar commented on pull request #4724: HBASE-27280 Add mutual authentication support to TLS
Posted by GitBox <gi...@apache.org>.
anmolnar commented on PR #4724:
URL: https://github.com/apache/hbase/pull/4724#issuecomment-1239577484
@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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #4724: HBASE-27280 Add mutual authentication support to TLS
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on PR #4724:
URL: https://github.com/apache/hbase/pull/4724#issuecomment-1252983721
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 1m 3s | Docker mode activated. |
||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 0s | No case conflicting files found. |
| +1 :green_heart: | hbaseanti | 0m 0s | Patch does not have any anti-patterns. |
| +1 :green_heart: | @author | 0m 0s | The patch does not contain any @author tags. |
||| _ master Compile Tests _ |
| +0 :ok: | mvndep | 0m 13s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 2m 12s | master passed |
| +1 :green_heart: | compile | 2m 44s | master passed |
| +1 :green_heart: | checkstyle | 0m 46s | master passed |
| +1 :green_heart: | spotless | 0m 39s | branch has no errors when running spotless:check. |
| +1 :green_heart: | spotbugs | 1m 45s | master passed |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 11s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 2m 12s | the patch passed |
| +1 :green_heart: | compile | 2m 47s | the patch passed |
| +1 :green_heart: | javac | 2m 47s | the patch passed |
| +1 :green_heart: | checkstyle | 0m 45s | the patch passed |
| +1 :green_heart: | whitespace | 0m 0s | The patch has no whitespace issues. |
| +1 :green_heart: | hadoopcheck | 7m 55s | Patch does not cause any errors with Hadoop 3.2.4 3.3.4. |
| +1 :green_heart: | spotless | 0m 39s | patch has no errors when running spotless:check. |
| +1 :green_heart: | spotbugs | 2m 1s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | asflicense | 0m 19s | The patch does not generate ASF License warnings. |
| | | 31m 25s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4724/9/artifact/yetus-general-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/4724 |
| Optional Tests | dupname asflicense javac spotbugs hadoopcheck hbaseanti spotless checkstyle compile |
| uname | Linux 1d05aec1af96 5.4.0-124-generic #140-Ubuntu SMP Thu Aug 4 02:23:37 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / 8bc6a5954a |
| Default Java | Temurin-1.8.0_345-b01 |
| Max. process+thread count | 64 (vs. ulimit of 30000) |
| modules | C: hbase-common hbase-server U: . |
| Console output | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4724/9/console |
| versions | git=2.17.1 maven=3.6.3 spotbugs=4.7.2 |
| Powered by | Apache Yetus 0.12.0 https://yetus.apache.org |
This message was automatically generated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache9 commented on pull request #4724: HBASE-27280 Add mutual authentication support to TLS
Posted by GitBox <gi...@apache.org>.
Apache9 commented on PR #4724:
URL: https://github.com/apache/hbase/pull/4724#issuecomment-1255689489
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache9 commented on a diff in pull request #4724: HBASE-27280 Add mutual authentication support to TLS
Posted by GitBox <gi...@apache.org>.
Apache9 commented on code in PR #4724:
URL: https://github.com/apache/hbase/pull/4724#discussion_r974435240
##########
hbase-common/src/main/java/org/apache/hadoop/hbase/io/crypto/tls/HBaseTrustManager.java:
##########
@@ -0,0 +1,160 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hbase.io.crypto.tls;
+
+import java.net.InetAddress;
+import java.net.Socket;
+import java.net.UnknownHostException;
+import java.security.cert.CertificateException;
+import java.security.cert.X509Certificate;
+import javax.net.ssl.SSLEngine;
+import javax.net.ssl.SSLException;
+import javax.net.ssl.X509ExtendedTrustManager;
+import org.apache.yetus.audience.InterfaceAudience;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * A custom TrustManager that supports hostname verification We attempt to perform verification
+ * using just the IP address first and if that fails will attempt to perform a reverse DNS lookup
+ * and verify using the hostname. This file has been copied from the Apache ZooKeeper project.
+ * @see <a href=
+ * "https://github.com/apache/zookeeper/blob/c74658d398cdc1d207aa296cb6e20de00faec03e/zookeeper-server/src/main/java/org/apache/zookeeper/common/ZKTrustManager.java">Base
+ * revision</a>
+ */
+@InterfaceAudience.Private
+public class HBaseTrustManager extends X509ExtendedTrustManager {
+
+ private static final Logger LOG = LoggerFactory.getLogger(HBaseTrustManager.class);
+
+ private final X509ExtendedTrustManager x509ExtendedTrustManager;
+ private final boolean serverHostnameVerificationEnabled;
+ private final boolean clientHostnameVerificationEnabled;
+
+ private final HBaseHostnameVerifier hostnameVerifier;
+
+ /**
+ * Instantiate a new HBaseTrustManager.
+ * @param x509ExtendedTrustManager The trustmanager to use for
+ * checkClientTrusted/checkServerTrusted logic
+ * @param serverHostnameVerificationEnabled If true, this TrustManager should verify hostnames of
+ * servers that this instance connects to.
+ * @param clientHostnameVerificationEnabled If true, the hostname of a client connecting to this
+ * machine will be verified.
+ */
+ HBaseTrustManager(X509ExtendedTrustManager x509ExtendedTrustManager,
+ boolean serverHostnameVerificationEnabled, boolean clientHostnameVerificationEnabled) {
+ this.x509ExtendedTrustManager = x509ExtendedTrustManager;
+ this.serverHostnameVerificationEnabled = serverHostnameVerificationEnabled;
+ this.clientHostnameVerificationEnabled = clientHostnameVerificationEnabled;
+ this.hostnameVerifier = new HBaseHostnameVerifier();
+ }
+
+ @Override
+ public X509Certificate[] getAcceptedIssuers() {
+ return x509ExtendedTrustManager.getAcceptedIssuers();
+ }
+
+ @Override
+ public void checkClientTrusted(X509Certificate[] chain, String authType, Socket socket)
+ throws CertificateException {
+ x509ExtendedTrustManager.checkClientTrusted(chain, authType, socket);
+ if (clientHostnameVerificationEnabled) {
+ performHostVerification(socket.getInetAddress(), chain[0]);
+ }
+ }
+
+ @Override
+ public void checkServerTrusted(X509Certificate[] chain, String authType, Socket socket)
+ throws CertificateException {
+ x509ExtendedTrustManager.checkServerTrusted(chain, authType, socket);
+ if (serverHostnameVerificationEnabled) {
+ performHostVerification(socket.getInetAddress(), chain[0]);
+ }
+ }
+
+ @Override
+ public void checkClientTrusted(X509Certificate[] chain, String authType, SSLEngine engine)
+ throws CertificateException {
+ x509ExtendedTrustManager.checkClientTrusted(chain, authType, engine);
+ if (clientHostnameVerificationEnabled) {
+ try {
+ performHostVerification(InetAddress.getByName(engine.getPeerHost()), chain[0]);
+ } catch (UnknownHostException e) {
+ throw new CertificateException("Failed to verify host", e);
+ }
+ }
+ }
+
+ @Override
+ public void checkServerTrusted(X509Certificate[] chain, String authType, SSLEngine engine)
+ throws CertificateException {
+ x509ExtendedTrustManager.checkServerTrusted(chain, authType, engine);
+ if (serverHostnameVerificationEnabled) {
+ try {
+ performHostVerification(InetAddress.getByName(engine.getPeerHost()), chain[0]);
+ } catch (UnknownHostException e) {
+ throw new CertificateException("Failed to verify host", e);
+ }
+ }
+ }
+
+ @Override
+ public void checkClientTrusted(X509Certificate[] chain, String authType)
+ throws CertificateException {
+ x509ExtendedTrustManager.checkClientTrusted(chain, authType);
+ }
+
+ @Override
+ public void checkServerTrusted(X509Certificate[] chain, String authType)
+ throws CertificateException {
+ x509ExtendedTrustManager.checkServerTrusted(chain, authType);
+ }
+
+ /**
+ * Compares peer's hostname with the one stored in the provided client certificate. Performs
+ * verification with the help of provided HostnameVerifier.
+ * @param inetAddress Peer's inet address.
+ * @param certificate Peer's certificate
+ * @throws CertificateException Thrown if the provided certificate doesn't match the peer
+ * hostname.
+ */
+ private void performHostVerification(InetAddress inetAddress, X509Certificate certificate)
+ throws CertificateException {
+ String hostAddress = "";
+ String hostName = "";
+ try {
+ hostAddress = inetAddress.getHostAddress();
+ hostnameVerifier.verify(hostAddress, certificate);
+ } catch (SSLException addressVerificationException) {
+ try {
Review Comment:
For a client, usually we need to use reverse dns to get its hostname, but for a server, usually we will use a hostname when connecting, so we should just check the host name first? But probably the algo here only adds an extra verification, as we will create the InetAddress with hostname so there is no reverse lookup when calling inetAddress.getHostName.
##########
hbase-common/src/main/java/org/apache/hadoop/hbase/io/crypto/tls/HBaseHostnameVerifier.java:
##########
@@ -0,0 +1,368 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hbase.io.crypto.tls;
+
+import java.net.InetAddress;
+import java.net.UnknownHostException;
+import java.security.cert.Certificate;
+import java.security.cert.CertificateParsingException;
+import java.security.cert.X509Certificate;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.List;
+import java.util.Locale;
+import java.util.NoSuchElementException;
+import java.util.Objects;
+import java.util.regex.Pattern;
+import javax.naming.InvalidNameException;
+import javax.naming.NamingException;
+import javax.naming.directory.Attribute;
+import javax.naming.directory.Attributes;
+import javax.naming.ldap.LdapName;
+import javax.naming.ldap.Rdn;
+import javax.net.ssl.HostnameVerifier;
+import javax.net.ssl.SSLException;
+import javax.net.ssl.SSLPeerUnverifiedException;
+import javax.net.ssl.SSLSession;
+import javax.security.auth.x500.X500Principal;
+import org.apache.yetus.audience.InterfaceAudience;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * When enabled in {@link X509Util}, handles verifying that the hostname of a peer matches the
+ * certificate it presents.
+ * <p/>
+ * This file has been copied from the Apache ZooKeeper project.
+ * @see <a href=
Review Comment:
I think this file is mainly copy of the one in HttpClient?
https://github.com/apache/httpcomponents-client/blob/master/httpclient5/src/main/java/org/apache/hc/client5/http/ssl/DefaultHostnameVerifier.java
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #4724: HBASE-27280 Add mutual authentication support to TLS
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on PR #4724:
URL: https://github.com/apache/hbase/pull/4724#issuecomment-1224959936
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 0m 40s | Docker mode activated. |
| -0 :warning: | yetus | 0m 3s | Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck |
||| _ Prechecks _ |
||| _ master Compile Tests _ |
| +1 :green_heart: | mvninstall | 2m 48s | master passed |
| +1 :green_heart: | compile | 0m 14s | master passed |
| +1 :green_heart: | shadedjars | 3m 55s | branch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 0m 16s | master passed |
||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 2m 30s | the patch passed |
| +1 :green_heart: | compile | 0m 14s | the patch passed |
| +1 :green_heart: | javac | 0m 14s | the patch passed |
| +1 :green_heart: | shadedjars | 3m 54s | patch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 0m 13s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | unit | 1m 56s | hbase-common in the patch passed. |
| | | 17m 39s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4724/1/artifact/yetus-jdk11-hadoop3-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/4724 |
| Optional Tests | javac javadoc unit shadedjars compile |
| uname | Linux d58cdf908bff 5.4.0-1071-aws #76~18.04.1-Ubuntu SMP Mon Mar 28 17:49:57 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / 950ad8dd3e |
| Default Java | AdoptOpenJDK-11.0.10+9 |
| Test Results | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4724/1/testReport/ |
| Max. process+thread count | 191 (vs. ulimit of 30000) |
| modules | C: hbase-common U: hbase-common |
| Console output | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4724/1/console |
| versions | git=2.17.1 maven=3.6.3 |
| Powered by | Apache Yetus 0.12.0 https://yetus.apache.org |
This message was automatically generated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #4724: HBASE-27280 Add mutual authentication support to TLS
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on PR #4724:
URL: https://github.com/apache/hbase/pull/4724#issuecomment-1251700181
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 1m 49s | Docker mode activated. |
| -0 :warning: | yetus | 0m 3s | Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck |
||| _ Prechecks _ |
||| _ master Compile Tests _ |
| +0 :ok: | mvndep | 0m 56s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 2m 46s | master passed |
| +1 :green_heart: | compile | 0m 55s | master passed |
| +1 :green_heart: | shadedjars | 3m 55s | branch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 0m 37s | master passed |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 11s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 2m 21s | the patch passed |
| +1 :green_heart: | compile | 0m 56s | the patch passed |
| +1 :green_heart: | javac | 0m 56s | the patch passed |
| +1 :green_heart: | shadedjars | 3m 53s | patch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 0m 37s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | unit | 1m 59s | hbase-common in the patch passed. |
| +1 :green_heart: | unit | 191m 10s | hbase-server in the patch passed. |
| | | 214m 40s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4724/7/artifact/yetus-jdk11-hadoop3-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/4724 |
| Optional Tests | javac javadoc unit shadedjars compile |
| uname | Linux d494301eadad 5.4.0-1071-aws #76~18.04.1-Ubuntu SMP Mon Mar 28 17:49:57 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / 3f5c0a505a |
| Default Java | Eclipse Adoptium-11.0.16.1+1 |
| Test Results | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4724/7/testReport/ |
| Max. process+thread count | 2696 (vs. ulimit of 30000) |
| modules | C: hbase-common hbase-server U: . |
| Console output | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4724/7/console |
| versions | git=2.17.1 maven=3.6.3 |
| Powered by | Apache Yetus 0.12.0 https://yetus.apache.org |
This message was automatically generated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache9 commented on pull request #4724: HBASE-27280 Add mutual authentication support to TLS
Posted by GitBox <gi...@apache.org>.
Apache9 commented on PR #4724:
URL: https://github.com/apache/hbase/pull/4724#issuecomment-1250237722
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache9 commented on a diff in pull request #4724: HBASE-27280 Add mutual authentication support to TLS
Posted by GitBox <gi...@apache.org>.
Apache9 commented on code in PR #4724:
URL: https://github.com/apache/hbase/pull/4724#discussion_r975076484
##########
hbase-common/src/main/java/org/apache/hadoop/hbase/io/crypto/tls/HBaseTrustManager.java:
##########
@@ -0,0 +1,160 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hbase.io.crypto.tls;
+
+import java.net.InetAddress;
+import java.net.Socket;
+import java.net.UnknownHostException;
+import java.security.cert.CertificateException;
+import java.security.cert.X509Certificate;
+import javax.net.ssl.SSLEngine;
+import javax.net.ssl.SSLException;
+import javax.net.ssl.X509ExtendedTrustManager;
+import org.apache.yetus.audience.InterfaceAudience;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * A custom TrustManager that supports hostname verification We attempt to perform verification
+ * using just the IP address first and if that fails will attempt to perform a reverse DNS lookup
+ * and verify using the hostname. This file has been copied from the Apache ZooKeeper project.
+ * @see <a href=
+ * "https://github.com/apache/zookeeper/blob/c74658d398cdc1d207aa296cb6e20de00faec03e/zookeeper-server/src/main/java/org/apache/zookeeper/common/ZKTrustManager.java">Base
+ * revision</a>
+ */
+@InterfaceAudience.Private
+public class HBaseTrustManager extends X509ExtendedTrustManager {
+
+ private static final Logger LOG = LoggerFactory.getLogger(HBaseTrustManager.class);
+
+ private final X509ExtendedTrustManager x509ExtendedTrustManager;
+ private final boolean serverHostnameVerificationEnabled;
+ private final boolean clientHostnameVerificationEnabled;
+
+ private final HBaseHostnameVerifier hostnameVerifier;
+
+ /**
+ * Instantiate a new HBaseTrustManager.
+ * @param x509ExtendedTrustManager The trustmanager to use for
+ * checkClientTrusted/checkServerTrusted logic
+ * @param serverHostnameVerificationEnabled If true, this TrustManager should verify hostnames of
+ * servers that this instance connects to.
+ * @param clientHostnameVerificationEnabled If true, the hostname of a client connecting to this
+ * machine will be verified.
+ */
+ HBaseTrustManager(X509ExtendedTrustManager x509ExtendedTrustManager,
+ boolean serverHostnameVerificationEnabled, boolean clientHostnameVerificationEnabled) {
+ this.x509ExtendedTrustManager = x509ExtendedTrustManager;
+ this.serverHostnameVerificationEnabled = serverHostnameVerificationEnabled;
+ this.clientHostnameVerificationEnabled = clientHostnameVerificationEnabled;
+ this.hostnameVerifier = new HBaseHostnameVerifier();
+ }
+
+ @Override
+ public X509Certificate[] getAcceptedIssuers() {
+ return x509ExtendedTrustManager.getAcceptedIssuers();
+ }
+
+ @Override
+ public void checkClientTrusted(X509Certificate[] chain, String authType, Socket socket)
+ throws CertificateException {
+ x509ExtendedTrustManager.checkClientTrusted(chain, authType, socket);
+ if (clientHostnameVerificationEnabled) {
+ performHostVerification(socket.getInetAddress(), chain[0]);
+ }
+ }
+
+ @Override
+ public void checkServerTrusted(X509Certificate[] chain, String authType, Socket socket)
+ throws CertificateException {
+ x509ExtendedTrustManager.checkServerTrusted(chain, authType, socket);
+ if (serverHostnameVerificationEnabled) {
+ performHostVerification(socket.getInetAddress(), chain[0]);
+ }
+ }
+
+ @Override
+ public void checkClientTrusted(X509Certificate[] chain, String authType, SSLEngine engine)
+ throws CertificateException {
+ x509ExtendedTrustManager.checkClientTrusted(chain, authType, engine);
+ if (clientHostnameVerificationEnabled) {
+ try {
+ performHostVerification(InetAddress.getByName(engine.getPeerHost()), chain[0]);
+ } catch (UnknownHostException e) {
+ throw new CertificateException("Failed to verify host", e);
+ }
+ }
+ }
+
+ @Override
+ public void checkServerTrusted(X509Certificate[] chain, String authType, SSLEngine engine)
+ throws CertificateException {
+ x509ExtendedTrustManager.checkServerTrusted(chain, authType, engine);
+ if (serverHostnameVerificationEnabled) {
+ try {
+ performHostVerification(InetAddress.getByName(engine.getPeerHost()), chain[0]);
+ } catch (UnknownHostException e) {
+ throw new CertificateException("Failed to verify host", e);
+ }
+ }
+ }
+
+ @Override
+ public void checkClientTrusted(X509Certificate[] chain, String authType)
+ throws CertificateException {
+ x509ExtendedTrustManager.checkClientTrusted(chain, authType);
+ }
+
+ @Override
+ public void checkServerTrusted(X509Certificate[] chain, String authType)
+ throws CertificateException {
+ x509ExtendedTrustManager.checkServerTrusted(chain, authType);
+ }
+
+ /**
+ * Compares peer's hostname with the one stored in the provided client certificate. Performs
+ * verification with the help of provided HostnameVerifier.
+ * @param inetAddress Peer's inet address.
+ * @param certificate Peer's certificate
+ * @throws CertificateException Thrown if the provided certificate doesn't match the peer
+ * hostname.
+ */
+ private void performHostVerification(InetAddress inetAddress, X509Certificate certificate)
+ throws CertificateException {
+ String hostAddress = "";
+ String hostName = "";
+ try {
+ hostAddress = inetAddress.getHostAddress();
+ hostnameVerifier.verify(hostAddress, certificate);
+ } catch (SSLException addressVerificationException) {
+ try {
Review Comment:
But anyway, reverse dns lookup is a bit expensive under some scenario. From my experience, if there is no valid response, a dns lookup can hang the program for several seconds...
So it is possible to add a config to disable reverse dns lookup for client verification?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #4724: HBASE-27280 Add mutual authentication support to TLS
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on PR #4724:
URL: https://github.com/apache/hbase/pull/4724#issuecomment-1242212087
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 3m 33s | Docker mode activated. |
| -0 :warning: | yetus | 0m 3s | Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck |
||| _ Prechecks _ |
||| _ master Compile Tests _ |
| +0 :ok: | mvndep | 0m 39s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 2m 6s | master passed |
| +1 :green_heart: | compile | 0m 48s | master passed |
| +1 :green_heart: | shadedjars | 4m 5s | branch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 0m 35s | master passed |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 11s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 2m 8s | the patch passed |
| +1 :green_heart: | compile | 0m 51s | the patch passed |
| +1 :green_heart: | javac | 0m 51s | the patch passed |
| +1 :green_heart: | shadedjars | 4m 4s | patch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 0m 34s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | unit | 1m 41s | hbase-common in the patch passed. |
| +1 :green_heart: | unit | 200m 16s | hbase-server in the patch passed. |
| | | 223m 37s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4724/5/artifact/yetus-jdk8-hadoop3-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/4724 |
| Optional Tests | javac javadoc unit shadedjars compile |
| uname | Linux 3688f2bcde78 5.4.0-1081-aws #88~18.04.1-Ubuntu SMP Thu Jun 23 16:29:17 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / 793f020e13 |
| Default Java | AdoptOpenJDK-1.8.0_282-b08 |
| Test Results | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4724/5/testReport/ |
| Max. process+thread count | 2923 (vs. ulimit of 30000) |
| modules | C: hbase-common hbase-server U: . |
| Console output | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4724/5/console |
| versions | git=2.17.1 maven=3.6.3 |
| Powered by | Apache Yetus 0.12.0 https://yetus.apache.org |
This message was automatically generated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #4724: HBASE-27280 Add mutual authentication support to TLS
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on PR #4724:
URL: https://github.com/apache/hbase/pull/4724#issuecomment-1226318413
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 0m 52s | Docker mode activated. |
| -0 :warning: | yetus | 0m 3s | Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck |
||| _ Prechecks _ |
||| _ master Compile Tests _ |
| +0 :ok: | mvndep | 0m 33s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 2m 29s | master passed |
| +1 :green_heart: | compile | 0m 57s | master passed |
| +1 :green_heart: | shadedjars | 3m 55s | branch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 0m 36s | master passed |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 11s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 2m 25s | the patch passed |
| +1 :green_heart: | compile | 0m 56s | the patch passed |
| +1 :green_heart: | javac | 0m 56s | the patch passed |
| +1 :green_heart: | shadedjars | 3m 56s | patch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 0m 36s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | unit | 1m 53s | hbase-common in the patch passed. |
| +1 :green_heart: | unit | 195m 59s | hbase-server in the patch passed. |
| | | 216m 58s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4724/2/artifact/yetus-jdk11-hadoop3-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/4724 |
| Optional Tests | javac javadoc unit shadedjars compile |
| uname | Linux eb478b3f396e 5.4.0-1071-aws #76~18.04.1-Ubuntu SMP Mon Mar 28 17:49:57 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / b44bfc52cc |
| Default Java | AdoptOpenJDK-11.0.10+9 |
| Test Results | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4724/2/testReport/ |
| Max. process+thread count | 2667 (vs. ulimit of 30000) |
| modules | C: hbase-common hbase-server U: . |
| Console output | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4724/2/console |
| versions | git=2.17.1 maven=3.6.3 |
| Powered by | Apache Yetus 0.12.0 https://yetus.apache.org |
This message was automatically generated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache9 commented on a diff in pull request #4724: HBASE-27280 Add mutual authentication support to TLS
Posted by GitBox <gi...@apache.org>.
Apache9 commented on code in PR #4724:
URL: https://github.com/apache/hbase/pull/4724#discussion_r975352182
##########
hbase-common/src/main/java/org/apache/hadoop/hbase/io/crypto/tls/HBaseHostnameVerifier.java:
##########
@@ -0,0 +1,274 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hbase.io.crypto.tls;
+
+import java.net.InetAddress;
+import java.security.cert.Certificate;
+import java.security.cert.CertificateParsingException;
+import java.security.cert.X509Certificate;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.List;
+import java.util.Locale;
+import java.util.NoSuchElementException;
+import java.util.Objects;
+import java.util.Optional;
+import javax.naming.InvalidNameException;
+import javax.naming.NamingException;
+import javax.naming.directory.Attribute;
+import javax.naming.directory.Attributes;
+import javax.naming.ldap.LdapName;
+import javax.naming.ldap.Rdn;
+import javax.net.ssl.HostnameVerifier;
+import javax.net.ssl.SSLException;
+import javax.net.ssl.SSLPeerUnverifiedException;
+import javax.net.ssl.SSLSession;
+import javax.security.auth.x500.X500Principal;
+import org.apache.yetus.audience.InterfaceAudience;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.apache.hbase.thirdparty.com.google.common.net.InetAddresses;
+
+/**
+ * When enabled in {@link X509Util}, handles verifying that the hostname of a peer matches the
+ * certificate it presents.
+ * <p/>
+ * This file has been copied from the Apache ZooKeeper project.
+ * @see <a href=
+ * "https://github.com/apache/zookeeper/blob/5820d10d9dc58c8e12d2e25386fdf92acb360359/zookeeper-server/src/main/java/org/apache/zookeeper/common/ZKHostnameVerifier.java">Base
+ * revision</a>
+ */
+@InterfaceAudience.Private
+final class HBaseHostnameVerifier implements HostnameVerifier {
+
+ private final Logger LOG = LoggerFactory.getLogger(HBaseHostnameVerifier.class);
+
+ /**
+ * Note: copied from Apache httpclient with some minor modifications. We want host verification,
+ * but depending on the httpclient jar caused unexplained performance regressions (even when the
+ * code was not used).
+ */
+ private static final class SubjectName {
+
+ static final int DNS = 2;
+ static final int IP = 7;
+
+ private final String value;
+ private final int type;
+
+ SubjectName(final String value, final int type) {
+ if (type != DNS && type != IP) {
+ throw new IllegalArgumentException("Invalid type: " + type);
+ }
+ this.value = Objects.requireNonNull(value);
+ this.type = type;
+ }
+
+ public int getType() {
+ return type;
+ }
+
+ public String getValue() {
+ return value;
+ }
+
+ @Override
+ public String toString() {
+ return value;
+ }
+
+ }
+
+ @Override
+ public boolean verify(final String host, final SSLSession session) {
+ try {
+ final Certificate[] certs = session.getPeerCertificates();
+ final X509Certificate x509 = (X509Certificate) certs[0];
+ verify(host, x509);
+ return true;
+ } catch (final SSLException ex) {
+ LOG.debug("Unexpected exception", ex);
+ return false;
+ }
+ }
+
+ void verify(final String host, final X509Certificate cert) throws SSLException {
+ final List<SubjectName> subjectAlts = getSubjectAltNames(cert);
+ if (subjectAlts != null && !subjectAlts.isEmpty()) {
+ Optional<InetAddress> inetAddress = parseIpAddress(host);
+ if (inetAddress.isPresent()) {
+ matchIPAddress(host, inetAddress.get(), subjectAlts);
+ } else {
+ matchDNSName(host, subjectAlts);
+ }
+ } else {
+ // CN matching has been deprecated by rfc2818 and can be used
+ // as fallback only when no subjectAlts are available
+ final X500Principal subjectPrincipal = cert.getSubjectX500Principal();
+ final String cn = extractCN(subjectPrincipal.getName(X500Principal.RFC2253));
+ if (cn == null) {
+ throw new SSLException("Certificate subject for <" + host + "> doesn't contain "
+ + "a common name and does not have alternative names");
+ }
+ matchCN(host, cn);
+ }
+ }
+
+ private static void matchIPAddress(final String host, final InetAddress inetAddress,
+ final List<SubjectName> subjectAlts) throws SSLException {
+ for (final SubjectName subjectAlt : subjectAlts) {
+ if (subjectAlt.getType() == SubjectName.IP) {
+ Optional<InetAddress> parsed = parseIpAddress(subjectAlt.getValue());
+ if (parsed.filter(altAddr -> altAddr.equals(inetAddress)).isPresent()) {
+ return;
+ }
+ }
+ }
+ throw new SSLPeerUnverifiedException("Certificate for <" + host + "> doesn't match any "
+ + "of the subject alternative names: " + subjectAlts);
+ }
+
+ private static void matchDNSName(final String host, final List<SubjectName> subjectAlts)
+ throws SSLException {
+ final String normalizedHost = host.toLowerCase(Locale.ROOT);
+ for (final SubjectName subjectAlt : subjectAlts) {
+ if (subjectAlt.getType() == SubjectName.DNS) {
+ final String normalizedSubjectAlt = subjectAlt.getValue().toLowerCase(Locale.ROOT);
+ if (matchIdentityStrict(normalizedHost, normalizedSubjectAlt)) {
+ return;
+ }
+ }
+ }
+ throw new SSLPeerUnverifiedException("Certificate for <" + host + "> doesn't match any "
+ + "of the subject alternative names: " + subjectAlts);
+ }
+
+ private static void matchCN(final String host, final String cn) throws SSLException {
+ final String normalizedHost = host.toLowerCase(Locale.ROOT);
+ final String normalizedCn = cn.toLowerCase(Locale.ROOT);
+ if (!matchIdentityStrict(normalizedHost, normalizedCn)) {
+ throw new SSLPeerUnverifiedException("Certificate for <" + host + "> doesn't match "
+ + "common name of the certificate subject: " + cn);
+ }
+ }
+
+ private static boolean matchIdentity(final String host, final String identity,
+ final boolean strict) {
+ // RFC 2818, 3.1. Server Identity
+ // "...Names may contain the wildcard
+ // character * which is considered to match any single domain name
+ // component or component fragment..."
+ // Based on this statement presuming only singular wildcard is legal
+ final int asteriskIdx = identity.indexOf('*');
+ if (asteriskIdx != -1) {
+ final String prefix = identity.substring(0, asteriskIdx);
+ final String suffix = identity.substring(asteriskIdx + 1);
+ if (!prefix.isEmpty() && !host.startsWith(prefix)) {
+ return false;
+ }
+ if (!suffix.isEmpty() && !host.endsWith(suffix)) {
+ return false;
+ }
+ // Additional sanity checks on content selected by wildcard can be done here
+ if (strict) {
+ final String remainder = host.substring(prefix.length(), host.length() - suffix.length());
+ return !remainder.contains(".");
+ }
+ return true;
+ }
+ return host.equalsIgnoreCase(identity);
+ }
+
+ private static boolean matchIdentityStrict(final String host, final String identity) {
+ return matchIdentity(host, identity, true);
+ }
+
+ private static String extractCN(final String subjectPrincipal) throws SSLException {
+ if (subjectPrincipal == null) {
+ return null;
+ }
+ try {
+ final LdapName subjectDN = new LdapName(subjectPrincipal);
+ final List<Rdn> rdns = subjectDN.getRdns();
+ for (int i = rdns.size() - 1; i >= 0; i--) {
+ final Rdn rds = rdns.get(i);
+ final Attributes attributes = rds.toAttributes();
+ final Attribute cn = attributes.get("cn");
+ if (cn != null) {
+ try {
+ final Object value = cn.get();
+ if (value != null) {
+ return value.toString();
+ }
+ } catch (final NoSuchElementException ignore) {
+ // ignore exception
+ } catch (final NamingException ignore) {
+ // ignore exception
+ }
+ }
+ }
+ return null;
+ } catch (final InvalidNameException e) {
+ throw new SSLException(subjectPrincipal + " is not a valid X500 distinguished name");
+ }
+ }
+
+ private static Optional<InetAddress> parseIpAddress(String host) {
+ try {
+ host = host.trim();
+ // Uri strings only work for ipv6 and are wrapped with brackets
+ // Unfortunately InetAddresses can't handle a mixed input, so we
+ // check here and choose which parse method to use.
+ if (host.startsWith("[") && host.endsWith("]")) {
+ return Optional.of(InetAddresses.forUriString(host));
+ } else {
+ return Optional.of(InetAddresses.forString(host));
+ }
+ } catch (Exception e) {
Review Comment:
Is this a normal behavior? I mean, by design, here we could get an invalid ip address? I think we could also pass in a host name here? Then I suppose we should use something like InetAddresses.isInetAddress to test whether this is a valid op address first. Throwing exception is a bit expensive...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] bbeaudreault commented on a diff in pull request #4724: HBASE-27280 Add mutual authentication support to TLS
Posted by GitBox <gi...@apache.org>.
bbeaudreault commented on code in PR #4724:
URL: https://github.com/apache/hbase/pull/4724#discussion_r975485838
##########
hbase-common/src/main/java/org/apache/hadoop/hbase/io/crypto/tls/HBaseHostnameVerifier.java:
##########
@@ -0,0 +1,274 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hbase.io.crypto.tls;
+
+import java.net.InetAddress;
+import java.security.cert.Certificate;
+import java.security.cert.CertificateParsingException;
+import java.security.cert.X509Certificate;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.List;
+import java.util.Locale;
+import java.util.NoSuchElementException;
+import java.util.Objects;
+import java.util.Optional;
+import javax.naming.InvalidNameException;
+import javax.naming.NamingException;
+import javax.naming.directory.Attribute;
+import javax.naming.directory.Attributes;
+import javax.naming.ldap.LdapName;
+import javax.naming.ldap.Rdn;
+import javax.net.ssl.HostnameVerifier;
+import javax.net.ssl.SSLException;
+import javax.net.ssl.SSLPeerUnverifiedException;
+import javax.net.ssl.SSLSession;
+import javax.security.auth.x500.X500Principal;
+import org.apache.yetus.audience.InterfaceAudience;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.apache.hbase.thirdparty.com.google.common.net.InetAddresses;
+
+/**
+ * When enabled in {@link X509Util}, handles verifying that the hostname of a peer matches the
+ * certificate it presents.
+ * <p/>
+ * This file has been copied from the Apache ZooKeeper project.
+ * @see <a href=
+ * "https://github.com/apache/zookeeper/blob/5820d10d9dc58c8e12d2e25386fdf92acb360359/zookeeper-server/src/main/java/org/apache/zookeeper/common/ZKHostnameVerifier.java">Base
+ * revision</a>
+ */
+@InterfaceAudience.Private
+final class HBaseHostnameVerifier implements HostnameVerifier {
+
+ private final Logger LOG = LoggerFactory.getLogger(HBaseHostnameVerifier.class);
+
+ /**
+ * Note: copied from Apache httpclient with some minor modifications. We want host verification,
+ * but depending on the httpclient jar caused unexplained performance regressions (even when the
+ * code was not used).
+ */
+ private static final class SubjectName {
+
+ static final int DNS = 2;
+ static final int IP = 7;
+
+ private final String value;
+ private final int type;
+
+ SubjectName(final String value, final int type) {
+ if (type != DNS && type != IP) {
+ throw new IllegalArgumentException("Invalid type: " + type);
+ }
+ this.value = Objects.requireNonNull(value);
+ this.type = type;
+ }
+
+ public int getType() {
+ return type;
+ }
+
+ public String getValue() {
+ return value;
+ }
+
+ @Override
+ public String toString() {
+ return value;
+ }
+
+ }
+
+ @Override
+ public boolean verify(final String host, final SSLSession session) {
+ try {
+ final Certificate[] certs = session.getPeerCertificates();
+ final X509Certificate x509 = (X509Certificate) certs[0];
+ verify(host, x509);
+ return true;
+ } catch (final SSLException ex) {
+ LOG.debug("Unexpected exception", ex);
+ return false;
+ }
+ }
+
+ void verify(final String host, final X509Certificate cert) throws SSLException {
+ final List<SubjectName> subjectAlts = getSubjectAltNames(cert);
+ if (subjectAlts != null && !subjectAlts.isEmpty()) {
+ Optional<InetAddress> inetAddress = parseIpAddress(host);
+ if (inetAddress.isPresent()) {
+ matchIPAddress(host, inetAddress.get(), subjectAlts);
+ } else {
+ matchDNSName(host, subjectAlts);
+ }
+ } else {
+ // CN matching has been deprecated by rfc2818 and can be used
+ // as fallback only when no subjectAlts are available
+ final X500Principal subjectPrincipal = cert.getSubjectX500Principal();
+ final String cn = extractCN(subjectPrincipal.getName(X500Principal.RFC2253));
+ if (cn == null) {
+ throw new SSLException("Certificate subject for <" + host + "> doesn't contain "
+ + "a common name and does not have alternative names");
+ }
+ matchCN(host, cn);
+ }
+ }
+
+ private static void matchIPAddress(final String host, final InetAddress inetAddress,
+ final List<SubjectName> subjectAlts) throws SSLException {
+ for (final SubjectName subjectAlt : subjectAlts) {
+ if (subjectAlt.getType() == SubjectName.IP) {
+ Optional<InetAddress> parsed = parseIpAddress(subjectAlt.getValue());
+ if (parsed.filter(altAddr -> altAddr.equals(inetAddress)).isPresent()) {
+ return;
+ }
+ }
+ }
+ throw new SSLPeerUnverifiedException("Certificate for <" + host + "> doesn't match any "
+ + "of the subject alternative names: " + subjectAlts);
+ }
+
+ private static void matchDNSName(final String host, final List<SubjectName> subjectAlts)
+ throws SSLException {
+ final String normalizedHost = host.toLowerCase(Locale.ROOT);
+ for (final SubjectName subjectAlt : subjectAlts) {
+ if (subjectAlt.getType() == SubjectName.DNS) {
+ final String normalizedSubjectAlt = subjectAlt.getValue().toLowerCase(Locale.ROOT);
+ if (matchIdentityStrict(normalizedHost, normalizedSubjectAlt)) {
+ return;
+ }
+ }
+ }
+ throw new SSLPeerUnverifiedException("Certificate for <" + host + "> doesn't match any "
+ + "of the subject alternative names: " + subjectAlts);
+ }
+
+ private static void matchCN(final String host, final String cn) throws SSLException {
+ final String normalizedHost = host.toLowerCase(Locale.ROOT);
+ final String normalizedCn = cn.toLowerCase(Locale.ROOT);
+ if (!matchIdentityStrict(normalizedHost, normalizedCn)) {
+ throw new SSLPeerUnverifiedException("Certificate for <" + host + "> doesn't match "
+ + "common name of the certificate subject: " + cn);
+ }
+ }
+
+ private static boolean matchIdentity(final String host, final String identity,
+ final boolean strict) {
+ // RFC 2818, 3.1. Server Identity
+ // "...Names may contain the wildcard
+ // character * which is considered to match any single domain name
+ // component or component fragment..."
+ // Based on this statement presuming only singular wildcard is legal
+ final int asteriskIdx = identity.indexOf('*');
+ if (asteriskIdx != -1) {
+ final String prefix = identity.substring(0, asteriskIdx);
+ final String suffix = identity.substring(asteriskIdx + 1);
+ if (!prefix.isEmpty() && !host.startsWith(prefix)) {
+ return false;
+ }
+ if (!suffix.isEmpty() && !host.endsWith(suffix)) {
+ return false;
+ }
+ // Additional sanity checks on content selected by wildcard can be done here
+ if (strict) {
+ final String remainder = host.substring(prefix.length(), host.length() - suffix.length());
+ return !remainder.contains(".");
+ }
+ return true;
+ }
+ return host.equalsIgnoreCase(identity);
+ }
+
+ private static boolean matchIdentityStrict(final String host, final String identity) {
+ return matchIdentity(host, identity, true);
+ }
+
+ private static String extractCN(final String subjectPrincipal) throws SSLException {
+ if (subjectPrincipal == null) {
+ return null;
+ }
+ try {
+ final LdapName subjectDN = new LdapName(subjectPrincipal);
+ final List<Rdn> rdns = subjectDN.getRdns();
+ for (int i = rdns.size() - 1; i >= 0; i--) {
+ final Rdn rds = rdns.get(i);
+ final Attributes attributes = rds.toAttributes();
+ final Attribute cn = attributes.get("cn");
+ if (cn != null) {
+ try {
+ final Object value = cn.get();
+ if (value != null) {
+ return value.toString();
+ }
+ } catch (final NoSuchElementException ignore) {
+ // ignore exception
+ } catch (final NamingException ignore) {
+ // ignore exception
+ }
+ }
+ }
+ return null;
+ } catch (final InvalidNameException e) {
+ throw new SSLException(subjectPrincipal + " is not a valid X500 distinguished name");
+ }
+ }
+
+ private static Optional<InetAddress> parseIpAddress(String host) {
+ try {
+ host = host.trim();
+ // Uri strings only work for ipv6 and are wrapped with brackets
+ // Unfortunately InetAddresses can't handle a mixed input, so we
+ // check here and choose which parse method to use.
+ if (host.startsWith("[") && host.endsWith("]")) {
+ return Optional.of(InetAddresses.forUriString(host));
+ } else {
+ return Optional.of(InetAddresses.forString(host));
+ }
+ } catch (Exception e) {
Review Comment:
Thanks! I'm making that change now as well as the requested config to disable reverse dns fallback
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #4724: HBASE-27280 Add mutual authentication support to TLS
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on PR #4724:
URL: https://github.com/apache/hbase/pull/4724#issuecomment-1254189367
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 0m 45s | Docker mode activated. |
| -0 :warning: | yetus | 0m 3s | Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck |
||| _ Prechecks _ |
||| _ master Compile Tests _ |
| +0 :ok: | mvndep | 0m 44s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 2m 19s | master passed |
| +1 :green_heart: | compile | 0m 48s | master passed |
| +1 :green_heart: | shadedjars | 4m 6s | branch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 0m 37s | master passed |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 12s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 2m 10s | the patch passed |
| +1 :green_heart: | compile | 0m 49s | the patch passed |
| +1 :green_heart: | javac | 0m 49s | the patch passed |
| +1 :green_heart: | shadedjars | 4m 5s | patch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 0m 35s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | unit | 1m 41s | hbase-common in the patch passed. |
| -1 :x: | unit | 234m 24s | hbase-server in the patch failed. |
| | | 256m 47s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4724/10/artifact/yetus-jdk8-hadoop3-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/4724 |
| Optional Tests | javac javadoc unit shadedjars compile |
| uname | Linux 8764b0c3b5a3 5.4.0-1081-aws #88~18.04.1-Ubuntu SMP Thu Jun 23 16:29:17 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / de127bde84 |
| Default Java | Temurin-1.8.0_345-b01 |
| unit | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4724/10/artifact/yetus-jdk8-hadoop3-check/output/patch-unit-hbase-server.txt |
| Test Results | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4724/10/testReport/ |
| Max. process+thread count | 2627 (vs. ulimit of 30000) |
| modules | C: hbase-common hbase-server U: . |
| Console output | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4724/10/console |
| versions | git=2.17.1 maven=3.6.3 |
| Powered by | Apache Yetus 0.12.0 https://yetus.apache.org |
This message was automatically generated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache9 commented on a diff in pull request #4724: HBASE-27280 Add mutual authentication support to TLS
Posted by GitBox <gi...@apache.org>.
Apache9 commented on code in PR #4724:
URL: https://github.com/apache/hbase/pull/4724#discussion_r977130153
##########
hbase-common/src/main/java/org/apache/hadoop/hbase/io/crypto/tls/HBaseTrustManager.java:
##########
@@ -0,0 +1,167 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hbase.io.crypto.tls;
+
+import java.net.InetAddress;
+import java.net.Socket;
+import java.net.UnknownHostException;
+import java.security.cert.CertificateException;
+import java.security.cert.X509Certificate;
+import javax.net.ssl.SSLEngine;
+import javax.net.ssl.SSLException;
+import javax.net.ssl.X509ExtendedTrustManager;
+import org.apache.yetus.audience.InterfaceAudience;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * A custom TrustManager that supports hostname verification We attempt to perform verification
+ * using just the IP address first and if that fails will attempt to perform a reverse DNS lookup
+ * and verify using the hostname. This file has been copied from the Apache ZooKeeper project.
+ * @see <a href=
+ * "https://github.com/apache/zookeeper/blob/c74658d398cdc1d207aa296cb6e20de00faec03e/zookeeper-server/src/main/java/org/apache/zookeeper/common/ZKTrustManager.java">Base
+ * revision</a>
+ */
+@InterfaceAudience.Private
+public class HBaseTrustManager extends X509ExtendedTrustManager {
+
+ private static final Logger LOG = LoggerFactory.getLogger(HBaseTrustManager.class);
+
+ private final X509ExtendedTrustManager x509ExtendedTrustManager;
+ private final boolean hostnameVerificationEnabled;
+ private final boolean allowReverseDnsLookup;
+
+ private final HBaseHostnameVerifier hostnameVerifier;
+
+ /**
+ * Instantiate a new HBaseTrustManager.
+ * @param x509ExtendedTrustManager The trustmanager to use for
+ * checkClientTrusted/checkServerTrusted logic
+ * @param hostnameVerificationEnabled If true, this TrustManager should verify hostnames of peers
+ * when checking trust.
+ * @param allowReverseDnsLookup If true, we will fall back on reverse dns if resolving of
+ * host fails
+ */
+ HBaseTrustManager(X509ExtendedTrustManager x509ExtendedTrustManager,
+ boolean hostnameVerificationEnabled, boolean allowReverseDnsLookup) {
+ this.x509ExtendedTrustManager = x509ExtendedTrustManager;
+ this.hostnameVerificationEnabled = hostnameVerificationEnabled;
+ this.allowReverseDnsLookup = allowReverseDnsLookup;
+ this.hostnameVerifier = new HBaseHostnameVerifier();
+ }
+
+ @Override
+ public X509Certificate[] getAcceptedIssuers() {
+ return x509ExtendedTrustManager.getAcceptedIssuers();
+ }
+
+ @Override
+ public void checkClientTrusted(X509Certificate[] chain, String authType, Socket socket)
+ throws CertificateException {
+ x509ExtendedTrustManager.checkClientTrusted(chain, authType, socket);
+ if (hostnameVerificationEnabled) {
+ performHostVerification(socket.getInetAddress(), chain[0]);
+ }
+ }
+
+ @Override
+ public void checkServerTrusted(X509Certificate[] chain, String authType, Socket socket)
+ throws CertificateException {
+ x509ExtendedTrustManager.checkServerTrusted(chain, authType, socket);
+ if (hostnameVerificationEnabled) {
+ performHostVerification(socket.getInetAddress(), chain[0]);
+ }
+ }
+
+ @Override
+ public void checkClientTrusted(X509Certificate[] chain, String authType, SSLEngine engine)
+ throws CertificateException {
+ x509ExtendedTrustManager.checkClientTrusted(chain, authType, engine);
+ if (hostnameVerificationEnabled) {
+ try {
+ performHostVerification(InetAddress.getByName(engine.getPeerHost()), chain[0]);
+ } catch (UnknownHostException e) {
+ throw new CertificateException("Failed to verify host", e);
+ }
+ }
+ }
+
+ @Override
+ public void checkServerTrusted(X509Certificate[] chain, String authType, SSLEngine engine)
+ throws CertificateException {
+ x509ExtendedTrustManager.checkServerTrusted(chain, authType, engine);
+ if (hostnameVerificationEnabled) {
+ try {
+ performHostVerification(InetAddress.getByName(engine.getPeerHost()), chain[0]);
+ } catch (UnknownHostException e) {
+ throw new CertificateException("Failed to verify host", e);
+ }
+ }
+ }
+
+ @Override
+ public void checkClientTrusted(X509Certificate[] chain, String authType)
+ throws CertificateException {
+ x509ExtendedTrustManager.checkClientTrusted(chain, authType);
+ }
+
+ @Override
+ public void checkServerTrusted(X509Certificate[] chain, String authType)
+ throws CertificateException {
+ x509ExtendedTrustManager.checkServerTrusted(chain, authType);
+ }
+
+ /**
+ * Compares peer's hostname with the one stored in the provided client certificate. Performs
+ * verification with the help of provided HostnameVerifier.
+ * @param inetAddress Peer's inet address.
+ * @param certificate Peer's certificate
+ * @throws CertificateException Thrown if the provided certificate doesn't match the peer
+ * hostname.
+ */
+ private void performHostVerification(InetAddress inetAddress, X509Certificate certificate)
+ throws CertificateException {
+ String hostAddress = "";
+ String hostName = "";
+ try {
+ hostAddress = inetAddress.getHostAddress();
+ hostnameVerifier.verify(hostAddress, certificate);
+ } catch (SSLException addressVerificationException) {
+ if (!allowReverseDnsLookup) {
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #4724: HBASE-27280 Add mutual authentication support to TLS
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on PR #4724:
URL: https://github.com/apache/hbase/pull/4724#issuecomment-1253106511
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 0m 59s | Docker mode activated. |
| -0 :warning: | yetus | 0m 2s | Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck |
||| _ Prechecks _ |
||| _ master Compile Tests _ |
| +0 :ok: | mvndep | 0m 14s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 3m 14s | master passed |
| +1 :green_heart: | compile | 1m 12s | master passed |
| +1 :green_heart: | shadedjars | 4m 28s | branch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 0m 48s | master passed |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 11s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 3m 1s | the patch passed |
| +1 :green_heart: | compile | 1m 10s | the patch passed |
| +1 :green_heart: | javac | 1m 10s | the patch passed |
| +1 :green_heart: | shadedjars | 4m 35s | patch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 0m 48s | the patch passed |
||| _ Other Tests _ |
| -1 :x: | unit | 1m 45s | hbase-common in the patch failed. |
| +1 :green_heart: | unit | 203m 20s | hbase-server in the patch passed. |
| | | 228m 0s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4724/9/artifact/yetus-jdk11-hadoop3-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/4724 |
| Optional Tests | javac javadoc unit shadedjars compile |
| uname | Linux c32472dcf4ef 5.4.0-1081-aws #88~18.04.1-Ubuntu SMP Thu Jun 23 16:29:17 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / 8bc6a5954a |
| Default Java | Eclipse Adoptium-11.0.16.1+1 |
| unit | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4724/9/artifact/yetus-jdk11-hadoop3-check/output/patch-unit-hbase-common.txt |
| Test Results | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4724/9/testReport/ |
| Max. process+thread count | 2683 (vs. ulimit of 30000) |
| modules | C: hbase-common hbase-server U: . |
| Console output | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4724/9/console |
| versions | git=2.17.1 maven=3.6.3 |
| Powered by | Apache Yetus 0.12.0 https://yetus.apache.org |
This message was automatically generated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache9 commented on a diff in pull request #4724: HBASE-27280 Add mutual authentication support to TLS
Posted by GitBox <gi...@apache.org>.
Apache9 commented on code in PR #4724:
URL: https://github.com/apache/hbase/pull/4724#discussion_r977124782
##########
hbase-common/src/main/java/org/apache/hadoop/hbase/io/crypto/tls/HBaseHostnameVerifier.java:
##########
@@ -0,0 +1,284 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hbase.io.crypto.tls;
+
+import java.net.InetAddress;
+import java.security.cert.Certificate;
+import java.security.cert.CertificateParsingException;
+import java.security.cert.X509Certificate;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.List;
+import java.util.Locale;
+import java.util.NoSuchElementException;
+import java.util.Objects;
+import java.util.Optional;
+import javax.naming.InvalidNameException;
+import javax.naming.NamingException;
+import javax.naming.directory.Attribute;
+import javax.naming.directory.Attributes;
+import javax.naming.ldap.LdapName;
+import javax.naming.ldap.Rdn;
+import javax.net.ssl.HostnameVerifier;
+import javax.net.ssl.SSLException;
+import javax.net.ssl.SSLPeerUnverifiedException;
+import javax.net.ssl.SSLSession;
+import javax.security.auth.x500.X500Principal;
+import org.apache.yetus.audience.InterfaceAudience;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.apache.hbase.thirdparty.com.google.common.net.InetAddresses;
+
+/**
+ * When enabled in {@link X509Util}, handles verifying that the hostname of a peer matches the
+ * certificate it presents.
+ * <p/>
+ * This file has been copied from the Apache ZooKeeper project.
+ * @see <a href=
+ * "https://github.com/apache/zookeeper/blob/5820d10d9dc58c8e12d2e25386fdf92acb360359/zookeeper-server/src/main/java/org/apache/zookeeper/common/ZKHostnameVerifier.java">Base
+ * revision</a>
+ */
+@InterfaceAudience.Private
+final class HBaseHostnameVerifier implements HostnameVerifier {
+
+ private final Logger LOG = LoggerFactory.getLogger(HBaseHostnameVerifier.class);
+
+ /**
+ * Note: copied from Apache httpclient with some minor modifications. We want host verification,
+ * but depending on the httpclient jar caused unexplained performance regressions (even when the
+ * code was not used).
+ */
+ private static final class SubjectName {
+
+ static final int DNS = 2;
+ static final int IP = 7;
+
+ private final String value;
+ private final int type;
+
+ SubjectName(final String value, final int type) {
+ if (type != DNS && type != IP) {
+ throw new IllegalArgumentException("Invalid type: " + type);
+ }
+ this.value = Objects.requireNonNull(value);
+ this.type = type;
+ }
+
+ public int getType() {
+ return type;
+ }
+
+ public String getValue() {
+ return value;
+ }
+
+ @Override
+ public String toString() {
+ return value;
+ }
+
+ }
+
+ @Override
+ public boolean verify(final String host, final SSLSession session) {
+ try {
+ final Certificate[] certs = session.getPeerCertificates();
+ final X509Certificate x509 = (X509Certificate) certs[0];
+ verify(host, x509);
+ return true;
+ } catch (final SSLException ex) {
+ LOG.debug("Unexpected exception", ex);
+ return false;
+ }
+ }
+
+ void verify(final String host, final X509Certificate cert) throws SSLException {
+ final List<SubjectName> subjectAlts = getSubjectAltNames(cert);
+ if (subjectAlts != null && !subjectAlts.isEmpty()) {
+ Optional<InetAddress> inetAddress = parseIpAddress(host);
+ if (inetAddress.isPresent()) {
+ matchIPAddress(host, inetAddress.get(), subjectAlts);
+ } else {
+ matchDNSName(host, subjectAlts);
+ }
+ } else {
+ // CN matching has been deprecated by rfc2818 and can be used
+ // as fallback only when no subjectAlts are available
+ final X500Principal subjectPrincipal = cert.getSubjectX500Principal();
+ final String cn = extractCN(subjectPrincipal.getName(X500Principal.RFC2253));
+ if (cn == null) {
+ throw new SSLException("Certificate subject for <" + host + "> doesn't contain "
+ + "a common name and does not have alternative names");
+ }
+ matchCN(host, cn);
+ }
+ }
+
+ private static void matchIPAddress(final String host, final InetAddress inetAddress,
+ final List<SubjectName> subjectAlts) throws SSLException {
+ for (final SubjectName subjectAlt : subjectAlts) {
+ if (subjectAlt.getType() == SubjectName.IP) {
+ Optional<InetAddress> parsed = parseIpAddress(subjectAlt.getValue());
+ if (parsed.filter(altAddr -> altAddr.equals(inetAddress)).isPresent()) {
+ return;
+ }
+ }
+ }
+ throw new SSLPeerUnverifiedException("Certificate for <" + host + "> doesn't match any "
+ + "of the subject alternative names: " + subjectAlts);
+ }
+
+ private static void matchDNSName(final String host, final List<SubjectName> subjectAlts)
+ throws SSLException {
+ final String normalizedHost = host.toLowerCase(Locale.ROOT);
+ for (final SubjectName subjectAlt : subjectAlts) {
+ if (subjectAlt.getType() == SubjectName.DNS) {
+ final String normalizedSubjectAlt = subjectAlt.getValue().toLowerCase(Locale.ROOT);
+ if (matchIdentityStrict(normalizedHost, normalizedSubjectAlt)) {
+ return;
+ }
+ }
+ }
+ throw new SSLPeerUnverifiedException("Certificate for <" + host + "> doesn't match any "
+ + "of the subject alternative names: " + subjectAlts);
+ }
+
+ private static void matchCN(final String host, final String cn) throws SSLException {
+ final String normalizedHost = host.toLowerCase(Locale.ROOT);
+ final String normalizedCn = cn.toLowerCase(Locale.ROOT);
+ if (!matchIdentityStrict(normalizedHost, normalizedCn)) {
+ throw new SSLPeerUnverifiedException("Certificate for <" + host + "> doesn't match "
+ + "common name of the certificate subject: " + cn);
+ }
+ }
+
+ private static boolean matchIdentity(final String host, final String identity,
+ final boolean strict) {
+ // RFC 2818, 3.1. Server Identity
+ // "...Names may contain the wildcard
+ // character * which is considered to match any single domain name
+ // component or component fragment..."
+ // Based on this statement presuming only singular wildcard is legal
+ final int asteriskIdx = identity.indexOf('*');
+ if (asteriskIdx != -1) {
+ final String prefix = identity.substring(0, asteriskIdx);
+ final String suffix = identity.substring(asteriskIdx + 1);
+ if (!prefix.isEmpty() && !host.startsWith(prefix)) {
+ return false;
+ }
+ if (!suffix.isEmpty() && !host.endsWith(suffix)) {
+ return false;
+ }
+ // Additional sanity checks on content selected by wildcard can be done here
+ if (strict) {
+ final String remainder = host.substring(prefix.length(), host.length() - suffix.length());
+ return !remainder.contains(".");
+ }
+ return true;
+ }
+ return host.equalsIgnoreCase(identity);
+ }
+
+ private static boolean matchIdentityStrict(final String host, final String identity) {
+ return matchIdentity(host, identity, true);
+ }
+
+ private static String extractCN(final String subjectPrincipal) throws SSLException {
+ if (subjectPrincipal == null) {
+ return null;
+ }
+ try {
+ final LdapName subjectDN = new LdapName(subjectPrincipal);
+ final List<Rdn> rdns = subjectDN.getRdns();
+ for (int i = rdns.size() - 1; i >= 0; i--) {
+ final Rdn rds = rdns.get(i);
+ final Attributes attributes = rds.toAttributes();
+ final Attribute cn = attributes.get("cn");
+ if (cn != null) {
+ try {
+ final Object value = cn.get();
+ if (value != null) {
+ return value.toString();
+ }
+ } catch (final NoSuchElementException ignore) {
+ // ignore exception
+ } catch (final NamingException ignore) {
+ // ignore exception
+ }
+ }
+ }
+ return null;
+ } catch (final InvalidNameException e) {
+ throw new SSLException(subjectPrincipal + " is not a valid X500 distinguished name");
+ }
+ }
+
+ private static Optional<InetAddress> parseIpAddress(String host) {
+ host = host.trim();
+ // Uri strings only work for ipv6 and are wrapped with brackets
+ // Unfortunately InetAddresses can't handle a mixed input, so we
+ // check here and choose which parse method to use.
+ if (host.startsWith("[") && host.endsWith("]")) {
+ return parseIpAddressUriString(host);
+ } else {
+ return parseIpAddressString(host);
+ }
+ }
+
+ private static Optional<InetAddress> parseIpAddressUriString(String host) {
+ if (InetAddresses.isUriInetAddress(host)) {
+ return Optional.of(InetAddresses.forUriString(host));
+ }
+ return Optional.empty();
+ }
+
+ private static Optional<InetAddress> parseIpAddressString(String host) {
+ if (InetAddresses.isInetAddress(host)) {
+ return Optional.of(InetAddresses.forString(host));
+ }
+ return Optional.empty();
+ }
+
+ @SuppressWarnings("MixedMutabilityReturnType")
+ private static List<SubjectName> getSubjectAltNames(final X509Certificate cert) {
+ try {
+ final Collection<List<?>> entries = cert.getSubjectAlternativeNames();
+ if (entries == null) {
+ return Collections.emptyList();
+ }
+ final List<SubjectName> result = new ArrayList<SubjectName>();
+ for (List<?> entry : entries) {
+ final Integer type = entry.size() >= 2 ? (Integer) entry.get(0) : null;
+ 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[]
Review Comment:
Will this be a follow on issue?
##########
hbase-common/src/main/java/org/apache/hadoop/hbase/io/crypto/tls/HBaseTrustManager.java:
##########
@@ -0,0 +1,167 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hbase.io.crypto.tls;
+
+import java.net.InetAddress;
+import java.net.Socket;
+import java.net.UnknownHostException;
+import java.security.cert.CertificateException;
+import java.security.cert.X509Certificate;
+import javax.net.ssl.SSLEngine;
+import javax.net.ssl.SSLException;
+import javax.net.ssl.X509ExtendedTrustManager;
+import org.apache.yetus.audience.InterfaceAudience;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * A custom TrustManager that supports hostname verification We attempt to perform verification
+ * using just the IP address first and if that fails will attempt to perform a reverse DNS lookup
+ * and verify using the hostname. This file has been copied from the Apache ZooKeeper project.
+ * @see <a href=
+ * "https://github.com/apache/zookeeper/blob/c74658d398cdc1d207aa296cb6e20de00faec03e/zookeeper-server/src/main/java/org/apache/zookeeper/common/ZKTrustManager.java">Base
+ * revision</a>
+ */
+@InterfaceAudience.Private
+public class HBaseTrustManager extends X509ExtendedTrustManager {
+
+ private static final Logger LOG = LoggerFactory.getLogger(HBaseTrustManager.class);
+
+ private final X509ExtendedTrustManager x509ExtendedTrustManager;
+ private final boolean hostnameVerificationEnabled;
+ private final boolean allowReverseDnsLookup;
+
+ private final HBaseHostnameVerifier hostnameVerifier;
+
+ /**
+ * Instantiate a new HBaseTrustManager.
+ * @param x509ExtendedTrustManager The trustmanager to use for
+ * checkClientTrusted/checkServerTrusted logic
+ * @param hostnameVerificationEnabled If true, this TrustManager should verify hostnames of peers
+ * when checking trust.
+ * @param allowReverseDnsLookup If true, we will fall back on reverse dns if resolving of
+ * host fails
+ */
+ HBaseTrustManager(X509ExtendedTrustManager x509ExtendedTrustManager,
+ boolean hostnameVerificationEnabled, boolean allowReverseDnsLookup) {
+ this.x509ExtendedTrustManager = x509ExtendedTrustManager;
+ this.hostnameVerificationEnabled = hostnameVerificationEnabled;
+ this.allowReverseDnsLookup = allowReverseDnsLookup;
+ this.hostnameVerifier = new HBaseHostnameVerifier();
+ }
+
+ @Override
+ public X509Certificate[] getAcceptedIssuers() {
+ return x509ExtendedTrustManager.getAcceptedIssuers();
+ }
+
+ @Override
+ public void checkClientTrusted(X509Certificate[] chain, String authType, Socket socket)
+ throws CertificateException {
+ x509ExtendedTrustManager.checkClientTrusted(chain, authType, socket);
+ if (hostnameVerificationEnabled) {
+ performHostVerification(socket.getInetAddress(), chain[0]);
+ }
+ }
+
+ @Override
+ public void checkServerTrusted(X509Certificate[] chain, String authType, Socket socket)
+ throws CertificateException {
+ x509ExtendedTrustManager.checkServerTrusted(chain, authType, socket);
+ if (hostnameVerificationEnabled) {
+ performHostVerification(socket.getInetAddress(), chain[0]);
+ }
+ }
+
+ @Override
+ public void checkClientTrusted(X509Certificate[] chain, String authType, SSLEngine engine)
+ throws CertificateException {
+ x509ExtendedTrustManager.checkClientTrusted(chain, authType, engine);
+ if (hostnameVerificationEnabled) {
+ try {
+ performHostVerification(InetAddress.getByName(engine.getPeerHost()), chain[0]);
+ } catch (UnknownHostException e) {
+ throw new CertificateException("Failed to verify host", e);
+ }
+ }
+ }
+
+ @Override
+ public void checkServerTrusted(X509Certificate[] chain, String authType, SSLEngine engine)
+ throws CertificateException {
+ x509ExtendedTrustManager.checkServerTrusted(chain, authType, engine);
+ if (hostnameVerificationEnabled) {
+ try {
+ performHostVerification(InetAddress.getByName(engine.getPeerHost()), chain[0]);
+ } catch (UnknownHostException e) {
+ throw new CertificateException("Failed to verify host", e);
+ }
+ }
+ }
+
+ @Override
+ public void checkClientTrusted(X509Certificate[] chain, String authType)
+ throws CertificateException {
+ x509ExtendedTrustManager.checkClientTrusted(chain, authType);
+ }
+
+ @Override
+ public void checkServerTrusted(X509Certificate[] chain, String authType)
+ throws CertificateException {
+ x509ExtendedTrustManager.checkServerTrusted(chain, authType);
+ }
+
+ /**
+ * Compares peer's hostname with the one stored in the provided client certificate. Performs
+ * verification with the help of provided HostnameVerifier.
+ * @param inetAddress Peer's inet address.
+ * @param certificate Peer's certificate
+ * @throws CertificateException Thrown if the provided certificate doesn't match the peer
+ * hostname.
+ */
+ private void performHostVerification(InetAddress inetAddress, X509Certificate certificate)
+ throws CertificateException {
+ String hostAddress = "";
+ String hostName = "";
+ try {
+ hostAddress = inetAddress.getHostAddress();
+ hostnameVerifier.verify(hostAddress, certificate);
+ } catch (SSLException addressVerificationException) {
+ if (!allowReverseDnsLookup) {
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] bbeaudreault closed pull request #4724: HBASE-27280 Add mutual authentication support to TLS
Posted by GitBox <gi...@apache.org>.
bbeaudreault closed pull request #4724: HBASE-27280 Add mutual authentication support to TLS
URL: https://github.com/apache/hbase/pull/4724
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #4724: HBASE-27280 Add mutual authentication support to TLS
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on PR #4724:
URL: https://github.com/apache/hbase/pull/4724#issuecomment-1224965827
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 5m 40s | Docker mode activated. |
||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 0s | No case conflicting files found. |
| +1 :green_heart: | hbaseanti | 0m 0s | Patch does not have any anti-patterns. |
| +1 :green_heart: | @author | 0m 0s | The patch does not contain any @author tags. |
||| _ master Compile Tests _ |
| +1 :green_heart: | mvninstall | 2m 29s | master passed |
| +1 :green_heart: | compile | 0m 34s | master passed |
| +1 :green_heart: | checkstyle | 0m 14s | master passed |
| +1 :green_heart: | spotless | 0m 42s | branch has no errors when running spotless:check. |
| +1 :green_heart: | spotbugs | 0m 31s | master passed |
||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 2m 9s | the patch passed |
| +1 :green_heart: | compile | 0m 32s | the patch passed |
| -0 :warning: | javac | 0m 32s | hbase-common generated 9 new + 0 unchanged - 0 fixed = 9 total (was 0) |
| -0 :warning: | checkstyle | 0m 14s | hbase-common: The patch generated 5 new + 0 unchanged - 0 fixed = 5 total (was 0) |
| +1 :green_heart: | whitespace | 0m 0s | The patch has no whitespace issues. |
| +1 :green_heart: | hadoopcheck | 7m 59s | Patch does not cause any errors with Hadoop 3.2.4 3.3.4. |
| +1 :green_heart: | spotless | 0m 39s | patch has no errors when running spotless:check. |
| +1 :green_heart: | spotbugs | 0m 35s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | asflicense | 0m 11s | The patch does not generate ASF License warnings. |
| | | 27m 44s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4724/1/artifact/yetus-general-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/4724 |
| Optional Tests | dupname asflicense javac spotbugs hadoopcheck hbaseanti spotless checkstyle compile |
| uname | Linux 771d76ca6fe2 5.4.0-122-generic #138-Ubuntu SMP Wed Jun 22 15:00:31 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / 950ad8dd3e |
| Default Java | AdoptOpenJDK-1.8.0_282-b08 |
| javac | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4724/1/artifact/yetus-general-check/output/diff-compile-javac-hbase-common.txt |
| checkstyle | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4724/1/artifact/yetus-general-check/output/diff-checkstyle-hbase-common.txt |
| Max. process+thread count | 69 (vs. ulimit of 30000) |
| modules | C: hbase-common U: hbase-common |
| Console output | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4724/1/console |
| versions | git=2.17.1 maven=3.6.3 spotbugs=4.2.2 |
| Powered by | Apache Yetus 0.12.0 https://yetus.apache.org |
This message was automatically generated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] bbeaudreault commented on pull request #4724: HBASE-27280 Add mutual authentication support to TLS
Posted by GitBox <gi...@apache.org>.
bbeaudreault commented on PR #4724:
URL: https://github.com/apache/hbase/pull/4724#issuecomment-1242505172
Test failures are unrelated.
Spotbugs failure also unrelated.
This PR is good to merge.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #4724: HBASE-27280 Add mutual authentication support to TLS
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on PR #4724:
URL: https://github.com/apache/hbase/pull/4724#issuecomment-1241438524
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 0m 38s | Docker mode activated. |
| -0 :warning: | yetus | 0m 2s | Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck |
||| _ Prechecks _ |
||| _ master Compile Tests _ |
| +0 :ok: | mvndep | 1m 1s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 2m 50s | master passed |
| +1 :green_heart: | compile | 0m 58s | master passed |
| +1 :green_heart: | shadedjars | 4m 11s | branch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 0m 39s | master passed |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 11s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 2m 35s | the patch passed |
| +1 :green_heart: | compile | 0m 59s | the patch passed |
| +1 :green_heart: | javac | 0m 59s | the patch passed |
| +1 :green_heart: | shadedjars | 4m 12s | patch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 0m 37s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | unit | 2m 6s | hbase-common in the patch passed. |
| +1 :green_heart: | unit | 193m 45s | hbase-server in the patch passed. |
| | | 217m 4s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4724/4/artifact/yetus-jdk11-hadoop3-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/4724 |
| Optional Tests | javac javadoc unit shadedjars compile |
| uname | Linux a5e34f2135a9 5.4.0-1081-aws #88~18.04.1-Ubuntu SMP Thu Jun 23 16:29:17 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / 793f020e13 |
| Default Java | AdoptOpenJDK-11.0.10+9 |
| Test Results | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4724/4/testReport/ |
| Max. process+thread count | 2820 (vs. ulimit of 30000) |
| modules | C: hbase-common hbase-server U: . |
| Console output | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4724/4/console |
| versions | git=2.17.1 maven=3.6.3 |
| Powered by | Apache Yetus 0.12.0 https://yetus.apache.org |
This message was automatically generated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache9 commented on a diff in pull request #4724: HBASE-27280 Add mutual authentication support to TLS
Posted by GitBox <gi...@apache.org>.
Apache9 commented on code in PR #4724:
URL: https://github.com/apache/hbase/pull/4724#discussion_r973733235
##########
hbase-common/src/main/java/org/apache/hadoop/hbase/io/crypto/tls/HBaseTrustManager.java:
##########
@@ -0,0 +1,160 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hbase.io.crypto.tls;
+
+import java.net.InetAddress;
+import java.net.Socket;
+import java.net.UnknownHostException;
+import java.security.cert.CertificateException;
+import java.security.cert.X509Certificate;
+import javax.net.ssl.SSLEngine;
+import javax.net.ssl.SSLException;
+import javax.net.ssl.X509ExtendedTrustManager;
+import org.apache.yetus.audience.InterfaceAudience;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * A custom TrustManager that supports hostname verification We attempt to perform verification
Review Comment:
This is not the default behavior of java's TrustManager implementation?
##########
hbase-common/src/main/java/org/apache/hadoop/hbase/io/crypto/tls/X509Util.java:
##########
@@ -110,6 +122,47 @@ private static String[] getCBCCiphers() {
private static final String[] DEFAULT_CIPHERS_JAVA9 =
ObjectArrays.concat(getGCMCiphers(), getCBCCiphers(), String.class);
+ /**
+ * Enum specifying the client auth requirement of server-side TLS sockets created by this
+ * X509Util.
+ * <ul>
+ * <li>NONE - do not request a client certificate.</li>
+ * <li>WANT - request a client certificate, but allow anonymous clients to connect.</li>
+ * <li>NEED - require a client certificate, disconnect anonymous clients.</li>
+ * </ul>
+ * If the config property is not set, the default value is NEED.
+ */
+ public enum ClientAuth {
Review Comment:
Why not juse use the netty ones directly?
##########
hbase-common/src/main/java/org/apache/hadoop/hbase/io/crypto/tls/HBaseHostnameVerifier.java:
##########
@@ -0,0 +1,368 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hbase.io.crypto.tls;
+
+import java.net.InetAddress;
+import java.net.UnknownHostException;
+import java.security.cert.Certificate;
+import java.security.cert.CertificateParsingException;
+import java.security.cert.X509Certificate;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.List;
+import java.util.Locale;
+import java.util.NoSuchElementException;
+import java.util.Objects;
+import java.util.regex.Pattern;
+import javax.naming.InvalidNameException;
+import javax.naming.NamingException;
+import javax.naming.directory.Attribute;
+import javax.naming.directory.Attributes;
+import javax.naming.ldap.LdapName;
+import javax.naming.ldap.Rdn;
+import javax.net.ssl.HostnameVerifier;
+import javax.net.ssl.SSLException;
+import javax.net.ssl.SSLPeerUnverifiedException;
+import javax.net.ssl.SSLSession;
+import javax.security.auth.x500.X500Principal;
+import org.apache.yetus.audience.InterfaceAudience;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * When enabled in {@link X509Util}, handles verifying that the hostname of a peer matches the
+ * certificate it presents.
+ * <p/>
+ * This file has been copied from the Apache ZooKeeper project.
+ * @see <a href=
+ * "https://github.com/apache/zookeeper/blob/5820d10d9dc58c8e12d2e25386fdf92acb360359/zookeeper-server/src/main/java/org/apache/zookeeper/common/ZKHostnameVerifier.java">Base
+ * revision</a>
+ */
+@InterfaceAudience.Private
+final class HBaseHostnameVerifier implements HostnameVerifier {
Review Comment:
Is it possible to just make use of guava's InetAddresses? It has a isInetAddress method which works for both ipv4 and ipv6, and then we could create a InetAddress instance and will know whether it is ipv4 or ipv6.
##########
hbase-common/src/main/java/org/apache/hadoop/hbase/io/crypto/tls/HBaseHostnameVerifier.java:
##########
@@ -0,0 +1,368 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hbase.io.crypto.tls;
+
+import java.net.InetAddress;
+import java.net.UnknownHostException;
+import java.security.cert.Certificate;
+import java.security.cert.CertificateParsingException;
+import java.security.cert.X509Certificate;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.List;
+import java.util.Locale;
+import java.util.NoSuchElementException;
+import java.util.Objects;
+import java.util.regex.Pattern;
+import javax.naming.InvalidNameException;
+import javax.naming.NamingException;
+import javax.naming.directory.Attribute;
+import javax.naming.directory.Attributes;
+import javax.naming.ldap.LdapName;
+import javax.naming.ldap.Rdn;
+import javax.net.ssl.HostnameVerifier;
+import javax.net.ssl.SSLException;
+import javax.net.ssl.SSLPeerUnverifiedException;
+import javax.net.ssl.SSLSession;
+import javax.security.auth.x500.X500Principal;
+import org.apache.yetus.audience.InterfaceAudience;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * When enabled in {@link X509Util}, handles verifying that the hostname of a peer matches the
+ * certificate it presents.
+ * <p/>
+ * This file has been copied from the Apache ZooKeeper project.
+ * @see <a href=
+ * "https://github.com/apache/zookeeper/blob/5820d10d9dc58c8e12d2e25386fdf92acb360359/zookeeper-server/src/main/java/org/apache/zookeeper/common/ZKHostnameVerifier.java">Base
+ * revision</a>
+ */
+@InterfaceAudience.Private
+final class HBaseHostnameVerifier implements HostnameVerifier {
+
+ private final Logger LOG = LoggerFactory.getLogger(HBaseHostnameVerifier.class);
+
+ /**
+ * Note: copied from Apache httpclient with some minor modifications. We want host verification,
+ * but depending on the httpclient jar caused unexplained performance regressions (even when the
+ * code was not used).
+ */
+ private static final class SubjectName {
+
+ static final int DNS = 2;
+ static final int IP = 7;
+
+ private final String value;
+ private final int type;
+
+ static SubjectName IP(final String value) {
Review Comment:
Use lower case here?
##########
hbase-common/src/main/java/org/apache/hadoop/hbase/io/crypto/tls/HBaseHostnameVerifier.java:
##########
@@ -0,0 +1,368 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hbase.io.crypto.tls;
+
+import java.net.InetAddress;
+import java.net.UnknownHostException;
+import java.security.cert.Certificate;
+import java.security.cert.CertificateParsingException;
+import java.security.cert.X509Certificate;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.List;
+import java.util.Locale;
+import java.util.NoSuchElementException;
+import java.util.Objects;
+import java.util.regex.Pattern;
+import javax.naming.InvalidNameException;
+import javax.naming.NamingException;
+import javax.naming.directory.Attribute;
+import javax.naming.directory.Attributes;
+import javax.naming.ldap.LdapName;
+import javax.naming.ldap.Rdn;
+import javax.net.ssl.HostnameVerifier;
+import javax.net.ssl.SSLException;
+import javax.net.ssl.SSLPeerUnverifiedException;
+import javax.net.ssl.SSLSession;
+import javax.security.auth.x500.X500Principal;
+import org.apache.yetus.audience.InterfaceAudience;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * When enabled in {@link X509Util}, handles verifying that the hostname of a peer matches the
+ * certificate it presents.
+ * <p/>
+ * This file has been copied from the Apache ZooKeeper project.
+ * @see <a href=
+ * "https://github.com/apache/zookeeper/blob/5820d10d9dc58c8e12d2e25386fdf92acb360359/zookeeper-server/src/main/java/org/apache/zookeeper/common/ZKHostnameVerifier.java">Base
+ * revision</a>
+ */
+@InterfaceAudience.Private
+final class HBaseHostnameVerifier implements HostnameVerifier {
+
+ private final Logger LOG = LoggerFactory.getLogger(HBaseHostnameVerifier.class);
+
+ /**
+ * Note: copied from Apache httpclient with some minor modifications. We want host verification,
+ * but depending on the httpclient jar caused unexplained performance regressions (even when the
+ * code was not used).
+ */
+ private static final class SubjectName {
+
+ static final int DNS = 2;
+ static final int IP = 7;
+
+ private final String value;
+ private final int type;
+
+ static SubjectName IP(final String value) {
+ return new SubjectName(value, IP);
+ }
+
+ static SubjectName DNS(final String value) {
+ return new SubjectName(value, DNS);
+ }
+
+ SubjectName(final String value, final int type) {
+ if (type != DNS && type != IP) {
+ throw new IllegalArgumentException("Invalid type: " + type);
+ }
+ this.value = Objects.requireNonNull(value);
+ this.type = type;
+ }
+
+ public int getType() {
+ return type;
+ }
+
+ public String getValue() {
+ return value;
+ }
+
+ @Override
+ public String toString() {
+ return value;
+ }
+
+ }
+
+ /**
+ * Note: copied from Apache httpclient. We want host verification, but depending on the httpclient
+ * jar caused unexplained performance regressions (even when the code was not used).
+ */
+ private static final class InetAddressUtils {
+
+ private InetAddressUtils() {
+ }
+
+ private static final Pattern IPV4_PATTERN = Pattern
+ .compile("^(25[0-5]|2[0-4]\\d|[0-1]?\\d?\\d)(\\.(25[0-5]|2[0-4]\\d|[0-1]?\\d?\\d)){3}$");
+
+ private static final Pattern IPV6_STD_PATTERN =
+ Pattern.compile("^(?:[0-9a-fA-F]{1,4}:){7}[0-9a-fA-F]{1,4}$");
+
+ @SuppressWarnings("checkstyle:linelength")
+ private static final Pattern IPV6_HEX_COMPRESSED_PATTERN = Pattern.compile(
+ "^((?:[0-9A-Fa-f]{1,4}(?::[0-9A-Fa-f]{1,4})*)?)::((?:[0-9A-Fa-f]{1,4}(?::[0-9A-Fa-f]{1,4})*)?)$");
+
+ static boolean isIPv4Address(final String input) {
+ return IPV4_PATTERN.matcher(input).matches();
+ }
+
+ static boolean isIPv6StdAddress(final String input) {
+ return IPV6_STD_PATTERN.matcher(input).matches();
+ }
+
+ static boolean isIPv6HexCompressedAddress(final String input) {
+ return IPV6_HEX_COMPRESSED_PATTERN.matcher(input).matches();
+ }
+
+ static boolean isIPv6Address(final String input) {
+ return isIPv6StdAddress(input) || isIPv6HexCompressedAddress(input);
+ }
+
+ }
+
+ enum HostNameType {
+
+ IPv4(7),
+ IPv6(7),
+ DNS(2);
+
+ final int subjectType;
+
+ HostNameType(final int subjectType) {
+ this.subjectType = subjectType;
+ }
+
+ }
+
+ @Override
+ public boolean verify(final String host, final SSLSession session) {
+ try {
+ final Certificate[] certs = session.getPeerCertificates();
+ final X509Certificate x509 = (X509Certificate) certs[0];
+ verify(host, x509);
+ return true;
+ } catch (final SSLException ex) {
+ LOG.debug("Unexpected exception", ex);
+ return false;
+ }
+ }
+
+ void verify(final String host, final X509Certificate cert) throws SSLException {
+ final HostNameType hostType = determineHostFormat(host);
+ final List<SubjectName> subjectAlts = getSubjectAltNames(cert);
+ if (subjectAlts != null && !subjectAlts.isEmpty()) {
+ switch (hostType) {
+ case IPv4:
+ matchIPAddress(host, subjectAlts);
+ break;
+ case IPv6:
+ matchIPv6Address(host, subjectAlts);
+ break;
+ default:
+ matchDNSName(host, subjectAlts);
+ }
+ } else {
+ // CN matching has been deprecated by rfc2818 and can be used
+ // as fallback only when no subjectAlts are available
+ final X500Principal subjectPrincipal = cert.getSubjectX500Principal();
+ final String cn = extractCN(subjectPrincipal.getName(X500Principal.RFC2253));
+ if (cn == null) {
+ throw new SSLException("Certificate subject for <" + host + "> doesn't contain "
+ + "a common name and does not have alternative names");
+ }
+ matchCN(host, cn);
+ }
+ }
+
+ private static void matchIPAddress(final String host, final List<SubjectName> subjectAlts)
+ throws SSLException {
+ for (int i = 0; i < subjectAlts.size(); i++) {
+ final SubjectName subjectAlt = subjectAlts.get(i);
+ if (subjectAlt.getType() == SubjectName.IP) {
+ if (host.equals(subjectAlt.getValue())) {
+ return;
+ }
+ }
+ }
+ throw new SSLPeerUnverifiedException("Certificate for <" + host + "> doesn't match any "
+ + "of the subject alternative names: " + subjectAlts);
+ }
+
+ private static void matchIPv6Address(final String host, final List<SubjectName> subjectAlts)
+ throws SSLException {
+ final String normalisedHost = normaliseAddress(host);
+ for (int i = 0; i < subjectAlts.size(); i++) {
+ final SubjectName subjectAlt = subjectAlts.get(i);
+ if (subjectAlt.getType() == SubjectName.IP) {
+ final String normalizedSubjectAlt = normaliseAddress(subjectAlt.getValue());
+ if (normalisedHost.equals(normalizedSubjectAlt)) {
+ return;
+ }
+ }
+ }
+ throw new SSLPeerUnverifiedException("Certificate for <" + host + "> doesn't match any "
+ + "of the subject alternative names: " + subjectAlts);
+ }
+
+ private static void matchDNSName(final String host, final List<SubjectName> subjectAlts)
+ throws SSLException {
+ final String normalizedHost = host.toLowerCase(Locale.ROOT);
+ for (int i = 0; i < subjectAlts.size(); i++) {
+ final SubjectName subjectAlt = subjectAlts.get(i);
+ if (subjectAlt.getType() == SubjectName.DNS) {
+ final String normalizedSubjectAlt = subjectAlt.getValue().toLowerCase(Locale.ROOT);
+ if (matchIdentityStrict(normalizedHost, normalizedSubjectAlt)) {
+ return;
+ }
+ }
+ }
+ throw new SSLPeerUnverifiedException("Certificate for <" + host + "> doesn't match any "
+ + "of the subject alternative names: " + subjectAlts);
+ }
+
+ private static void matchCN(final String host, final String cn) throws SSLException {
+ final String normalizedHost = host.toLowerCase(Locale.ROOT);
+ final String normalizedCn = cn.toLowerCase(Locale.ROOT);
+ if (!matchIdentityStrict(normalizedHost, normalizedCn)) {
+ throw new SSLPeerUnverifiedException("Certificate for <" + host + "> doesn't match "
+ + "common name of the certificate subject: " + cn);
+ }
+ }
+
+ private static boolean matchIdentity(final String host, final String identity,
+ final boolean strict) {
+ // RFC 2818, 3.1. Server Identity
+ // "...Names may contain the wildcard
+ // character * which is considered to match any single domain name
+ // component or component fragment..."
+ // Based on this statement presuming only singular wildcard is legal
+ final int asteriskIdx = identity.indexOf('*');
+ if (asteriskIdx != -1) {
+ final String prefix = identity.substring(0, asteriskIdx);
+ final String suffix = identity.substring(asteriskIdx + 1);
+ if (!prefix.isEmpty() && !host.startsWith(prefix)) {
+ return false;
+ }
+ if (!suffix.isEmpty() && !host.endsWith(suffix)) {
+ return false;
+ }
+ // Additional sanity checks on content selected by wildcard can be done here
+ if (strict) {
+ final String remainder = host.substring(prefix.length(), host.length() - suffix.length());
+ return !remainder.contains(".");
+ }
+ return true;
+ }
+ return host.equalsIgnoreCase(identity);
+ }
+
+ private static boolean matchIdentityStrict(final String host, final String identity) {
+ return matchIdentity(host, identity, true);
+ }
+
+ private static String extractCN(final String subjectPrincipal) throws SSLException {
+ if (subjectPrincipal == null) {
+ return null;
+ }
+ try {
+ final LdapName subjectDN = new LdapName(subjectPrincipal);
+ final List<Rdn> rdns = subjectDN.getRdns();
+ for (int i = rdns.size() - 1; i >= 0; i--) {
+ final Rdn rds = rdns.get(i);
+ final Attributes attributes = rds.toAttributes();
+ final Attribute cn = attributes.get("cn");
+ if (cn != null) {
+ try {
+ final Object value = cn.get();
+ if (value != null) {
+ return value.toString();
+ }
+ } catch (final NoSuchElementException ignore) {
+ // ignore exception
+ } catch (final NamingException ignore) {
+ // ignore exception
+ }
+ }
+ }
+ return null;
+ } catch (final InvalidNameException e) {
+ throw new SSLException(subjectPrincipal + " is not a valid X500 distinguished name");
+ }
+ }
+
+ private static HostNameType determineHostFormat(final String host) {
+ if (InetAddressUtils.isIPv4Address(host)) {
+ return HostNameType.IPv4;
+ }
+ String s = host;
+ if (s.startsWith("[") && s.endsWith("]")) {
+ s = host.substring(1, host.length() - 1);
+ }
+ if (InetAddressUtils.isIPv6Address(s)) {
+ return HostNameType.IPv6;
+ }
+ return HostNameType.DNS;
+ }
+
+ @SuppressWarnings("MixedMutabilityReturnType")
+ private static List<SubjectName> getSubjectAltNames(final X509Certificate cert) {
+ try {
+ final Collection<List<?>> entries = cert.getSubjectAlternativeNames();
+ if (entries == null) {
+ return Collections.emptyList();
+ }
+ final List<SubjectName> result = new ArrayList<SubjectName>();
+ for (List<?> entry : entries) {
+ final Integer type = entry.size() >= 2 ? (Integer) entry.get(0) : null;
+ 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[]
+ if (o instanceof String) {
+ result.add(new SubjectName((String) o, type));
+ }
+ }
+ }
+ }
+ return result;
+ } catch (final CertificateParsingException ignore) {
+ return Collections.emptyList();
+ }
+ }
+
+ /*
+ * Normalize IPv6 or DNS name.
+ */
+ private static String normaliseAddress(final String hostname) {
Review Comment:
normalize?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #4724: HBASE-27280 Add mutual authentication support to TLS
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on PR #4724:
URL: https://github.com/apache/hbase/pull/4724#issuecomment-1242499794
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 1m 12s | Docker mode activated. |
| -0 :warning: | yetus | 0m 2s | Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck |
||| _ Prechecks _ |
||| _ master Compile Tests _ |
| +0 :ok: | mvndep | 0m 11s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 2m 36s | master passed |
| +1 :green_heart: | compile | 1m 4s | master passed |
| +1 :green_heart: | shadedjars | 3m 50s | branch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 0m 43s | master passed |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 14s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 2m 36s | the patch passed |
| +1 :green_heart: | compile | 1m 6s | the patch passed |
| +1 :green_heart: | javac | 1m 6s | the patch passed |
| +1 :green_heart: | shadedjars | 3m 50s | patch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 0m 43s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | unit | 2m 30s | hbase-common in the patch passed. |
| -1 :x: | unit | 229m 48s | hbase-server in the patch failed. |
| | | 253m 4s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4724/6/artifact/yetus-jdk11-hadoop3-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/4724 |
| Optional Tests | javac javadoc unit shadedjars compile |
| uname | Linux 5d4ec647595d 5.4.0-124-generic #140-Ubuntu SMP Thu Aug 4 02:23:37 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / f3f88ff6c1 |
| Default Java | AdoptOpenJDK-11.0.10+9 |
| unit | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4724/6/artifact/yetus-jdk11-hadoop3-check/output/patch-unit-hbase-server.txt |
| Test Results | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4724/6/testReport/ |
| Max. process+thread count | 2580 (vs. ulimit of 30000) |
| modules | C: hbase-common hbase-server U: . |
| Console output | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4724/6/console |
| versions | git=2.17.1 maven=3.6.3 |
| Powered by | Apache Yetus 0.12.0 https://yetus.apache.org |
This message was automatically generated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #4724: HBASE-27280 Add mutual authentication support to TLS
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on PR #4724:
URL: https://github.com/apache/hbase/pull/4724#issuecomment-1242220457
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 1m 13s | Docker mode activated. |
| -0 :warning: | yetus | 0m 3s | Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck |
||| _ Prechecks _ |
||| _ master Compile Tests _ |
| +0 :ok: | mvndep | 0m 12s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 2m 45s | master passed |
| +1 :green_heart: | compile | 1m 4s | master passed |
| +1 :green_heart: | shadedjars | 3m 50s | branch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 0m 45s | master passed |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 13s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 2m 36s | the patch passed |
| +1 :green_heart: | compile | 1m 6s | the patch passed |
| +1 :green_heart: | javac | 1m 6s | the patch passed |
| +1 :green_heart: | shadedjars | 3m 47s | patch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 0m 46s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | unit | 2m 32s | hbase-common in the patch passed. |
| -1 :x: | unit | 208m 55s | hbase-server in the patch failed. |
| | | 231m 41s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4724/5/artifact/yetus-jdk11-hadoop3-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/4724 |
| Optional Tests | javac javadoc unit shadedjars compile |
| uname | Linux 4b81858fe4f3 5.4.0-124-generic #140-Ubuntu SMP Thu Aug 4 02:23:37 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / 793f020e13 |
| Default Java | AdoptOpenJDK-11.0.10+9 |
| unit | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4724/5/artifact/yetus-jdk11-hadoop3-check/output/patch-unit-hbase-server.txt |
| Test Results | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4724/5/testReport/ |
| Max. process+thread count | 2475 (vs. ulimit of 30000) |
| modules | C: hbase-common hbase-server U: . |
| Console output | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4724/5/console |
| versions | git=2.17.1 maven=3.6.3 |
| Powered by | Apache Yetus 0.12.0 https://yetus.apache.org |
This message was automatically generated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache9 commented on a diff in pull request #4724: HBASE-27280 Add mutual authentication support to TLS
Posted by GitBox <gi...@apache.org>.
Apache9 commented on code in PR #4724:
URL: https://github.com/apache/hbase/pull/4724#discussion_r974252013
##########
hbase-common/src/main/java/org/apache/hadoop/hbase/io/crypto/tls/HBaseTrustManager.java:
##########
@@ -0,0 +1,160 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hbase.io.crypto.tls;
+
+import java.net.InetAddress;
+import java.net.Socket;
+import java.net.UnknownHostException;
+import java.security.cert.CertificateException;
+import java.security.cert.X509Certificate;
+import javax.net.ssl.SSLEngine;
+import javax.net.ssl.SSLException;
+import javax.net.ssl.X509ExtendedTrustManager;
+import org.apache.yetus.audience.InterfaceAudience;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * A custom TrustManager that supports hostname verification We attempt to perform verification
Review Comment:
The response is incomplete? Seems missed several words...
I think the reason why netty does not expose SslParameters is that it has several different ssl implementation, and it will try to make use of the openssl one if possible, which is not initialized by jdk's SslParameters. Let me take a look of the identification algo.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache9 commented on a diff in pull request #4724: HBASE-27280 Add mutual authentication support to TLS
Posted by GitBox <gi...@apache.org>.
Apache9 commented on code in PR #4724:
URL: https://github.com/apache/hbase/pull/4724#discussion_r975327112
##########
hbase-common/src/main/java/org/apache/hadoop/hbase/io/crypto/tls/HBaseHostnameVerifier.java:
##########
@@ -0,0 +1,368 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hbase.io.crypto.tls;
+
+import java.net.InetAddress;
+import java.net.UnknownHostException;
+import java.security.cert.Certificate;
+import java.security.cert.CertificateParsingException;
+import java.security.cert.X509Certificate;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.List;
+import java.util.Locale;
+import java.util.NoSuchElementException;
+import java.util.Objects;
+import java.util.regex.Pattern;
+import javax.naming.InvalidNameException;
+import javax.naming.NamingException;
+import javax.naming.directory.Attribute;
+import javax.naming.directory.Attributes;
+import javax.naming.ldap.LdapName;
+import javax.naming.ldap.Rdn;
+import javax.net.ssl.HostnameVerifier;
+import javax.net.ssl.SSLException;
+import javax.net.ssl.SSLPeerUnverifiedException;
+import javax.net.ssl.SSLSession;
+import javax.security.auth.x500.X500Principal;
+import org.apache.yetus.audience.InterfaceAudience;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * When enabled in {@link X509Util}, handles verifying that the hostname of a peer matches the
+ * certificate it presents.
+ * <p/>
+ * This file has been copied from the Apache ZooKeeper project.
+ * @see <a href=
Review Comment:
Let's keep our own copy :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] bbeaudreault commented on a diff in pull request #4724: HBASE-27280 Add mutual authentication support to TLS
Posted by GitBox <gi...@apache.org>.
bbeaudreault commented on code in PR #4724:
URL: https://github.com/apache/hbase/pull/4724#discussion_r954069133
##########
hbase-common/src/test/java/org/apache/hadoop/hbase/io/crypto/tls/TestHBaseTrustManager.java:
##########
@@ -0,0 +1,290 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hbase.io.crypto.tls;
+
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+import java.math.BigInteger;
+import java.net.InetAddress;
+import java.net.Socket;
+import java.security.KeyPair;
+import java.security.KeyPairGenerator;
+import java.security.Security;
+import java.security.cert.X509Certificate;
+import java.util.ArrayList;
+import java.util.Calendar;
+import java.util.Date;
+import java.util.List;
+import java.util.Random;
+import javax.net.ssl.X509ExtendedTrustManager;
+import org.apache.hadoop.hbase.HBaseClassTestRule;
+import org.apache.hadoop.hbase.testclassification.MiscTests;
+import org.apache.hadoop.hbase.testclassification.SmallTests;
+import org.bouncycastle.asn1.x500.X500NameBuilder;
+import org.bouncycastle.asn1.x500.style.BCStyle;
+import org.bouncycastle.asn1.x509.BasicConstraints;
+import org.bouncycastle.asn1.x509.Extension;
+import org.bouncycastle.asn1.x509.GeneralName;
+import org.bouncycastle.asn1.x509.GeneralNames;
+import org.bouncycastle.asn1.x509.KeyUsage;
+import org.bouncycastle.cert.X509v3CertificateBuilder;
+import org.bouncycastle.cert.jcajce.JcaX509CertificateConverter;
+import org.bouncycastle.cert.jcajce.JcaX509v3CertificateBuilder;
+import org.bouncycastle.jce.provider.BouncyCastleProvider;
+import org.bouncycastle.operator.ContentSigner;
+import org.bouncycastle.operator.jcajce.JcaContentSignerBuilder;
+import org.junit.AfterClass;
+import org.junit.Before;
+import org.junit.BeforeClass;
+import org.junit.ClassRule;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+import org.mockito.invocation.InvocationOnMock;
+import org.mockito.stubbing.Answer;
+
+//
+/**
+ * Test cases taken and adapted from Apache ZooKeeper Project. We can only test calls to
+ * HBaseTrustManager using Sockets (not SSLEngines). This can be fine since the logic is the same.
+ * @see <a href=
+ * "https://github.com/apache/zookeeper/blob/c74658d398cdc1d207aa296cb6e20de00faec03e/zookeeper-server/src/test/java/org/apache/zookeeper/common/HBaseTrustManagerTest.java">Base
+ * revision</a>
+ */
+@Category({ MiscTests.class, SmallTests.class })
+public class TestHBaseTrustManager {
Review Comment:
this class was copied verbatim
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] bbeaudreault commented on a diff in pull request #4724: HBASE-27280 Add mutual authentication support to TLS
Posted by GitBox <gi...@apache.org>.
bbeaudreault commented on code in PR #4724:
URL: https://github.com/apache/hbase/pull/4724#discussion_r954070605
##########
hbase-common/src/test/java/org/apache/hadoop/hbase/io/crypto/tls/TestX509Util.java:
##########
@@ -142,6 +142,38 @@ public void cleanUp() {
Security.setProperty("com.sun.security.enableCRLDP", Boolean.FALSE.toString());
}
+ @Test
+ public void testCreateSSLContextWithClientAuthDefault() throws Exception {
Review 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #4724: HBASE-27280 Add mutual authentication support to TLS
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on PR #4724:
URL: https://github.com/apache/hbase/pull/4724#issuecomment-1226334405
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 0m 36s | Docker mode activated. |
| -0 :warning: | yetus | 0m 3s | Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck |
||| _ Prechecks _ |
||| _ master Compile Tests _ |
| +0 :ok: | mvndep | 0m 40s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 2m 5s | master passed |
| +1 :green_heart: | compile | 0m 49s | master passed |
| +1 :green_heart: | shadedjars | 4m 5s | branch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 0m 34s | master passed |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 11s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 2m 13s | the patch passed |
| +1 :green_heart: | compile | 0m 48s | the patch passed |
| +1 :green_heart: | javac | 0m 48s | the patch passed |
| +1 :green_heart: | shadedjars | 4m 2s | patch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 0m 34s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | unit | 1m 30s | hbase-common in the patch passed. |
| +1 :green_heart: | unit | 204m 32s | hbase-server in the patch passed. |
| | | 224m 49s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4724/2/artifact/yetus-jdk8-hadoop3-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/4724 |
| Optional Tests | javac javadoc unit shadedjars compile |
| uname | Linux abff7af1c2f9 5.4.0-1081-aws #88~18.04.1-Ubuntu SMP Thu Jun 23 16:29:17 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / b44bfc52cc |
| Default Java | AdoptOpenJDK-1.8.0_282-b08 |
| Test Results | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4724/2/testReport/ |
| Max. process+thread count | 2786 (vs. ulimit of 30000) |
| modules | C: hbase-common hbase-server U: . |
| Console output | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4724/2/console |
| versions | git=2.17.1 maven=3.6.3 |
| Powered by | Apache Yetus 0.12.0 https://yetus.apache.org |
This message was automatically generated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #4724: HBASE-27280 Add mutual authentication support to TLS
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on PR #4724:
URL: https://github.com/apache/hbase/pull/4724#issuecomment-1252958163
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 1m 8s | Docker mode activated. |
| -0 :warning: | yetus | 0m 4s | Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck |
||| _ Prechecks _ |
||| _ master Compile Tests _ |
| +0 :ok: | mvndep | 0m 17s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 2m 41s | master passed |
| +1 :green_heart: | compile | 1m 4s | master passed |
| +1 :green_heart: | shadedjars | 3m 47s | branch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 0m 45s | master passed |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 13s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 2m 34s | the patch passed |
| +1 :green_heart: | compile | 1m 4s | the patch passed |
| +1 :green_heart: | javac | 1m 4s | the patch passed |
| +1 :green_heart: | shadedjars | 3m 47s | patch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 0m 42s | the patch passed |
||| _ Other Tests _ |
| -1 :x: | unit | 1m 35s | hbase-common in the patch failed. |
| +1 :green_heart: | unit | 232m 6s | hbase-server in the patch passed. |
| | | 253m 53s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4724/8/artifact/yetus-jdk11-hadoop3-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/4724 |
| Optional Tests | javac javadoc unit shadedjars compile |
| uname | Linux adacf85111c7 5.4.0-124-generic #140-Ubuntu SMP Thu Aug 4 02:23:37 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / 8bc6a5954a |
| Default Java | Eclipse Adoptium-11.0.16.1+1 |
| unit | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4724/8/artifact/yetus-jdk11-hadoop3-check/output/patch-unit-hbase-common.txt |
| Test Results | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4724/8/testReport/ |
| Max. process+thread count | 2302 (vs. ulimit of 30000) |
| modules | C: hbase-common hbase-server U: . |
| Console output | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4724/8/console |
| versions | git=2.17.1 maven=3.6.3 |
| Powered by | Apache Yetus 0.12.0 https://yetus.apache.org |
This message was automatically generated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] bbeaudreault commented on pull request #4724: HBASE-27280 Add mutual authentication support to TLS
Posted by GitBox <gi...@apache.org>.
bbeaudreault commented on PR #4724:
URL: https://github.com/apache/hbase/pull/4724#issuecomment-1255213876
Pre-commit is weirdly failing trying to checkout from git. I just squashed, rebased, and force pushed. No code changes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] bbeaudreault commented on pull request #4724: HBASE-27280 Add mutual authentication support to TLS
Posted by GitBox <gi...@apache.org>.
bbeaudreault commented on PR #4724:
URL: https://github.com/apache/hbase/pull/4724#issuecomment-1255285105
@Apache9 sorry, but any idea why this keeps continually failing?
https://ci-hbase.apache.org/blue/organizations/jenkins/HBase-PreCommit-GitHub-PR/detail/PR-4724/15/pipeline/
https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4724/15/console
> Caused: hudson.plugins.git.GitException: Could not checkout de8ee7bd69e08ae29e7dc8137c0391ba7bf3d561
Also: hudson.plugins.git.GitException: Command "git checkout -f de8ee7bd69e08ae29e7dc8137c0391ba7bf3d561" returned status code 128:
stdout:
stderr: fatal: reference is not a tree: de8ee7bd69e08ae29e7dc8137c0391ba7bf3d561
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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org