Skip to content

fix(auth): align mTLS transport and bound token certificate startup polling and well-known discovery - #14748

Open
macastelaz wants to merge 1 commit into
googleapis:mainfrom
macastelaz:fix/mtls-transport-startup-polling
Open

macastelaz wants to merge 1 commit into
googleapis:mainfrom
macastelaz:fix/mtls-transport-startup-polling

Conversation

@macastelaz

Copy link
Copy Markdown
Contributor

Summary

Fixes two certificate discovery asymmetries between mTLS transport initialization (MtlsUtils / X509Provider) and Agent Identity bound-token acquisition (AgentIdentityUtils):

  1. Add 30-second initial startup polling to MtlsUtils for the well-known SPIFFE directory:

    • When GOOGLE_API_CERTIFICATE_CONFIG (or certConfigPathOverride) or its referenced cert_path / key_path resides in /var/run/secrets/workload-spiffe-credentials/ (resolving ancestor symlinks via resolveRealPathAllowingMissing), MtlsUtils now polls for up to 30 seconds (50 × 100ms + 50 × 500ms) on initial container startup (!isInitialStartupCompleted()) until the configuration file, cert_path, and key_path are non-empty readable regular files and the JSON configuration parses cleanly.
    • MtlsUtils and AgentIdentityUtils share the initialStartupCompleted state so the JVM performs the 30-second cold-start poll at most once across both transport and token initialization.
    • Non-retryable states (missing files outside the well-known SPIFFE directory, directories, unreadable files, valid ECP-only configs, or incomplete workload configs missing cert_path/key_path) fail immediately with 0 sleeps.
  2. Remove unconfigured legacy credentialbundle.pem and certificates.pem + private_key.pem fallbacks in AgentIdentityUtils:

    • When GOOGLE_API_CERTIFICATE_CONFIG is unset, AgentIdentityUtils.getWellKnownCertificatePathWithRetry() now checks only the GKE Pod Certificate bundle (/var/run/secrets/workload-spiffe-credentials/x509.credential-bundle.private-key.pem), matching MtlsUtils.getWorkloadCertPath() and X509Provider 1:1 so AgentIdentityUtils never mints a certificate-bound token for a certificate that the transport ignores.

Testing

  • Added unit tests in MtlsUtilsTest, X509ProviderTest, and AgentIdentityUtilsTest covering:
    • Delayed config and cert/key creation in /var/run/secrets/workload-spiffe-credentials/
    • Config outside the well-known SPIFFE directory referencing cert_path/key_path inside the well-known SPIFFE directory
    • Partial/truncated JSON writes during startup polling
    • 0-byte certificate/key files during startup polling
    • Symlinked ancestor paths on the well-known SPIFFE directory
    • Startup poll timeout (99 sleeps) and shared initialStartupCompleted state across MtlsUtils and AgentIdentityUtils (0 sleeps on subsequent calls)
    • Ignoring unconfigured credentialbundle.pem and certificates.pem + private_key.pem when GOOGLE_API_CERTIFICATE_CONFIG is unset

@macastelaz
macastelaz requested review from a team as code owners October 23, 2026 13:40

@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 container startup polling in MtlsUtils to wait for credential file readiness when configuration or certificate files reside in the well-known SPIFFE directory, and unifies startup state tracking across mTLS transport initialization and Agent Identity token binding. The feedback suggests minor code simplifications by removing redundant checks in MtlsUtils (specifically in isExistingNonFileOrUnreadable and the exception handling block) and proposes a concurrency optimization to check the startup completion flag inside the polling loop to prevent unnecessary sleep cycles.

Comment on lines +204 to +206
private static boolean isExistingNonFileOrUnreadable(File file) {
return file.exists() && (!file.isFile() || (!file.canRead() && file.exists()));
}

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.

medium

The expression (!file.canRead() && file.exists()) contains a redundant file.exists() check because file.exists() is already verified at the beginning of the method. This can be simplified to !file.isFile() || !file.canRead().

Suggested change
private static boolean isExistingNonFileOrUnreadable(File file) {
return file.exists() && (!file.isFile() || (!file.canRead() && file.exists()));
}
private static boolean isExistingNonFileOrUnreadable(File file) {
return file.exists() && (!file.isFile() || !file.canRead());
}
References
  1. When implementing property parsing or validation logic, ensure that null checks and validation steps are not redundant with checks already performed by upstream callers or preceding logic in the same method.

boolean shouldPoll = isPathInWellKnownSpiffeDir(configFile.getPath());
int maxCycles = shouldPoll ? TOTAL_POLL_CYCLES : 1;

for (int cycle = 0; cycle < maxCycles; cycle++) {

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.

medium

In a highly concurrent startup environment, multiple threads might call waitForExplicitConfigStartupIfNeeded simultaneously. Once one thread successfully completes the polling and sets initialStartupCompleted = true, other polling threads can immediately stop polling and return null instead of continuing to sleep and check. This avoids redundant disk I/O and sleep cycles.

    for (int cycle = 0; cycle < maxCycles; cycle++) {
      if (initialStartupCompleted) {
        return null;
      }

Comment on lines +256 to +260
if (configFile.length() > 0) {
// Valid JSON missing required cert_configs, cert_path, or key_path; do not poll.
initialStartupCompleted = true;
throw e;
}

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.

medium

The check configFile.length() > 0 is redundant here because isNonEmptyReadableFile(configFile) was already verified on line 227, which guarantees that configFile.length() > 0 is true. We can safely remove this check and directly mark startup as completed and rethrow the exception.

          // Valid JSON missing required cert_configs, cert_path, or key_path; do not poll.
          initialStartupCompleted = true;
          throw e;
References
  1. When implementing property parsing or validation logic, ensure that null checks and validation steps are not redundant with checks already performed by upstream callers or preceding logic in the same method.

This branch has not been deployed

No deployments
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.

1 participant