Skip to content

{Keyvault} Remove useless AAD endpoint and honor user input --subscription to support cross sub operations #26539

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.

Sign up for GitHub

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

Merged
evelyn-ys merged 4 commits into from
Jun 14, 2023
Merged

{Keyvault} Remove useless AAD endpoint and honor user input --subscription to support cross sub operations #26539

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
767450b
Removed unused aad endpoint
evelyn-ys May 26, 2023
696e1df
honor user input subscription
evelyn-ys May 29, 2023
91fdce4
avoid keyerror
evelyn-ys May 29, 2023
e40bbc4
add data plane cross sub support
evelyn-ys May 30, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/azure-cli/azure/cli/command_modules/keyvault/_client_factory.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -289,7 +289,7 @@ def data_plane_azure_keyvault_secret_client(cli_ctx, command_args):
def _prepare_data_plane_azure_keyvault_client(cli_ctx, command_args, resource_type):
version = str(get_api_version(cli_ctx, resource_type))
profile = Profile(cli_ctx=cli_ctx)
credential, _, _ = profile.get_login_credentials()
credential, _, _ = profile.get_login_credentials(subscription_id=cli_ctx.data.get('subscription_id'))
vault_url = \
command_args.get('hsm_name', None) or \
command_args.get('vault_base_url', None) or \
Expand Down
23 changes: 9 additions & 14 deletions src/azure-cli/azure/cli/command_modules/keyvault/custom.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -401,15 +401,14 @@ def recover_vault_or_hsm(cmd, client, resource_group_name=None, location=None, v


def recover_hsm(cmd, client, hsm_name, resource_group_name, location, no_wait=False):
from azure.cli.core._profile import Profile
from azure.cli.core._profile import Profile, _TENANT_ID

ManagedHsm = cmd.get_models('ManagedHsm', resource_type=ResourceType.MGMT_KEYVAULT)
ManagedHsmSku = cmd.get_models('ManagedHsmSku', resource_type=ResourceType.MGMT_KEYVAULT)

# tenantId and sku shouldn't be required
profile = Profile(cli_ctx=cmd.cli_ctx)
_, _, tenant_id = profile.get_login_credentials(
resource=cmd.cli_ctx.cloud.endpoints.active_directory_graph_resource_id)
Comment on lines -411 to -412
Copy link
Member

@jiasli jiasli May 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This is indeed why a token for AD Graph is retrieved.

https://github.com/Azure/msrestazure-for-python/blob/8849f398b6ebd4607de63c2f5d1318f44ec1d822/msrestazure/azure_active_directory.py#L592

class MSIAuthentication(BasicTokenAuthentication):
    def __init__(self, port=50342, **kwargs):
        ....
        self.set_token()

When msrestazure.azure_active_directory.MSIAuthentication is initialized, it gets an access token immediately, not until signed_session or get_token is called, so get_login_credentials will trigger a web request to get AD Graph token.

This behavior will be changed in #25959 in order to align with other MSAL-based credentials - azure.cli.core.auth.msal_authentication.UserCredential， azure.cli.core.auth.msal_authentication.ServicePrincipalCredential.

Copy link
Member

@jiasli jiasli May 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Another question, why does the original code choose active_directory_graph_resource_id instead of active_directory_resource_id (ARM's resource ID)? Is it because the name is confusing?

Update: Well. It looks like #3631 simply copied the code for creating AD Graph client but only kept the tenant_id.

Copy link
Member Author

@evelyn-ys evelyn-ys May 29, 2023

Choose a reason for hiding this comment

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

When msrestazure.azure_active_directory.MSIAuthentication is initialized, it gets an access token immediately, not until signed_session or get_token is called, so get_login_credentials will trigger a web request to get AD Graph token.

This explains why AAD call shows up when we create keyvault in Cloudshell sandbox as it's following MSI workflow.

Copy link
Member Author

@evelyn-ys evelyn-ys May 29, 2023

Choose a reason for hiding this comment

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

Anyway the whole call for get_login_credentials has been removed. Doesn't matter which resource id we need to use anymore😉

tenant_id = profile.get_subscription(subscription=cmd.cli_ctx.data.get('subscription_id', None))[_TENANT_ID]
Copy link
Member

@jiasli jiasli Jun 14, 2023

Choose a reason for hiding this comment

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

None is the default of default in dict.get: https://docs.python.org/3/library/stdtypes.html#dict.get


# Use 'Recover' as 'create_mode' temporarily since it's a bug from service side making 'create_mode' case-sensitive
# Will change it back to CreateMode.recover.value('recover') from SDK definition after service fix
Expand All @@ -426,7 +425,7 @@ def recover_hsm(cmd, client, hsm_name, resource_group_name, location, no_wait=Fa


def recover_vault(cmd, client, vault_name, resource_group_name, location, no_wait=False):
from azure.cli.core._profile import Profile
from azure.cli.core._profile import Profile, _TENANT_ID

VaultCreateOrUpdateParameters = cmd.get_models('VaultCreateOrUpdateParameters',
resource_type=ResourceType.MGMT_KEYVAULT)
Expand All @@ -436,8 +435,7 @@ def recover_vault(cmd, client, vault_name, resource_group_name, location, no_wai
Sku = cmd.get_models('Sku', resource_type=ResourceType.MGMT_KEYVAULT)
SkuName = cmd.get_models('SkuName', resource_type=ResourceType.MGMT_KEYVAULT)
profile = Profile(cli_ctx=cmd.cli_ctx)
_, _, tenant_id = profile.get_login_credentials(
resource=cmd.cli_ctx.cloud.endpoints.active_directory_graph_resource_id)
tenant_id = profile.get_subscription(subscription=cmd.cli_ctx.data.get('subscription_id', None))[_TENANT_ID]

params = VaultCreateOrUpdateParameters(location=location,
properties={'tenant_id': tenant_id,
Expand Down Expand Up @@ -596,7 +594,7 @@ def create_hsm(cmd, client,

administrators = [admin.strip().replace('\r', '').replace('\n', '') for admin in administrators]

from azure.cli.core._profile import Profile
from azure.cli.core._profile import Profile, _TENANT_ID

if not sku:
sku = 'Standard_B1'
Expand All @@ -606,8 +604,7 @@ def create_hsm(cmd, client,
ManagedHsmSku = cmd.get_models('ManagedHsmSku', resource_type=ResourceType.MGMT_KEYVAULT)

profile = Profile(cli_ctx=cmd.cli_ctx)
_, _, tenant_id = profile.get_login_credentials(
resource=cmd.cli_ctx.cloud.endpoints.active_directory_graph_resource_id)
tenant_id = profile.get_subscription(subscription=cmd.cli_ctx.data.get('subscription_id', None))[_TENANT_ID]

properties = ManagedHsmProperties(tenant_id=tenant_id,
enable_purge_protection=enable_purge_protection,
Expand Down Expand Up @@ -656,7 +653,7 @@ def create_vault(cmd, client, # pylint: disable=too-many-locals, too-many-state
# if client.get raise exception, we can take it as no existing vault found
# just continue the normal creation process
pass
from azure.cli.core._profile import Profile
from azure.cli.core._profile import Profile, _TENANT_ID
from azure.cli.command_modules.role import graph_client_factory, GraphError

VaultCreateOrUpdateParameters = cmd.get_models('VaultCreateOrUpdateParameters',
Expand All @@ -671,11 +668,9 @@ def create_vault(cmd, client, # pylint: disable=too-many-locals, too-many-state
VaultProperties = cmd.get_models('VaultProperties', resource_type=ResourceType.MGMT_KEYVAULT)

profile = Profile(cli_ctx=cmd.cli_ctx)
_, _, tenant_id = profile.get_login_credentials(
resource=cmd.cli_ctx.cloud.endpoints.active_directory_graph_resource_id)

graph_client = graph_client_factory(cmd.cli_ctx)
subscription = profile.get_subscription()
subscription = profile.get_subscription(subscription=cmd.cli_ctx.data.get('subscription_id', None))
tenant_id = subscription[_TENANT_ID]

# if bypass or default_action was specified create a NetworkRuleSet
# if neither were specified we will parse it from parameter `--network-acls`
Expand Down