Repository navigation
fix(auth): present the full certificate chain for mTLS #14621
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.
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
base: agentic-identities-bound-token
Are you sure you want to change the base?
Changes from all commits
2247ddd
fb43614
b2876a4
33b43bf
6b1f7c7
f68c9c7
2dae679
f581f96
228aa05
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,151 @@ | ||
| /* | ||
| * Copyright 2026 Google LLC | ||
| * | ||
| * Redistribution and use in source and binary forms, with or without | ||
| * modification, are permitted provided that the following conditions are | ||
| * met: | ||
| * | ||
| * * Redistributions of source code must retain the above copyright | ||
| * notice, this list of conditions and the following disclaimer. | ||
| * * Redistributions in binary form must reproduce the above | ||
| * copyright notice, this list of conditions and the following disclaimer | ||
| * in the documentation and/or other materials provided with the | ||
| * distribution. | ||
| * * Neither the name of Google LLC nor the names of its | ||
| * contributors may be used to endorse or promote products derived from | ||
| * this software without specific prior written permission. | ||
| * | ||
| * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS | ||
| * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT | ||
| * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR | ||
| * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT | ||
| * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, | ||
| * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT | ||
| * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, | ||
| * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY | ||
| * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT | ||
| * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE | ||
| * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. | ||
| */ | ||
|
|
||
| package com.google.auth.mtls; | ||
|
|
||
| import com.google.api.client.util.PemReader; | ||
| import java.io.ByteArrayInputStream; | ||
| import java.io.IOException; | ||
| import java.io.InputStream; | ||
| import java.io.InputStreamReader; | ||
| import java.nio.charset.StandardCharsets; | ||
| import java.security.GeneralSecurityException; | ||
| import java.security.KeyFactory; | ||
| import java.security.KeyStore; | ||
| import java.security.PrivateKey; | ||
| import java.security.cert.CertificateException; | ||
| import java.security.cert.CertificateFactory; | ||
| import java.security.cert.X509Certificate; | ||
| import java.security.spec.PKCS8EncodedKeySpec; | ||
| import java.util.ArrayList; | ||
| import java.util.List; | ||
| import org.jspecify.annotations.NullMarked; | ||
| import org.jspecify.annotations.Nullable; | ||
|
|
||
| /** | ||
| * Builds the mTLS {@link KeyStore} from PEM-encoded certificates and a private key. | ||
| * | ||
| * <p>This replaces {@code com.google.api.client.util.SecurityUtils#createMtlsKeyStore}, which keeps | ||
| * only the first certificate. Here the key entry holds every certificate in the input, so the | ||
| * intermediate certificates are presented too. Some peers (for example, another workload that | ||
| * trusts only a root CA) need them to verify the client certificate. | ||
| * | ||
| * <p>Everything else matches the upstream method: key store type (JKS), alias, empty password, | ||
| * accepted key format (PKCS#8 {@code PRIVATE KEY}), which private key is chosen, and error | ||
| * messages. Any input that the upstream method accepted is still accepted, even though more of it | ||
| * is now used: once a certificate and a private key have been found (the point where the upstream | ||
| * method stopped reading), a malformed or unreadable later section ends the input instead of | ||
| * failing, and an extra certificate that cannot be parsed is left out of the chain. Unlike the | ||
| * upstream method, the input is decoded as UTF-8 rather than the platform default charset, and it | ||
| * is read to the end. | ||
| */ | ||
| @NullMarked | ||
| final class MtlsKeyStoreUtils { | ||
|
|
||
| private MtlsKeyStoreUtils() {} | ||
|
|
||
| /** | ||
| * Creates a key store holding one private key entry. | ||
| * | ||
| * @param certAndKey PEM input containing one or more {@code CERTIFICATE} sections and a PKCS#8 | ||
| * {@code PRIVATE KEY} section, in any order. The first certificate must be the one that | ||
| * matches the private key (the leaf); the remaining certificates are kept in input order as | ||
| * the rest of the chain. If there are several private keys, the one used is the same as | ||
| * upstream: the last one before the first certificate, or else the first one after it. The | ||
| * stream is read to the end and is not closed. | ||
| * @throws IllegalArgumentException if the input has no certificate or no private key, or if a PEM | ||
| * section before that point is malformed | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: The phrase |
||
| * @throws GeneralSecurityException if the first certificate or the private key cannot be parsed, | ||
| * or the key store cannot be created | ||
| * @throws IOException if the input cannot be read before a certificate and a private key are | ||
| * found | ||
| */ | ||
| static KeyStore createMtlsKeyStore(InputStream certAndKey) | ||
| throws GeneralSecurityException, IOException { | ||
| List<byte[]> certificates = new ArrayList<>(); | ||
| byte @Nullable [] privateKey = null; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: JSpecify gives |
||
| PemReader reader = new PemReader(new InputStreamReader(certAndKey, StandardCharsets.UTF_8)); | ||
| while (true) { | ||
| PemReader.Section section; | ||
| try { | ||
| section = reader.readNextSection(); | ||
| } catch (IllegalArgumentException | IOException e) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Catching |
||
| if (!certificates.isEmpty() && privateKey != null) { | ||
| // Malformed or unreadable trailing content after a usable certificate and key: stop | ||
| // here, as the upstream method (which stopped reading at this point) would have. | ||
| break; | ||
| } | ||
| throw e; | ||
| } | ||
| if (section == null) { | ||
| break; | ||
| } | ||
| if ("CERTIFICATE".equals(section.getTitle())) { | ||
| certificates.add(section.getBase64DecodedBytes()); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: Since |
||
| } else if ("PRIVATE KEY".equals(section.getTitle()) | ||
| && (privateKey == null || certificates.isEmpty())) { | ||
| // Same choice as upstream: a later key replaces an earlier one until the first certificate | ||
| // has been seen. | ||
| privateKey = section.getBase64DecodedBytes(); | ||
| } | ||
| } | ||
| if (certificates.isEmpty()) { | ||
| throw new IllegalArgumentException("certificate is missing from certAndKey string"); | ||
| } | ||
| if (privateKey == null) { | ||
| throw new IllegalArgumentException("private key is missing from certAndKey string"); | ||
| } | ||
|
|
||
| CertificateFactory certFactory = CertificateFactory.getInstance("X.509"); | ||
| List<X509Certificate> chain = new ArrayList<>(certificates.size()); | ||
| chain.add(parseCertificate(certFactory, certificates.get(0))); | ||
| for (int i = 1; i < certificates.size(); i++) { | ||
| try { | ||
| chain.add(parseCertificate(certFactory, certificates.get(i))); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: Appending every extra
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd prefer to keep createMtlsKeyStore faithful to the certificates provided in the PEM stream (matching CertificateFactory.generateCertificates / PKCS12KeyStore / OpenSSL SSL_CTX_use_certificate_chain_file), rather than heuristically filtering out certificates during keystore loading (which has edge cases around cert.verify() with disabled algorithms and RFC 5280 self-issued key-rollover intermediates). GKE pod certificate bundles only include [leaf, intermediate], and if a user-configured chain includes a certificate that violates the JDK's TLS algorithm constraints, failing rather than silently dropping certificates from the configured chain seems like the expected behavior. Thoughts? |
||
| } catch (CertificateException | RuntimeException e) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: Silently dropping an unparsable intermediate here leaves no trace when a peer later rejects the leaf-only chain. Can we log a debug or warning message via |
||
| // The upstream method never parsed extra certificates; leave this one out rather than | ||
| // fail. | ||
| } | ||
| } | ||
| PrivateKey key = | ||
| KeyFactory.getInstance(chain.get(0).getPublicKey().getAlgorithm()) | ||
| .generatePrivate(new PKCS8EncodedKeySpec(privateKey)); | ||
|
macastelaz marked this conversation as resolved.
|
||
|
|
||
| KeyStore keyStore = KeyStore.getInstance("JKS"); | ||
| keyStore.load(null); | ||
|
macastelaz marked this conversation as resolved.
|
||
| keyStore.setKeyEntry("alias", key, new char[] {}, chain.toArray(new X509Certificate[0])); | ||
| return keyStore; | ||
| } | ||
|
|
||
| private static X509Certificate parseCertificate(CertificateFactory certFactory, byte[] der) | ||
| throws CertificateException { | ||
| return (X509Certificate) certFactory.generateCertificate(new ByteArrayInputStream(der)); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we null-check |
||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -32,7 +32,6 @@ | |
|
|
||
| import com.google.api.client.json.JsonParser; | ||
| import com.google.api.client.json.gson.GsonFactory; | ||
| import com.google.api.client.util.SecurityUtils; | ||
| import com.google.api.core.InternalApi; | ||
| import com.google.common.annotations.VisibleForTesting; | ||
| import com.google.common.collect.ImmutableList; | ||
|
|
@@ -148,7 +147,7 @@ static KeyStore getKeyStore(InputStream metadata, ProcessProvider processProvide | |
| } | ||
|
|
||
| // Create mTLS key store with the input certificates from shell command. | ||
| return SecurityUtils.createMtlsKeyStore(process.getInputStream()); | ||
| return MtlsKeyStoreUtils.createMtlsKeyStore(process.getInputStream()); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Reading stdout to EOF after |
||
| } | ||
|
|
||
| @VisibleForTesting | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Since every certificate in the input now goes on the wire and JDK 25+ servers reject client chains longer than 8 by default, could we note here that the file should hold only the leaf and its intermediates?