Uploaded image for project: 'Solr'
  1. Solr
  2. SOLR-8970

SSLTestConfig behaves really stupid if keystore can't be found

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 6.0.1, 6.1, 7.0
    • Component/s: None
    • Labels:
      None

      Description

      The SSLTestConfig constructor lets the call (notable SolrTestCaseJ4) tell it wether clientAuth should be used (note SolrTestCaseJ4 calls this boolean "trySslClientAuth") but it has a hardcoded assumption that the keystore file it can use (for both the keystore and the truststore) will exist at a fixed path in the solr install.

      when this works, it works fine - but if end users subclass/reuse SolrTestCaseJ4 in their own projects, they may do so in a way that prevents the SSLTestConfig keystore assumptions from being true, and yet they won't get any sort of clear error.

      1. SOLR-8970.patch
        8 kB
        Hoss Man
      2. SOLR-8970.patch
        5 kB
        Hoss Man
      3. SOLR-8970.patch
        6 kB
        Hoss Man

        Issue Links

          Activity

          Hide
          hossman Hoss Man added a comment -

          SSLTestConfig already sanity checks whether TEST_KEYSTORE exists...

            public static File TEST_KEYSTORE = ExternalPaths.SERVER_HOME == null ? null
                : new File(ExternalPaths.SERVER_HOME, "../etc/test/solrtest.keystore");  
            private static String TEST_KEYSTORE_PATH = TEST_KEYSTORE != null
                && TEST_KEYSTORE.exists() ? TEST_KEYSTORE.getAbsolutePath() : null;
          

          We should...

          • change how TEST_KEYSTORE_PATH is initialized so that people using SolrTestCaseJ4/SSLTestConfig can set a sysprop when running tests to change which path to use when looking for the file
          • change SSLTestConfig's constructors so that if clientAuth==true but TEST_KEYSTORE_PATH==null (either because the TEST_KEYSTORE file does not exist and no sysprop was used to override it) then we fail fast with a useful error telling people to either:
            • set the test sysprop
            • ensure the expected file is in TEST_KEYSTORE
            • add @SupressSSL to their tests.
          Show
          hossman Hoss Man added a comment - SSLTestConfig already sanity checks whether TEST_KEYSTORE exists... public static File TEST_KEYSTORE = ExternalPaths.SERVER_HOME == null ? null : new File(ExternalPaths.SERVER_HOME, "../etc/test/solrtest.keystore" ); private static String TEST_KEYSTORE_PATH = TEST_KEYSTORE != null && TEST_KEYSTORE.exists() ? TEST_KEYSTORE.getAbsolutePath() : null ; We should... change how TEST_KEYSTORE_PATH is initialized so that people using SolrTestCaseJ4/SSLTestConfig can set a sysprop when running tests to change which path to use when looking for the file change SSLTestConfig's constructors so that if clientAuth==true but TEST_KEYSTORE_PATH==null (either because the TEST_KEYSTORE file does not exist and no sysprop was used to override it) then we fail fast with a useful error telling people to either: set the test sysprop ensure the expected file is in TEST_KEYSTORE add @SupressSSL to their tests.
          Hide
          hossman Hoss Man added a comment -

          as things stand now, combined with SOLR-8971, this problem manifests itself in user tests in the most obtuse way possible...

          Date: Mon, 11 Apr 2016 12:29:42 -0400
          From: Joe Lawson
          Subject: Solr 6 - AbstractSolrTestCase Error Unable to build KeyStore from file: null
          
          I'm upgrading a plugin and use the AbstractSolrTestCase for tests. My tests
          work fine in 5.X but when I upgraded to 6.X the tests sometimes throw an
          error during initialization. Basically it says,
          "org.apache.solr.common.SolrException: Error instantiating
          shardHandlerFactory class
          [org.apache.solr.handler.component.HttpShardHandlerFactory]: Unable to
          build KeyStore from file: null"
          
          ...
          
          > NOTE: reproduce with: ant test  -Dtestcase=TestConstructedPhrases
          >> -Dtests.seed=48D5F3D29EAB417 -Dtests.locale=en
          >> -Dtests.timezone=America/Blanc-Sablon -Dtests.asserts=true
          >> -Dtests.file.encoding=UTF-8
          >
          >
          >> org.apache.solr.common.SolrException: Error instantiating
          >> shardHandlerFactory class
          >> [org.apache.solr.handler.component.HttpShardHandlerFactory]: Unable to
          >> build KeyStore from file: null
          >
          >
          >> at __randomizedtesting.SeedInfo.seed([48D5F3D29EAB417]:0)
          >
          > at
          >> org.apache.solr.handler.component.ShardHandlerFactory.newInstance(ShardHandlerFactory.java:52)
          >
          > at org.apache.solr.core.CoreContainer.load(CoreContainer.java:404)
          >
          > at org.apache.solr.util.TestHarness.<init>(TestHarness.java:164)
          >
          > at org.apache.solr.util.TestHarness.<init>(TestHarness.java:127)
          >
          > at org.apache.solr.util.TestHarness.<init>(TestHarness.java:133)
          >
          > at org.apache.solr.util.TestHarness.<init>(TestHarness.java:96)
          >
          > at org.apache.solr.SolrTestCaseJ4.createCore(SolrTestCaseJ4.java:598)
          >
          > at org.apache.solr.SolrTestCaseJ4.initCore(SolrTestCaseJ4.java:588)
          >
          > at org.apache.solr.SolrTestCaseJ4.initCore(SolrTestCaseJ4.java:430)
          >
          > at org.apache.solr.SolrTestCaseJ4.initCore(SolrTestCaseJ4.java:419)
          >
          > at
          >> org.apache.solr.search.HonLuceneSynonymTestCase.beforeClass(HonLuceneSynonymTestCase.java:36)
          
          Show
          hossman Hoss Man added a comment - as things stand now, combined with SOLR-8971 , this problem manifests itself in user tests in the most obtuse way possible... Date: Mon, 11 Apr 2016 12:29:42 -0400 From: Joe Lawson Subject: Solr 6 - AbstractSolrTestCase Error Unable to build KeyStore from file: null I'm upgrading a plugin and use the AbstractSolrTestCase for tests. My tests work fine in 5.X but when I upgraded to 6.X the tests sometimes throw an error during initialization. Basically it says, "org.apache.solr.common.SolrException: Error instantiating shardHandlerFactory class [org.apache.solr.handler.component.HttpShardHandlerFactory]: Unable to build KeyStore from file: null" ... > NOTE: reproduce with: ant test -Dtestcase=TestConstructedPhrases >> -Dtests.seed=48D5F3D29EAB417 -Dtests.locale=en >> -Dtests.timezone=America/Blanc-Sablon -Dtests.asserts=true >> -Dtests.file.encoding=UTF-8 > > >> org.apache.solr.common.SolrException: Error instantiating >> shardHandlerFactory class >> [org.apache.solr.handler.component.HttpShardHandlerFactory]: Unable to >> build KeyStore from file: null > > >> at __randomizedtesting.SeedInfo.seed([48D5F3D29EAB417]:0) > > at >> org.apache.solr.handler.component.ShardHandlerFactory.newInstance(ShardHandlerFactory.java:52) > > at org.apache.solr.core.CoreContainer.load(CoreContainer.java:404) > > at org.apache.solr.util.TestHarness.<init>(TestHarness.java:164) > > at org.apache.solr.util.TestHarness.<init>(TestHarness.java:127) > > at org.apache.solr.util.TestHarness.<init>(TestHarness.java:133) > > at org.apache.solr.util.TestHarness.<init>(TestHarness.java:96) > > at org.apache.solr.SolrTestCaseJ4.createCore(SolrTestCaseJ4.java:598) > > at org.apache.solr.SolrTestCaseJ4.initCore(SolrTestCaseJ4.java:588) > > at org.apache.solr.SolrTestCaseJ4.initCore(SolrTestCaseJ4.java:430) > > at org.apache.solr.SolrTestCaseJ4.initCore(SolrTestCaseJ4.java:419) > > at >> org.apache.solr.search.HonLuceneSynonymTestCase.beforeClass(HonLuceneSynonymTestCase.java:36)
          Hide
          joekiller Joseph Lawson added a comment -

          It would be nice if the keystore blob was included in the test jar so that reusing SolrTestCaseJ4 in projects wouldn't require much setup beyond extending or just using the class. Letting it be overridden via sysprops is good too.

          Show
          joekiller Joseph Lawson added a comment - It would be nice if the keystore blob was included in the test jar so that reusing SolrTestCaseJ4 in projects wouldn't require much setup beyond extending or just using the class. Letting it be overridden via sysprops is good too.
          Hide
          joekiller Joseph Lawson added a comment -

          Also this affects 6.0

          Show
          joekiller Joseph Lawson added a comment - Also this affects 6.0
          Hide
          hossman Hoss Man added a comment -

          It would be nice if the keystore blob was included in the test jar...

          Agreed, but I think that would be a lot trickier? ... i'm pretty sure the way the keystore field is currently used in tests involves letting jetty load the file itself via a path specified in the jetty.xml test config? in any case – adding a sysprop to override seems like a good first step for now.

          I'm attaching a patch with what i've got so far – but i'm putting this on the back burner until i verify it's working sanely with clientAuth ... AFAICT at the moment, the clientAuth randomization isn't actually resulting in clientAuth being required anywhere!

          Show
          hossman Hoss Man added a comment - It would be nice if the keystore blob was included in the test jar... Agreed, but I think that would be a lot trickier? ... i'm pretty sure the way the keystore field is currently used in tests involves letting jetty load the file itself via a path specified in the jetty.xml test config? in any case – adding a sysprop to override seems like a good first step for now. I'm attaching a patch with what i've got so far – but i'm putting this on the back burner until i verify it's working sanely with clientAuth ... AFAICT at the moment, the clientAuth randomization isn't actually resulting in clientAuth being required anywhere!
          Hide
          hossman Hoss Man added a comment -

          updated patch to compile against master – but after working on other SSL test related issues and getting more familiar with the code i realize what Joseph suggested isn't hard at all – it would just require a bit of refactoring in how SSLConfig.createContextFactory works so that SSLTestConfig can override the the method completely and produce it's own KeyStore object (instead of a path). All the plumbing to load KeyStore objects from resources files in the classpath already exists.

          I'll look into this soon.

          Show
          hossman Hoss Man added a comment - updated patch to compile against master – but after working on other SSL test related issues and getting more familiar with the code i realize what Joseph suggested isn't hard at all – it would just require a bit of refactoring in how SSLConfig.createContextFactory works so that SSLTestConfig can override the the method completely and produce it's own KeyStore object (instead of a path). All the plumbing to load KeyStore objects from resources files in the classpath already exists. I'll look into this soon.
          Hide
          joekiller Joseph Lawson added a comment -

          Thanks for sticking with this.

          Show
          joekiller Joseph Lawson added a comment - Thanks for sticking with this.
          Hide
          hossman Hoss Man added a comment -

          updated patch that goes the direction Joseph suggested, instead of a sys prop override, there's not a copy of the cert in the resources directory that's used so it can always be found.

          still running full test, but I'm pretty happy with this and plan to move forward as soon as i see precommit pass.

          Show
          hossman Hoss Man added a comment - updated patch that goes the direction Joseph suggested, instead of a sys prop override, there's not a copy of the cert in the resources directory that's used so it can always be found. still running full test, but I'm pretty happy with this and plan to move forward as soon as i see precommit pass.
          Hide
          hossman Hoss Man added a comment -
          Show
          hossman Hoss Man added a comment - Current jira spam counter messures are probably preventing gitbot comments... http://git-wip-us.apache.org/repos/asf/lucene-solr/commit/76063648 http://git-wip-us.apache.org/repos/asf/lucene-solr/commit/1d7094c9
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 6942fe2d202a0345c4baf1b6292be7d8d5fd2f9e in lucene-solr's branch refs/heads/master from Steve Rowe
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=6942fe2 ]

          SOLR-8970: IntelliJ config: add src/resources/ as a java-resource dir to the solr-test-framework module, so that resources there get copied into the compilation output dir.

          Show
          jira-bot ASF subversion and git services added a comment - Commit 6942fe2d202a0345c4baf1b6292be7d8d5fd2f9e in lucene-solr's branch refs/heads/master from Steve Rowe [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=6942fe2 ] SOLR-8970 : IntelliJ config: add src/resources/ as a java-resource dir to the solr-test-framework module, so that resources there get copied into the compilation output dir.
          Hide
          steve_rowe Steve Rowe added a comment -

          Manually adding info here for the branch_6x commit I did for the IntelliJ fixes, which for some reason didn't get autocommented here - from the commit email to commits@l.a.o:

          Repository: lucene-solr
          Updated Branches:
          refs/heads/branch_6x f73997bb4 -> 29e7d64da

          SOLR-8970: IntelliJ config: add src/resources/ as a java-resource dir to the solr-test-framework module, so that resources there get copied into the compilation output dir.

          Project: http://git-wip-us.apache.org/repos/asf/lucene-solr/repo
          Commit: http://git-wip-us.apache.org/repos/asf/lucene-solr/commit/29e7d64d
          Tree: http://git-wip-us.apache.org/repos/asf/lucene-solr/tree/29e7d64d
          Diff: http://git-wip-us.apache.org/repos/asf/lucene-solr/diff/29e7d64d

          Branch: refs/heads/branch_6x
          Commit: 29e7d64da14a78bf8f1173a01d1553f69a27e9c7
          Parents: f73997b
          Author: Steve Rowe <sarowe@apache.org>
          Authored: Mon May 16 20:55:32 2016 -0400
          Committer: Steve Rowe <sarowe@apache.org>
          Committed: Mon May 16 20:56:15 2016 -0400

          Show
          steve_rowe Steve Rowe added a comment - Manually adding info here for the branch_6x commit I did for the IntelliJ fixes, which for some reason didn't get autocommented here - from the commit email to commits@l.a.o: Repository: lucene-solr Updated Branches: refs/heads/branch_6x f73997bb4 -> 29e7d64da SOLR-8970 : IntelliJ config: add src/resources/ as a java-resource dir to the solr-test-framework module, so that resources there get copied into the compilation output dir. Project: http://git-wip-us.apache.org/repos/asf/lucene-solr/repo Commit: http://git-wip-us.apache.org/repos/asf/lucene-solr/commit/29e7d64d Tree: http://git-wip-us.apache.org/repos/asf/lucene-solr/tree/29e7d64d Diff: http://git-wip-us.apache.org/repos/asf/lucene-solr/diff/29e7d64d Branch: refs/heads/branch_6x Commit: 29e7d64da14a78bf8f1173a01d1553f69a27e9c7 Parents: f73997b Author: Steve Rowe <sarowe@apache.org> Authored: Mon May 16 20:55:32 2016 -0400 Committer: Steve Rowe <sarowe@apache.org> Committed: Mon May 16 20:56:15 2016 -0400
          Hide
          steve_rowe Steve Rowe added a comment -

          Reopening to backport to 6.0.1.

          Show
          steve_rowe Steve Rowe added a comment - Reopening to backport to 6.0.1.
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 124301d69812e4b9a83c440c70736c6d301baf44 in lucene-solr's branch refs/heads/branch_6_0 from Chris Hostetter
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=124301d ]

          SOLR-8970: Change SSLTestConfig to use a keystore file that is included as a resource in the test-framework jar so users subclassing SolrTestCaseJ4 don't need to preserve magic paths

          (cherry picked from commit 76063648ae05a935459f2ea5ed53c4df1caa713d)

          Show
          jira-bot ASF subversion and git services added a comment - Commit 124301d69812e4b9a83c440c70736c6d301baf44 in lucene-solr's branch refs/heads/branch_6_0 from Chris Hostetter [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=124301d ] SOLR-8970 : Change SSLTestConfig to use a keystore file that is included as a resource in the test-framework jar so users subclassing SolrTestCaseJ4 don't need to preserve magic paths (cherry picked from commit 76063648ae05a935459f2ea5ed53c4df1caa713d)
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit f6117793024c626cf0b07f390112cdec99de4847 in lucene-solr's branch refs/heads/branch_6_0 from Steve Rowe
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=f611779 ]

          SOLR-8970: IntelliJ config: add src/resources/ as a java-resource dir to the solr-test-framework module, so that resources there get copied into the compilation output dir.

          Show
          jira-bot ASF subversion and git services added a comment - Commit f6117793024c626cf0b07f390112cdec99de4847 in lucene-solr's branch refs/heads/branch_6_0 from Steve Rowe [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=f611779 ] SOLR-8970 : IntelliJ config: add src/resources/ as a java-resource dir to the solr-test-framework module, so that resources there get copied into the compilation output dir.
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit c105675a90134b75d00cf109d5ae4383e44c09d0 in lucene-solr's branch refs/heads/branch_6_0 from Steve Rowe
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=c105675 ]

          SOLR-8970: branch_6_0: move CHANGES entry from 'Other Changes' section to 'Bug fixes'

          Show
          jira-bot ASF subversion and git services added a comment - Commit c105675a90134b75d00cf109d5ae4383e44c09d0 in lucene-solr's branch refs/heads/branch_6_0 from Steve Rowe [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=c105675 ] SOLR-8970 : branch_6_0: move CHANGES entry from 'Other Changes' section to 'Bug fixes'
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 4031d3fd469a7df58bc5642c7a3caec01aa6b23c in lucene-solr's branch refs/heads/branch_6_0 from Steve Rowe
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=4031d3f ]

          Revert "SOLR-8970: Change SSLTestConfig to use a keystore file that is included as a resource in the test-framework jar so users subclassing SolrTestCaseJ4 don't need to preserve magic paths"

          This reverts commit 124301d69812e4b9a83c440c70736c6d301baf44.

          Show
          jira-bot ASF subversion and git services added a comment - Commit 4031d3fd469a7df58bc5642c7a3caec01aa6b23c in lucene-solr's branch refs/heads/branch_6_0 from Steve Rowe [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=4031d3f ] Revert " SOLR-8970 : Change SSLTestConfig to use a keystore file that is included as a resource in the test-framework jar so users subclassing SolrTestCaseJ4 don't need to preserve magic paths" This reverts commit 124301d69812e4b9a83c440c70736c6d301baf44.
          Hide
          steve_rowe Steve Rowe added a comment -

          Reopening to deal with branch_6_0 test failures (mostly reset connections) apparently due to the backport of this issue - I've reverted the backport pending investigation.

          Show
          steve_rowe Steve Rowe added a comment - Reopening to deal with branch_6_0 test failures (mostly reset connections) apparently due to the backport of this issue - I've reverted the backport pending investigation.
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 3cc008253513977d1554de3be1c34f35bc4461ae in lucene-solr's branch refs/heads/branch_6_0 from Chris Hostetter
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=3cc0082 ]

          SOLR-8970: Change SSLTestConfig to use a keystore file that is included as a resource in the test-framework jar so users subclassing SolrTestCaseJ4 don't need to preserve magic paths

          (cherry picked from commit 76063648ae05a935459f2ea5ed53c4df1caa713d)

          Show
          jira-bot ASF subversion and git services added a comment - Commit 3cc008253513977d1554de3be1c34f35bc4461ae in lucene-solr's branch refs/heads/branch_6_0 from Chris Hostetter [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=3cc0082 ] SOLR-8970 : Change SSLTestConfig to use a keystore file that is included as a resource in the test-framework jar so users subclassing SolrTestCaseJ4 don't need to preserve magic paths (cherry picked from commit 76063648ae05a935459f2ea5ed53c4df1caa713d)
          Hide
          steve_rowe Steve Rowe added a comment -

          When I backported this issue to branch_6_0, I missed some commits under SOLR-5776 and SOLR-9068, because I was working off of CHANGES.txt, and those issues aren’t mentioned in there.

          I have now pushed all the necessary commits, and three local runs passed. I’ve included mention of all the related issues in the branch_6_0 CHANGES entry.

          Show
          steve_rowe Steve Rowe added a comment - When I backported this issue to branch_6_0, I missed some commits under SOLR-5776 and SOLR-9068 , because I was working off of CHANGES.txt, and those issues aren’t mentioned in there. I have now pushed all the necessary commits, and three local runs passed. I’ve included mention of all the related issues in the branch_6_0 CHANGES entry.
          Hide
          steve_rowe Steve Rowe added a comment -

          Bulk close issues included in the 6.0.1 release.

          Show
          steve_rowe Steve Rowe added a comment - Bulk close issues included in the 6.0.1 release.

            People

            • Assignee:
              hossman Hoss Man
              Reporter:
              hossman Hoss Man
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development