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

CLI: simplify Azure authentication #724

Open
dtivel opened this issue Jun 20, 2024 · 7 comments
Open

CLI: simplify Azure authentication #724

dtivel opened this issue Jun 20, 2024 · 7 comments
Labels
investigate Priority:3 Work that is nice to have

Comments

@dtivel
Copy link
Collaborator

dtivel commented Jun 20, 2024

This thread generated some ideas on how we can improve the CLI user experience for Azure authentication, which is currently used by Azure Key Vault signing. In the near future, it will also be used by Trusted Signing signing.

Background

Sign CLI's Azure Key Vault signing exposes 4 authentication-specific options. The original intent was to support 2 authentication strategies:

  • tenant ID + client ID + client secret
  • managed identity

Problem statement

Managed identity support is provided through Azure SDK for .NET's DefaultAzureCredential class. This class actually supports 10 authentication strategies, which are tried in a fixed order until one succeeds. In most cases, this may work like magic. However, there are several problems with this solution.

  1. --azure-key-vault-managed-identity doesn't actually guarantee we'll use the managed identity authentication strategy. Authentication can fail because DefaultAzureCredential succeeded in obtaining a token with an earlier (unwanted) authentication strategy. This really happens.
  2. There's no clear support for other authentication strategies (like Azure CLI). You could use the --azure-key-vault-managed-identity option, set a bunch of ExcludeXXXCredential options for all the options before Azure CLI authentication strategy, but that's a hacky, terrible user experience.
  3. From a CLI UX perspective, to formally support other authentication strategies (like Azure CLI), we'd need to expose an option for it and validate the various combinations of options.

Proposed solution

First, deprecate the 4 authentication-specific options. Continue to honor the options but output a warning that users should migrate to the new solution, which is one of the following options:

  • Provide no authentication-specific options. Use DefaultAzureCredential behavior.
  • Use a new option (-act, --azure-credential-type) and specify the specific authentication strategy you want.

At some time in the future, remove the 4 authentication-specific options. By default, Azure Key Vault signing will require no Azure authentication options. It will support all authentication strategies provided by DefaultAzureCredential in the order that class provides.

For users who want to use a specific authentication strategy, a new option (-act, --azure-credential-type) will enable that. Note that this list of accepted values represents only a subset of strategies provided by DefaultAzureCredential on the principal that we'd add others when needed.

Value Strategy
environment EnvironmentCredential
managed-identity ManagedIdentityCredential

CC @clairernovotny, @joelverhagen, @dlemstra

@dtivel dtivel added the Priority:3 Work that is nice to have label Jun 20, 2024
@dlemstra
Copy link
Contributor

I think we should also make the "old" TenantIdOption obsolete and introduce a new option called -ati, --azure-tenant-id that sets the TenantId property of the DefaultAzureCredentialOptions. The --azure-key-vault-tenant-id option should print a warning when it is being used.

And when we add managed-identity we should probably also add options to set the ManagedIdentityClientId and ManagedIdentityResourceId properties of the DefaultAzureCredentialOptions.

@dlemstra
Copy link
Contributor

I think we should also add azure-cli as an option. I am using az login in my pipeline and I would like to only have the credential type enabled.

@clairernovotny
Copy link
Member

We discussed this a bit offline and for now decided to go with using DefaultAzureCredential as the default authentication mechanism and not to have an additional credential type option. We will document that authentication happens using that mechanism and defer to the Azure docs on which options are evaluated in what order.

While an issue exists with DefaultAzureCredential in one case, we don't yet know the root cause, if it's specific to the JavaScript version, something with AzDo, or something else entirely. Once we have a root cause, we can evaluate the best way to ensure it doesn't happen here.

@janstaelensskyline
Copy link

Does this mean we can still authenticate using a shared secret (holding the azure.client.secret) by placing them into the runner environment variables instead of using the now obsolete arguments? We recently noticed the obsolete warning and, upon looking into ManagedIdentityCredentials, it appears this requires us to define which repositories have access to it in Azure itself. Reference.

We currently have a GitHub organization with workflows, an Azure devops environment and a legacy Gerrit/Jenkins setup. We heavily rely on platform-independent tooling and encrypted shared secrets across all three CI/CD platforms, as configuring security for each new repository individually is not feasible (there's a few thousand). We have reusable pipelines on all three platforms that include the sign tool prerelease among other tooling.

Therefore, I am hoping we can continue using the different platform secret vaults with Azure application client secrets, and so on. I'll continue doing more research to see how to adapt, but wanted to already check-in here.

@dlemstra
Copy link
Contributor

dlemstra commented Jul 4, 2024

We are using the DefaultAzureCredential class and that means you will need to set environment variables if you want to use the old options:

AZURE_TENANT_ID = --azure-key-vault-tenant-id
AZURE_CLIENT_ID = --azure-key-vault-client-id
AZURE_CLIENT_SECRET = --azure-key-vault-client-secret

@MikeWilliams-UK
Copy link

We have been using 0.9.1-beta.23356.1 which requires us to specify the following arguments

  -kvt, --azure-key-vault-tenant-id <azure-key-vault-tenant-id>                 Tenant ID to authenticate to Azure Key Vault.
  -kvi, --azure-key-vault-client-id <azure-key-vault-client-id>                 Client ID to authenticate to Azure Key Vault.
  -kvs, --azure-key-vault-client-secret <azure-key-vault-client-secret>         Client secret to authenticate to Azure Key Vault.

While testing a new certificate (kindly generated by @ChrisSfanos) I updated to 0.9.1-beta.24361.2 See #743

0.9.1-beta.24361.2 accepts the above arguments but gives us a warning message below

"The client secret options are obsolete and should no longer be specified."

Looking at the CLI options using "sign code azure-key-vault -?" shows there are two new arguments which I think are the replacements

  -mici, --managed-identity-client-id <managed-identity-client-id>              The client id of a user assigned ManagedIdentity.
  -miri, --managed-identity-resource-id <managed-identity-resource-id>          The resource id of a user assigned ManagedIdentity.

Who can generate a managed identity for us?
Or do we set the environment variables AZURE_TENANT_ID, AZURE_CLIENT_ID & AZURE_CLIENT_SECRET mentioned above

@ChrisSfanos
Copy link
Member

ChrisSfanos commented Aug 1, 2024 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
investigate Priority:3 Work that is nice to have
Projects
None yet
Development

No branches or pull requests

6 participants