implement identity bindings - #5083
Conversation
|
Azure Pipelines: Successfully started running 1 pipeline(s). 1 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
github-actions
Bot
added
Azure.Identity
|
Thank you for your contribution Mitch Connors (@therealmitchconnors)! We will review the pull request and get back to you soon. |
github-project-automation
Bot
moved this to Untriaged
in Azure Identity SDK Improvements
There was a problem hiding this comment.
Pull request overview
Adds opt-in AKS workload identity binding support to WorkloadIdentityCredential.
Changes:
- Adds a custom token-proxy transport with CA and SNI support.
- Adds proxy configuration, tests, troubleshooting guidance, and feature dependencies.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
sdk/identity/azure_identity/TROUBLESHOOTING.md |
Documents proxy configuration failures. |
sdk/identity/azure_identity/src/workload_identity_credential.rs |
Integrates opt-in proxy support and tests. |
sdk/identity/azure_identity/src/lib.rs |
Registers the proxy module. |
sdk/identity/azure_identity/src/custom_token_proxy.rs |
Implements proxy validation, TLS, URL rewriting, and CA rotation. |
sdk/identity/azure_identity/Cargo.toml |
Adds the proxy feature and reqwest dependency. |
Cargo.lock |
Records the dependency update. |
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
Heath Stewart (heaths)
left a comment
Heath Stewart (heaths)
left a comment
There was a problem hiding this comment.
Please update the PR title to be more descriptive as well.
| "aarch", | ||
| "accountendpoint", | ||
| "accountkey", | ||
| "addrs", |
There was a problem hiding this comment.
Service directory "misspellings" should go into sdk/identity/.cspell.json. There are agent instructions that help do this e.g., the /check-spelling skill.
| [features] | ||
| default = ["azure_core/default"] | ||
| default = ["azure_core/default", "azure_proxy"] | ||
| azure_proxy = ["dep:reqwest", "azure_core/reqwest_rustls"] |
There was a problem hiding this comment.
- Can you describe this more?
- No need to repeat "azure". It's a feature in an
azure_*crate already. No other crate does that.
| ### Features Added | ||
|
|
||
| - Added support for Arc-connected servers when using the `ManagedIdentityCredential`. | ||
| - Added opt-in AKS identity binding support to `WorkloadIdentityCredentialOptions` through `enable_azure_proxy`. |
There was a problem hiding this comment.
I recommend spelling out "AKS" in any documentation at least the first time it's used. Better for SEO and for people who may not be as familiar with it.
| /// requests tokens directly from Microsoft Entra ID. See the | ||
| /// [AKS identity bindings documentation](https://learn.microsoft.com/azure/aks/identity-bindings-concepts) | ||
| /// for guidance about enabling this option. | ||
| pub enable_azure_proxy: bool, |
There was a problem hiding this comment.
Is the word "azure" here really needed? This is all "azure". If it really is a product name called "Azure Proxy" fine, but seems redundant.
| // Licensed under the MIT License. | ||
|
|
||
| use crate::env::Env; | ||
| #[cfg(feature = "azure_proxy")] |
There was a problem hiding this comment.
Is this module even needed if the feature isn't enabled? Probably easier just to conditionally include it from lib.rs if enabled, and have fewer cfg conditions in workload_identity_credential.rs.
| let ca_file = optional_env(env, AZURE_KUBERNETES_CA_FILE); | ||
| let ca_data = optional_env(env, AZURE_KUBERNETES_CA_DATA); | ||
|
|
||
| if proxy.is_none() { |
There was a problem hiding this comment.
Use:
let Some(proxy) = proxy else {
// ...
}Then you don't need the proxy.expect further down. While correct, this is still a code smell and initially raises red flags. Generally, unwrap() or expect() should not be in production code if they can be avoided otherwise.
| ) | ||
| } | ||
|
|
||
| fn optional_env(env: &Env, name: &str) -> Option<String> { |
There was a problem hiding this comment.
I'd add #[inline(always)] here or even make this a macro.
| env.var(name).ok().filter(|value| !value.is_empty()) | ||
| } | ||
|
|
||
| fn invalid_configuration(name: &'static str, message: impl Into<String>) -> Error { |
There was a problem hiding this comment.
Same here for #[inline(always)] or macro.
| let mut builder = reqwest::Client::builder() | ||
| .tls_backend_rustls() | ||
| .https_only(true) | ||
| .redirect(reqwest::redirect::Policy::none()); |
There was a problem hiding this comment.
This is going to be a problem for 1P who won't use reqwest. Probably need to instead define a pub trait that reqwest::Client can implement when the reqwest feature is enabled for azure_identity—need to define features similar to how we do for azure_core—but so can other HTTP stacks, which means you also need to expose the transport like how we do for azure_core::http::ClientOptions.
| fn read_ca_file(path: &std::path::Path) -> azure_core::Result<Vec<u8>> { | ||
| let data = fs::read(path).with_context_fn(ErrorKind::Credential, || { | ||
| format!( | ||
| "failed to read {AZURE_KUBERNETES_CA_FILE} {}", |
There was a problem hiding this comment.
| "failed to read {AZURE_KUBERNETES_CA_FILE} {}", | |
| "failed to read {AZURE_KUBERNETES_CA_FILE}: {}", |
github-project-automation
Bot
moved this from Untriaged
to In Progress
in Azure Identity SDK Improvements
Fixes #3456
Adds a custom token proxy implementing identity binding.
See attached issue and links for design, compare to implementation in go client.