Add method for creating an SSLContext that merges both the trust store and custom certificates

[Eric-Alvarez]

Before this PR

After this PR

==COMMIT_MSG==
==COMMIT_MSG==

Possible downsides?

[changelog-app]

Generate changelog in changelog/@unreleased

What do the change types mean?

• feature: A new feature of the service.
• improvement: An incremental improvement in the functionality or operation of the service.
• fix: Remedies the incorrect behaviour of a component of the service in a backwards-compatible way.
• break: Has the potential to break consumers of this service's API, inclusive of both Palantir services
  and external consumers of the service's API (e.g. customer-written software or integrations).
• deprecation: Advertises the intention to remove service functionality without any change to the
  operation of the service itself.
• manualTask: Requires the possibility of manual intervention (running a script, eyeballing configuration,
  performing database surgery, ...) at the time of upgrade for it to succeed.
• migration: A fully automatic upgrade migration task with no engineer input required.

Note: only one type should be chosen.

How are new versions calculated?

• ❗The break and manual task changelog types will result in a major release!
• 🐛 The fix changelog type will result in a minor release in most cases, and a patch release version for patch branches. This behaviour is configurable in autorelease.
• ✨ All others will result in a minor version release.

Type

• Feature
• Improvement
• Fix
• Break
• Deprecation
• Manual task
• Migration

Description

Add method for creating an SSLContext that merges both a configured trust store and custom certificates.

Check the box to generate changelog(s)

• Generate changelog entry

[Eric-Alvarez]

I noticed most of these methods aren't explicitly tested, do I need to add tests for my additions?

[Eric-Alvarez]

This would be convenient if we made it public, this PR would not be necessary if this method was available.

[pkoenig10]

I would prefer not to add these methods.

The use case seems very niche and they introduce a fairly large footgun. I don't want to implicitly endorse adding custom certificates to truststore. Doing so makes it very hard to reason about the security of our services because can no longer be sure that the truststore being used by the application matches what is rendered by our production infrastructure.

For your use case, I'd recommend just writing this yourself.

[Eric-Alvarez]

@pkoenig10 I'm happy to not add the top level methods, but exposing the ability to load a keystore from a truststore with default ca certs is innocuous and very useful for handling these edge cases.

[Eric-Alvarez]

@pkoenig10 Could you take a look at the update? Making this method accessible will eliminate the need to copy paste a lot of code for loading truststores and default ca certs (although the latter can be handled with adding the jvm_certs into the trust store). I'd rather not copy paste the method for loading truststores as it will become out of date and might fail in the future.

[Eric-Alvarez]

Unsure if it would be preferred to put this in a different class rather than making this class public.

[pkoenig10]

I'd like to better understand why we think this behavior should be provided by this library. This library is intended to be a utility to create Conjure clients, not a general keystore library.

APIs live forever and can be (ab)used in unexpected ways, so I generally hold a pretty high bar when adding new APIs. I'm not convinced that this behavior crosses that threshold. This behavior is fairly niche and I don't think it's unreasonable to require consumers who want this behavior to implement it themselves.

Addressing some specific comments:

Making this method accessible will eliminate the need to copy paste a lot of code for loading truststores and default ca certs

What is the problem with duplicating this code? Duplicating code is generally preferable to providing API that we are unsure about or may later regret.

I'd rather not copy paste the method for loading truststores as it will become out of date and might fail in the future.

Can you explain what you mean by "out of date"? What specific failure modes are you concerned about?

[Eric-Alvarez]

Security vulnerabilities, performance improvements, and adding/removing keystore types are all areas that could be a concern with duplicating the code.
It seems this library is a generic trust store/key store library for the most part. There's no mention of conjure outside of package names in the whole thing. And the return types are generic java types rather than anything specific to conjure clients.

We have a standard truststore format, it would be nice if we were able to load it without copy pasting code from another repo that could break or become insecure. Loading a truststore or keystore seems like a pretty normal use-case that could be re-used in many places. Taking a hard line that we're not allowed to iterate on or use any keystore utilities and everything needs to be copy pasted seems like an objectively worse state of the world.
I'm not sure I see the risk of abuse with simply providing a convenience method to return a loaded keystore, as you mentioned I am completely able to do this already, just in a worse way that could add bugs or be insecure.

I defer to @carterkozak