-
Notifications
You must be signed in to change notification settings - Fork 3.3k
[Core] Allow authentication via environment variables #27938
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
base: dev
Are you sure you want to change the base?
Conversation
️✔️AzureCLI-FullTest
|
️✔️AzureCLI-BreakingChangeTest
|
|
Core |
|
Thank you for your contribution VoxSecundus! We will review the pull request and get back to you soon. |
|
@microsoft-github-policy-service agree company="Alces Flight Ltd" |
| def is_guid(guid): | ||
| import uuid | ||
| try: | ||
| uuid.UUID(guid) | ||
| return True | ||
| except (ValueError, TypeError): | ||
| return False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function is already defined at
azure-cli/src/azure-cli-core/azure/cli/core/util.py
Lines 1233 to 1239 in a5198b5
| def is_guid(guid): | |
| import uuid | |
| try: | |
| uuid.UUID(guid) | |
| return True | |
| except ValueError: | |
| return False |
|
|
||
| # If no login data, look for service principal credential in environment variables | ||
| if not self._entries and env_var_auth_configured(): | ||
| logger.warning("Using service principal credential configured in environment variables.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unconditionally showing warnings can breaks pipelines which enables failOnStderr (#18372).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I'm not sure what you mean. How should I make this warning shown "conditionally"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I'm not sure what you mean. How should I make this warning shown "conditionally"?
Well. This warning shouldn't be printed at all, as it doesn't really qualify as a warning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Understood. It's been changed to a logger.debug call.
| # If no login data, look for service principal credential in environment variables | ||
| if not self._entries and env_var_auth_configured(): | ||
| logger.warning("Using service principal credential configured in environment variables.") | ||
| self._entries = [load_env_var_credential()] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
load_entry is designed solely for looking up service principal credentials stored on the hard disk. It may not be an ideal place for loading environment variables.
|
Thank you very much for the contribution. We understand #10241 is a highly demanded feature. Actually, I already have a draft work on supporting environment variable credential: https://github.com/jiasli/azure-cli/tree/env-cred, which utilizes the code from #22124. I will certainly take your PR into consideration while implementing this feature. |
4e5a35a to
1bf27e3
Compare
|
Any update? |
|
Starting to hold my breath soon! |
We should decide which account takes higher priority:
For I think it is more reasonable for the account from environment variable to take higher priority, as environment variables are kept in memory, like a temporary session, but the account persisted by $ MY_ENV_VAR=TEST_VALUE python3 -c "import os; print(os.environ['MY_ENV_VAR'])"
TEST_VALUELetting the account from |
|
@jiasli I'm happy to make that change if you are. |
445f7ea to
817982f
Compare
Related command
azAll commands that require the user to be logged in.
Description
Addresses #10241
This PR updates the
_profileandauth/identityazure-cli-coreclasses to support authenticate for service principals via environment variables, without having to runaz loginfirst. When logged in withaz login, any credentials specified as environment variables are ignored.New variables:
AZURE_SUBSCRIPTION_ID--subscription.AZURE_TENANT_IDAZURE_CLIENT_IDAZURE_CLIENT_SECRETTesting Guide
In a Bash terminal:
azcommands as usual:$ az vm list [ { ... }, ... ] $ az account show { "id": null, "name": "Environment Variable Subscription", "tenantId": "<redacted>", "user": { "name": "<redacted>", "type": "servicePrincipal" } } $ az account get-access-token { "accessToken": "<redacted>", "expiresOn": "2023-11-28 18:51:46.000000", "expires_on": 1701197506, "subscription": "None", "tenant": "<redacted>", "tokenType": "Bearer" }History Notes
N/A
This checklist is used to make sure that common guidelines for a pull request are followed.
The PR title and description has followed the guideline in Submitting Pull Requests.
I adhere to the Command Guidelines.
I adhere to the Error Handling Guidelines.