Skip to content
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

[BUG][Text Analytics]Text Analytics Cilent fail to authenticate with token credential in USGOV and China #18520

Closed
Luyunmt opened this issue Feb 8, 2021 · 22 comments
Assignees
Labels
Client This issue points to a problem in the data-plane of the library. Cognitive - Text Analytics

Comments

@Luyunmt
Copy link
Contributor

Luyunmt commented Feb 8, 2021

We are running live Tests against other clouds like US Gov and Azure China Cloud. The goal is to check whether new azure sdk package work with other clouds or not.
In https://github.com/Azure/azure-sdk-for-net/blob/master/sdk/textanalytics/Azure.AI.TextAnalytics/src/TextAnalyticsClient.cs#L68 as follow.

image

The scope is hard code as ' https://cognitiveservices.azure.com/.default' leading to only work well in the public cloud.

The value of the scope in different clouds is follow:
Azure public cloud: https://cognitiveservices.azure.com/.default
USGOV cloud: https://cognitiveservices.azure.us/.default
China cloud: https://cognitiveservices.azure.cn/.default

AZURE_AUTHORITY_HOST setting:
USGOV : https://login.microsoftonline.us/
China: https://login.microsoftonline.cn/
@jongio @danieljurek @benbp

@ghost ghost added the needs-triage Workflow: This is a new issue that needs to be triaged to the appropriate team. label Feb 8, 2021
@tzhanl tzhanl added Client This issue points to a problem in the data-plane of the library. Cognitive - Text Analytics needs-team-triage Workflow: This issue needs the team to triage. test-sovereign-cloud labels Feb 8, 2021
@ghost ghost removed the needs-triage Workflow: This is a new issue that needs to be triaged to the appropriate team. label Feb 8, 2021
@maririos
Copy link
Member

maririos commented Feb 9, 2021

We have the same problem in FR #17192

@tg-msft do you know if there is a guidance for .NET on how to do this? or how to surface this to our customers?
For sdk tests, I see how some services have the default scope as an env variable that gets configured with the CI, but wonder how the user will know that in case they are in one of those clouds.

@maririos
Copy link
Member

maririos commented Feb 9, 2021

FYI @suhas92

@jongio
Copy link
Member

jongio commented Feb 10, 2021

@Luyunmt - Can you please give examples of how this is achieved in the sdk with other cloud endpoints?

@tg-msft
Copy link
Member

tg-msft commented Feb 10, 2021

Storage hardcodes that today so this a better question for @schaabs. If we're ready to add a common story for this, I think we should maybe stick this on the base ClientOptions or at least have a common TokenAuthenticationOptions type we can expose from any service's *ClientOptions that allow TokenCredential.

@Luyunmt
Copy link
Contributor Author

Luyunmt commented Feb 22, 2021

@schaabs
Copy link
Member

schaabs commented Feb 22, 2021

The client generally should handle which scopes are required to properly authorize the call. Some services (such as storage) use the same scope in all clouds which is why they are able to hard code the scope they use. Other services need to use different scopes in different clouds. In these cases, the client should, if possible, handle this. There are a couple of strategies a client can choose to determine the required scope.

First, if the service returns the required scopes or resource string via an authentication challenge (WWW-Authenticate header) the client should utilize this to determine the scope. Key Vault is an example of this and currently has it's own authentication policy which implements this discovery.

If the no such data is available through an authentication challenge, a service client may be able to determine the targeted cloud and required scope based off the resource endpoint the user specifies. However, this can break down if the service supports custom domain links, or if end users use private links.

We've avoided adding any client configuration for this up to this point as it introduces quite a bit of complexity to the user. The TokenAuthenticationOptions class @tg-msft suggests is very interesting. But we'd need to work out details such as what this means for services which don't support AAD (a shrinking minority. Also, while most azure services only use a singe scope to authenticate all calls, it's possible that different methods might require different scopes, so how would these options apply in that case.

@jongio
Copy link
Member

jongio commented Feb 23, 2021

@maririos - Can you see if cognitive returns the scope/resource string in an Auth challenge?

If so, does this work for default domains and resources with custom subdomain? To use AAD auth with Cognitive services, then the resource needs a custom subdomain, see: https://docs.microsoft.com/en-us/azure/cognitive-services/authentication?tabs=powershell#authenticate-with-azure-active-directory. For other services, a key is required and we therefore shouldn't have to worry about scope in other clouds.

@tg-msft - Are you comfortable with the following while @johanste fleshes out cloud environment design?

  1. Ask user to set scope in TokenCredentialContext
  2. Update TA and FR clients to honor the scope if set.

@johanste - Where are we with a design for cloud environment?

@tg-msft
Copy link
Member

tg-msft commented Feb 24, 2021

@schaabs - I'd think we'd only put TokenAuthenticationOptions on derived classes like SearchClientOptions if:

  • they supported AAD,
  • varied scopes across clouds, and
  • didn't provide an API to discover the right scope.

It'd unblock things today and ideally we'd [EditorBrowsable(Never)] them over time as they added automagic discovery. We could try guessing defaults based on endpoints as well with this being an override for situations where we get it wrong.

@jongio - I'm in favor of enabling something for forward progress here. I think @schaabs will have the best idea how to do that with the least long term debt and I'll get behind his plan.

@jongio
Copy link
Member

jongio commented Apr 29, 2021

@maririos - How do you recommend we proceed with this?

@maririos
Copy link
Member

We can def try the approach Ted suggested of adding the parameter to TextAnalyticsClientOptions class.
I currently don't have the bandwidth to do this, so if no one else could do it and it is a priority let me know and I'll ask for funding

@jongio
Copy link
Member

jongio commented Apr 30, 2021

@v-xuto - Can you please do a PR for this? @maririos can be advisor. Thanks

@v-xuto
Copy link
Member

v-xuto commented May 7, 2021

@jongio - Working on this.

@zedy-wj
Copy link
Member

zedy-wj commented May 8, 2021

@maririos some of the TA features are not enabled in UsGov. Healthcare tests are failing. Should we remove UsGov cloud or do a PR for this issue?

@maririos
Copy link
Member

Better to remove it. This is a service constrain so unless there is a way to disable tests per clouds, not much we can do

@zedy-wj
Copy link
Member

zedy-wj commented May 11, 2021

@jongio All tests about Healthcare failed. Because feature not support in others clouds except Public. @maririos suggested that we remove other clouds. Therefore, adding scopes in UsGov and China are meaningless now. Should we hold this issue until Healthcare service support in other regions?

@maririos
Copy link
Member

Even though the test won't pass, it will be good to make the proper changes in the TA library so users that don't need Healthcare can target other clouds

@zedy-wj
Copy link
Member

zedy-wj commented May 12, 2021

OK, I will do a PR for this issue.

@jongio
Copy link
Member

jongio commented Jun 28, 2021

We are holding on fixing this until the ACR design is complete. #21603 (comment)

@zedy-wj
Copy link
Member

zedy-wj commented Sep 30, 2021

Hi, @benbp , @maririos!
We have updated the code, this issue has been fixed with sovereign cloud test PR.

But there is a test named RecognizeHealthcareEntitiesBatchWithCancellation was not stable.

The pipeline run result is at here. Could you help to see this problem, any thoughts of it?

@maririos
Copy link
Member

There is a known issue with that test => #24052

We haven't had the time to look into it in order to fix it though

@maririos maririos removed the needs-team-triage Workflow: This issue needs the team to triage. label Oct 25, 2021
@maririos maririos removed their assignment Oct 25, 2021
@nisha-bhatia nisha-bhatia self-assigned this Nov 18, 2021
@zedy-wj
Copy link
Member

zedy-wj commented Jan 6, 2022

@nisha-bhatia , @maririos - Do you have any progress or plans to fix this issue?

@nisha-bhatia
Copy link
Member

This bug fix will go out in the next release.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Client This issue points to a problem in the data-plane of the library. Cognitive - Text Analytics
Projects
None yet
Development

No branches or pull requests

10 participants