-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Unable to use TokenCredentials instances from the azure/identity library in ServiceClientCredential based SDK's for Sovereign Azure Clouds #15945
Unable to use TokenCredentials instances from the azure/identity library in ServiceClientCredential based SDK's for Sovereign Azure Clouds #15945
Comments
Maybe a workaround is to create an instance of AzureIdentityCredentialAdapter with the TokenCredential and the correct scope then pass it to IosDpsClient? |
Absolutely, I'm hoping the ServiceClient can be fixed to take care of this, since it's already trying to handle interop between the token/serviceclient credentials. I can definitely give the workaround a try in the mean time. |
Thanks for reporting @prashantchari! Can you share how you got around this problem when using the older @sadasant This issue is for when using the |
Hello team! I’m Daniel. I’m here to help. Identity offers the possibility to specify authority hosts through the Would something similar to this code work for you? import { AzureAuthorityHosts, EnvironmentCredential } from “@azure/identity”;
const credential = new EnvironmentCredential({ authorityHost: AzureAuthorityHosts.AzureChina });
const client = new IotDpsClient(credential, subscriptionId, { baseUri }); We’re working on improving our documentation, and in the following weeks, we will have a sample showcasing how to use this feature. In the meantime, please let us know if this approach works for you. Your time is appreciated! |
I could be wrong but I think the problem is the scope not the auth host. TokenCredential does not carry info about scope so when ms-rest-js creates the adapter for the token credential passed in it won't know what scope to use. |
@voidfoo thank you! I see your point now. Give us some time to think on a good answer. |
@voidfoo , @prashantchari since the solution to this is likely going to need a public API change from our part, please give us a couple of days to coordinate an appropriate response. Thank you for reporting this problem to us. |
@voidfoo , @prashantchari We’re fully booked these days. It will take us some time to come up with a good answer. In the mean time, would it work for you to have your own copy of the import { ServiceClientCredentials, WebResource, TokenResponse } from "ms-rest-js";
import { TokenCredential } from "@azure/core-auth";
export class AzureIdentityCredentialAdapter implements ServiceClientCredentials {
private azureTokenCredential: TokenCredential;
private scopes: string | string[];
constructor(
azureTokenCredential: TokenCredential,
scopes: string | string[] = "https://<your-scope-endpoint>/.default"
) {
this.azureTokenCredential = azureTokenCredential;
this.scopes = scopes;
}
public async getToken(): Promise<TokenResponse> {
const accessToken = await this.azureTokenCredential.getToken(this.scopes);
if (accessToken !== null) {
const result: TokenResponse = {
accessToken: accessToken.token,
tokenType: "Bearer",
expiresOn: accessToken.expiresOnTimestamp,
};
return result;
} else {
throw new Error("Could find token for scope");
}
}
public async signRequest(webResource: WebResource) {
const tokenResponse = await this.getToken();
webResource.headers.set(
"authorization",
`${tokenResponse.tokenType} ${tokenResponse.accessToken}`
);
return Promise.resolve(webResource);
}
} With an Would that be a good workaround in the short term? We appreciate your time and feedback. |
Hey @sadasant , Thanks for the update. It looks like there was a push to enable tokencredentials as a first class citizen on the library I was looking at - azure/arm-deviceprovisioningservices. I'll scan on my side to see if I'm using SDKs that do not yet work on tokencredentials directly and look into the feasibility of having the above workaround for the affected services. Is it a reasonable assumption that all of the azure SDKs should eventually move to not requiring the adapter/working with tokencredentials directly at some point ? |
@prashantchari I’m happy to read that! - That is correct, the goal is to have all of the Azure SDKs use TokenCredentials. |
@prashantchari If your concerns are resolved, please feel free to close this issue. In any case, please let us know if there’s anything else we can do. Thank you for your time using the Azure SDKs! |
Uggh. I realized the sdk's just moved the problem around. This is misleading. Also, It appears to be very hard to avoid this problem given that the function signature for the SDK's that operate on ServiceClientCredentials take in TokenCredentials now, and internally errors out because of this issue. @sadasant , do you happen to have an issue that tracks the fixing of the larger change / adapter , so I know when to remove it's usage at my end ? |
@prashantchari hello again! Thank you for giving us more information. Let’s use this issue to track the problem you’re seeing. I will need some days to route this problem internally properly. I’ll answer back next week, as soon as possible. |
@prashantchari While the constructor signature for the SDKs now take in TokenCredentials, they support the older ServiceClientCredentials too.
So, you should be able to use the modified adapter that @sadasant shared above to create a If you can confirm that this works for you, we can then make a change to the ServiceClient constructor at https://github.com/Azure/ms-rest-js/blob/c47191b4255534771dc320960c39a25645823800/lib/serviceClient.ts#L190 as follows |
In conclusion
import { AzureAuthorityHosts, EnvironmentCredential } from “@azure/identity”;
const credential = new EnvironmentCredential({ authorityHost: AzureAuthorityHosts.AzureChina });
If you can confirm that this works for you, we can get working on a fix soon |
Hi @ramya-rao-a , Correct, I am passing in the base uri as you mentioned above, I think the solution works for me. |
Thanks for the confirmation @prashantchari @sadasant Can you make a PR to make the required changes to |
We have released a new version for @azure/ms-rest-js that should fix this issue. Please let us know if you can test it, and if it solves this issue for you. |
Hi @prashantchari. Thank you for opening this issue and giving us the opportunity to assist. We believe that this has been addressed. If you feel that further discussion is needed, please add a comment with the text “ |
Hi @prashantchari, since you haven’t asked that we “ |
On Azure China, Trying to use the TokenCredentials instances from the identity library that work with ServiceClientCredentials instances throw the error -
Looking into the code, it looks like SDK's that use the serviceclientcredentials instances default to public azure scopes in https://github.com/Azure/ms-rest-js/blob/de6aa5157603639001785b4a43afa5f325381dbd/lib/serviceClient.ts#L189 in AzureIdentityCredentialAdapter. It is my understanding that this is causing the auth flow to fail.
Example usage:
new IotDpsClient(
creds as any,
subscriptionId,
{
baseUri
}
);
where credentials is a ChainedTokenCredential object with authority
https://login.chinacloudapi.cn
, baseUri points tohttps://management.chinacloudapi.cn
, and IoTDpsClient is from https://www.npmjs.com/package/@azure/arm-deviceprovisioningservices.Am I using this correctly ?
The text was updated successfully, but these errors were encountered: