You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by GitBox <gi...@apache.org> on 2021/08/03 16:10:19 UTC
[GitHub] [druid] maytasm opened a new pull request #11538: Fix hostname validation not skipping with `druid.client.https.validateHostnames=false` in java 8u275 and later
maytasm opened a new pull request #11538:
URL: https://github.com/apache/druid/pull/11538
Fix hostname validation not skipping with `druid.client.https.validateHostnames=false` in java 8u275 and later
### Description
Hostname validation is not skipped even after setting `druid.client.https.validateHostnames=false` in java 8u275 and later. This is due to code change introduced in Java 8u275. To disable validate hostnames, Druid sets the endpointIdentificationAlgorithm of the SSLParameters in the SSLEngine to null (using the SSLEngine#setSSLParameters method). This is done in CustomCheckX509TrustManager#checkServerTrusted. In 8u242, the SSLEngine#setSSLParameters (https://github.com/openjdk/jdk8u/blob/b996dbe72ce338fdc430fbba1bb995ce622109e3/jdk/src/share/classes/sun/security/ssl/SSLEngineImpl.java#L2102 ) simply get the new endpointIdentificationAlgorithm from the given input SSLParameters (params) and set it without any verification of the new endpointIdentificationAlgorithm value. In 8u275, the SSLEngine#setSSLParameters (https://github.com/openjdk/jdk8u/blob/37d4863386fb7fccb846f46b1bc85b4c61fe1c88/jdk/src/share/classes/sun/security/ssl/SSLEngineImpl.java#L763 ) calls TransportContext#SSLCo
nfiguration#setSSLParameters (https://github.com/openjdk/jdk8u/blob/37d4863386fb7fccb846f46b1bc85b4c61fe1c88/jdk/src/share/classes/sun/security/ssl/SSLConfiguration.java#L199 ). This method will not set endpointIdentificationAlgorithm to the new value if the new value is null (https://github.com/openjdk/jdk8u/blob/37d4863386fb7fccb846f46b1bc85b4c61fe1c88/jdk/src/share/classes/sun/security/ssl/SSLConfiguration.java#L231 ). Hence, the endpointIdentificationAlgorithm would remains as HTTPS and trigger validate hostnames in X509TrustManagerImpl. The code that determines to validate hostname or not in X509TrustManagerImpl is actually the pre and post 8u275, which is to validate hostname if the identityAlg != null && identityAlg.length() != 0 (note that identityAlg variable is the EndpointIdentificationAlgorithm of the SSLParameters).
As a workaround, we can set EndpointIdentificationAlgorithm to an empty String instead. This will still cause the hostname validation to be skipped.
This PR has:
- [x] been self-reviewed.
- [ ] using the [concurrency checklist](https://github.com/apache/druid/blob/master/dev/code-review/concurrency.md) (Remove this item if the PR doesn't have any relation to concurrency.)
- [ ] added documentation for new or modified features or behaviors.
- [ ] added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
- [ ] added or updated version, license, or notice information in [licenses.yaml](https://github.com/apache/druid/blob/master/dev/license.md)
- [x] added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
- [ ] added unit tests or modified existing tests to cover new code paths, ensuring the threshold for [code coverage](https://github.com/apache/druid/blob/master/dev/code-review/code-coverage.md) is met.
- [ ] added integration tests.
- [x] been tested in a test Druid cluster.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] maytasm commented on pull request #11538: Fix hostname validation not skipping with `druid.client.https.validateHostnames=false` in java 8u275 and later
Posted by GitBox <gi...@apache.org>.
maytasm commented on pull request #11538:
URL: https://github.com/apache/druid/pull/11538#issuecomment-892381902
@jon-wei Thank you so much for reviewing and verifying with the IT. I can look into adding the new IT you suggested but this will require a separate job since we need a different cluster config. This will adds a bit of time to the build pipeline. Btw, I think our IT runs on container with Java OpenJdk 1.8.0_292. You mentioned that you tested with 282, is this a typo or did you had to change the version on the existing IT containers?
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] maytasm edited a comment on pull request #11538: Fix hostname validation not skipping with `druid.client.https.validateHostnames=false` in java 8u275 and later
Posted by GitBox <gi...@apache.org>.
maytasm edited a comment on pull request #11538:
URL: https://github.com/apache/druid/pull/11538#issuecomment-892425356
@jon-wei I have added unit test to verify this fix. The unit test verify that CustomCheckX509TrustManager does set the EndpointIdentificationAlgorithm to null or empty String when validateServerHostnames is set to false. The disadvange to the unit test is that we do not / can not control the specific version of OpenJdk8 that Travis uses to run the unit test. Currently Travis uses openjdk version "1.8.0_252". Hence, the unit test ran in Travis actually does not verify the bug caused by Java 8u275+. The advantages are that 1) the unit test adds very very little additional runtime to the pipeline 2) the unit test ensure that no regression in Java version that is used by Travis tests (which are openjdk version "1.8.0_252", some version of 11 and some version of 15 -- got to lazy to check the specific version) and 3) You can still run the unit test locally against any Java version by changing your IDE Java version, which I did against 8u275 to verify that the fix works. (I also build D
ruid and run the cluster with 8u275 to verify the fix). Anyway, having unit test is better than no unit test...although it will still fail code coverage as Travis does not use Java 8u275 or greater.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] maytasm edited a comment on pull request #11538: Fix hostname validation not skipping with `druid.client.https.validateHostnames=false` in java 8u275 and later
Posted by GitBox <gi...@apache.org>.
maytasm edited a comment on pull request #11538:
URL: https://github.com/apache/druid/pull/11538#issuecomment-892425356
@jon-wei I have added unit test to verify this fix. The unit test verify that CustomCheckX509TrustManager does set the EndpointIdentificationAlgorithm to null or empty String when validateServerHostnames is set to false. The disadvange to the unit test is that we do not / can not control the specific version of OpenJdk8 that Travis uses to run the unit test. Currently Travis uses openjdk version "1.8.0_252". Hence, the unit test ran in Travis actually does not verify the bug caused by Java 8u275+. The advantages are that 1) the unit test adds very very little additional runtime to the pipeline 2) the unit test ensure that no regression in Java version that is used by Travis tests (which are openjdk version "1.8.0_252", some version of 11 and some version of 15 -- got to lazy to check the specific version) and 3) You can still run the unit test locally against any Java version by changing your IDE Java version, which I did against 8u275 to verify that the fix works. (I also build D
ruid and run the cluster with 8u275 to verify the fix). Anyway, having unit test is better than no unit test...although it will still fail code coverage as Travis does not use Java 8u275 or greater (but pass code coverage for our test using Java 11 and Java 15 since those have the same change as 8u275)
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jon-wei commented on pull request #11538: Fix hostname validation not skipping with `druid.client.https.validateHostnames=false` in java 8u275 and later
Posted by GitBox <gi...@apache.org>.
jon-wei commented on pull request #11538:
URL: https://github.com/apache/druid/pull/11538#issuecomment-892879858
> This will adds a bit of time to the build pipeline. Btw, I think our IT runs on container with Java OpenJdk 1.8.0_292. You mentioned that you tested with 282, is this a typo or did you had to change the version on the existing IT containers?
I ran it in my local environment, I checked that it was 282, maybe it was using an older image there
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] maytasm merged pull request #11538: Fix hostname validation not skipping with `druid.client.https.validateHostnames=false` in java 8u275 and later
Posted by GitBox <gi...@apache.org>.
maytasm merged pull request #11538:
URL: https://github.com/apache/druid/pull/11538
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] maytasm merged pull request #11538: Fix hostname validation not skipping with `druid.client.https.validateHostnames=false` in java 8u275 and later
Posted by GitBox <gi...@apache.org>.
maytasm merged pull request #11538:
URL: https://github.com/apache/druid/pull/11538
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] maytasm commented on pull request #11538: Fix hostname validation not skipping with `druid.client.https.validateHostnames=false` in java 8u275 and later
Posted by GitBox <gi...@apache.org>.
maytasm commented on pull request #11538:
URL: https://github.com/apache/druid/pull/11538#issuecomment-892425356
@jon-wei I have added unit test to verify this fix. The unit test verify that CustomCheckX509TrustManager does set the EndpointIdentificationAlgorithm to null or empty String when validateServerHostnames is set to false. The disadvange to the unit test is that we do not / can not control the specific version of OpenJdk8 that Travis uses to run the unit test. Currently Travis uses openjdk version "1.8.0_252". Hence, the unit test ran in Travis actually does not verify the bug caused by Java 8u275+. The advantages are that 1) the unit test adds very very little additional runtime to the pipeline 2) the unit test ensure that no regression in Java version that is used by Travis tests (which are openjdk version "1.8.0_252", some version of 11 and some version of 15 -- got to lazy to check the specific version) and 3) You can still run the unit test locally against any Java version by changing your IDE Java version, which I did against 8u275 to verify that the fix works. (I also build D
ruid and run the cluster with 8u275 to verify the fix). Anyway, having unit test is better than no unit test :)
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] maytasm commented on pull request #11538: Fix hostname validation not skipping with `druid.client.https.validateHostnames=false` in java 8u275 and later
Posted by GitBox <gi...@apache.org>.
maytasm commented on pull request #11538:
URL: https://github.com/apache/druid/pull/11538#issuecomment-893279124
Merging this in. The only job failing is due to code coverage on 1.8.0_252. The new code will only be hit when running in Java version 8u275 or later.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] maytasm edited a comment on pull request #11538: Fix hostname validation not skipping with `druid.client.https.validateHostnames=false` in java 8u275 and later
Posted by GitBox <gi...@apache.org>.
maytasm edited a comment on pull request #11538:
URL: https://github.com/apache/druid/pull/11538#issuecomment-892425356
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] maytasm commented on pull request #11538: Fix hostname validation not skipping with `druid.client.https.validateHostnames=false` in java 8u275 and later
Posted by GitBox <gi...@apache.org>.
maytasm commented on pull request #11538:
URL: https://github.com/apache/druid/pull/11538#issuecomment-893279124
Merging this in. The only job failing is due to code coverage on 1.8.0_252. The new code will only be hit when running in Java version 8u275 or later.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jon-wei commented on pull request #11538: Fix hostname validation not skipping with `druid.client.https.validateHostnames=false` in java 8u275 and later
Posted by GitBox <gi...@apache.org>.
jon-wei commented on pull request #11538:
URL: https://github.com/apache/druid/pull/11538#issuecomment-892879858
> This will adds a bit of time to the build pipeline. Btw, I think our IT runs on container with Java OpenJdk 1.8.0_292. You mentioned that you tested with 282, is this a typo or did you had to change the version on the existing IT containers?
I ran it in my local environment, I checked that it was 282, maybe it was using an older image there
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] maytasm edited a comment on pull request #11538: Fix hostname validation not skipping with `druid.client.https.validateHostnames=false` in java 8u275 and later
Posted by GitBox <gi...@apache.org>.
maytasm edited a comment on pull request #11538:
URL: https://github.com/apache/druid/pull/11538#issuecomment-893279124
Merging this in. The only job failing is due to code coverage on 1.8.0_252. The new code will only be hit when running in Java version 8u275 or later (as seen from same job passing that are running on Java11 and Java15).
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org