feat: implement agentic identities authentication via cert bound tokens. #1842
Conversation
…trol the sleep time for threads. Fixed the agent_spiffe_cert.pem
vverman
added
the
do not merge
nbayati
left a comment
nbayati
left a comment
There was a problem hiding this comment.
Overall a really good PR! Just a few small comments on the implementation. I'll review the unit tests next.
| private AgentIdentityUtils() {} | ||
|
|
||
| /** | ||
| * Gets the Agent Identity certificate if available and enabled. |
There was a problem hiding this comment.
nit: we can make this documentation more accurate by saying something along the lines of "Loads and parses the Agent Identity certificate if available and not opted out. "
There was a problem hiding this comment.
Made the changes, do lmk if this looks good!
oauth2_http/java/com/google/auth/oauth2/AgentIdentityUtils.java
Outdated
Show resolved
Hide resolved
oauth2_http/java/com/google/auth/oauth2/AgentIdentityUtils.java
Outdated
Show resolved
Hide resolved
vverman
force-pushed
the
feat/agentic-identities-cloudrun
branch
from
e0d450a to
5353838
Compare
| import java.security.cert.CertificateFactory; | ||
| import java.security.cert.CertificateParsingException; | ||
| import java.security.cert.X509Certificate; | ||
| import java.util.*; |
There was a problem hiding this comment.
nit: Can you list the imports out?
| final class AgentIdentityUtils { | ||
| private static final Logger LOGGER = Logger.getLogger(AgentIdentityUtils.class.getName()); | ||
|
|
||
| static final String GOOGLE_API_CERTIFICATE_CONFIG = "GOOGLE_API_CERTIFICATE_CONFIG"; |
There was a problem hiding this comment.
I think there is a constant here:
Could we re-use the constant there?
| ImmutableList.of( | ||
| Pattern.compile("^agents\\.global\\.org-\\d+\\.system\\.id\\.goog$"), | ||
| Pattern.compile("^agents\\.global\\.proj-\\d+\\.system\\.id\\.goog$")); |
There was a problem hiding this comment.
Just want to double check this regex:
- It is supposed to be
proj-\d+and notproject-\d+ $end of string regex and nothing comes after it?
There was a problem hiding this comment.
Yes, that is correct. This is captured in point 1. here in the design doc
| boolean warned = false; | ||
|
|
||
| // Deterministic polling loop based on pre-calculated intervals. | ||
| for (long sleepInterval : POLLING_INTERVALS) { |
There was a problem hiding this comment.
I'm concerned about this given that this is to be opt-out and not opt-in. By default, customers may now experience up to 30s of busy waiting just because the filesystem on certain environments can't be loaded fast enough. This applies for customers who don't touch Agent Identities.
IIUC the code from above, the only way to to know if Agent Identities shouldn't be used:
- Load the cert file (up to 30s of busy waiting)
- Then load and parse the cert file to check the SAN
OR
Require users to explicitly add the new env var to their workloads + envs
Can you let me know if my understanding is correct? If this is the case that the cert has to be loaded, then I feel that opt-in would be better to avoid the possibility busy waiting for all customers.
| LOGGER.warning( | ||
| String.format( | ||
| "Certificate config file not found at %s (from %s environment variable). " | ||
| + "Retrying for up to %d seconds.", | ||
| certConfigPath, GOOGLE_API_CERTIFICATE_CONFIG, TOTAL_TIMEOUT_MS / 1000)); | ||
| warned = true; |
There was a problem hiding this comment.
Perhaps this would be better if we start using slf4j logging?
CC: @zhumin8
| for (long sleepInterval : POLLING_INTERVALS) { | ||
| try { | ||
| if (Files.exists(Paths.get(certConfigPath))) { | ||
| String certPath = extractCertPathFromConfig(certConfigPath); |
There was a problem hiding this comment.
IIUC, null is used to represent that the cert_path doesn't exist inside the certConfigPath. I think this would be a case where we wouldn't want to retry anymore since the certConfigPath is found.
| try { | ||
| if (Files.exists(Paths.get(certConfigPath))) { | ||
| String certPath = extractCertPathFromConfig(certConfigPath); | ||
| if (!Strings.isNullOrEmpty(certPath) && Files.exists(Paths.get(certPath))) { |
There was a problem hiding this comment.
This is testing that the certPath from certConfig exists, right? I think this would be the happy path.
Just want to confirm: What about the test case that the certPath from certConfig doesn't exist on the file system. Should we stop retrying and fail instead of continue to retry?
| static final String GOOGLE_API_PREVENT_AGENT_TOKEN_SHARING_FOR_GCP_SERVICES = | ||
| "GOOGLE_API_PREVENT_AGENT_TOKEN_SHARING_FOR_GCP_SERVICES"; | ||
|
|
||
| private static final List<Pattern> AGENT_IDENTITY_SPIFFE_PATTERNS = |
There was a problem hiding this comment.
nit: Can you add a link to what SPIFFE is? I'm ok with this acronym, but may be helpful as I don't think it's something as well known as URI, WWW, etc
| ImmutableList.of( | ||
| Pattern.compile("^agents\\.global\\.org-\\d+\\.system\\.id\\.goog$"), | ||
| Pattern.compile("^agents\\.global\\.proj-\\d+\\.system\\.id\\.goog$")); | ||
| private static final int SAN_URI_TYPE = 6; |
There was a problem hiding this comment.
nit: Same here, perhaps we can spell out SAN? I don't think this is as well known of an acronym
| private static final int FAST_POLL_CYCLES = 50; | ||
| private static final long FAST_POLL_INTERVAL_MS = 100; // 0.1 seconds | ||
| private static final long SLOW_POLL_INTERVAL_MS = 500; // 0.5 seconds | ||
| private static final long TOTAL_TIMEOUT_MS = 30000; // 30 seconds |
There was a problem hiding this comment.
Is there any specific reason for these pre-configured values? I'm guessing the data shows that 5s resolves for a vast majority of cases?
3 participants
This feature implements the ability for agentic identities to authenticate themselves via X509 cert bound tokens. We are limiting the scope here to only cloud run based agentic workloads.