Enable Custom Proxy in WorkloadIdentityCredential #47041
Conversation
anannya03
requested review from
KarishmaGhiya,
chlowell,
maorleger,
pvaneck and
xiangyan99
github-project-automation
bot
moved this to Untriaged
in Azure Identity SDK Improvements
There was a problem hiding this comment.
Pull Request Overview
Enables support for a custom token proxy (including SNI and custom CA trust) for WorkloadIdentityCredential.
- Adds proxy configuration parsing and validation via environment variables.
- Introduces a custom HttpClient/HttpResponse pair to route token requests through the proxy with optional CA override and SNI.
- Adds an SNI-capable SSLSocketFactory wrapper.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| IdentitySslUtil.java | Adds SNI-supporting SSLSocketFactory wrapper for custom proxy connections. |
| ProxyConfig.java | Introduces immutable holder for token proxy parameters (URL, SNI, CA info). |
| CustomTokenProxyHttpResponse.java | Implements HttpResponse abstraction over HttpURLConnection for proxy responses. |
| CustomTokenProxyHttpClient.java | Implements HttpClient that rewrites token requests, applies custom trust material and optional SNI. |
| CustomTokenProxyConfiguration.java | Parses and validates environment variables to build ProxyConfig. |
| import javax.net.ssl.HostnameVerifier; | ||
| import javax.net.ssl.HttpsURLConnection; | ||
| import javax.net.ssl.SNIHostName; | ||
| import javax.net.ssl.SNIServerName; |
There was a problem hiding this comment.
The import SNIServerName is unused in this file; remove it to reduce noise and keep imports minimal.
| import javax.net.ssl.SNIServerName; |
Copilot uses AI. Check for mistakes.
| private final int statusCode; | ||
| private final HttpHeaders headers; | ||
| private final HttpURLConnection connection; | ||
| private byte[] cachedRequestBodyBytes; |
There was a problem hiding this comment.
The field name cachedRequestBodyBytes is misleading because it stores the response body; rename to cachedResponseBodyBytes for clarity and consistency with its usage in getBodyAsByteArray().
Copilot generated this review using guidance from repository custom instructions.
| return headers; | ||
| } | ||
|
|
||
| public int extractStatusCode(HttpURLConnection connection) { |
There was a problem hiding this comment.
This helper method is only used internally and doesn't need to be public; change its visibility to private to narrow the API surface.
| public int extractStatusCode(HttpURLConnection connection) { | |
| private int extractStatusCode(HttpURLConnection connection) { |
Copilot uses AI. Check for mistakes.
| import java.io.ByteArrayInputStream; | ||
| import java.io.IOException; | ||
| import java.io.InputStream; | ||
| import java.lang.reflect.MalformedParametersException; |
There was a problem hiding this comment.
MalformedParametersException (from java.lang.reflect) is unrelated here and never thrown; remove the import and the throws clause (or replace with MalformedURLException if actually needed) to prevent confusion and inaccurate exception signaling.
| import java.lang.reflect.MalformedParametersException; |
Copilot uses AI. Check for mistakes.
| } | ||
|
|
||
|
|
||
| private URL rewriteTokenRequestForProxy(URL originalUrl) throws MalformedParametersException{ |
There was a problem hiding this comment.
MalformedParametersException (from java.lang.reflect) is unrelated here and never thrown; remove the import and the throws clause (or replace with MalformedURLException if actually needed) to prevent confusion and inaccurate exception signaling.
Copilot uses AI. Check for mistakes.
| import java.io.IOException; | ||
| import java.io.InputStream; | ||
| import java.lang.reflect.MalformedParametersException; | ||
| import java.lang.reflect.Proxy; |
There was a problem hiding this comment.
The import java.lang.reflect.Proxy is unused; remove it to keep imports clean.
| import java.lang.reflect.Proxy; |
Copilot uses AI. Check for mistakes.
| connection.setRequestMethod(request.getHttpMethod().toString()); | ||
| // connection.setConnectTimeout(10_000); | ||
| // connection.setReadTimeout(20_000); | ||
| connection.setDoOutput(true); |
There was a problem hiding this comment.
setDoOutput(true) is applied unconditionally; restrict this to methods that actually send a body (e.g., POST/PUT/PATCH) to avoid unintended semantics on GET/DELETE requests.
| connection.setDoOutput(true); | |
| // Only set doOutput for methods that support a body | |
| String method = request.getHttpMethod().toString(); | |
| if ("POST".equalsIgnoreCase(method) || "PUT".equalsIgnoreCase(method) || "PATCH".equalsIgnoreCase(method)) { | |
| connection.setDoOutput(true); | |
| } |
Copilot uses AI. Check for mistakes.
| if (request.getBodyAsBinaryData() != null) { | ||
| byte[] bytes = request.getBodyAsBinaryData().toBytes(); | ||
| if (bytes != null && bytes.length > 0) { | ||
| connection.getOutputStream().write(bytes); |
There was a problem hiding this comment.
The output stream is not closed or flushed, leading to a potential resource leak; wrap the write in a try-with-resources: try (OutputStream os = connection.getOutputStream()) { os.write(bytes); }.
| connection.getOutputStream().write(bytes); | |
| try (java.io.OutputStream os = connection.getOutputStream()) { | |
| os.write(bytes); | |
| } |
Copilot uses AI. Check for mistakes.
|
|
||
| private SSLContext getSSLContext() { | ||
| try { | ||
| // If no CA override provide, use default |
There was a problem hiding this comment.
Corrected wording from 'provide' to 'provided'.
| // If no CA override provide, use default | |
| // If no CA override provided, use default |
Copilot uses AI. Check for mistakes.
1 participant
Description
Please add an informative description that covers that changes made by the pull request and link all relevant issues.
If an SDK is being regenerated based on a new swagger spec, a link to the pull request containing these swagger spec changes has been included above.
All SDK Contribution checklist:
General Guidelines and Best Practices
Testing Guidelines