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

[Core] Add multi-tenant authentication policy #24019

Closed
wants to merge 16 commits into from

Conversation

mccoyp
Copy link
Member

@mccoyp mccoyp commented Apr 15, 2022

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:

from azure.data.tables import TableServiceClient
from azure.identity import ClientSecretCredential

endpoint = "https://storage-account.table.core.windows.net"
credential = ClientSecretCredential(
    tenant_id=tenant_A,  # our storage account lives in tenant_B
    client_id=...,
    client_secret=...,
)

# Note: Tables only supports auth challenges in API version 2020-12-06 and after (this will require new Tables SDK support)
client = TableServiceClient(credential=credential, endpoint=endpoint, api_version="2020-12-06")
for table in client.list_tables():
    print(table.name)

Current behavior: we call client.list_tables() with a BearerTokenCredentialPolicy:

  1. We fetch a token for tenant_A because of our credential
  2. The service rejects this token because our storage resource is in tenant_B
  3. We return the error response in an exception

New behavior: we call client.list_tables() with a BearerTokenChallengePolicy (from this PR):

  1. We fetch a token for tenant_A because of our credential
  2. The service rejects this token because our storage resource is in tenant_B
  3. We parse the WWW-Authenticate header of the response and fetch a token valid in tenant_B (for the scope provided in the same header)
  4. We re-send the original request, with the new token for tenant_B
  5. We get a successful response from the service

This 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 ChallengeCache to 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:

  1. If the resource we're making a request for exists in our tenant, we already provide a valid token. The request succeeds.
    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.
  2. If the resource exists in a different tenant, our request fails and prompts a 401. We make a second request with the correct tenant.
    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_tenant and discover_scopes. If both of these kwargs are set to False, this policy becomes functionally equivalent to the base BearerTokenCredentialPolicy.

All SDK Contribution checklist:

  • The pull request does not introduce [breaking changes]
  • CHANGELOG is updated for new features, bug fixes or other significant changes.
  • I have read the contribution guidelines.

General Guidelines and Best Practices

  • Title of the pull request is clear and informative.
  • There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For more information on cleaning up the commits in your PR, see this page.

Testing Guidelines

  • Pull request includes test coverage for the included changes.
    d

@mccoyp mccoyp added Client This issue points to a problem in the data-plane of the library. Azure.Core labels Apr 15, 2022
@azure-sdk
Copy link
Collaborator

azure-sdk commented Apr 15, 2022

API change check for azure-core

API changes have been detected in azure-core. You can review API changes here

@mccoyp mccoyp requested a review from xiangyan99 April 22, 2022 22:00
@mccoyp mccoyp marked this pull request as ready for review April 22, 2022 23:18
try:
challenge = _HttpChallenge(response.http_response.headers.get("WWW-Authenticate"))
# azure-identity credentials require an AADv2 scope but the challenge may specify an AADv1 resource
scope = challenge.scope or challenge.resource + "/.default" if self._discover_scopes else self._scopes
Copy link
Member

@annatisch annatisch May 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Raise an error with a useful message? Or,
  2. Default to using self._scopes?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For now, I'll go with the latter approach (and make this behavior clear in the class docstring)

@mccoyp mccoyp requested a review from annatisch May 2, 2022 17:47
@mccoyp
Copy link
Member Author

mccoyp commented Jun 14, 2022

Closing this PR in favor of a new, forthcoming PR that incorporates feedback from the review in Azure/azure-sdk#4302.

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

Successfully merging this pull request may close these issues.

Move multi-tenant authentication-supporting code into azure-core
5 participants