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 AZURE_AUTH_LOCATION #9312

Merged
merged 2 commits into from
Jan 8, 2020

Conversation

ellismg
Copy link
Member

@ellismg ellismg commented Jan 4, 2020

This change adds support to EnvironmentCredential so that it can
consider AZURE_AUTH_LOCATION as a location to get credentials. This
file can be generated with the Azure CLI.

The invasive part of the change here is refactoring things such that
instead of constructing the inner credential object eagerly when
EnvironmentCredential is constructed we build it on the first call to
GetToken or GetTokenAsync. This is done so that we do not have do
preform IO when constructing the credential object.

Contributes to #9309

@ellismg
Copy link
Member Author

ellismg commented Jan 4, 2020

I'm not 100% sure on the design here, @schaabs, but I think this is good enough to land. I could image creating an actual SdkAuthCredential type that would be constructed with a file path, and EnvironmentCredential could create that. We could consider exposing that type as way to allow folks to use these files without setting environment variables.

The reason I didn't take that approach at first is that I believe that this new token would need to be an IExtendedTokenCredential and we would have to change EnvironmentCredential to handle calling the IExtendedTokenCredential when delegating to an underlying credential if they exist, in order to not end up double wrapping errors in some cases. It started to feel like a bunch of machinery and it wasn't clear it was worthwhile. I also know that you were already a little worried regarding the number of credentials types we had and throwing more public ones into the picture didn't seem so great.

Note that this does yet support a SDK Auth file that uses clientCertificate instead of clientSecret. When we end up supporting that (I assume the major blocker is going to be parsing the .pem file into an X509Certificate2) it will be easy to plug that same support in here.

Copy link
Contributor

@jianghaolu jianghaolu left a comment

Choose a reason for hiding this comment

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

LGTM for auth files with a client secret

@ellismg
Copy link
Member Author

ellismg commented Jan 7, 2020

@schaabs I made the changes you and I discussed in person, could you please take another look? Note that at this time I have SdkClientAuthCredential as public. Do we need to do some surface area review?

@ellismg ellismg force-pushed the ellismg/add-sdk-auth-env-var-support branch 2 times, most recently from 9f53d5d to a58e964 Compare January 8, 2020 00:07
This change introduces a new type, `SdkAuthFileCredential` which can be
used to support authenticating with an "SDK Auth File", which the `az`
CLI can write.  The file itself is just a JSON document that has
information about the Tenent, Client ID and Client Secret. Under the
hood, we just create a ClientSecretCredential with the information from
the SDK Auth File.

We also add `AZURE_AUTH_LOCATION` to the list of environment variaibles
that the `EnvironmentCredential` considers.
@ellismg ellismg force-pushed the ellismg/add-sdk-auth-env-var-support branch from a58e964 to c2317e2 Compare January 8, 2020 00:16
@ellismg ellismg merged commit 324fdf9 into Azure:master Jan 8, 2020
@ellismg ellismg deleted the ellismg/add-sdk-auth-env-var-support branch January 8, 2020 19:11
ellismg added a commit to ellismg/azure-sdk-for-net that referenced this pull request Feb 6, 2020
This reverts commit 324fdf9.

There is ongoing discussion about the best way to surface this support,
either in Azure.Identity itself, as a standalone sample or some other
way.
ellismg added a commit that referenced this pull request Feb 6, 2020
This reverts commit 324fdf9.

There is ongoing discussion about the best way to surface this support,
either in Azure.Identity itself, as a standalone sample or some other
way.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants