Add method to client builder to use default SSLContext

[DSmithVA]

Running on OpenJDK as an example, this behavior split violates the least-surprise principle IMO:

1. utilizing default java trust that comes free with JVM - great
2. ignoring default java identity that comes free with JVM - frustrating

I know some platforms/runtimes don't have this javax.net.ssl.keyStore identity concept available, but a builder flag like useDefaultSslContext() would be fantastic in any case. That would internally just use SSLContext.getDefault() and leave its trust/identity as-is, so API users don't have to reimplement the same boilerplate over and over. If that sounds like an acceptable path I'd be happy to work on a PR.

The existing builder sslSocketFactory() API accepts an SSLSocketFactory/TrustManager, this change would internalize the boilerplate.

Related, this documentation is.. questionable:

fun sslSocketFactory(sslSocketFactory: <Error class: unknown class>, trustManager: <Error class: unknown class>): OkHttpClient.Builder Sets the socket factory and trust manager used to secure HTTPS connections. If unset, the system defaults will be used.

I would think the system default referenced here implies SSLContext.getDefault().getSocketFactory() or SSLSocketFactory.getDefault(), which is not the case.

[yschimke]

Yeah, I think we should definitely clarify the docs.

My understanding is that the main differences between SSLContextgetInstance("TLSv1.3") and SSLContext.getDefault(), are that the latter will use the value configured to SSLContext.setDefault(myCtxt) and is influenced by the system properties you mention.

I can definitely see the argument that OkHttp should use the JVM defaults.

But I'm not sure if we want to either a) change the current logic, since OkHttp wants to start with a clean slate and even set TLS Versions and Ciphersuites, i.e. it's opinionated. b) add more configuration, since that's what the current options are for.

cc @swankjesse

[yschimke]

Seems like a deliberate change in 2014 https://github.com/square/okhttp/pull/518

in response to https://github.com/square/okhttp/issues/184

[DSmithVA]

Maybe a public not-marked-internal helper method exposing the X509TrustManager? There's already code in okhttp to get it after all. Then JVM defaults for identity and trust is a one-liner instead of everyone having their own version of this old chestnut.

I don't know where such a public helper should live, since all the Platform stuff is internal, and okhttp-tls doesn't feel like a place for platform specific code.

Or i suppose one could just build a client and use the trust it found:

OkHttpClient.Builder builderWithDefaults() throws NoSuchAlgorithmException {
    OkHttpClient.Builder builder = new OkHttpClient.Builder();
    return builder.sslSocketFactory(
        SSLContext.getDefault().getSocketFactory(),
        Objects.requireNonNull(builder.build().x509TrustManager())
    );
}

[yschimke]

~Maybe we could hide the sslSocketFactory(sslSocketFactory) and add a default for trust manager?~

This won't work as the SslContext should be initialised with the trust manager.

[yschimke]

We can definitely add helpers in okhttp-tls to make this a two liner

[swankjesse]

I don't think it's appropriate for OkHttp to ever use the default SSLContext instance, because OkHttp mutates that thing and we don't want to interfere with other users of the same object. (And we don't want other users to interfere with us.)

We should change our docs to say this:

If unset, OkHttp will use an implementation from the host platform.

[swankjesse]

As for a more concise way to fetch the system default trust manager, we've got an API in okhttp-tls that gets CAs but not held certificates.

HandshakeCertificates clientCertificates = new HandshakeCertificates.Builder()
    .addPlatformTrustedCertificates()
    .build();
OkHttpClient client = new OkHttpClient.Builder()
    .sslSocketFactory(clientCertificates.sslSocketFactory(), clientCertificates.trustManager())
    .build();

Do you want it to collect held certificates also?

[yschimke]

I think we only mutate things when we create a SslSocketFactory ourselves.

https://github.com/square/okhttp/blob/5aaf6446ce041f4728b6066730517132e2622904/okhttp/src/commonJvmAndroid/kotlin/okhttp3/internal/platform/Platform.kt#L190

  open fun newSslSocketFactory(trustManager: X509TrustManager): SSLSocketFactory {
    try {
      return newSSLContext()
        .apply {
          init(null, arrayOf<TrustManager>(trustManager), null)
        }.socketFactory
    } catch (e: GeneralSecurityException) {
      throw AssertionError("No System TLS: $e", e) // The system has no TLS. Just give up.
    }
  }

I wonder whether we can make a nice api for this in okhttp-tls without re-loading all the certificates, which I believe has a memory cost.

[DSmithVA]

As for a more concise way to fetch the system default trust manager, we've got an API in okhttp-tls that gets CAs but not held certificates.

Ah I didn't see that. That works for me as it would preserve our code scans that detect and flag any code referencing standard javax TrustManager or KeyManager/Factory. We've burned thousands of hours troubleshooting SSL trust/identity over the years (hundreds of apps all rolling SSL trust/identity in different ways, often multiple keystores/truststores/hand-parsed certs per app). Mandating default java trust and identity has been a tremendous improvement.

I think we only mutate things when we create a SslSocketFactory ourselves.

Mutation would be fine if documented. Though I would naively expect an SSLSocketFactory passed in to be accepted as-is.

We should change our docs to say this:

If unset, OkHttp will use an implementation from the host platform.

I'd go more explicit like "If unset, OkHttp will create a new SSLContext that honors system trust, but not necessarily other system settings like identity, cipher suites or proxy settings." or whatever other wiggle room may be desired.

[swankjesse]

Yeah we should explicitly document what we take from the system and what we decide ourselves!