[Core] Add multi-tenant authentication policy#24019
[Core] Add multi-tenant authentication policy#24019mccoyp wants to merge 16 commits intoAzure:mainfrom
Conversation
|
API change check for API changes have been detected in |
sdk/core/azure-core/azure/core/pipeline/policies/_authentication.py
Outdated
Show resolved
Hide resolved
061c2bf to
d335299
Compare
sdk/core/azure-core/azure/core/pipeline/policies/_authentication.py
Outdated
Show resolved
Hide resolved
sdk/core/azure-core/azure/core/pipeline/policies/_authentication.py
Outdated
Show resolved
Hide resolved
sdk/core/azure-core/azure/core/pipeline/policies/_authentication.py
Outdated
Show resolved
Hide resolved
sdk/core/azure-core/azure/core/pipeline/policies/_authentication.py
Outdated
Show resolved
Hide resolved
sdk/core/azure-core/azure/core/pipeline/policies/_authentication.py
Outdated
Show resolved
Hide resolved
sdk/core/azure-core/azure/core/pipeline/policies/_authentication.py
Outdated
Show resolved
Hide resolved
sdk/core/azure-core/azure/core/pipeline/policies/_authentication.py
Outdated
Show resolved
Hide resolved
sdk/core/azure-core/azure/core/pipeline/policies/_authentication.py
Outdated
Show resolved
Hide resolved
sdk/core/azure-core/azure/core/pipeline/policies/_authentication.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
It looks like there's a possibility the neither challenge.scope nor challenge.resource are populated, in which case I think we'd just be sending "/.default" if discover_scopes is true. In this scenario would sending that be the best/correct thing to do and let it fail at the service?
Or should we be falling back to self._scopes in the case that the information in the challenge was insufficient?
There was a problem hiding this comment.
That's a good point -- I was thinking that we could let the invalid scope error during the service request in that case, but I don't think it would be a useful error experience. We would get something like
azure.core.exceptions.ClientAuthenticationError: DefaultAzureCredential failed to retrieve a token from the included credentials.
with further details of
Authentication failed: AADSTS70011: The provided request must include a 'scope' input parameter. The provided value for the input parameter 'scope' is not valid. The scope /.default is not valid.
That would indicate that there's an issue with scopes, but a user would have to understand what's going on under the hood to figure out that they should set discover_scopes to False. I guess the question is, when we detect that challenge.scope and challenge.resource are empty, should we:
- Raise an error with a useful message? Or,
- Default to using
self._scopes?
There was a problem hiding this comment.
For now, I'll go with the latter approach (and make this behavior clear in the class docstring)
sdk/core/azure-core/azure/core/pipeline/policies/_authentication.py
Outdated
Show resolved
Hide resolved
|
Closing this PR in favor of a new, forthcoming PR that incorporates feedback from the review in Azure/azure-sdk#4302. |
Description
Resolves #23613. For cross-language reference, here is .NET's Tables-internal implementation, here is Java's Tables-internal implementation, and here is JavaScript's challenge-handling callback method in Core.
Context: AAD supports authentication claims challenges, which are exposed in service requests as 401 responses when authentication fails. These 401 responses contain a header that includes the tenant ID where the requested resource lives -- this tenant ID can be passed along to token requests in order to get a valid token for the resource, even if the credential we initially provided to the client targeted a different tenant.
For example:
Current behavior: we call
client.list_tables()with aBearerTokenCredentialPolicy:tenant_Abecause of our credentialtenant_BNew behavior: we call
client.list_tables()with aBearerTokenChallengePolicy(from this PR):tenant_Abecause of our credentialtenant_BWWW-Authenticateheader of the response and fetch a token valid intenant_B(for the scope provided in the same header)tenant_BThis policy is based on the ChallengeAuthPolicy used by Key Vault, which currently supports multi-tenant authentication. The most significant change between this policy and KV's is that this policy doesn't use its
ChallengeCacheto remove the request body when a request is expected to prompt an auth challenge. Changing this behavior should reduce the number of requests we make:1a. In KV, we would send an empty request the first time in order to prompt an auth challenge, even though our tenant matches that of the resource. Our second request succeeds.
2a. In KV, we would have initially sent an empty request to prompt a 401. We still send a second request with the correct tenant.
For context, Key Vault drops the body of requests made to a new endpoint for security reasons: we want to ensure that the endpoint we're communicating with can accept our auth flow; that it is a correct KV endpoint (at least, that it follows the expected challenge auth flow); and that we don't send sensitive information (like key or secret values) in request bodies until the former conditions are met. Tables doesn't follow these security requirements today, so I decided to leave request bodies in initial requests by default -- KV's use of the policy will just have to be modified. This is prototyped in mccoyp#1.
By default, this policy will fetch tokens after a challenge response that are valid for the tenant and scope we get back in a challenge response (and only the scope provided in the response). There are two toggles to disable either part or all of this behavior:
discover_tenantanddiscover_scopes. If both of these kwargs are set toFalse, this policy becomes functionally equivalent to the baseBearerTokenCredentialPolicy.All SDK Contribution checklist:
General Guidelines and Best Practices
Testing Guidelines
d