Conscrypt's SSLContext fails when given a SunJSSE TrustManager.

[fbacchella]

The following tests fails:

import java.io.FileInputStream;
import java.io.IOException;
import java.io.InputStream;
import java.net.URL;
import java.security.KeyManagementException;
import java.security.KeyStore;
import java.security.KeyStoreException;
import java.security.NoSuchAlgorithmException;
import java.security.NoSuchProviderException;
import java.security.Provider;
import java.security.SecureRandom;
import java.security.Security;
import java.security.UnrecoverableKeyException;
import java.security.cert.CertificateException;

import javax.net.ssl.HttpsURLConnection;
import javax.net.ssl.KeyManager;
import javax.net.ssl.KeyManagerFactory;
import javax.net.ssl.SSLContext;
import javax.net.ssl.TrustManager;
import javax.net.ssl.TrustManagerFactory;

import org.conscrypt.Conscrypt;
import org.junit.BeforeClass;
import org.junit.Test;

public class TestTLS13 {

    @BeforeClass
    public static void loadconscrypt() {
        Provider p = Conscrypt.newProviderBuilder().build();
        Security.insertProviderAt(p, Security.getProviders().length);
    }

    private void prepare(String trustProvider, String tmftype, String contextProvider) throws NoSuchAlgorithmException, CertificateException, IOException, KeyStoreException, NoSuchProviderException, UnrecoverableKeyException, KeyManagementException {
        String javahome = System.getProperty("java.home");
        String cacertspath = javahome + "/lib/security/cacerts";

        KeyStore ks = KeyStore.getInstance("JKS", "SUN");
        ks.load(new FileInputStream(cacertspath), "changeit".toCharArray());

        TrustManagerFactory tmf = TrustManagerFactory.getInstance(tmftype, trustProvider);
        tmf.init(ks);
        TrustManager[] tm = tmf.getTrustManagers();

        KeyManagerFactory kmf = KeyManagerFactory.getInstance("NewSunX509", "SunJSSE");
        kmf.init(ks,  new char[] {});
        KeyManager[] km = kmf.getKeyManagers();

        SecureRandom sr = SecureRandom.getInstance("NativePRNGNonBlocking");

        SSLContext ctxt = SSLContext.getInstance("TLSv1.3", contextProvider);
        ctxt.init(km, tm, sr);

        URL url = new URL("https://www.google.com");
        HttpsURLConnection ctx = (HttpsURLConnection) url.openConnection();
        ctx.setSSLSocketFactory(ctxt.getSocketFactory());
        InputStream o = (InputStream) ctx.getContent();
        o.readAllBytes();
    }

    @Test
    public void testBase() throws NoSuchAlgorithmException, CertificateException, KeyStoreException, NoSuchProviderException, IOException, UnrecoverableKeyException, KeyManagementException {
        prepare("SunJSSE", "SunX509", "SunJSSE");
    }

    @Test
    public void testConscrypt() throws NoSuchAlgorithmException, CertificateException, KeyStoreException, NoSuchProviderException, IOException, UnrecoverableKeyException, KeyManagementException {
        prepare("Conscrypt", "PKIX", "Conscrypt");
    }

    @Test
    public void testMixed() throws NoSuchAlgorithmException, CertificateException, KeyStoreException, NoSuchProviderException, IOException, UnrecoverableKeyException, KeyManagementException {
        prepare("SunJSSE", "SunX509", "Conscrypt");
    }
}

To be exact, only the testMixed fails, with the exception:

javax.net.ssl.SSLHandshakeException: Unknown authType: GENERIC
	at org.conscrypt.SSLUtils.toSSLHandshakeException(SSLUtils.java:361)
	at org.conscrypt.ConscryptEngine.convertException(ConscryptEngine.java:1138)
	at org.conscrypt.ConscryptEngine.readPlaintextData(ConscryptEngine.java:1093)
	at org.conscrypt.ConscryptEngine.unwrap(ConscryptEngine.java:880)
	at org.conscrypt.ConscryptEngine.unwrap(ConscryptEngine.java:751)
	at org.conscrypt.ConscryptEngine.unwrap(ConscryptEngine.java:716)
	at org.conscrypt.ConscryptEngineSocket$SSLInputStream.processDataFromSocket(ConscryptEngineSocket.java:833)
	at org.conscrypt.ConscryptEngineSocket$SSLInputStream.access$100(ConscryptEngineSocket.java:706)
	at org.conscrypt.ConscryptEngineSocket.doHandshake(ConscryptEngineSocket.java:230)
	at org.conscrypt.ConscryptEngineSocket.startHandshake(ConscryptEngineSocket.java:209)
	at org.conscrypt.ConscryptEngineSocket.waitForHandshake(ConscryptEngineSocket.java:547)
	at org.conscrypt.ConscryptEngineSocket.getOutputStream(ConscryptEngineSocket.java:290)
	at java.base/sun.net.www.http.HttpClient.openServer(HttpClient.java:499)
	at java.base/sun.net.www.http.HttpClient.openServer(HttpClient.java:600)
	at java.base/sun.net.www.protocol.https.HttpsClient.<init>(HttpsClient.java:265)
	at java.base/sun.net.www.protocol.https.HttpsClient.New(HttpsClient.java:379)
	at java.base/sun.net.www.protocol.https.AbstractDelegateHttpsURLConnection.getNewHttpClient(AbstractDelegateHttpsURLConnection.java:189)
	at java.base/sun.net.www.protocol.http.HttpURLConnection.plainConnect0(HttpURLConnection.java:1232)
	at java.base/sun.net.www.protocol.http.HttpURLConnection.plainConnect(HttpURLConnection.java:1120)
	at java.base/sun.net.www.protocol.https.AbstractDelegateHttpsURLConnection.connect(AbstractDelegateHttpsURLConnection.java:175)
	at java.base/sun.net.www.protocol.http.HttpURLConnection.getInputStream0(HttpURLConnection.java:1653)
	at java.base/sun.net.www.protocol.http.HttpURLConnection.getInputStream(HttpURLConnection.java:1577)
	at java.base/java.net.URLConnection.getContent(URLConnection.java:752)
	at java.base/sun.net.www.protocol.https.HttpsURLConnectionImpl.getContent(HttpsURLConnectionImpl.java:404)
	at fr.jrds.TestTLS13.prepare(TestTLS13.java:60)
	at fr.jrds.TestTLS13.testMixed(TestTLS13.java:76)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:78)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:567)
	at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:59)
	at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
	at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:56)
	at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
	at org.junit.runners.ParentRunner$3.evaluate(ParentRunner.java:306)
	at org.junit.runners.BlockJUnit4ClassRunner$1.evaluate(BlockJUnit4ClassRunner.java:100)
	at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:366)
	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:103)
	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:63)
	at org.junit.runners.ParentRunner$4.run(ParentRunner.java:331)
	at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:79)
	at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:329)
	at org.junit.runners.ParentRunner.access$100(ParentRunner.java:66)
	at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:293)
	at org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:26)
	at org.junit.runners.ParentRunner$3.evaluate(ParentRunner.java:306)
	at org.junit.runners.ParentRunner.run(ParentRunner.java:413)
	at org.eclipse.jdt.internal.junit4.runner.JUnit4TestReference.run(JUnit4TestReference.java:93)
	at org.eclipse.jdt.internal.junit.runner.TestExecution.run(TestExecution.java:40)
	at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:529)
	at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:756)
	at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.run(RemoteTestRunner.java:452)
	at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.main(RemoteTestRunner.java:210)
Caused by: java.security.cert.CertificateException: Unknown authType: GENERIC
	at java.base/sun.security.validator.EndEntityChecker.checkTLSServer(EndEntityChecker.java:297)
	at java.base/sun.security.validator.EndEntityChecker.check(EndEntityChecker.java:152)
	at java.base/sun.security.validator.Validator.validate(Validator.java:277)
	at java.base/sun.security.ssl.X509TrustManagerImpl.checkTrusted(X509TrustManagerImpl.java:231)
	at java.base/sun.security.ssl.X509TrustManagerImpl.checkServerTrusted(X509TrustManagerImpl.java:132)
	at org.conscrypt.ConscryptEngineSocket$2.checkServerTrusted(ConscryptEngineSocket.java:156)
	at org.conscrypt.Platform.checkServerTrusted(Platform.java:330)
	at org.conscrypt.ConscryptEngine.verifyCertificateChain(ConscryptEngine.java:1643)
	at org.conscrypt.NativeCrypto.ENGINE_SSL_read_direct(Native Method)
	at org.conscrypt.NativeSsl.readDirectByteBuffer(NativeSsl.java:567)
	at org.conscrypt.ConscryptEngine.readPlaintextDataDirect(ConscryptEngine.java:1099)
	at org.conscrypt.ConscryptEngine.readPlaintextData(ConscryptEngine.java:1083)
	... 50 more

The problem is a SSLContext from the Conscrypt provider is used, with a TrustManager from SunJSSE, the default provider used.

But when I explicitly ensure than the TrustManager is comming from SSLContext, everything is OK.

The problem only arise withe the SSLContext is using the TLSv1.3. With TLSv1.2, everything is fine.

I tested that with Java 16 and Conscrypt 2.5.2.

Is that a bug or a feature ?

[prbprbprb]

I guess it's a bug... The authType parameter to checkServerTrusted() isn't well defined for TLS 1.3 due to the authentication changes.

BoringSSL and thus Conscrypt went with GENERIC but OpenJDK went with UNKNOWN (which to my mind is even less descriptive) (tested on OpenJDK 11 & 16). Unfortunately it matters when using a SunJSSE TrustManager as it uses this parameter to determine which key usage bits need to be set on the server certificate based on hard coded lists.

Changing Conscrypt is an option but with limitations... Once Android shipped with TLS1.3 as default, we got a number of bugs from apps with custom trust managers which didn't handle GENERIC as an authType, so if we change it to UNKNOWN we'll break those apps all over again.

On the plus side, this call is abstracted through platform-specific code, so we could rewrite GENERIC to UNKNOWN for OpenJDK only with hopefully less breakage or make it configurable.

In the meantime, you could achieve the same thing yourself with a delegating trust manager, e.g.

        TrustManagerFactory tmf = TrustManagerFactory.getInstance(tmftype, trustProvider);
        tmf.init(ks);
        TrustManager[] tm = tmf.getTrustManagers();
        TrustManager[] delegateTm = new TrustManager[] {
                // You should deal with the case where there's more than 1 ;)
                new DelegatingTrustManager((X509ExtendedTrustManager) tm[0])
        };

        [...]

        private static class DelegatingTrustManager extends X509ExtendedTrustManager {
            private final X509ExtendedTrustManager delegate;

            public DelegatingTrustManager(X509ExtendedTrustManager delegate) {
                this.delegate = delegate;
            }

            public void checkServerTrusted(X509Certificate[] chain, String authType, Socket socket) throws CertificateException {
                if ("GENERIC".equals(authType)) {
                    authType = "UNKNOWN";
                }
                delegate.checkServerTrusted(chain, authType, socket);
            }
           // etc - don't forget to delegate all methods
    }

[davidben]

Oh lovely. If KEMTLS becomes a thing, it may even need different key usage bit. It's a pity they didn't just make the API specify the desired key usage bit directly. (Or leave EE key usage enforcement to the certificate user. BoringSSL checks EE key usage itself and doesn't depend on X.509 doing it.) So that means "UNKNOWN" and "GENERIC" do not actually mean unknown/generic but signing key. (That this level of key usage is a separate field rather than implicit from the key type is, itself, a mistake in X.509. But we're stuck with that one for ECDSA and RSA.)

This is quite janky, but another possibility could be to pick one of "ECDHE_RSA" or "ECDHE_ECDSA", the TLS 1.2 values. That should be pretty compatible.

But the risk is yet other TrustManager implementations could be filtering the public key type based on the second half of the string. So maybe we need to select the string based on the key type of the leaf certificate. Except that only works for RSA and ECDSA keys, which is all we need to care about for compatibility but may change in the future.

Maybe we could do:

• If RSA or ECDSA, pick "ECDSA_RSA" or "ECDHE_ECDSA" accordingly. Hopefully that won't break anything. Risk: someone might be rejecting the TLS 1.2 values as a way of enforcing TLS 1.3. But this is a silly way to enforce TLS 1.3 when you can just disable the protocol.
• Otherwise, pick "UNKNOWN" to match OpenJDK I guess? RSA/ECDSA is all that exists right now (well, BoringSSL supports Ed25519, but I doubt anyone's using it), so this shouldn't break Android apps. Risk: if we do add a new key type later, those Android apps might need some more churn. Though if that new key type is a KEM, we'll have excitement here anyway.

[hessjcg]

Folks, this is a really bad bug. Conscrypt breaks the API contract created by the OpenJDK JCE implementation. We had to add a big workaround to our Cloud SQL Java Connector to account for this bug: GoogleCloudPlatform/cloud-sql-jdbc-socket-factory#1993

I vote to make Conscrypt work like OpenJDK when it runs on OpenJDK. That way, this library is truly a drop-in replacement for the OpenJDK default JCE.

[prbprbprb]

Yeah, we should have fixed this sooner, although it hasn't surfaced much as these days people tend to use Conscrypt's own TrustManager on OpenJDK.

I vote to make Conscrypt work like OpenJDK when it runs on OpenJDK. That way, this library is truly a drop-in replacement for the OpenJDK default JCE.

Kind of. If we do that then it's a potentially breaking change for applications that already have a custom TrustManager which is expecting GENERIC. I think what we'll need to do is select a default value based on the actual class of the TrustManager (Conscrypt vs SunJSSE) so that things Just Work for either of those. We can then also change the default to UNKNOWN on OpenJDK but with a mechanism to allow existing apps to overide it back to GENERIC such as a Property.

[mperktold]

In the meantime, you could achieve the same thing yourself with a delegating trust manager, e.g.

In your snippet, you override one overload of checkServerTrusted, where you map "GENERIC" to "UNKNOWN" before delegating, and you write that the other methods must delegate as well.

Could you please clarify: To achieve OpenJDK behavior, should all checkServerTrusted and checkClientTrueted overloads do this mapping, or only the checkServerTrusted overloads, or just the specific one you showed?