Conversation
…t Credential, and Managed Identity Credential Fixes #118
There was a problem hiding this comment.
Pull request overview
This pull request reinstates support for multiple Azure authentication methods that were broken when the Azure SDK for Rust removed the EnvironmentCredential type. The PR implements a credential chain that tries different authentication methods in order of precedence, fixing issue #118 where client secret authentication was broken.
Changes:
- Added a new
obtain_credential()function that implements a credential chain, trying WorkloadIdentityCredential, ClientSecretCredential, DeveloperToolsCredential, and ManagedIdentityCredential in order - Added imports for
Secret,ClientSecretCredential,ManagedIdentityCredential, andWorkloadIdentityCredentialfrom theazure_identitycrate - Modified
AzureRegistry::new()to use the newobtain_credential()function instead of directly creating aDeveloperToolsCredential
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| (azure_tenant_id, azure_client_id, azure_client_secret) | ||
| { | ||
| let secret = Secret::new(client_secret); | ||
| if let Ok(credential) = ClientSecretCredential::new(&tenant_id, client_id, secret, None) { |
There was a problem hiding this comment.
The parameters are passed inconsistently: tenant_id as a reference (&tenant_id) while client_id is passed by value. While this compiles, it creates an asymmetry that could confuse future maintainers. Consider verifying this matches the intended API signature and adding a comment explaining the reason for the different passing styles if this is intentional.
| if let Ok(credential) = ClientSecretCredential::new(&tenant_id, client_id, secret, None) { | |
| if let Ok(credential) = ClientSecretCredential::new(tenant_id, client_id, secret, None) { |
| debug!("Using ManagedIdentityCredential for authentication"); | ||
| return Ok(credential); | ||
| } | ||
| Err("No suitable credential found".into()) |
There was a problem hiding this comment.
The error message "No suitable credential found" could be more helpful by providing guidance on what the user should check. Consider including information about which authentication methods were tried and what environment variables or configurations are needed (e.g., "No suitable credential found. Please ensure one of the following is configured: WorkloadIdentityCredential (AZURE_FEDERATED_TOKEN_FILE), ClientSecretCredential (AZURE_TENANT_ID, AZURE_CLIENT_ID, AZURE_CLIENT_SECRET), Azure CLI login, or Managed Identity.").
| Err("No suitable credential found".into()) | |
| Err("No suitable credential found. Please ensure one of the following is configured: \ | |
| WorkloadIdentityCredential (AZURE_FEDERATED_TOKEN_FILE), ClientSecretCredential \ | |
| (AZURE_TENANT_ID, AZURE_CLIENT_ID, AZURE_CLIENT_SECRET), Azure CLI / DeveloperToolsCredential \ | |
| login, or a Managed Identity environment.".into()) |
| if let Ok(credential) = WorkloadIdentityCredential::new(None) { | ||
| debug!("Using WorkloadIdentityCredential for authentication"); | ||
| return Ok(credential); |
There was a problem hiding this comment.
Consider adding debug logging when credential initialization fails to aid in troubleshooting. For example, log when WorkloadIdentityCredential creation fails before trying the next credential type. This would help users understand why a particular credential method wasn't used without exposing sensitive information in production logs.
| if let Ok(credential) = WorkloadIdentityCredential::new(None) { | |
| debug!("Using WorkloadIdentityCredential for authentication"); | |
| return Ok(credential); | |
| match WorkloadIdentityCredential::new(None) { | |
| Ok(credential) => { | |
| debug!("Using WorkloadIdentityCredential for authentication"); | |
| return Ok(credential); | |
| } | |
| Err(e) => { | |
| debug!( | |
| "Failed to create WorkloadIdentityCredential, falling back to other authentication methods: {}", | |
| e | |
| ); | |
| } |
| fn obtain_credential( | ||
| ) -> Result<Arc<dyn azure_core::credentials::TokenCredential>, Box<dyn std::error::Error>> { | ||
| // We need to try various different types of credentials. The order of precedence is: | ||
| // - "Environment variables": in legacy Azure SDKs this included: | ||
| // - WorkloadIdentityCredential | ||
| // - ClientSecretCredential | ||
| // - Azure CLI credentials | ||
| // - Managed Identity: | ||
| // - AppServiceManagedIdentityCredential | ||
| // - VirtualMachineManagedIdentityCredential | ||
| let azure_tenant_id = std::env::var("AZURE_TENANT_ID").ok(); | ||
| let azure_client_id = std::env::var("AZURE_CLIENT_ID").ok(); | ||
| let azure_client_secret = std::env::var("AZURE_CLIENT_SECRET").ok(); | ||
|
|
||
| if let Ok(credential) = WorkloadIdentityCredential::new(None) { | ||
| debug!("Using WorkloadIdentityCredential for authentication"); | ||
| return Ok(credential); | ||
| } | ||
| // Only use a ClientSecretCredential if all three of the relevant environment variables are set. | ||
| if let (Some(tenant_id), Some(client_id), Some(client_secret)) = | ||
| (azure_tenant_id, azure_client_id, azure_client_secret) | ||
| { | ||
| let secret = Secret::new(client_secret); | ||
| if let Ok(credential) = ClientSecretCredential::new(&tenant_id, client_id, secret, None) { | ||
| debug!("Using ClientSecretCredential for authentication"); | ||
| return Ok(credential); | ||
| } | ||
| } | ||
| if let Ok(credential) = DeveloperToolsCredential::new(None) { | ||
| debug!("Using DeveloperToolsCredential for authentication"); | ||
| return Ok(credential); | ||
| } | ||
| if let Ok(credential) = ManagedIdentityCredential::new(None) { | ||
| debug!("Using ManagedIdentityCredential for authentication"); | ||
| return Ok(credential); | ||
| } | ||
| Err("No suitable credential found".into()) | ||
| } |
There was a problem hiding this comment.
The new obtain_credential function lacks test coverage. Given that other modules in this codebase have comprehensive test coverage (e.g., message.rs, processor.rs), consider adding unit tests for the credential chain logic. Tests should verify:
- Correct precedence order when multiple credential types are available
- Proper fallback behavior when credentials fail to initialize
- Appropriate error handling when no credentials are available
- Environment variable handling for ClientSecretCredential
This is particularly important since this function addresses a previously broken authentication mechanism (issue #118).
|
LGTM |
|
🎉 This PR is included in version 1.0.15 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
Based on the credentials in:
we can determine the methods for how each of the credential types worked. From our README, we have a defined order of precedence; we reinstate that ordering and explicitly try and obtain each type of credential.
Fixes #118