Improve TLS handling in OpenIDConnectAuthenticator

[abnsy]

I’m sending a small security improvement for the OIDC authenticator. When the kubeconfig doesn’t include idp-certificate authority-data the code leaves sslContext as null and falls back to the OS trust store. This can behave differently depending on the environment and isn’t always secure. In this update I added a default SSLContext that always uses the system CA.... If the user provides a custom CA, that one is still loaded and used instead. No behavior change other than making TLS more consistent and safer. Thanks!

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: abnsy Once this PR has been reviewed and has the lgtm label, please assign yue9944882 for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment Approvers can cancel approval by writing /approve cancel in a comment

[linux-foundation-easycla[bot]]

The committers listed above are authorized under a signed CLA.

• :white_check_mark: login: abnsy / name: Albern S. (8aef180ccd713e1ef156f9d3ee2c85dd60b2407d)

[k8s-ci-robot]

Welcome @abnsy!

It looks like this is your first PR to kubernetes-client/java 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-client/java has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. :smiley:

[brendandburns]

Can you clarify (especially w/ pointers to documentation) why this is preferable to using the OS default key store?

[abnsy]

Can you clarify (especially w/ pointers to documentation) why this is preferable to using the OS default key store?

Sure....happy to clarify.....The main reason is consistency. When sslcontext is null Java falls back to whatever CA bundle the OS or JVM provides. On containers, Windows, Linux distros,or JVM vendors the trusted roots can be completely different....By initializing SSLContext explicitly, the client always behaves the same way and we know exactly what trust store is used... JSSE also recommends using your own SSLContext if you want predictable TLS behavior https://docs.oracle.com/javase/8/docs/technotes/guides/security/jsse/JSSERefGuide.html#Customization Not trying to override the OS trust store just avoiding the silent fallback that varies across environments... thanks

[brendandburns]

So it seems to me that by specifying a null keystore when you initialize the trust store you get whatever CA bundles are OS installed anyway, so it's not clear to me that this code is actually doing anything.

It also seems to me based on reading https://docs.oracle.com/javase/8/docs/technotes/guides/security/jsse/JSSERefGuide.html#CustomizingStores that you can specify all this stuff via java properties and the defaults will just work.

Perhaps it would be useful to add a unit test which illustrates the problem with the current code and then show how this update fixes it?

Apologies for being picky here, but whenever we mess with encrypt/auth code I want to make sure it's for a good reason.

[abnsy]

So it seems to me that by specifying a null keystore when you initialize the trust store you get whatever CA bundles are OS installed anyway, so it's not clear to me that this code is actually doing anything.

It also seems to me based on reading https://docs.oracle.com/javase/8/docs/technotes/guides/security/jsse/JSSERefGuide.html#CustomizingStores that you can specify all this stuff via java properties and the defaults will just work.

Perhaps it would be useful to add a unit test which illustrates the problem with the current code and then show how this update fixes it?

Apologies for being picky here, but whenever we mess with encrypt/auth code I want to make sure it's for a good reason.

Thanks for the detailed feedback that helps.

About the null keystore part you’re right that when you do tmf.init((KeyStore) null), you end up with whatever CA set the JVM/OS provides The intent wasn’t to replace that, but to make the behavior explicit instead of relying on the implicit fallback that only happens when sslContext is left null. Right now when sslContext stays null the behavior depends on which HttpsURLConnection implementation you hit and what the environment does. For example:

• some distroless container images ship with a very limited CA set,
• some enterprise Windows environments inject internal root CAs,
• Alpine uses a different CA bundle than Debian-based images,
• and some embedded JVM runtimes include their own CA store.

So the fallback is whatever the platform feels like, while explicitly initializing SSLContext always gives the same behavior across environments. I'm absolutely fine switching this to TLSv1.2 to stay consistent with the rest of the code and avoid older protocol versions.

On the unit test idea.....yeah, that makes sense. I can put together a small test showing the difference between:

1. sslContext == null (current behavior)
2. sslContext initialized with tmf.init(null)
3. sslContext initialized with custom CA

[karianna]

@yue9944882 I think the GH actions need to be triggered by an ownser/admin of this repo @abnsy Looks good overall, can you address Mat's feeedback?