镜像站点 · 本页由第三方 GitHub 只读镜像提供,非 GitHub 官方站点,不接受任何登录或凭据输入。前往 github.com
Skip to content

feat: Enable Bound Token for Agentic Identities - #13873

Merged
macastelaz merged 41 commits into
googleapis:agentic-identities-bound-tokenfrom
macastelaz:pr-13169-fixes
Oct 3, 2026
Merged

macastelaz merged 41 commits into
googleapis:agentic-identities-bound-tokenfrom
macastelaz:pr-13169-fixes

Conversation

@macastelaz

@macastelaz macastelaz commented Jul 23, 2026 •

Copy link
Copy Markdown
Contributor

This PR introduces a feature which enables the auth library to acquire bound access-tokens and bound id-tokens in Agentic Environments.

  1. We detect certs in default paths and check if they match the SPIFFE format for agents.

  2. If 1. is a yes then we call the MDS endpoint in a POST request with the certificate in the body.

Note this PR was based on #13169


Manual Testing & End-to-End Verification

We verified this feature end-to-end across both a Live Cloud Run Agent Identity environment (testing against the live Google Metadata Server, Security Token Service, and Vertex AI with the Java Agent Development Kit (com.google.adk:google-adk:1.9.0)) and a 10-Scenario Local Mock MDS Simulation Harness (testing exact HTTP request payloads, true cryptographic certificate/key rotation, combined bundle private-key stripping, well-known directory discovery, non-agent SPIFFE fallback, environment variable precedence, non-atomic rotation retries, and asynchronous container startup polling).

1. Live Cloud Run Agent Identity Verification (<PROJECT_ID>, us-central1)

We deployed a containerized Java test application built against this branch (google-auth-library-oauth2-http:1.50.0-SNAPSHOT @ 27776f6d62a) + Java ADK (com.google.adk:google-adk:1.9.0) to Cloud Run with Agent Identity enabled (--functional-type=agent --identity-type=agent-identity).

Execution A: Default Bound Token Acquisition (GOOGLE_API_ENABLE_RUNTIME_BOUND_TOKEN unset / default true)

  • Cloud Run Job Executions: agent-bound-token-java-test-cjrgp (latest run with Step 4D) & agent-bound-token-java-test-dd9gm
  • Workload Certificate Discovery:
    • Detected /var/run/secrets/workload-spiffe-credentials/credentials.json
    • Extracted Leaf Certificate SAN URI: spiffe://agents.global.org-<ORG_NUMBER>.system.id.goog/resources/run/projects/<PROJECT_NUMBER>/locations/us-central1/jobs/agent-bound-token-java-test
    • Leaf Certificate SHA-256 (x5t#S256): 5Ggf2ofOIkHkvBAwfku6eKQVI5pYzW50whzv-VwSAM8
  • Bound Access Token (ComputeEngineCredentials):
    • Successfully acquired bound OAuth2 Access Token via POST to Metadata Server (ya29.d.c0AZ4bNp...).
  • Bound ID Token (IdTokenCredentials) & Cryptographic Binding Verification (RFC 8705 § 3.1):
    • Acquired bound ID Token for target audience https://example-target-service.run.app.
    • Decoded JWT payload inline (textPayload) and verified live Google STS embedded the cnf (Confirmation) claim containing the SHA-256 thumbprint (x5t#S256) of the workload's leaf X.509 certificate:
      "cnf": {
        "x5t#S256": "5Ggf2ofOIkHkvBAwfku6eKQVI5pYzW50whzv-VwSAM8"
      }
    • Verified an exact byte-for-byte match ([PASS] JWT x5t#S256 thumbprint EXACTLY matches local leaf certificate SHA-256!).
  • Java ADK (com.google.adk:google-adk:1.9.0) + google-genai & mTLS Proof-of-Possession Verification (Steps 4A, 4B, 4C, 4D):
    • Step 4A (ADK HttpClientFactory Non-mTLS Transport): Inspected ADK's shared OkHttpClient (sun.security.ssl.SSLSocketFactoryImpl, no client cert). Calling Google APIs over this non-mTLS channel with the bound token is rejected at the auth layer with HTTP 401 UNAUTHENTICATED.
    • Step 4B (Live ADK LlmAgent + Gemini Turn on Vertex AI): Invoking InMemoryRunner.runAsync(...) with Gemini (gemini-2.5-flash) fails on turn 1 with com.google.genai.errors.ClientException: 401 . Request had invalid authentication credentials, confirming the known limitation where google-genai sends bound tokens over a non-mTLS channel.
    • Step 4C (True mTLS Contrast Test — Matching Workload Certificate): Presenting the exact same bound access token over a true mTLS OkHttpClient (configured with /var/run/secrets/workload-spiffe-credentials/certificates.pem + private_key.pem, x5t#S256 = 5Ggf2ofOIkHkvBAwfku6eKQVI5pYzW50whzv-VwSAM8) to https://cloudresourcemanager.mtls.googleapis.com/v1/projects/<PROJECT_ID> passes authentication (HTTP 403 PERMISSION_DENIED IAM check instead of 401 UNAUTHENTICATED), proving Google API Frontend verified the token binding against the TLS client certificate handshake.
    • Step 4D (Sender-Constraining / Proof-of-Possession Negative Test — Mismatched Client Certificate): Presenting the exact same bound access token to https://cloudresourcemanager.mtls.googleapis.com/v1/projects/<PROJECT_ID> over an mTLS OkHttpClient configured with a different X.509 client certificate (x5t#S256 = -57ZjYVm89oczfdO02Hf3Sz-FaYQIVH1DD3c9zaBIKA) is rejected at the auth layer with HTTP 401 UNAUTHENTICATED, confirming that Google API Frontend enforces cryptographic thumbprint matching (cnf.x5t#S256 == SHA256(TLS client cert)).

Execution B: Opt-Out Unbound Token Acquisition (GOOGLE_API_ENABLE_RUNTIME_BOUND_TOKEN=false)

  • Cloud Run Job Execution: agent-bound-token-java-test-t7b98
  • Updated job environment variable --set-env-vars="GOOGLE_API_ENABLE_RUNTIME_BOUND_TOKEN=false,GOOGLE_CLOUD_LOCATION=global,GOOGLE_GENAI_USE_VERTEXAI=true".
  • Verified ComputeEngineCredentials and IdTokenCredentials fell back to standard HTTP GET requests against MDS and issued standard unbound tokens (decoded JWT payload confirmed absence of the cnf claim).
  • Java ADK Live Agent Turn (Step 4B): With GOOGLE_API_ENABLE_RUNTIME_BOUND_TOKEN=false, the unbound token works over google-genai's non-mTLS channel and the live ADK LlmAgent + Gemini turn SUCCEEDS:
    [ADK Event] author=bound-token-verify-agent, content=Hello! Yes, as an agent designed to test bound token behavior with the Java ADK, I am expected to receive and process bound tokens.

2. Local End-to-End Simulation & Wire Verification (10 Scenarios)

To verify internal wire-level, discovery, rotation, environment-variable, and error-handling behavior between the client and MDS, we executed our local simulation suite (LocalSimulationRunner.java) spinning up a local Mock MDS (HttpServer) across 10 end-to-end scenarios:

  1. Bound Access Token (ComputeEngineCredentials + GOOGLE_API_CERTIFICATE_CONFIG): Verified HTTP POST to /computeMetadata/v1/instance/service-accounts/default/token?scopes=https://www.googleapis.com/auth/cloud-platform, verified JSON body {"certificate_chain": "-----BEGIN CERTIFICATE-----\n..."} (serialized as a single PEM string), and verified no extra fields are included in the JSON payload.
  2. Bound ID Token (IdTokenCredentials + GOOGLE_API_CERTIFICATE_CONFIG): Verified HTTP POST to /computeMetadata/v1/instance/service-accounts/default/identity?audience=https://target.run.app (with audience passed as a URL query parameter) and JSON body {"certificate_chain": "-----BEGIN CERTIFICATE-----\n..."}.
  3. True Live Cryptographic Certificate & Key Rotation on Disk (Gen-1 $\rightarrow$ Gen-2): Generated a new X.509 SPIFFE certificate and matching 2048-bit RSA key pair on disk, updated file mtime, and verified AgentIdentityUtils.getAgentIdentityCertInfo() invalidated its cache, verified the new key pair, and transmitted the rotated Gen-2 certificate chain (!req3.body.equals(req1.body)).
  4. Opt-Out Override (GOOGLE_API_ENABLE_RUNTIME_BOUND_TOKEN=false): Verified setting GOOGLE_API_ENABLE_RUNTIME_BOUND_TOKEN=false switches requests back to HTTP GET with an empty request body.
  5. Well-Known Directory Combined Bundle (credentialbundle.pem) + Private Key Stripping: Unset GOOGLE_API_CERTIFICATE_CONFIG, wrote a combined credentialbundle.pem containing both -----BEGIN CERTIFICATE----- and -----BEGIN PRIVATE KEY----- in the well-known directory, and verified that AgentIdentityUtils resolved certPath == keyPath, verified the key pair, and stripped the PRIVATE KEY block from the transmitted POST payload.
  6. Well-Known Directory Separate Files (certificates.pem + private_key.pem): Unset GOOGLE_API_CERTIFICATE_CONFIG, removed credentialbundle.pem, placed separate certificates.pem and private_key.pem in the well-known directory, and verified bound HTTP POST acquisition.
  7. Non-Agent SPIFFE Trust Domain Fallback (shouldRequestBoundToken == false): Configured a valid X.509 certificate with a standard GKE Workload Identity SAN (spiffe://my-standard-gke-project.svc.id.goog/ns/default/sa/my-ksa); verified AgentIdentityUtils did not throw, cached shouldRequestBoundToken = false, and fell back to standard HTTP GET.
  8. Environment Variable Precedence & GOOGLE_API_USE_CLIENT_CERTIFICATE Matrix:
    • 8a: Legacy GOOGLE_API_PREVENT_AGENT_TOKEN_SHARING_FOR_GCP_SERVICES=false (with GOOGLE_API_ENABLE_RUNTIME_BOUND_TOKEN unset) $\rightarrow$ falls back to HTTP GET.
    • 8b: Modern GOOGLE_API_ENABLE_RUNTIME_BOUND_TOKEN=true overrides legacy GOOGLE_API_PREVENT_AGENT_TOKEN_SHARING_FOR_GCP_SERVICES=false $\rightarrow$ sends bound HTTP POST.
    • 8c: GOOGLE_API_USE_CLIENT_CERTIFICATE=false with valid agent certs on disk $\rightarrow$ disables bound token (HTTP GET) without startup polling.
    • 8d: GOOGLE_API_USE_CLIENT_CERTIFICATE=true with missing cert files $\rightarrow$ fails closed with IOException after retries instead of silently downgrading to an unbound token.
  9. Non-Atomic Rotation / Transient Key-Pair Mismatch Recovery (CERT_KEY_MATCH_RETRIES): Wrote a new Gen-3 non-prod SPIFFE certificate (spiffe://agents-nonprod.global.org-54321.system.id.goog/...) first while delaying the matching private_key.pem update on a background thread; verified loadAndVerifyCredentials() retried cleanly via CERT_KEY_MATCH_RETRIES and transmitted Gen-3.
  10. Asynchronous Container Startup Credential Delivery Polling (TOTAL_POLL_CYCLES): Started with an empty well-known directory on initial startup (GOOGLE_API_USE_CLIENT_CERTIFICATE=true), delivered certificates.pem + private_key.pem asynchronously from a background thread after ~180ms, and verified the initial refreshAccessToken() polled until the files arrived and succeeded with a bound HTTP POST.

3. Reproducible Test Artifacts & Execution Logs (gpaste - Internal Corp Access Only)

Artifact / Log Description Link
Combined Verification Summary & Logs Complete report & console outputs from Live Cloud Run (Bound cjrgp & Opt-Out t7b98) + Java ADK + Local 10-Scenario Simulation https://paste.googleplex.com/4671448266964992
CloudRunAgentVerifyApp.java Live Cloud Run + Java ADK verification app (validates SPIFFE cert, ADC access token, ID token cnf.x5t#S256 match, and ADK Steps 4A/4B/4C/4D) https://paste.googleplex.com/5431546950057984
LocalSimulationRunner.java Self-contained local E2E test harness with Mock MDS (HttpServer) testing all 10 scenarios https://paste.googleplex.com/4541159570014208
Dockerfile Multi-stage Docker build resolving Java ADK 1.9.0 + google-genai and overlaying our local PR JAR https://paste.googleplex.com/6569302979903488
deploy_cloud_run_job.sh Turnkey script to build via Cloud Build, deploy Cloud Run Job with --identity-type=agent-identity, and execute https://paste.googleplex.com/4908078735163392
build_and_run_local.sh Turnkey script to compile and run the local simulation https://paste.googleplex.com/6449866457350144
Raw Cloud Logging — Bound Run (cjrgp) Plain-text Cloud Run logs from gcloud logging read for default Bound Token execution (with inline decoded JWT & ADK Steps 4A/4B/4C/4D) https://paste.googleplex.com/5404054864396288
Raw Cloud Logging — Opt-Out Run (t7b98) Plain-text Cloud Run logs from gcloud logging read for GOOGLE_API_ENABLE_RUNTIME_BOUND_TOKEN=false execution (showing live ADK Gemini response) https://paste.googleplex.com/5990735250325504

Quick Reproduction Steps

To replicate the live Cloud Run test in any GCP project with Agent Identity enabled:

# 1. Place CloudRunAgentVerifyApp.java, LocalSimulationRunner.java, Dockerfile, and deploy_cloud_run_job.sh
#    in a directory alongside your local google-cloud-java checkout.
chmod +x deploy_cloud_run_job.sh build_and_run_local.sh

# 2. Run the local 10-scenario simulation suite:
./build_and_run_local.sh

# 3. Deploy and execute the Cloud Run Job with Agent Identity enabled (Default Bound Token mode):
./deploy_cloud_run_job.sh <PROJECT_ID> us-central1

# 4. To test opt-out behavior (unblocks ADK non-mTLS Gemini calls):
gcloud alpha run jobs update agent-bound-token-java-test \
  --project=<PROJECT_ID> --region=us-central1 \
  --set-env-vars="GOOGLE_API_ENABLE_RUNTIME_BOUND_TOKEN=false,GOOGLE_CLOUD_LOCATION=global,GOOGLE_GENAI_USE_VERTEXAI=true"
gcloud alpha run jobs execute agent-bound-token-java-test \
  --project=<PROJECT_ID> --region=us-central1 --wait

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request introduces Agent Identity token binding support for Cloud Run. It adds AgentIdentityUtils to resolve, load, and verify certificates and private keys, and updates ComputeEngineCredentials to request bound tokens via POST requests when a valid certificate chain is present. The review feedback suggests a cohesive improvement to implement a single-read pattern for certificate files. By reading the certificate chain once, caching it in CertInfo, and passing it to parseCertificate and getBoundTokenPayload, the implementation can avoid redundant disk I/O and prevent potential race conditions during certificate rotation.

@macastelaz macastelaz changed the title Pr 13169 fixes feat: Enable Bound Token for Agentic Identities Jul 23, 2026
// Environment variables
static final String GOOGLE_API_CERTIFICATE_CONFIG = "GOOGLE_API_CERTIFICATE_CONFIG";
static final String GOOGLE_API_PREVENT_TOKEN_SHARING_FOR_GCP_SERVICES =
"GOOGLE_API_PREVENT_TOKEN_SHARING_FOR_GCP_SERVICES";

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Note that based on googleapis/google-cloud-python#17698 (comment) this is not yet finalized

@macastelaz
macastelaz marked this pull request as ready for review July 24, 2026 03:21
@macastelaz
macastelaz requested review from a team as code owners July 24, 2026 03:21
@lsirac
lsirac requested a review from nbayati July 24, 2026 17:43
@macastelaz macastelaz closed this Jul 28, 2026
@macastelaz macastelaz reopened this Jul 28, 2026
@macastelaz
macastelaz requested a review from a team as a code owner July 28, 2026 18:35
vverman and others added 13 commits July 29, 2026 03:35
1. POST request to MDS with cert-chain

2. Cert-key matching

3. Included logic to consider the user's choice by looking at GOOGLE_API_USE_CLIENT_CERTIFICATE env variable

4. Bound ID tokens.

# Conflicts:
#	google-auth-library-java/oauth2_http/javatests/com/google/auth/oauth2/ComputeEngineCredentialsTest.java
#	google-auth-library-java/oauth2_http/javatests/com/google/auth/oauth2/MockMetadataServerTransport.java
…etry logic.

Nit fixes.

# Conflicts:
#	google-auth-library-java/oauth2_http/javatests/com/google/auth/oauth2/ComputeEngineCredentialsTest.java
# Conflicts:
#	google-auth-library-java/oauth2_http/java/com/google/auth/oauth2/ComputeEngineCredentials.java
Comment on lines +196 to +200
if (san.size() >= 2
&& san.get(0) instanceof Integer
&& (Integer) san.get(0) == SAN_URI_TYPE) {
Object value = san.get(1);
if (value instanceof String) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: Is it possible to invert the if checks to have guards here to reduce the nesting for this method?

if san.size < 1 || !san.get(0) instanceof Integer || san.get(0) != SAN_URI_TYPE, same with value !isntanceof String?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done — inverted the conditions into continue guard clauses in shouldRequestBoundToken to flatten the loop nesting.

static boolean isCachedInfoValid(
final CachedAgentIdentityInfo cached,
final String certConfigPath,
final String wellKnownDir) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: the wellKnownDir path shouldn't change from different invocations? Can we just refernece the wellknowndir constant instead of using it as a param?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done — removed the wellKnownDir parameter from isCachedInfoValid and referenced AgentIdentityUtils.getWellKnownDir() directly inside the method.

* @throws IOException If an I/O error occurs while reading the files, or if the key-pair
* verification fails after retries.
*/
static CertInfo getAgentIdentityCertInfo() throws IOException {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This logic is a bit hard for me to follow and I think we can try to simplify it.

  1. getCachedAgentIdentityInfo() looks like it can return null and we should guard against that
  2. The Fast-Path checks sense, I think we should clarify why we can hard-code certPresent = true for this case.
  3. From what I see, if ResolvedCertAndKeyPaths == null, then configExists always is false. If that's the case, then shouldEnableMtls should always return false. I think we guard against that, then shouldEnableMtls doesn't need the configExists param as we can check it here in getAgentIdentityCertInfo()

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Updated getAgentIdentityCertInfo() to address these points:

  1. Added an explicit initialCached != null guard before calling isCachedInfoValid.
  2. Added an inline comment explaining that isCachedInfoValid verifies the cached certificate file still exists on disk with matching metadata, which guarantees certsPresent = true in the fast-path.
  3. Removed the redundant paths != null checks (since resolveCertAndKeyPaths never returns null). Note that shouldEnableMtls(certsPresent, configExists) still needs configExists because configExists == false does not always evaluate to false: when certsPresent == true && configExists == false (certificates discovered in the well-known directory without a config file), shouldEnableMtls(true, false) returns true when GOOGLE_API_USE_CLIENT_CERTIFICATE="true" (Case 1), whereas it returns false when GOOGLE_API_USE_CLIENT_CERTIFICATE is unset (Case 3). Added Javadoc on shouldEnableMtls to clarify this.

@macastelaz
macastelaz requested a review from lqiu96 October 1, 2026 21:21
final CachedAgentIdentityInfo initialCached) throws IOException {
String useClientCert = getUseClientCertificateEnv();
boolean explicitMtls = isMtlsExplicitlyEnabled();
if (!explicitMtls && !Files.exists(Paths.get(wellKnownDir))) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

getWellKnownCertificatePathWithRetry only runs when GOOGLE_API_CERTIFICATE_CONFIG is unset, where configExists is always false. Since shouldEnableMtls now returns false whenever GOOGLE_API_USE_CLIENT_CERTIFICATE is not "true" and configExists is false, getWellKnownCertificatePathWithRetry can return new ResolvedCertAndKeyPaths(null, null, false) immediately when !explicitMtls instead of probing wellKnownDir and its certificate files on every token refresh.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done — updated getWellKnownCertificatePathWithRetry to return new ResolvedCertAndKeyPaths(null, null, false) immediately when !isMtlsExplicitlyEnabled().

}
}
}
initialStartupCompleted = true;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Setting initialStartupCompleted = true here before the debug log at line 734 means initialStartupCompleted always prints true in that log message, even on the initial startup call. Also, when isMtlsExplicitlyDisabled() is true, we should skip logging the missing well-known certificate fallback message just like getPathsFromConfigWithRetry does.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

With the early return when !isMtlsExplicitlyEnabled() at the top of getWellKnownCertificatePathWithRetry (from the comment above), the remainder of this method only runs when explicit mTLS is enabled, so the !explicitMtls fallback log at the end was unreachable and has been removed.

for (int cycle = 0; cycle < maxCycles; cycle++) {
try {
if (AgentIdentityCacheUtils.checkExistsOrAccessDenied(Paths.get(certConfigPath))) {
ResolvedCertAndKeyPaths paths = extractPathsFromConfig(certConfigPath);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

small nit (possible I may be missing some edge case), so if we can't or shouldn't do this then please ignore.

I think all paths that lead to a valid ResolvedCertAndKeyPaths should just have the certpath and keypaths already validated so we don't need to validate this in the calling method.

L567 and L570 don't need explicit checkExistsOrAccessDenied calls here as it'll know that it's either valid or null since it got back a ResolvedCertAndKeyPaths object.

There are also a lot of paths.getCertPath() calls from a ResolvedCertAndKeyPaths so perhaps certPath and keyPath fields should just be a Path object.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

  • On validating file existence inside extractPathsFromConfig: The edge case here is when certificate_config.json itself resides outside wellKnownDir (shouldPoll == false initially) while its cert_path / key_path point inside wellKnownDir and haven't been delivered yet on cycle 0. getPathsFromConfigWithRetry needs the parsed certPath and keyPath from extractPathsFromConfig before checking file existence so it can inspect isPathInWellKnownDir(...) and enable startup polling (shouldPoll = true; maxCycles = TOTAL_POLL_CYCLES).
  • On using Path vs. String in ResolvedCertAndKeyPaths: Both the inputs (JSON config strings and fallbackCached.certMetadata.getPath()) and the downstream consumers (loadAndVerifyCredentials, FileMetadata, isPostResolutionCacheHit, readCertificateChain, and readPrivateKey) take and store String, so keeping String avoids String -> Path -> String round-trip conversions and null-guarded .toString() calls.

Comment on lines +562 to +563
&& (isPathInWellKnownDir(paths.getCertPath())
|| isPathInWellKnownDir(paths.getKeyPath()))) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

maybe worth adding a comment in the code, I'm not sure why this needs to check if it exists in the well known dir here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done — added an inline comment explaining that the config file itself may reside outside wellKnownDir while referencing cert_path or key_path inside wellKnownDir that are still being delivered at startup.

@lqiu96

lqiu96 commented Oct 2, 2026

Copy link
Copy Markdown
Member

I think the code looks good to me. I will do a final pass through the tests tomorrow to see if there is any cases that I may have missed.

@macastelaz
macastelaz requested a review from lqiu96 October 2, 2026 14:09
Comment on lines +572 to +575
// Incomplete workload config (missing cert_path or key_path) will never become ready;
// fail fast without polling.
lastParseException = e;
break;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

qq: for this case, it mentions fail-fast but this ends up going to the fallback value in the cache. Is this intended or should we bubble this up to the user/ calling method?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good catch on the comment wording — "fail fast without polling" here meant skipping the 30-second startup polling loop (via break;), not bypassing the steady-state cache fallback:

  • On initial startup (fallbackCached == null), breaking out of the loop immediately throws the IOException (with IncompleteWorkloadConfigException as the cause) without waiting 30s.
  • In steady state (fallbackCached != null, meaning a valid config and verified certificate were already cached earlier), breaking out of the loop falls back to the previously validated cached paths if a later config rewrite is incomplete, matching the behavior when the config file is deleted or unreadable after caching.

Updated the inline comment in 9afe807bca3 to make this distinction explicit.

* @throws IOException If an I/O error occurs while reading the files, or if the key-pair
* verification fails after retries.
*/
static CertInfo getAgentIdentityCertInfo() throws IOException {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

additional thoughts:

Tracing this from ComputeEngineCredentials, I see this is called from refreshAcessToken() -> getBoundToken() which already handles the request coalescing to mitigate the thundering herd possibility. Perhaps it maybe worth a small callout in the javadocs the mention this for future maintainers, so they know why we don't have any need for coalescing here or concerns about syncing across multiple requests

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good call — added a note to getAgentIdentityCertInfo()'s Javadoc in 9afe807bca3 calling out that callers like ComputeEngineCredentials.refreshAccessToken() invoke this via getBoundTokenPayload() under OAuth2Credentials's token refresh coalescing.

* Utility class for in-memory caching and filesystem metadata validation of Agent Identity
* certificates and configuration files.
*/
final class AgentIdentityCacheUtils {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

For the util classes, can you add some quick @NullMarked and @nullable annotations. Feel free to add them in follow up PRs (I don't think annotations are blocking for this)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done in 9afe807bca3 — added @NullMarked and @Nullable annotations across AgentIdentityCacheUtils, AgentIdentityCertificateValidationUtils, and AgentIdentityUtils.

@lqiu96 lqiu96 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, thanks for all the quick iterations! I took one final pass throughout the PR and I think the logic makes sense. I don't have any additional concerns with the code and I think we can make follow fixes after additional testing/ feedback.

@macastelaz
macastelaz merged commit acc7c9d into googleapis:agentic-identities-bound-token Oct 3, 2026
174 of 179 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants