Solr
  1. Solr
  2. SOLR-5868

HttpClient should be configured to use ALLOW_ALL_HOSTNAME hostname verifier to simplify SSL setup

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 4.7
    • Fix Version/s: 4.9, 6.0
    • Component/s: None
    • Labels:
      None

      Description

      The default HttpClient hostname verifier is the BROWSER_COMPATIBLE_HOSTNAME_VERIFIER which verifies the hostname that is being connected to matches the hostname presented within the certificate. This is meant to protect clients that are making external requests out across the internet, but requests within the the SOLR cluster should be trusted and can be relaxed to simplify the SSL/certificate setup process.

      1. SOLR-5868.patch
        7 kB
        Steve Davids
      2. SOLR-5868.patch
        2 kB
        Steve Davids

        Issue Links

          Activity

          Hide
          Steve Davids added a comment -

          In the current HttpClientUtil paradigm this can be achieved by retrieving the url scheme and setting the hostname verifier on the SSLSocketFactory: https://gist.github.com/sdavids13/9577027

          If the HTTPClientBuilder approach is introduced (SOLR-5604) then it can be simply done via:

          HttpClientBuilder.create().useSystemProperties().setHostnameVerifier(new AllowAllHostnameVerifier())...;
          
          Show
          Steve Davids added a comment - In the current HttpClientUtil paradigm this can be achieved by retrieving the url scheme and setting the hostname verifier on the SSLSocketFactory: https://gist.github.com/sdavids13/9577027 If the HTTPClientBuilder approach is introduced ( SOLR-5604 ) then it can be simply done via: HttpClientBuilder.create().useSystemProperties().setHostnameVerifier( new AllowAllHostnameVerifier())...;
          Hide
          Steve Davids added a comment -

          Patch attached to work in the current form of the trunk (non HttpClientBuilder version).

          Show
          Steve Davids added a comment - Patch attached to work in the current form of the trunk (non HttpClientBuilder version).
          Hide
          Shawn Heisey added a comment -

          In my opinion, by default, HttpClient should fully verify the certificate for anything it talks to, including the hostname.

          I can understand the motivation here, and I agree that this needs to be possible ... so make it configurable, but don't make it the default. Or is there something about this that I'm not grasping?

          Show
          Shawn Heisey added a comment - In my opinion, by default, HttpClient should fully verify the certificate for anything it talks to, including the hostname. I can understand the motivation here, and I agree that this needs to be possible ... so make it configurable, but don't make it the default. Or is there something about this that I'm not grasping?
          Hide
          Steve Davids added a comment -

          No, it can be configurable if we desire, but it will need to come from a system property since there isn't a global way to set HttpClientUtil params up-front at startup. This was more of an observation that while using solr in a distributed fashion it would be a nightmare to keep up with changes to your certificate when dealing with host names. HttpClient's default value is completely reasonable for making external requests to other systems but I believe in this case the reasonable default is to simplify the maintenance nightmare that ensues with having the stricter policy, especially since all of the requests are being made within the client's own clustered environment.

          Show
          Steve Davids added a comment - No, it can be configurable if we desire, but it will need to come from a system property since there isn't a global way to set HttpClientUtil params up-front at startup. This was more of an observation that while using solr in a distributed fashion it would be a nightmare to keep up with changes to your certificate when dealing with host names. HttpClient's default value is completely reasonable for making external requests to other systems but I believe in this case the reasonable default is to simplify the maintenance nightmare that ensues with having the stricter policy, especially since all of the requests are being made within the client's own clustered environment.
          Hide
          Steve Davids added a comment -

          Would a system property of "solr.ssl.checkName" or "solr.ssl.checkPeerName" be acceptable? This is modeled off of mod_ssl http://httpd.apache.org/docs/trunk/mod/mod_ssl.html#sslproxycheckpeername. I can update the patch to reflect the changes if this is the route we would like to go.

          Show
          Steve Davids added a comment - Would a system property of "solr.ssl.checkName" or "solr.ssl.checkPeerName" be acceptable? This is modeled off of mod_ssl http://httpd.apache.org/docs/trunk/mod/mod_ssl.html#sslproxycheckpeername . I can update the patch to reflect the changes if this is the route we would like to go.
          Hide
          Steve Davids added a comment -

          Uploaded a new patch that allows clients to specify a system property of "solr.ssl.checkPeerName". If the value is set to false the AllowAllHostnameVerifier will be wired up, for all other cases the default HttpClient HostnameVerifier will be enforced (BrowserCompatHostnameVerifier).

          Show
          Steve Davids added a comment - Uploaded a new patch that allows clients to specify a system property of "solr.ssl.checkPeerName". If the value is set to false the AllowAllHostnameVerifier will be wired up, for all other cases the default HttpClient HostnameVerifier will be enforced (BrowserCompatHostnameVerifier).
          Hide
          Mark Miller added a comment -

          Any comments on this Shawn Heisey?

          Show
          Mark Miller added a comment - Any comments on this Shawn Heisey ?
          Hide
          Shawn Heisey added a comment -

          If a system property is the only reasonable way to get the config there, then I think that would be OK. If a solrconfig.xml option is doable without a ton of work, I think one should be available.

          Show
          Shawn Heisey added a comment - If a system property is the only reasonable way to get the config there, then I think that would be OK. If a solrconfig.xml option is doable without a ton of work, I think one should be available.
          Hide
          Mark Miller added a comment -

          I'm going to remove the calls to apache commons for the boolean stuff - ant test seems to show we don't have access to that from solrj yet and we don't want to add the dependency just for this. Looks good otherwise though.

          Show
          Mark Miller added a comment - I'm going to remove the calls to apache commons for the boolean stuff - ant test seems to show we don't have access to that from solrj yet and we don't want to add the dependency just for this. Looks good otherwise though.
          Hide
          Steve Davids added a comment -

          We may want to consider adding a LOG.warn in there if there isn't an HTTPS scheme defined. Maybe something like:

          LOG.warn("Unable to configure SSL Peer Name Verifier since https support hasn't been configured, please verify javax.net.ssl.* system properties are properly configured.

          Show
          Steve Davids added a comment - We may want to consider adding a LOG.warn in there if there isn't an HTTPS scheme defined. Maybe something like: LOG.warn("Unable to configure SSL Peer Name Verifier since https support hasn't been configured, please verify javax.net.ssl.* system properties are properly configured.
          Hide
          Steve Rowe added a comment -

          Mark, do you still plan to get this into 4.7.1?

          Show
          Steve Rowe added a comment - Mark, do you still plan to get this into 4.7.1?
          Hide
          Steve Davids added a comment -

          Mark Miller Would you like me to update the patch to remove the BooleanUtils call?

          Show
          Steve Davids added a comment - Mark Miller Would you like me to update the patch to remove the BooleanUtils call?
          Hide
          Mark Miller added a comment -

          No, thats okay, I have a checkout somewhere with it. Was going to try and get this in 4.7.1 but didn't get to it - I'll commit soon though.

          Show
          Mark Miller added a comment - No, thats okay, I have a checkout somewhere with it. Was going to try and get this in 4.7.1 but didn't get to it - I'll commit soon though.
          Hide
          Uwe Schindler added a comment -

          Move issue to Solr 4.9.

          Show
          Uwe Schindler added a comment - Move issue to Solr 4.9.
          Hide
          ASF subversion and git services added a comment -

          Commit 1588312 from markrmiller@apache.org in branch 'dev/trunk'
          [ https://svn.apache.org/r1588312 ]

          SOLR-5868: HttpClient should be configured to use ALLOW_ALL_HOSTNAME hostname verifier to simplify SSL setup.

          Show
          ASF subversion and git services added a comment - Commit 1588312 from markrmiller@apache.org in branch 'dev/trunk' [ https://svn.apache.org/r1588312 ] SOLR-5868 : HttpClient should be configured to use ALLOW_ALL_HOSTNAME hostname verifier to simplify SSL setup.
          Hide
          ASF subversion and git services added a comment -

          Commit 1588314 from markrmiller@apache.org in branch 'dev/branches/branch_4x'
          [ https://svn.apache.org/r1588314 ]

          SOLR-5868: HttpClient should be configured to use ALLOW_ALL_HOSTNAME hostname verifier to simplify SSL setup.

          Show
          ASF subversion and git services added a comment - Commit 1588314 from markrmiller@apache.org in branch 'dev/branches/branch_4x' [ https://svn.apache.org/r1588314 ] SOLR-5868 : HttpClient should be configured to use ALLOW_ALL_HOSTNAME hostname verifier to simplify SSL setup.
          Hide
          David Smiley added a comment -

          There's a lingering entry for this in CHANGES.txt on the 4x below the "Getting Started" section; probably as a result of a merge problem. If this issue isn't resolved them it should be removed; if it is resolved then it should be moved to the right spot.

          Show
          David Smiley added a comment - There's a lingering entry for this in CHANGES.txt on the 4x below the "Getting Started" section; probably as a result of a merge problem. If this issue isn't resolved them it should be removed; if it is resolved then it should be moved to the right spot.
          Hide
          ASF subversion and git services added a comment -

          Commit 1618285 from hossman@apache.org in branch 'dev/trunk'
          [ https://svn.apache.org/r1618285 ]

          SOLR-5868: fixing botched changes.txt placement/merge from 4.9

          Show
          ASF subversion and git services added a comment - Commit 1618285 from hossman@apache.org in branch 'dev/trunk' [ https://svn.apache.org/r1618285 ] SOLR-5868 : fixing botched changes.txt placement/merge from 4.9
          Hide
          ASF subversion and git services added a comment -

          Commit 1618286 from hossman@apache.org in branch 'dev/branches/branch_4x'
          [ https://svn.apache.org/r1618286 ]

          SOLR-5868: fixing botched changes.txt placement/merge from 4.9 (merge r1618285)

          Show
          ASF subversion and git services added a comment - Commit 1618286 from hossman@apache.org in branch 'dev/branches/branch_4x' [ https://svn.apache.org/r1618286 ] SOLR-5868 : fixing botched changes.txt placement/merge from 4.9 (merge r1618285)
          Hide
          Hoss Man added a comment -

          resolving old issue that was fixed in 4.9 (after cleaning up CHANGES.txt mess)

          Show
          Hoss Man added a comment - resolving old issue that was fixed in 4.9 (after cleaning up CHANGES.txt mess)

            People

            • Assignee:
              Mark Miller
              Reporter:
              Steve Davids
            • Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development