Support setKeystore setTrustore operations on clients [java]

[chicobento]

Background
Current client builders such as OtlpHttpSpanExporterBuilder provides operations (generally named setClientTls and setTrustredCertificates) for handling the mTLS certificates that takes only the raw pem files as byte array. The conversion of pem->key/trustores is then handled internally.

In some scenarios, the pem->trust/keystore conversion is handled externally, so ideally all clients should accept the already-built key/trustores.

Solution
Provide overloaded setClientTls and setTrustedCertificates methods for passing the *stores.

ps: Although this issue is referring specifically to OtlpHttpSpanExporterBuilder, a similar pattern is being followed by other classes that interacts with external nodes, hence this ideally should be fixed on all similar cases.

[breedx-splk]

This is interesting. @chicobento do you have a real-world example of where the trust store is built "externally"? I'm curious how it might be different than what the builder is doing today.

It wouldn't exactly be a conventional method overload, as you suggested, because the names and types are different. So this kinda suggests new methods and new builder fields...which further complicates it because it could allow the user to specify both, which is always a problem.

If the user configured the builder with conflicting/mismatched trust stores and pem, what do you suggest the builder do?

[chicobento]

This is interesting. @chicobento do you have a real-world example of where the trust store is built "externally"? I'm curious how it might be different than what the builder is doing today.

In our case, we have a library that keeps polling for file changes and converts all pem files in a specific folder structure to Keystores/Truststores or SslContext automatically. For the conversion we support many strategies (bouncy castle, openssl, in-house lib), multi-threaded or whatelse. Doing that via k8s sidecar would work too (for keystores/truststores).

It wouldn't exactly be a conventional method overload, as you suggested, because the names and types are different. So this kinda suggests new methods and new builder fields...which further complicates it because it could allow the user to specify both, which is always a problem.

You're right. Perhaps, to simplify things further and still give the flexibility to construct the stores beforehand, just a single new method setSslContext would solve for both scenarios and not have to deal with the passwords.

e.g OkHttpGrpcServiceBuilder

if (sslContext != null) {
  clientBuilder.sslSocketFactory(sslContext.getSocketFactory());
} else if (trustedCertificatesPem != null) {
      try {
        X509TrustManager trustManager = TlsUtil.trustManager(trustedCertificatesPem);
        X509KeyManager keyManager = null;
        if (privateKeyPem != null && certificatePem != null) {
          keyManager = TlsUtil.keyManager(privateKeyPem, certificatePem);
        }
        clientBuilder.sslSocketFactory(
            TlsUtil.sslSocketFactory(keyManager, trustManager), trustManager);
      } catch (SSLException e) {
        throw new IllegalStateException(
            "Could not set trusted certificates, are they valid X.509 in PEM format?", e);
      }
}

If the user configured the builder with conflicting/mismatched trust stores and pem, what do you suggest the builder do?

We can either document a priority. I'd suggest that SSLContext takes precence as it is less expensive from computation point-of-view and more flexible. Eg:

TLS is handled in the following priority

• sslContext
• setClientTls/setTrustedCertificates

Or explicitly validate at the set or build method calls. E.g:

  public OkHttpGrpcServiceBuilder<ReqMarshalerT, ResUnMarshalerT> setTrustedCertificates(
      byte[] trustedCertificatesPem) {
    requireNonNull(trustedCertificatesPem, "trustedCertificatesPem");
    checkArgument(sslContext == null, "cannot set trustedCertificatesPem when sslContext is defined")
    this.trustedCertificatesPem = trustedCertificatesPem;
    return this;
  }

[chicobento]

hi @breedx-splk , I opened the issue #5245 which could be solved by this as well.

I think that there more scenarios, such as combined pem formats that the current code provided by TlsUtils would break.
Moreover, adding support for passing SSLContext would facilitate to use some certificate reload tools such as sslcontext-kickstart that can hot reload certificates inside a SSLContext.

[jack-berg]

With #5280 merged, let's close this and open new issues when this capability is requested for other exporters.

[chicobento]

Thanks a lot guys, great job! Will try propagate this change to any possible missing classes if I find some time.