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

AppAuthentication 1.1.0-preview: TenantId is mandatory in connection string with RunAs=App #4169

Closed
johanclasson opened this issue Mar 28, 2018 · 5 comments
Assignees

Comments

@johanclasson
Copy link

johanclasson commented Mar 28, 2018

The ClientCertificateAzureServiceTokenProvider seams to only use the tenant id in its method GetTokenAsync if the authority parameter is null or white space. But some clients, for example the KeyVaultClient of Microsoft.Azure.KeyVault, provides a authority parameter. This makes the TenantId-part of the connection string possibly redundant.

For example, the following code works just fine:

var azureServiceTokenProvider = new AzureServiceTokenProvider(
    $"RunAs=App;AppId={appId};TenantId=NotNeeded;CertificateThumbprint={thumbprint};CertificateStoreLocation=CurrentUser");
var keyVaultClient = new KeyVaultClient(
    new KeyVaultClient.AuthenticationCallback(azureServiceTokenProvider.KeyVaultTokenCallback));
var secret = await keyVaultClient.GetSecretAsync(secretIdentifier).ConfigureAwait(false);

I propose making the TenantId optional and instead throw in ClientCertificateAzureServiceTokenProvider.GetTokenAsync if authority is null or white space and _tenantId is not set.

@varunsh-coder
Copy link
Contributor

Thanks for the feedback. I agree, the TenantId should not be mandatory if it will not be used in some scenarios. We will make a change in a future release to make it optional in the connection string, and throw in GetTokenAsync, if it is needed for that scenario.

@varunsh-coder
Copy link
Contributor

After thinking more on this, we think this change will cause confusion for some users. If they do not specify tenant Id, and it works for Key Vault, but not for other scenarios, it will not be easy for users to know what to change to fix the issue. This way, even though the tenant id is not required for key vault scenario, the usage causes no confusion.

@abatishchev
Copy link
Contributor

Can you please give an example of other services that use the same format of connection string and which do require TenantId?

@varunsh-coder
Copy link
Contributor

If you see the usage here, https://docs.microsoft.com/en-us/azure/key-vault/service-to-service-authentication#using-the-library

string accessToken = await azureServiceTokenProvider2.GetAccessTokenAsync("https://management.azure.com/").ConfigureAwait(false);

Anytime you use the library to get an access token for an Azure service, you will need to specify the tenant in the connection string, if you use a cert.

This is the other pattern where tenant is not required.

var azureServiceTokenProvider1 = new AzureServiceTokenProvider();
var kv = new KeyVaultClient(new KeyVaultClient.AuthenticationCallback(azureServiceTokenProvider1.KeyVaultTokenCallback));

@varunsh-coder
Copy link
Contributor

Closing this as per the details provided above.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants