Skip to content

Improve TLS handling in OpenIDConnectAuthenticator #4417

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
abnsy wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Improve TLS handling in OpenIDConnectAuthenticator #4417

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 22 additions & 6 deletions util/src/main/java/io/kubernetes/client/util/authenticators/OpenIDConnectAuthenticator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,13 +113,27 @@ public Map<String, Object> refresh(Map<String, Object> config) {
String clientSecret = (String) config.getOrDefault(OIDC_CLIENT_SECRET, "");
String idpCert = (String) config.get(OIDC_IDP_CERT_DATA);


SSLContext sslContext = null;

// Initialize a secure default SSLContext using the system default TrustManager.
Copy link

@macarte macarte Dec 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While this should be the preferred use case, I would recommend an opt-out flag and document it in the release notes so that if this causes unforeseen issues for adopters, they can specify the flag rather than rolling back

// This ensures that we always have certificate validation even when no idp-certificate-authority-data is provided.
try {
Copy link

@macarte macarte Nov 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this new block be in the 'else' part of the following test, i.e. if(idpCert != null) { existing block } else { this block }, otherwise this block runs even when there is an idpCert but the result is not used. Unless there's some state outside of this method that changes based on this block and those changes are required for the following if(idpCert != null) block

TrustManagerFactory defaultTmf =
TrustManagerFactory.getInstance(TrustManagerFactory.getDefaultAlgorithm());
defaultTmf.init((KeyStore) null);
SSLContext defaultSslContext = SSLContext.getInstance("TLS");
Copy link
Contributor

@brendandburns brendandburns Nov 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's switch this to "TLSv1.2" to make it consistent with the code below, we may want to switch to configuring the SSL engine to support both TLSv1.2 and TLSv1.3, but I definitely don't want to potentially enable older versions of TLS.

defaultSslContext.init(defaultTmf.getTrustManagers(), null, new SecureRandom());
Copy link

@macarte macarte Dec 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The trust manager should be the second parameter, the first being the key manager (which is null)

defaultSslContext.init(null, defaultTmf.getTrustManagers(), new SecureRandom());

sslContext = defaultSslContext;
} catch (KeyStoreException | NoSuchAlgorithmException | KeyManagementException e) {
throw new RuntimeException("Unable to initialize default TLS context", e);
}

if (idpCert != null) {
// fist, lets get the pem
// first, let's get the pem (custom CA provided by user)
String pemCert = new String(Base64.getDecoder().decode(idpCert));
Copy link

@macarte macarte Dec 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For consistency across environments, specify the encoding explicitly:

new String(Base64.getDecoder().decode(idpCert), StandardCharsets.UTF_8);


// next lets get a cert object
// next let's get a cert object
// need an alias name to store the certificate in a keystore. Also
// java keystores need passwords. this value is as good as any as
// there isn't anything actually secret being stored.
Expand All @@ -143,10 +157,11 @@ public Map<String, Object> refresh(Map<String, Object> config) {
TrustManagerFactory tmf = TrustManagerFactory.getInstance("PKIX");
tmf.init(ks);

// TODO would be good to make this more dyanamic. Doesn't seem like
// a good way to do this.
sslContext = SSLContext.getInstance("TLSv1.2");
sslContext.init(null, tmf.getTrustManagers(), new SecureRandom());
// Use TLSv1.2 for the custom context initialized with the provided CA(s)
SSLContext idpSslContext = SSLContext.getInstance("TLSv1.2");
idpSslContext.init(null, tmf.getTrustManagers(), new SecureRandom());
// override sslContext to use the idp-specific SSLContext
sslContext = idpSslContext;

} catch (KeyStoreException
| NoSuchAlgorithmException
Expand All @@ -157,6 +172,7 @@ public Map<String, Object> refresh(Map<String, Object> config) {
}
}


// check the identity provider's configuration url for a token endpoint
String tokenURL = loadTokenURL(issuer, sslContext);

Expand Down