-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Add Workload Identity Auth Support in KV JCA #47051
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
Conversation
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.
Pull Request Overview
This PR adds support for Azure Workload Identity authentication for Azure Kubernetes Service (AKS) workloads in the Key Vault JCA library. This enables credential-free authentication for AKS pods using federated tokens.
Key changes:
- Implemented federated token-based authentication flow using environment variables (
AZURE_FEDERATED_TOKEN_FILE,AZURE_CLIENT_ID,AZURE_TENANT_ID) - Added automatic detection and prioritization of Workload Identity authentication
- Updated documentation with detailed authentication method examples and selection logic
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| sdk/keyvault/azure-security-keyvault-jca/src/main/java/com/azure/security/keyvault/jca/implementation/utils/AccessTokenUtil.java | Added Workload Identity support with detection logic, token file reading, and OAuth2 client assertion flow implementation |
| sdk/keyvault/azure-security-keyvault-jca/README.md | Added comprehensive authentication method documentation with examples for Service Principal, Managed Identity, and Workload Identity |
| sdk/keyvault/azure-security-keyvault-jca/CHANGELOG.md | Documented new Workload Identity feature in the unreleased version section |
| requestBody.append("grant_type=client_credentials") | ||
| .append("&client_id=").append(clientId) | ||
| .append("&client_assertion_type=urn:ietf:params:oauth:client-assertion-type:jwt-bearer") | ||
| .append("&client_assertion=").append(federatedToken) | ||
| .append("&scope=").append(resource); |
Copilot
AI
Nov 4, 2025
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 clientId, federatedToken, and resource parameters should be URL-encoded before being appended to the request body to prevent malformed requests or potential injection issues. Other methods in this class (e.g., getAccessToken on line 149 and KeyVaultClient.getAccessTokenByHttpRequest on lines 196-199) properly URL-encode parameters. Add URLEncoder.encode(parameter, \"UTF-8\") for these values.
| requestBody.append("grant_type=client_credentials") | |
| .append("&client_id=").append(clientId) | |
| .append("&client_assertion_type=urn:ietf:params:oauth:client-assertion-type:jwt-bearer") | |
| .append("&client_assertion=").append(federatedToken) | |
| .append("&scope=").append(resource); | |
| try { | |
| requestBody.append("grant_type=client_credentials") | |
| .append("&client_id=").append(URLEncoder.encode(clientId, "UTF-8")) | |
| .append("&client_assertion_type=urn:ietf:params:oauth:client-assertion-type:jwt-bearer") | |
| .append("&client_assertion=").append(URLEncoder.encode(federatedToken, "UTF-8")) | |
| .append("&scope=").append(URLEncoder.encode(resource, "UTF-8")); | |
| } catch (UnsupportedEncodingException e) { | |
| LOGGER.log(WARNING, "Failed to URL-encode parameters for access token request.", e); | |
| return null; | |
| } |
| java.nio.file.Path path = java.nio.file.Paths.get(tokenFilePath); | ||
| String token = new String(java.nio.file.Files.readAllBytes(path), java.nio.charset.StandardCharsets.UTF_8).trim(); |
Copilot
AI
Nov 4, 2025
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.
Use standard Java imports instead of fully-qualified class names. Add import java.nio.file.Path;, import java.nio.file.Paths;, import java.nio.file.Files;, and import java.nio.charset.StandardCharsets; at the top of the file, then use Path, Paths, Files, and StandardCharsets directly. This follows the existing code style where other Java classes are imported.
|
|
||
| try { | ||
| String tokenFilePath = System.getenv(PROPERTY_AZURE_FEDERATED_TOKEN_FILE); | ||
| String clientId = System.getenv(PROPERTY_AZURE_CLIENT_ID); |
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.
I think it would be better to use the fields (clientId and tenantId) of KeyVaultClient and fallback to the environment variables only if unset. This would allow configuring a different identity then the default pod identity.
String clientId = (identity != null && !identity.isEmpty()) ? identity : System.getenv(PROPERTY_AZURE_CLIENT_ID)|
Any ETA for this implementation ? |
|
Hi @g2vinay. Thank you for your interest in helping to improve the Azure SDK experience and for your contribution. We've noticed that there hasn't been recent engagement on this pull request. If this is still an active work stream, please let us know by pushing some changes or leaving a comment. Otherwise, we'll close this out in 7 days. |
|
Hi Azure SDK Team, Our team is currently blocked from moving to AKS due to the absence of this feature. Please prioritize this enhancement, as it is critical for our migration plans. @g2vinay |
| * `azure.keyvault.tenant-id`: The Microsoft Entra ID tenant ID (required for Service Principal authentication). | ||
| * `azure.keyvault.client-id`: The client/application ID (required for Service Principal authentication). | ||
| * `azure.keyvault.client-secret`: The client secret (required for Service Principal authentication). | ||
| * `azure.keyvault.managed-identity`: The user-assigned managed identity object ID (optional, for user-assigned managed identity). |
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: Change above 4 lines to format like this: (Only required when using ...)
| **Note:** | ||
| - **For Service Principal authentication**: Set all four properties (`uri`, `tenant-id`, `client-id`, `client-secret`) | ||
| - **For Managed Identity authentication**: Only set `azure.keyvault.uri` (and optionally `managed-identity` for user-assigned identity) | ||
| - **For Workload Identity authentication (AKS)**: Only set `azure.keyvault.uri` - the provider automatically detects Workload Identity environment variables |
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.
How about avoid duplication by adding link to other section of current file?
| && System.getenv(PROPERTY_AZURE_CLIENT_ID) != null | ||
| && !System.getenv(PROPERTY_AZURE_CLIENT_ID).isEmpty() | ||
| && System.getenv(PROPERTY_AZURE_TENANT_ID) != null | ||
| && !System.getenv(PROPERTY_AZURE_TENANT_ID).isEmpty(); |
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.
How about using a util method like in this PR: https://github.com/Azure/azure-sdk-for-java/pull/47749/files
| // Read the federated token from the file | ||
| String federatedToken = readTokenFromFile(tokenFilePath); | ||
|
|
||
| if (federatedToken == null || federatedToken.isEmpty()) { |
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 util method mentioned above can be used here, too.
| private static String readTokenFromFile(String tokenFilePath) throws IOException { | ||
| LOGGER.entering("AccessTokenUtil", "readTokenFromFile", tokenFilePath); | ||
|
|
||
| java.nio.file.Path path = java.nio.file.Paths.get(tokenFilePath); |
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.
Import the package here.
rujche
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.
Please update the PR according to the comments and this PR: https://github.com/Azure/azure-sdk-for-java/pull/47749/files
|
Hi @g2vinay. Thank you for your contribution. Since there hasn't been recent engagement, we're going to close this out. Feel free to respond with a comment containing |
|
Sorry, @writemevenkat, only the original author can reopen this pull request. |
Adds Workload Identity Auth Support in KV JCA