[
https://issues.apache.org/jira/browse/SOLR-9304?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16444395#comment-16444395
]
Hoss Man commented on SOLR-9304:
--------------------------------
I think the fix approach in this patch looks correct, allthough 2 related
things bother me regarding the testing of this issue...
# the only tests added are reflection based inspection of the final
SchemaRegistery -- which not only means they'll be brittle if/when we upgrade
commons-http, but it also means that we're not actaully testing that
{{checkPeerNames==false}} does what we say it does. We assert that
{{HttpClientUtil.getSchemaRegisteryProvider().getSchemaRegistry().lookup("https")}}
is a {{ConnectionSocketFactory}} that uses {{NoopHostnameVerifier}}, but that
doesn't prove prove that invalid hostnames will ignored when that property is
set. (Somewhere down the road either the solr code or the http-commons could
be refactored so that that code is irelevant)
# It makes no sense that {{SSLTestConfig}} is checking the value of
{{System.getProperty(HttpClientUtil.SYS_PROP_CHECK_PEER_NAME)}} -- this
completely predates this patch, and as far as I can tell is a blatent bug
introduced by SOLR-4509 as part of that refactoring, but we should address it
here. The behavior of all our SSL testing should be deterministic regardless
of what env/sys-props the user has set.
I'm about to attach an updated version of the patch with some improvements to
address these concerns...
* minor refactoring to HttpClientUtilTest to reduce duplication
* re-add {{create-keystores.sh}}
** this is the script that creates the keystore our SSL testing uses, and it
appears that i removed this in SOLR-10791
** it really should have been moved to {{solr/test-framework/src/resources/}}
prior to that (when the original keystore location was copied/moved).
* improve {{create-keystores.sh}} so that it generates 2 different keystores:
** (the existing) keystore that uses "localhost" and the loopback IP
** another (new) keystore that uses bogus hostname/ip combo that should fail
peer name validation on any machine.
* Add an option to {{SSLTestConfig}} to make peer name validation configurable,
and pick the keystore to use based on that choice.
** When SSLTestConfig's {{checkPeerName=true}}, the config will use the
exsiting "localhost" keystore
** if it's {{checkPeerName=false}} the (new) keystore containing the bogus
hostname/ip combo will be used to ensure that all the SSL client code truly is
ignoring the peer name in the cert.
* Change {{SSLTestConfig}} so that by default it does *NOT* do peer name
validation
** this is technically a change in the default testing behavior, but in my
opinion a minor one since in the past it was only ever validating "localhost"
** if anything it now means less false negatives if someone has "localhost"
configured improperly on their machine.
*** we could potentially randomize this as part of that {{@RandomizeSSL}}
annotation -- i personally don't see a lot of value in doing that, but i'm open
to it if other people feel strongly.
* Add 2 new tests to TestMiniSolrCloudClusterSSL:
** one that ensures an {{SSLTestConfig}} with {{checkPeerName=true}} is usable
and works and clients can talk to the servers
** one that "tests the test" to ensure that if {{checkPeerName=false}} and the
servers are using our "bogus hostname cert" that a client who trust's that
cert, but has set {{HttpClientUtil.SYS_PROP_CHECK_PEER_NAME=true}} will get an
{{SSLException}} if it tries to talk to those servers.
----
I'm still doing some manual testing, but feedback appreciated. Please note
that because of the new (binary) keystore files,the patch was generated with
{{git diff --staged --binary}}. You should be able to use {{git apply}} just
fine, but other patch based tools may not be happy with it.
> -Dsolr.ssl.checkPeerName=false ignored on master
> ------------------------------------------------
>
> Key: SOLR-9304
> URL: https://issues.apache.org/jira/browse/SOLR-9304
> Project: Solr
> Issue Type: Bug
> Security Level: Public(Default Security Level. Issues are Public)
> Affects Versions: 7.0
> Reporter: Hoss Man
> Priority: Major
> Attachments: SOLR-9304-uses-deprecated.patch, SOLR-9304.patch,
> SOLR-9304.patch, SOLR-9304.patch, SOLR-9304.patch, SOLR-9304.patch,
> SOLR-9304.patch
>
>
> {{-Dsolr.ssl.checkPeerName=false}} is completely ignored on master...
> {noformat}
> hossman@tray:~/lucene/dev/solr [master] $ find -name \*.java | xargs grep
> checkPeerName
> ./solrj/src/java/org/apache/solr/client/solrj/impl/HttpClientUtil.java:
> public static final String SYS_PROP_CHECK_PEER_NAME =
> "solr.ssl.checkPeerName";
> hossman@tray:~/lucene/dev/solr [master] $ find -name \*.java | xargs grep
> SYS_PROP_CHECK_PEER_NAME
> ./test-framework/src/java/org/apache/solr/util/SSLTestConfig.java:
> boolean sslCheckPeerName =
> toBooleanDefaultIfNull(toBooleanObject(System.getProperty(HttpClientUtil.SYS_PROP_CHECK_PEER_NAME)),
> true);
> ./solrj/src/java/org/apache/solr/client/solrj/impl/HttpClientUtil.java:
> public static final String SYS_PROP_CHECK_PEER_NAME =
> "solr.ssl.checkPeerName";
> {noformat}
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org
For additional commands, e-mail: dev-h...@lucene.apache.org
