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 subscription ids for cli authorizer #780

Conversation

tony-brewerio
Copy link

This adds support for specifying an explicit subscription id for get-access-token call, which currently always obtains a token for the default subscription.

Required to fix subscription id regression in azurerm terraform provider, please see the link below.
hashicorp/terraform-provider-azurerm#23068

I am not sure if this is a good implementation per se, because of how it interacts with tenants.
Azure CLI requires you to specify either a subscription id or a tenant id, but not both, so I've added two checks there.

Should this perhaps be refactored into a separate Authorizer?
One that would not accept tenant ids in its configuration and will always return an error or an empty result when asked for auxiliary ids?

@tony-brewerio tony-brewerio requested a review from a team as a code owner December 14, 2023 21:34
@hashicorp-cla
Copy link

hashicorp-cla commented Dec 14, 2023

CLA assistant check
All committers have signed the CLA.

@manicminer
Copy link
Contributor

@tony-brewerio Thanks for looking at this. I'll take a look separately at hashicorp/terraform-provider-azurerm#23068, but unfortunately this will not help to resolve that issue.

When you specify az account get-access-token --subscription=00000000-0000-0000-0000-000000000000, the subscription ID is not actually used in the token request. It's just a convenience for the user, and therefore has no net effect. Authentication requests are always scoped per tenant.

Accordingly, whilst the fix for the underlying issue is likely to be needed in the SDK, as there is no real need to support translating a subscription ID to the associated tenant ID, I'll close this PR and we probably won't look to merge this as-is.

@manicminer manicminer closed this Dec 14, 2023
@tony-brewerio
Copy link
Author

@manicminer The issue this PR tries to solve is not related to tenants at all.

Maybe I have not explained it in detail in the provider issue, but here's my use case.
I have multiple user accounts, all within a single tenant, that are used to manage multiple different Azure subscriptions.
These accounts only have access to their specific subscriptions and not the others.

When you pass subscription id to az account get-access-token, it will generate a correct token for the account that is allowed to access the subscription.
Without it, it will generate a token for whatever account was logged in last via az login, or the one you've set with az account set.
In my case, I would have to constantly switch between accounts with az account set to be able to use azurerm provider at all.
It also makes cross-account terraform configurations impossible, since multiple aliased provider instances, with different subscription id, all will authenticate with the same user and will have access to only one of the subscriptions.

Which is why I am using azurerm 3.43 and cannot upgrade - since 3.44 broke subscription id handling for azurerm provider.
My multi-account setup does work in 3.43 including aliased providers with different subscription ids.

Given what you have said, I imagine that if all my accounts were in different tenants, then I could configure explicit tenant id for providers and it would work, but alas.

My idea was to pass subscription id from azurerm to cli authenticator, which currently does not happen, and that would fix it for me.

@tony-brewerio
Copy link
Author

@manicminer Is is possible to revive the PR?

I have reviewed the code a bit, and it looks like a safer option would be to verify that tenant id matches the one that's configured for the subscription.
This would support both tenant and subscription id in the input and would produce what feels like an expected result.
The only fail case the would be if tenant does not have access to an explicitly provided subscription, which would result in errors down the line anyway.

I am open to any suggestions at how to rework the PR as well, perhaps with separating out a different authenticator for this use case?

@soerennielsen
Copy link

Another vote to revive it from here. This is a real problem for advanced use cases.

@manicminer Was it actually tested to work or not?

@manicminer
Copy link
Contributor

I am looking into this, please see my comment in AzureRM issue #23068 for more info, thanks!

@manicminer
Copy link
Contributor

Superseded by #1008

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.

4 participants