Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SSLConfigurationReloaderTests.testPEMTrustReloadException fails on Java 8 FIPS #39580

Closed
gwbrown opened this issue Mar 1, 2019 · 5 comments · Fixed by #40092
Closed

SSLConfigurationReloaderTests.testPEMTrustReloadException fails on Java 8 FIPS #39580

gwbrown opened this issue Mar 1, 2019 · 5 comments · Fixed by #40092
Assignees
Labels
:Security/TLS SSL/TLS, Certificates >test-failure Triaged test failures from CI

Comments

@gwbrown
Copy link
Contributor

gwbrown commented Mar 1, 2019

Example failure: https://elasticsearch-ci.elastic.co/job/elastic+elasticsearch+6.6+matrix-java-periodic/ES_BUILD_JAVA=java11,ES_RUNTIME_JAVA=java8fips,nodes=immutable&&linux&&docker/144/console

This test started failing on Feb. 27, and has failed (at time of writing) 27 times since, on 6.6, 6.7, 7.0, 7.x, and master. It appears to only be failing when running on Java 8 FIPS.

I haven't reproduced this myself since I don't have a FIPS JVM set up.

Reproduce line is:

./gradlew :x-pack:plugin:core:unitTest \
  -Dtests.seed=15827440A5F661CE \
  -Dtests.class=org.elasticsearch.xpack.core.ssl.SSLConfigurationReloaderTests \
  -Dtests.method="testPEMTrustReloadException" \
  -Dtests.security.manager=true \
  -Dtests.locale=ar-BH \
  -Dtests.timezone=America/Cancun \
  -Dcompiler.java=11 \
  -Druntime.java=8FIPS \
  -Djavax.net.ssl.keyStorePassword=password \
  -Djavax.net.ssl.trustStorePassword=password

Stack trace from one of the failures on Master:

java.lang.AssertionError
	at __randomizedtesting.SeedInfo.seed([CA9D5FCC489B6DA4:85CDFD5FAC7BE967]:0)
	at org.junit.Assert.fail(Assert.java:86)
	at org.junit.Assert.assertTrue(Assert.java:41)
	at org.junit.Assert.assertNotNull(Assert.java:712)
	at org.junit.Assert.assertNotNull(Assert.java:722)
	at org.elasticsearch.xpack.core.ssl.SSLConfigurationReloaderTests.testPEMTrustReloadException(SSLConfigurationReloaderTests.java:501)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at com.carrotsearch.randomizedtesting.RandomizedRunner.invoke(RandomizedRunner.java:1750)
	at com.carrotsearch.randomizedtesting.RandomizedRunner$8.evaluate(RandomizedRunner.java:938)
	at com.carrotsearch.randomizedtesting.RandomizedRunner$9.evaluate(RandomizedRunner.java:974)
	at com.carrotsearch.randomizedtesting.RandomizedRunner$10.evaluate(RandomizedRunner.java:988)
	at com.carrotsearch.randomizedtesting.rules.StatementAdapter.evaluate(StatementAdapter.java:36)
	at org.apache.lucene.util.TestRuleSetupTeardownChained$1.evaluate(TestRuleSetupTeardownChained.java:49)
	at org.apache.lucene.util.AbstractBeforeAfterRule$1.evaluate(AbstractBeforeAfterRule.java:45)
	at org.apache.lucene.util.TestRuleThreadAndTestName$1.evaluate(TestRuleThreadAndTestName.java:48)
	at org.apache.lucene.util.TestRuleIgnoreAfterMaxFailures$1.evaluate(TestRuleIgnoreAfterMaxFailures.java:64)
	at org.apache.lucene.util.TestRuleMarkFailure$1.evaluate(TestRuleMarkFailure.java:47)
	at com.carrotsearch.randomizedtesting.rules.StatementAdapter.evaluate(StatementAdapter.java:36)
	at com.carrotsearch.randomizedtesting.ThreadLeakControl$StatementRunner.run(ThreadLeakControl.java:368)
	at com.carrotsearch.randomizedtesting.ThreadLeakControl.forkTimeoutingTask(ThreadLeakControl.java:817)
	at com.carrotsearch.randomizedtesting.ThreadLeakControl$3.evaluate(ThreadLeakControl.java:468)
	at com.carrotsearch.randomizedtesting.RandomizedRunner.runSingleTest(RandomizedRunner.java:947)
	at com.carrotsearch.randomizedtesting.RandomizedRunner$5.evaluate(RandomizedRunner.java:832)
	at com.carrotsearch.randomizedtesting.RandomizedRunner$6.evaluate(RandomizedRunner.java:883)
	at com.carrotsearch.randomizedtesting.RandomizedRunner$7.evaluate(RandomizedRunner.java:894)
	at org.apache.lucene.util.AbstractBeforeAfterRule$1.evaluate(AbstractBeforeAfterRule.java:45)
	at com.carrotsearch.randomizedtesting.rules.StatementAdapter.evaluate(StatementAdapter.java:36)
	at org.apache.lucene.util.TestRuleStoreClassName$1.evaluate(TestRuleStoreClassName.java:41)
	at com.carrotsearch.randomizedtesting.rules.NoShadowingOrOverridesOnMethodsRule$1.evaluate(NoShadowingOrOverridesOnMethodsRule.java:40)
	at com.carrotsearch.randomizedtesting.rules.NoShadowingOrOverridesOnMethodsRule$1.evaluate(NoShadowingOrOverridesOnMethodsRule.java:40)
	at com.carrotsearch.randomizedtesting.rules.StatementAdapter.evaluate(StatementAdapter.java:36)
	at com.carrotsearch.randomizedtesting.rules.StatementAdapter.evaluate(StatementAdapter.java:36)
	at com.carrotsearch.randomizedtesting.rules.StatementAdapter.evaluate(StatementAdapter.java:36)
	at org.apache.lucene.util.TestRuleAssertionsRequired$1.evaluate(TestRuleAssertionsRequired.java:53)
	at org.apache.lucene.util.TestRuleMarkFailure$1.evaluate(TestRuleMarkFailure.java:47)
	at org.apache.lucene.util.TestRuleIgnoreAfterMaxFailures$1.evaluate(TestRuleIgnoreAfterMaxFailures.java:64)
	at org.apache.lucene.util.TestRuleIgnoreTestSuites$1.evaluate(TestRuleIgnoreTestSuites.java:54)
	at com.carrotsearch.randomizedtesting.rules.StatementAdapter.evaluate(StatementAdapter.java:36)
	at com.carrotsearch.randomizedtesting.ThreadLeakControl$StatementRunner.run(ThreadLeakControl.java:368)
	at java.lang.Thread.run(Thread.java:748)

Not muting this since it doesn't appear to be impacting PRs.

@gwbrown gwbrown added >test-failure Triaged test failures from CI :Security/TLS SSL/TLS, Certificates labels Mar 1, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-security

@jkakavas jkakavas self-assigned this Mar 1, 2019
@jkakavas
Copy link
Member

jkakavas commented Mar 1, 2019

This does reproduce in a FIPS JVM and has to do with the changes introduced in #39408 . We didn't pick this up since we don't run tests in a FIPS JVM in PRs. I will investigate

@jkakavas
Copy link
Member

jkakavas commented Mar 1, 2019

This has to do with how certificates are read from files in the default java security provider and the security provider offered by BouncyCastle. The former reads the certificate from file in sun.security.provider.X509Provider with

    private Collection<? extends java.security.cert.Certificate>
        parseX509orPKCS7Cert(InputStream is)
        throws CertificateException, IOException
    {
        int peekByte;
        byte[] data;
        PushbackInputStream pbis = new PushbackInputStream(is);
        Collection<X509CertImpl> coll = new ArrayList<>();

        // Test the InputStream for end-of-stream.  If the stream's
        // initial state is already at end-of-stream then return
        // an empty collection.  Otherwise, push the byte back into the
        // stream and let readOneBlock look for the first certificate.
        peekByte = pbis.read();
        if (peekByte == -1) {
            return new ArrayList<>(0);
        } else {
            pbis.unread(peekByte);
            data = readOneBlock(pbis);
        }

        // If we end up with a null value after reading the first block
        // then we know the end-of-stream has been reached and no certificate
        // data has been found.
        if (data == null) {
            throw new CertificateException("No certificate data found"); // It throws here in our test !
        }

        try {
            PKCS7 pkcs7 = new PKCS7(data);
            X509Certificate[] certs = pkcs7.getCertificates();
            // certs are optional in PKCS #7
            if (certs != null) {
                return Arrays.asList(certs);
            } else {
                // no certificates provided
                return new ArrayList<>(0);
            }
        } catch (ParsingException e) {
            while (data != null) {
                coll.add(new X509CertImpl(data));
                data = readOneBlock(pbis);
            }
        }
        return coll;
    }

which throws an exception for the malformed file, while the latter reads the certificate from file in org.bouncycastle.jcajce.provider.CertificateFactory with

   public Collection engineGenerateCertificates(InputStream inStream) throws CertificateException {
        List certs = new ArrayList();
        BufferedInputStream in = new BufferedInputStream(inStream);

        Certificate certificate;
        while((certificate = this.readCertificate(in)) != null) {
            certs.add(certificate);
        }

        return certs;
    }

that returns an empty Arraylist and doesn't throw.

@tvernum
Copy link
Contributor

tvernum commented Mar 3, 2019

In ssl-config we worked around this by failing if the cert parsing returned and empty list:

@tvernum
Copy link
Contributor

tvernum commented Mar 4, 2019

I'm going to mute this on FIPS.

tvernum added a commit to tvernum/elasticsearch that referenced this issue Mar 4, 2019
tvernum added a commit that referenced this issue Mar 4, 2019
tvernum added a commit that referenced this issue Mar 4, 2019
tvernum added a commit that referenced this issue Mar 4, 2019
tvernum added a commit that referenced this issue Mar 4, 2019
tvernum added a commit that referenced this issue Mar 4, 2019
jkakavas added a commit to jkakavas/elasticsearch that referenced this issue Mar 15, 2019
With SUN security provider, a CertificateException is thrown when
attempting to parse a Certificate from a PEM file on disk with
`sun.security.provider.X509Provider#parseX509orPKCS7Cert`

When using the BouncyCastle Security provider (as we do in fips
tests) the parsing happens in
CertificateFactory#engineGenerateCertificates which doesn't throw
an exception but returns an empty list.

In order to have a consistent behavior, this change makes it so
that we throw a CertificateException when attempting to read
a PEM file from disk and failing to do so in either Security
Provider

Resolves: elastic#39580
jkakavas added a commit that referenced this issue Mar 18, 2019
With SUN security provider, a CertificateException is thrown when
attempting to parse a Certificate from a PEM file on disk with
`sun.security.provider.X509Provider#parseX509orPKCS7Cert`

When using the BouncyCastle Security provider (as we do in fips
tests) the parsing happens in
CertificateFactory#engineGenerateCertificates which doesn't throw
an exception but returns an empty list.

In order to have a consistent behavior, this change makes it so
that we throw a CertificateException when attempting to read
a PEM file from disk and failing to do so in either Security
Provider

Resolves: #39580
jkakavas added a commit that referenced this issue Mar 18, 2019
With SUN security provider, a CertificateException is thrown when
attempting to parse a Certificate from a PEM file on disk with
`sun.security.provider.X509Provider#parseX509orPKCS7Cert`

When using the BouncyCastle Security provider (as we do in fips
tests) the parsing happens in
CertificateFactory#engineGenerateCertificates which doesn't throw
an exception but returns an empty list.

In order to have a consistent behavior, this change makes it so
that we throw a CertificateException when attempting to read
a PEM file from disk and failing to do so in either Security
Provider

Resolves: #39580
jkakavas added a commit that referenced this issue Mar 18, 2019
With SUN security provider, a CertificateException is thrown when
attempting to parse a Certificate from a PEM file on disk with
`sun.security.provider.X509Provider#parseX509orPKCS7Cert`

When using the BouncyCastle Security provider (as we do in fips
tests) the parsing happens in
CertificateFactory#engineGenerateCertificates which doesn't throw
an exception but returns an empty list.

In order to have a consistent behavior, this change makes it so
that we throw a CertificateException when attempting to read
a PEM file from disk and failing to do so in either Security
Provider

Resolves: #39580
jkakavas added a commit that referenced this issue Mar 18, 2019
With SUN security provider, a CertificateException is thrown when
attempting to parse a Certificate from a PEM file on disk with
`sun.security.provider.X509Provider#parseX509orPKCS7Cert`

When using the BouncyCastle Security provider (as we do in fips
tests) the parsing happens in
CertificateFactory#engineGenerateCertificates which doesn't throw
an exception but returns an empty list.

In order to have a consistent behavior, this change makes it so
that we throw a CertificateException when attempting to read
a PEM file from disk and failing to do so in either Security
Provider

Resolves: #39580
jkakavas added a commit that referenced this issue Mar 18, 2019
With SUN security provider, a CertificateException is thrown when
attempting to parse a Certificate from a PEM file on disk with
`sun.security.provider.X509Provider#parseX509orPKCS7Cert`

When using the BouncyCastle Security provider (as we do in fips
tests) the parsing happens in
CertificateFactory#engineGenerateCertificates which doesn't throw
an exception but returns an empty list.

In order to have a consistent behavior, this change makes it so
that we throw a CertificateException when attempting to read
a PEM file from disk and failing to do so in either Security
Provider

Resolves: #39580
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Security/TLS SSL/TLS, Certificates >test-failure Triaged test failures from CI
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants