Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
16 changes: 14 additions & 2 deletions src/azure-cli-core/azure/cli/core/auth/identity.py
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,10 @@
"Select the account you want to log in with. "
"For more information on login with Azure CLI, see https://go.microsoft.com/fwlink/?linkid=2271136")

PASSWORD_CERTIFICATE_WARNING = (
"Passing the service principal certificate with `--password` is deprecated and will be removed "
"by version 2.74. Please use `--certificate` instead.")

logger = get_logger(__name__)


Expand Down Expand Up @@ -303,7 +307,9 @@ def build_from_credential(cls, tenant_id, client_id, credential):
return ServicePrincipalAuth(entry)

@classmethod
def build_credential(cls, secret_or_certificate=None, client_assertion=None, use_cert_sn_issuer=None):
def build_credential(cls, secret_or_certificate=None,
certificate=None, use_cert_sn_issuer=None,
client_assertion=None):
"""Build credential from user input. The credential looks like below, but only one key can exist.
{
'client_secret': 'my_secret',
Expand All @@ -312,9 +318,15 @@ def build_credential(cls, secret_or_certificate=None, client_assertion=None, use
}
"""
entry = {}
if secret_or_certificate:
if certificate:
entry[_CERTIFICATE] = os.path.expanduser(certificate)
if use_cert_sn_issuer:
entry[_USE_CERT_SN_ISSUER] = use_cert_sn_issuer
elif secret_or_certificate:
# TODO: Make secret_or_certificate secret only
user_expanded = os.path.expanduser(secret_or_certificate)
if os.path.isfile(user_expanded):
logger.warning(PASSWORD_CERTIFICATE_WARNING)
entry[_CERTIFICATE] = user_expanded
if use_cert_sn_issuer:
entry[_USE_CERT_SN_ISSUER] = use_cert_sn_issuer
Expand Down
17 changes: 12 additions & 5 deletions src/azure-cli-core/azure/cli/core/auth/tests/test_identity.py
Original file line number Diff line number Diff line change
Expand Up @@ -265,29 +265,36 @@ def test_service_principal_auth_client_assertion(self):

def test_build_credential(self):
# secret
cred = ServicePrincipalAuth.build_credential("test_secret")
cred = ServicePrincipalAuth.build_credential(secret_or_certificate="test_secret")
assert cred == {"client_secret": "test_secret"}

# secret with '~', which is preserved as-is
cred = ServicePrincipalAuth.build_credential("~test_secret")
cred = ServicePrincipalAuth.build_credential(secret_or_certificate="~test_secret")
assert cred == {"client_secret": "~test_secret"}

# certificate as password (deprecated)
current_dir = os.path.dirname(os.path.realpath(__file__))
test_cert_file = os.path.join(current_dir, 'sp_cert.pem')
cred = ServicePrincipalAuth.build_credential(secret_or_certificate=test_cert_file)
assert cred == {'certificate': test_cert_file}

# certificate
current_dir = os.path.dirname(os.path.realpath(__file__))
test_cert_file = os.path.join(current_dir, 'sp_cert.pem')
cred = ServicePrincipalAuth.build_credential(test_cert_file)
cred = ServicePrincipalAuth.build_credential(certificate=test_cert_file)
assert cred == {'certificate': test_cert_file}

# certificate path with '~', which expands to HOME folder
import shutil
home = os.path.expanduser('~')
home_cert = os.path.join(home, 'sp_cert.pem') # C:\Users\username\sp_cert.pem
shutil.copyfile(test_cert_file, home_cert)
cred = ServicePrincipalAuth.build_credential(os.path.join('~', 'sp_cert.pem')) # ~\sp_cert.pem
cred = ServicePrincipalAuth.build_credential(certificate=os.path.join('~', 'sp_cert.pem')) # ~\sp_cert.pem
assert cred == {'certificate': home_cert}
os.remove(home_cert)

cred = ServicePrincipalAuth.build_credential(test_cert_file, use_cert_sn_issuer=True)
# Certificate with use_cert_sn_issuer=True
cred = ServicePrincipalAuth.build_credential(certificate=test_cert_file, use_cert_sn_issuer=True)
assert cred == {'certificate': test_cert_file, 'use_cert_sn_issuer': True}

# client assertion
Expand Down
4 changes: 2 additions & 2 deletions src/azure-cli/azure/cli/command_modules/profile/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,7 @@ def load_arguments(self, command):
c.argument('username', options_list=['--username', '-u'],
help='User name, service principal client ID, or managed identity ID.')
c.argument('password', options_list=['--password', '-p'],
help='Provide credentials such as a user password, a service principal secret or a PEM file '
'with key and public certificate. Will prompt if not given.')
help='User password or service principal secret. Will prompt if not given.')
c.argument('tenant', options_list=['--tenant', '-t'], validator=validate_tenant,
help='The Microsoft Entra tenant, must be provided when using a service principal.')
c.argument('scopes', options_list=['--scope'], nargs='+',
Expand All @@ -66,6 +65,7 @@ def load_arguments(self, command):
# Service principal
c.argument('service_principal', action='store_true',
help='Log in with a service principal.')
c.argument('certificate', help='PEM file with key and public certificate.')
c.argument('use_cert_sn_issuer', action='store_true',
help='Use Subject Name + Issuer (SN+I) authentication in order to support automatic '
'certificate rolls.')
Expand Down
8 changes: 6 additions & 2 deletions src/azure-cli/azure/cli/command_modules/profile/_help.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,10 @@
For more details, see https://go.microsoft.com/fwlink/?linkid=2276314


[WARNING] Passing the service principal certificate with `--password` is deprecated and will be removed
by version 2.74. Please use `--certificate` instead.


To log in with a service principal, specify --service-principal.


Expand All @@ -35,8 +39,8 @@
text: az login -u [email protected] -p VerySecret
- name: Log in with a service principal using client secret. Use -p=secret if the first character of the password is '-'.
text: az login --service-principal -u http://azure-cli-2016-08-05-14-31-15 -p VerySecret --tenant contoso.onmicrosoft.com
- name: Log in with a service principal using client certificate.
text: az login --service-principal -u http://azure-cli-2016-08-05-14-31-15 -p ~/mycertfile.pem --tenant contoso.onmicrosoft.com
- name: Log in with a service principal using certificate.
text: az login --service-principal -u http://azure-cli-2016-08-05-14-31-15 --certificate ~/mycertfile.pem --tenant contoso.onmicrosoft.com
- name: Log in with a system-assigned managed identity.
text: az login --identity
- name: Log in with a user-assigned managed identity. You must specify the client ID, object ID or resource ID of the user-assigned managed identity with --username.
Expand Down
9 changes: 6 additions & 3 deletions src/azure-cli/azure/cli/command_modules/profile/custom.py
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ def login(cmd, username=None, password=None, tenant=None, scopes=None, allow_no_
# Device code flow
use_device_code=False,
# Service principal
service_principal=None, use_cert_sn_issuer=None, client_assertion=None,
service_principal=None, certificate=None, use_cert_sn_issuer=None, client_assertion=None,
# Managed identity
identity=False):
"""Log in to access Azure subscriptions"""
Expand Down Expand Up @@ -148,7 +148,7 @@ def login(cmd, username=None, password=None, tenant=None, scopes=None, allow_no_
logger.warning(_CLOUD_CONSOLE_LOGIN_WARNING)

if username:
if not (password or client_assertion):
if not (password or client_assertion or certificate):
Copy link
Member

Choose a reason for hiding this comment

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

Shall we also add certificate check in

if any([password, service_principal, tenant]) and identity:
raise CLIError("usage error: '--identity' is not applicable with other arguments")
if any([password, service_principal, username, identity]) and use_device_code:
raise CLIError("usage error: '--use-device-code' is not applicable with other arguments")

Copy link
Member Author

Choose a reason for hiding this comment

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

No, as missing all 3 types of credentials will result in prompting for secrets.

Copy link
Member

@evelyn-ys evelyn-ys Oct 28, 2024

Choose a reason for hiding this comment

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

I mean, if user pass in --certificate together with --identity/--use-device-code, we should raise error as well, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

--certificate will be discarded in that case. As you can see, client_assertion is not checked either. We can do that in a separate PR.

try:
password = prompt_pass('Password: ')
except NoTTYException:
Expand All @@ -158,7 +158,10 @@ def login(cmd, username=None, password=None, tenant=None, scopes=None, allow_no_

if service_principal:
from azure.cli.core.auth.identity import ServicePrincipalAuth
password = ServicePrincipalAuth.build_credential(password, client_assertion, use_cert_sn_issuer)
Copy link
Member Author

Choose a reason for hiding this comment

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

Passing keyword arguments as positional arguments is fragile and may break unexpectedly.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree, I am not a big fan of positional arguments in general.

password = ServicePrincipalAuth.build_credential(
secret_or_certificate=password,
certificate=certificate, use_cert_sn_issuer=use_cert_sn_issuer,
client_assertion=client_assertion)

login_experience_v2 = cmd.cli_ctx.config.getboolean('core', 'login_experience_v2', fallback=True)
# Send login_experience_v2 config to telemetry
Expand Down