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 #9311

Closed
wants to merge 1 commit into from

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.

@ellismg ellismg force-pushed the ellismg/add-sdk-auth-env-var-support branch from 34a2021 to 27f8512 Compare January 4, 2020 00:55
throw new Exception(builder.ToString());
}

return new ClientSecretCredential(tenantId, clientId, clientSecret, _pipeline.WithAuthorityHost(new Uri(activeDirectoryEndpointUrl)));
Copy link
Member Author

Choose a reason for hiding this comment

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

Note that in cases where the SDK Auth File has a different Authority than what is configured on the pipeline we prefer the one in the file. That feels like it's the correct behavior, but I wanted to call out the choice here.

}
else
{
StringBuilder builder = new StringBuilder("Environment variables not fully configured. AZURE_TENANT_ID and AZURE_CLIENT_ID must be set, along with either AZURE_CLIENT_SECRET or AZURE_USERNAME and AZURE_PASSWORD. Alternately, AZURE_AUTH_LOCATION may be used to specify the location of a Azure SDK Auth file. Currently set variables [");
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 know that Jon has an open PR to basically remove all of this text. It has definitely gotten longer. I'm not sure if we'd rather reference some page in our documentation about how the EnvironmentCredential in the error (maybe via a shortlink)?

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 ellismg force-pushed the ellismg/add-sdk-auth-env-var-support branch from 27f8512 to 4a45e79 Compare January 4, 2020 00:57
@ellismg
Copy link
Member Author

ellismg commented Jan 4, 2020

Closing in favor of #9312 (which has the correct remote for the HEAD branch).

@ellismg ellismg closed this Jan 4, 2020
@ellismg ellismg deleted the ellismg/add-sdk-auth-env-var-support branch January 8, 2020 00:16
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.

1 participant