Repository navigation
feat(temporal): add workload identity authentication - #561
aringuyen3 wants to merge 3 commits into
Conversation
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
Close because the test for #560 was triggered |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if authenticated_options.get("tls") is None: | ||
| authenticated_options["tls"] = False | ||
| client = await Client.connect(api_key=token.token, **authenticated_options) |
There was a problem hiding this comment.
When TEMPORAL_AUTH_SCOPE is set, both client factories omit tls, so this branch sends the Azure bearer token with tls=False. If the network does not provide separate encryption, someone watching the traffic can copy and reuse the token until it expires. Default authenticated connections to TLS, and require an explicit choice for plaintext connections.
How this was verified: Both factories pass connection options without tls, and connect_temporal_client adds tls=False alongside the token in api_key.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agentex/lib/core/clients/temporal/auth.py
Line: 98-100
Comment:
**Tokens travel without encryption**
When `TEMPORAL_AUTH_SCOPE` is set, both client factories omit `tls`, so this branch sends the Azure bearer token with `tls=False`. If the network does not provide separate encryption, someone watching the traffic can copy and reuse the token until it expires. Default authenticated connections to TLS, and require an explicit choice for plaintext connections.
**How this was verified:** Both factories pass connection options without `tls`, and `connect_temporal_client` adds `tls=False` alongside the token in `api_key`.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
| except Exception: | ||
| await credential.close() | ||
| raise |
There was a problem hiding this comment.
Cancelling startup during credential.get_token or Client.connect skips this cleanup because asyncio.CancelledError is not an Exception. No refresh session exists yet to close the credential, so startup timeouts can leave its HTTP resources open. Close the credential on cancellation as well, then re-raise it.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agentex/lib/core/clients/temporal/auth.py
Line: 101-103
Comment:
**Cancelled startup skips cleanup**
Cancelling startup during `credential.get_token` or `Client.connect` skips this cleanup because `asyncio.CancelledError` is not an `Exception`. No refresh session exists yet to close the credential, so startup timeouts can leave its HTTP resources open. Close the credential on cancellation as well, then re-raise it.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| TemporalTokenSession( | ||
| client=client, | ||
| credential=credential, | ||
| scope=scope, | ||
| token=token, | ||
| ).start() | ||
| return client |
There was a problem hiding this comment.
Each authenticated connection starts a task that holds its client and credential forever. Callers receive only the client and have no supported way to stop that task. Creating and discarding clients in the same event loop leaves every old session refreshing tokens and holding resources. Expose an async close operation and call it when the worker or application shuts down.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agentex/lib/core/clients/temporal/auth.py
Line: 105-111
Comment:
**Old clients keep refreshing**
Each authenticated connection starts a task that holds its client and credential forever. Callers receive only the client and have no supported way to stop that task. Creating and discarding clients in the same event loop leaves every old session refreshing tokens and holding resources. Expose an async close operation and call it when the worker or application shuts down.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
#560
Default authenticated connections should use TLS before this PR merges.
Fix with agent prompt
Summary
Adds Azure Workload Identity authentication when
TEMPORAL_AUTH_SCOPEis set.Diagram
%%{init: {'theme': 'neutral'}}%% flowchart TD A[Temporal client factory] --> B{TEMPORAL_AUTH_SCOPE set?} B -->|No| C[Connect with existing options] B -->|Yes| D[Get Azure token] D --> E[Connect with token; missing TLS becomes false] E --> F[Return client and start refresh task] F --> G[Wait until refresh time] G --> H{Get valid token?} H -->|Yes| I[Update client.api_key] H -->|No| J[Retry after 30 seconds] I --> G J --> HReviews (1) · Last reviewed commit: "test(temporal): import TLSConfig from te..." · Reviewed by Greptile