-
Notifications
You must be signed in to change notification settings - Fork 319
Authentication errors include HTTP responses #3302
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
base: main
Are you sure you want to change the base?
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 enhances error handling in the Azure Identity SDK by including raw HTTP responses in credential errors. This allows users to inspect the full response (headers, body, status) when authentication fails, improving debuggability.
Key changes:
- Introduced
handle_entra_response()to centralize Entra ID token response processing and error handling - Modified
authentication_error()to wrap inner errors for certain credentials (AzurePipelinesCredential, WorkloadIdentityCredential) - Updated error message format to use newline before troubleshooting link instead of period
- Added
error_codesfield toEntraIdErrorResponsestruct
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
sdk/identity/azure_identity/src/lib.rs |
Added handle_entra_response() for centralized error handling; updated authentication_error() to wrap errors and extract error codes from responses |
sdk/identity/azure_identity/src/client_secret_credential.rs |
Replaced custom response handling with handle_entra_response(); updated tests to verify error structure |
sdk/identity/azure_identity/src/client_certificate_credential.rs |
Replaced custom response handling with handle_entra_response(); updated tests to verify error structure |
sdk/identity/azure_identity/src/client_assertion_credential.rs |
Replaced custom response handling with handle_entra_response(); added authentication_error() call in get_token(); updated tests |
sdk/identity/azure_identity/src/workload_identity_credential.rs |
Updated tests to verify wrapped error structure with raw response |
sdk/identity/azure_identity/src/managed_identity_credential.rs |
Added run_error_response_test() helper function; replaced old test with parameterized tests for IMDS and App Service sources |
sdk/identity/azure_identity/src/imds_managed_identity_credential.rs |
Updated error handling to use ErrorKind::HttpResponse with raw response; capitalized error messages |
sdk/identity/azure_identity/src/azure_pipelines_credential.rs |
Updated error handling to include raw response; updated test to verify error structure |
sdk/identity/azure_identity/TROUBLESHOOTING.md |
Added example code demonstrating how to access raw HTTP responses from credential errors |
sdk/identity/azure_identity/CHANGELOG.md |
Added entry in Features Added section documenting the new error handling capability |
| ErrorKind::HttpResponse { status: status_code, error_code: Some(status_code.canonical_reason().to_string()), raw_response: None }, | ||
| format!("{status_code} response from the OIDC endpoint. Check service connection ID and pipeline configuration. {err_headers}"), | ||
| ) | ||
| let message = format!( |
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'm assuming this is because status and err_headers are references to resp and thus you can't call format! in the with_message call, right?
I had to spend a couple of seconds to figure that out so it might be worth a comment.
heaths
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.
Nothing blocking, but there's a couple things I'd clean up now and things to consider for later.
| #[derive(Debug, Default, Deserialize)] | ||
| #[serde(default)] | ||
| struct EntraIdErrorResponse { | ||
| error_codes: Vec<i32>, |
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.
For the error_description, mainly, you might rely on borrowing for less memory overhead if you're just trying to pick out a few things. We do that for ErrorResponseInternal and you can find more details at https://serde.rs/lifetimes.html#borrowing-data-in-a-derived-impl.
| let mut details = err.to_string(); | ||
| if wrap_inner_error { | ||
| // details is e.g. "ClientAssertionCredential authentication failed. <more details>". | ||
| // We want only the "<more details>" part because we want to return an error like | ||
| // "OtherCredential authentication failed. <more details>". |
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.
Not blocking, but I'd urge a cheaper way in the future. If you could do something in err to tell it came from an internal implementation detail or even pass something down to ClientAssertionCredential to tell it which name to use, you could avoid all this fragile and (slightly) time-wasting string manipulation.
|
|
||
| ### Features Added | ||
|
|
||
| - A `get_token()` error motivated by an HTTP response carries that response. See the [troubleshooting guide](https://aka.ms/azsdk/rust/identity/troubleshoot#find-relevant-information-in-errors) for example code showing how to access the response. |
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.
"motivated". Perhaps "caused"? Even if that's an arcane but valid use - which I normally appreciate - I imagine users might be scratching their heads as well.
|
|
||
| - __Correlation ID and Timestamp__: The correlation ID and timestamp identify the request in server-side logs. This information can be useful to support engineers diagnosing unexpected Microsoft Entra ID failures. | ||
|
|
||
| Many credential errors also carry the HTTP response that motivated them. The example below demonstrates how to access that response in such a case. |
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.
| Many credential errors also carry the HTTP response that motivated them. The example below demonstrates how to access that response in such a case. | |
| Many credential errors also carry the HTTP response that caused them. The example below demonstrates how to access that response in such a case. |
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.
But they still get the message you want by default, right? This is just to get more details for...why? You might put that in the comments and even explain in what scenarios they may want to drill into details. Ideally, the message you formatted above is what they see if they just print the error e.g., eprintln!("{err}").
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.
Yes; printing the error produces a message like the above which probably contains everything developers need. The raw response is useful mainly because it includes headers. I'll refocus this example on that to avoid implying the response is more generally useful. Really I just want to publish this snippet so that the rare developer who wants the response has some guidance toward getting it
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 the snippet is fine. I just wanted to make it clear that they probably don't need to, and maybe telling them for what reasons they might do so, like getting headers, like you said. You might just keep the code as-is, but explain a reason they might want to do this i.e., it shouldn't be the norm.
| } | ||
| } | ||
| _ => { | ||
| // TODO: handle other kinds of error |
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.
Tip: idiomatic thing here is todo!("handle other kinds of errors").
get_token()implementations consistently return an error ofErrorKind::Credentialand when the error is motivated by an HTTP response, the caller can downcast the error to access that response. Closes #3127