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

Support Tenant Id Challenges / Hints #21378

Merged
merged 52 commits into from
Jun 23, 2021
Merged

Conversation

christothes
Copy link
Member

@christothes christothes commented May 26, 2021

The focus of these changes are to add the tenant discovery capability to Azure.Identity.
It also implements a new auth policy in all Azure.Storage clients that implement this capability.

#19404

@ghost ghost added Azure.Core Azure.Identity Storage Storage Service (Queues, Blobs, Files) labels May 26, 2021
@christothes christothes changed the title Feature/multi tenant auth Implement multi-tenant authorization May 26, 2021
@christothes christothes changed the title Implement multi-tenant authorization Support Tenant Id Challenges / Hints May 26, 2021
_options = options ??= new TokenCredentialOptions();
_pipeline = CredentialPipeline.GetInstance(options);
_allowMultiTenantAuthentication = options?.AllowMultiTenantAuthentication ?? true;
_pipeline = CredentialPipeline.GetInstance(options ?? new TokenCredentialOptions());
Copy link
Member

Choose a reason for hiding this comment

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

passing null as opposed to a TokenCredentialOptions uses the singleton instance of CredentialPipeline this would change that behavior.

Copy link
Member Author

Choose a reason for hiding this comment

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

I may be missing something, but I believe this should have the same behavior as what is there currently:

options ??= new TokenCredentialOptions();
_pipeline = CredentialPipeline.GetInstance(options);

@weshaggard weshaggard closed this Jun 22, 2021
@weshaggard weshaggard reopened this Jun 22, 2021
@ericsampson
Copy link

@christothes, I don't want to be a pain in the ass, and so I really didn't want to even mention this, but when you get a minute would you be able to comment on my questions here

For an example of the impact that I'm concerned about, the SqlClient library recently added support for enabling the use of DefaultAzureCredentials via the SQL connection string, but there's no way to customize the chosen providers or set these new EnableTenantDiscovery/AllowMultiTenantAuthentication options. Other than env vars, which is really not great at an enterprise scale.
The SqlClient library was planning on upgrading to this Az.Identity release in order to have the Multi-tenant issue resolved transparently for customers, but now it seems like that's not going to be the case given the default behavior.

Thanks for taking the time to help me understand the reason for the default choices :)
Cheers

@christothes
Copy link
Member Author

@christothes, I don't want to be a pain in the ass, and so I really didn't want to even mention this, but when you get a minute would you be able to comment on my questions here

For an example of the impact that I'm concerned about, the SqlClient library recently added support for enabling the use of DefaultAzureCredentials via the SQL connection string, but there's no way to customize the chosen providers or set these new EnableTenantDiscovery/AllowMultiTenantAuthentication options. Other than env vars, which is really not great at an enterprise scale.
The SqlClient library was planning on upgrading to this Az.Identity release in order to have the Multi-tenant issue resolved transparently for customers, but now it seems like that's not going to be the case given the default behavior.

Thanks for taking the time to help me understand the reason for the default choices :)
Cheers

Hi @ericsampson - The default behavior is to avoid breaking changes or authorization requests for unexpected tenants. For example, sending an unauthenticated initial request where that didn't happen previously, or sending token requests to a tenant that you do not wish to share PII details with.

Setting environment or AppConfig setting can be configured in a variety of ways depending on how your service is deployed to Azure, but that seems outside the scope of this PR :).

@christothes christothes enabled auto-merge (squash) June 23, 2021 17:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Azure.Core Azure.Identity Storage Storage Service (Queues, Blobs, Files)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants