Key Vault’s properties.tenantId should accept “adfs”

[sadasant]

Hello team!

The swagger for Azure Key Vault specifies that the tenantId property can only be a uuid (an example can be found here: source link. However, non-uuids exist, such as adfs.

We’ve got a customer asking about this issue in one of our packages: https://github.com/Azure/ms-rest-nodeauth/issues/132

[ghost]

Thanks for the feedback! We are routing this to the appropriate team for follow-up. cc @RandalliLama, @schaabs, @jlichwa.

Issue Details

---

Hello team! The swagger for Azure Key Vault specifies that the `tenantId` property can only be a `uuid` (an example can be found here: [source link](https://github.com/Azure/azure-rest-api-specs/blob/c234651f2885c07ff4a77e50c883145949fda5fa/specification/keyvault/resource-manager/Microsoft.KeyVault/preview/2020-04-01-preview/keyvault.json#L1078-L1082). However, non-uuids exist, such as `adfs`. We’ve got a customer asking about this issue in one of our packages: https://github.com/Azure/ms-rest-nodeauth/issues/132

Author:	sadasant
Assignees:	-
Labels:	`KeyVault`, `Service Attention`
Milestone:	-

[sadasant]

cc: @TheOnlyWei

Once this issue is resolved, we will be able to generate new @azure/arm-keyvault-* clients with the fix included.

[TheOnlyWei]

@sadasant I believe the UUID enforicement for keyvault API makes sense. I just don't understand why we are passing "adfs" as tenant Id for the login. Who do we contact to request a feature to allow tenant UUID for the login in an ADFS Azure Stack environment?

Link for login sample: https://github.com/Azure-Samples/Hybrid-JavaScript-Samples/blob/a23af3e2da7cf433b2759f654107becd1c28b862/secret/index.js#L106

[AshokCMSFT]

Azure Key Vault works with Azure Active Directory for authentication. You must specify a Azure Active Directory tenantID which is always a UUID

[TheOnlyWei]

@AshokCMSFT I am talking about inconsistency in ADFS environment authentication. The string "adfs" is required for "tenantId" in the example I showed in my previous comment. I know it works as UUID in AAD environment.

@sebansal Why is this closed? What is the resolution here?

[mancyc]

Not really a 'bug', because the workaround is to use the tenantID as UUID for ADFS environment. I agree it is not a great user-experience, but the inconsistency between AAD/ADFS authentication and why ADFS specifically requires "adfs" appended to the authority is something that needs to be addressed by ADAL/MSAL. KeyVault doesn't own this piece of auth.

[sadasant]

I don’t think this is specific to the ADAL/MSAL team. They say they specifically don’t differentiate by either case, they treat Azure AD and ADFS authentications in the same way (see this in the MSAL docs: ADFS Support). For ADAL/MSAL, “adfs” is a valid tenant that they send through.

This is really just a feature of Azure https://docs.microsoft.com/en-us/azure/active-directory/hybrid/how-to-connect-fed-single-adfs-multitenant-federation but perhaps I’m missing something.

If this is just a feature of Azure, why not have it on the specs? That tenant ID can be either a UUID or adfs?

[sadasant]

After some back and forward, this is clearer to me: So, the underlying issue that @TheOnlyWei is trying to use the Key Vault management client to create an access policy in the AD FS environment. In the ADFS environments there’s no tenant ID but only these “adfs” references that only map correctly in the local AD FS network, and since the Key Vault management client only works with UUID tenant IDs, this means that the Key Vault service does not support creating access policies in the ADFS environment. Is that correct?

cc: @mancyc , @AshokCMSFT

[TheOnlyWei]

@sadasant That is correct in that ADAL login used in the sample code linked above uses "adfs" as tenant Id and Key Vault creation uses UUID of tenant Id. ADFS environments do have a tenant Id, but the issue here is inconsistent convention. If we are to accept "adfs" as tenant Id in several SDKs, then it doesn't make sense to not accept it for Key Vault, and vice versa, if we only accept UUID as tenant Id in Key Vault, it doesn't make sense why we even need to use "adfs".

Currently you can work around this conflict by using UUID tenant Id for Key Vault creation, but you must enter "adfs" as tenant Id for login into Azure Stack environment. Which is confusing for the user. I think we might want to be consistent with Azure. Any idea what is the convention for Azure?

[sadasant]

Right, but this is not a bug on the the SDK clients, nor a bug on the rest specs. This is really a feature request for the Key Vault service team to support “adfs” tenants in their access policy API.

[TheOnlyWei]

Part of this is to also spread awareness of the different interpretations of tenant Id used by SDKs and RPs so as to have consistency in user experience because the SDK teams do not test their code on Azure Stack environments. If this is expected behavior after your investigations and is consistent with Azure, then we can leave this issue for now.

[sadasant]

@AshokCMSFT , @mancyc please help us with a statement when you find some time 🙏

[mancyc]

KeyVault does support access policies on ADFS in Stack. TenantId from Stack environment should work. Alternatively, you can use flag -BypassObjectIdValidation that allows you to specify an object ID without validating that the object exists in Azure Active Directory, hence skipping the Graph call. https://docs.microsoft.com/en-us/powershell/module/az.keyvault/set-azkeyvaultaccesspolicy?view=azps-6.4.0