-
Notifications
You must be signed in to change notification settings - Fork 127
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
ImdsManagedIdentitySource (Managed Identity authentication) exception when parsing the identity endpoint response #4723
Comments
Thank you Mateusz! I think the json type that server returns has changed, and we need to fix it in the SDK, I'll look into this. |
## 1.5.1 (2023-07-06) ### Bugs Fixed - [microsoft#4723]](Azure/azure-sdk-for-cpp#4723) Accept a wider variety of token responses.
…32425) * [azure-identity-cpp] Update to 1.5.1 ## 1.5.1 (2023-07-06) ### Bugs Fixed - [#4723]](Azure/azure-sdk-for-cpp#4723) Accept a wider variety of token responses. * [azure-core-cpp] Update to 1.10.1 ## 1.10.1 (2023-07-06) ### Breaking Changes - [[#4662]](Azure/azure-sdk-for-cpp#4662) `Azure::Core::Operation<T>::GetRawResponseInternal()` is now deprecated and no longer requires an overload. ### Other Changes - Empty diagnostic messages will no longer be generated. * [azure-core-amqp-cpp] Update to 1.0.0-beta.1 ## 1.0.0-beta.1 (2023-07-06) ### Features Added - Initial release * Dependency search fix * Dependency link fix * x-add-version --------- Co-authored-by: Anton Kolesnyk <[email protected]>
The fix is available in Identity 1.5.1, which was released and is now available on github and in vcpkg registries. |
Anton, I was able to use v1.5.1 to obtain an access token, thank you for the quick fix! |
Thanks Mateusz, I'm very glad to hear that. And also thank you for all the details you put into this issue! I also improved the logging around token parsing (#4744), so should something like this happen the next time, you could
I.e. there hopefully would be no need to debug the SDK in order to get detailed enough information to report the bug. Although I also hope that we have it now covered once and for all :) |
### Rationale for this change We want to use the Azure SDK for C++ to read/write to Azure blob storage. Obviously this is pretty important for building an `AzureFileSystem`. ### What changes are included in this PR? Builds the the relevant parts of the azure SDK as a cmake external project. Adds a couple of simple tests that just assert that the Azure SDK is working and a couple of lines in `AzureFileSystem` to initialise the blob storage client to ensure the build is working correctly in all environments. I started with the build setup from #12914 but I did make few changes. 1. Although its atypical for this project we chose to switch from cmake's `ExternalProject` to `FetchContent`. `FetchContent` is recomended by the Azure docs https://github.com/Azure/azure-sdk-for-cpp#cmake-project--fetch-content. It also solves a few problems including: automatically linking system curl and ssl instead of bootstrapping vcpkg and installing curl and ssl from there. 2. Only build one version of the Azure SDK for C++ because it contains all the components. Previously we were unnecessarily building 5 different versions of the whole thing on top of each other. This created race conditions for which version each component came from. 3. We are using `azure-core_1.10.2` which is a very recent version. There are a couple of important reasons for this 1. [an important managed identity fix](Azure/azure-sdk-for-cpp#4723), 2. [fixed support for curl versions < 7.71.0](Azure/azure-sdk-for-cpp#4792). There will be follow up PRs to enable Azure in the manylinux builds. We need to update `vcpkg` first so we can get a version of the Azure SDK which contains [an important managed identity fix](Azure/azure-sdk-for-cpp#4723). ### Are these changes tested? Yes. There is a simple test that just runs the Azure client against azurite. Additionally just initialising the client in `AzureFileSystem` goes a long way towards ensuring the build is working. ### Are there any user-facing changes? No * Closes: #29847 Lead-authored-by: Thomas Newton <[email protected]> Co-authored-by: Sutou Kouhei <[email protected]> Co-authored-by: Sutou Kouhei <[email protected]> Co-authored-by: Antoine Pitrou <[email protected]> Co-authored-by: shefali singh <[email protected]> Signed-off-by: Sutou Kouhei <[email protected]>
Describe the bug
Hello,
I'm hitting an issue with the Managed Identity authentication (ManagedIdentityCredential class). I'm getting the following exception when trying to obtain an access token:
GetToken(): [json.exception.type_error.302] type must be number, but is string
The exception comes from the TokenCredentialImpl::ParseToken method in token_credential_impl.cpp when trying to obtain the expiresInPropertyName value from parsed json. It looks like the code expects json to contain a number, but the identity endpoint to which the request is sent returns a string instead.
Exception or Stack Trace
OLEDB sandbox.exe!
Azure::Identity::_detail::TokenCredentialImpl::GetToken'::
1'::catch$20() Line 136 C++[External Code]
OLEDB sandbox.exe!Azure::Identity::_detail::TokenCredentialImpl::GetToken(const Azure::Core::Context & context, const std::function<std::unique_ptr<Azure::Identity::_detail::TokenCredentialImpl::TokenRequest,std::default_deleteAzure::Identity::_detail::TokenCredentialImpl::TokenRequest> __cdecl(void)> & createRequest, const std::function<std::unique_ptr<Azure::Identity::_detail::TokenCredentialImpl::TokenRequest,std::default_deleteAzure::Identity::_detail::TokenCredentialImpl::TokenRequest> __cdecl(enum Azure::Core::Http::HttpStatusCode,Azure::Core::Http::RawResponse const &)> & shouldRetry) Line 125 C++
OLEDB sandbox.exe!Azure::Identity::_detail::ImdsManagedIdentitySource::GetToken::__l2::<lambda_1>::operator()() Line 360 C++
[External Code]
OLEDB sandbox.exe!Azure::Identity::_detail::TokenCache::GetToken(const std::string & scopeString, std::chrono::duration<__int64,std::ratio<1,10000000>> minimumExpiration, const std::function<Azure::Core::Credentials::AccessToken __cdecl(void)> & getNewToken) Line 116 C++
OLEDB sandbox.exe!Azure::Identity::_detail::ImdsManagedIdentitySource::GetToken(const Azure::Core::Credentials::TokenRequestContext & tokenRequestContext, const Azure::Core::Context & context) Line 359 C++
OLEDB sandbox.exe!Azure::Identity::ManagedIdentityCredential::GetToken(const Azure::Core::Credentials::TokenRequestContext & tokenRequestContext, const Azure::Core::Context & context) Line 60 C++
OLEDB sandbox.exe!main() Line 46 C++
To Reproduce
The client application is very simple:
Azure::Core::Context context; Azure::Core::Credentials::TokenRequestContext tokenRequestContext; Azure::Core::Credentials::AccessToken accessToken; tokenRequestContext.Scopes.push_back("https://database.windows.net"); Azure::Identity::ManagedIdentityCredential managedIdentityCredential; try { accessToken = managedIdentityCredential.GetToken(tokenRequestContext, context); } catch (const std::exception& e) { std::cout << "Exception caught: " << e.what() << std::endl; }
Please note that variables such as MSI_ENDPOINT, MSI_SECRET, IDENTITY_ENDPOINT, IDENTITY_HEADER are not defined in my environment, so the CreateManagedIdentitySource function (managed_identity_credential.cpp) uses the ImdsManagedIdentitySource implementation which targets the "http://169.254.169.254/metadata/identity/oauth2/token" endpoint.
Expected behavior
The library should be able to correctly parse the json response from the identity endpoint.
Setup (please complete the following information):
Additional context
The example response from the endpoint is as follows:
{ "access_token": "...", "client_id": "...", "expires_in": "84671", "expires_on": "1687364401", "ext_expires_in": "86399", "not_before": "1687277701", "resource": "https://database.windows.net", "token_type": "Bearer" }
Please note that the "expires_in" property has a string value.
Here's another response when using the ClientSecretCredential class (different endpoint):
{ "token_type": "Bearer", "expires_in": 86399, "ext_expires_in": 86399, "access_token": "..." }
In this case, the "expires_in" value is a number (which is what the TokenCredentialImpl::ParseToken method expects).
Information Checklist
The text was updated successfully, but these errors were encountered: