-
Notifications
You must be signed in to change notification settings - Fork 261
feat: implement agentic identities authentication via cert bound tokens. #1842
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: main
Are you sure you want to change the base?
Conversation
…trol the sleep time for threads. Fixed the agent_spiffe_cert.pem
nbayati
left a comment
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. "
| long startTime = timeService.currentTimeMillis(); | ||
| boolean warned = false; | ||
|
|
||
| while (true) { |
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.
The current while (true) polling loop relies on checking the elapsed time in each iteration. While this works, a more deterministic and clearer approach would be to pre-calculates the number of fast and slow poll cycles and builds a list of sleep intervals. Then we can use a simple for loop to iterate through these fixed intervals.
This will not only make the code more readable, but it'd also be less error-prone and safer to maintain over the long term.
| } catch (Exception e) { | ||
| // Ignore exceptions during polling and retry | ||
| LOGGER.log(Level.FINE, "Error while polling for certificate files"); | ||
| } |
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.
Catching any exception and logging it at every loop iteration can make the logs noisy, instead you can use the warned flag to surface the user friendly warning here once.
We can also limit the catch to only the type of exceptions we are interested in, which would be the IOException thrown from extractCertPathFromConfig().
e0d450a to
5353838
Compare
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.