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

[Profile] BREAKING CHANGE: az login: --password no longer accepts service principal certificate #30092

Merged
merged 1 commit into from
Nov 8, 2024
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
22 changes: 5 additions & 17 deletions src/azure-cli-core/azure/cli/core/auth/identity.py
Original file line number Diff line number Diff line change
Expand Up @@ -37,10 +37,6 @@
"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 @@ -196,7 +192,7 @@ def login_with_service_principal(self, client_id, credential, scopes):
"""
sp_auth = ServicePrincipalAuth.build_from_credential(self.tenant_id, client_id, credential)
client_credential = sp_auth.get_msal_client_credential()
cca = ConfidentialClientApplication(client_id, client_credential, **self._msal_app_kwargs)
cca = ConfidentialClientApplication(client_id, client_credential=client_credential, **self._msal_app_kwargs)
Copy link
Member Author

Choose a reason for hiding this comment

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

ConfidentialClientApplication's client_credential is a keyword argument. #29439 passes a positional argument, breaking tests such as azure.cli.core.auth.tests.test_identity.TestIdentity.test_login_with_service_principal_certificate.

Also see Azure/azure-cli-dev-tools#318

result = cca.acquire_token_for_client(scopes)
check_result(result)

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

@classmethod
def build_credential(cls, secret_or_certificate=None,
def build_credential(cls, client_secret=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.
Expand All @@ -318,20 +314,12 @@ def build_credential(cls, secret_or_certificate=None,
}
"""
entry = {}
if certificate:
if client_secret:
entry[_CLIENT_SECRET] = client_secret
elif 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
else:
entry[_CLIENT_SECRET] = secret_or_certificate
elif client_assertion:
entry[_CLIENT_ASSERTION] = client_assertion
return entry
Expand Down
16 changes: 3 additions & 13 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 @@ -264,20 +264,10 @@ def test_service_principal_auth_client_assertion(self):
assert client_credential == {'client_assertion': 'test_jwt'}

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

# secret with '~', which is preserved as-is
cred = ServicePrincipalAuth.build_credential(secret_or_certificate="~test_secret")
assert cred == {"client_secret": "~test_secret"}
Comment on lines -271 to -273
Copy link
Member Author

Choose a reason for hiding this comment

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

The secret is no longer expanded, so this check is meaningless.


# 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')
Expand All @@ -297,7 +287,7 @@ def test_build_credential(self):
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
# client_assertion
cred = ServicePrincipalAuth.build_credential(client_assertion="test_jwt")
assert cred == {"client_assertion": "test_jwt"}

Expand Down
24 changes: 17 additions & 7 deletions src/azure-cli-core/azure/cli/core/auth/util.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,12 @@
AccessToken = namedtuple("AccessToken", ["token", "expires_on"])


PASSWORD_CERTIFICATE_WARNING = (
"The error may be caused by passing a service principal certificate with --password. "
"Please note that --password no longer accepts a service principal certificate. "
"To pass a service principal certificate, use --certificate instead.")


def aad_error_handler(error, **kwargs):
""" Handle the error from AAD server returned by ADAL or MSAL. """

Expand All @@ -30,17 +36,21 @@ def aad_error_handler(error, **kwargs):
"below, please mention the hostname '%s'", socket.gethostname())

error_description = error.get('error_description')
error_codes = error.get('error_codes')

# Build recommendation message
login_command = _generate_login_command(**kwargs)
login_message = (
# Cloud Shell uses IMDS-like interface for implicit login. If getting token/cert failed,
# we let the user explicitly log in to AAD with MSAL.
"Please explicitly log in with:\n{}" if error.get('error') == 'broker_error'
else "Interactive authentication is needed. Please run:\n{}").format(login_command)
if error_codes and 7000215 in error_codes:
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the full message of 7000215?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is in the PR description:

AADSTS7000215: Invalid client secret provided. Ensure the secret being sent in the request is the client secret value, not the client secret ID, for a secret added to app 'dd5baa90-0c41-4cab-a458-e8c33d3249bb'.

recommendation = PASSWORD_CERTIFICATE_WARNING
else:
login_command = _generate_login_command(**kwargs)
recommendation = (
# Cloud Shell uses IMDS-like interface for implicit login. If getting token/cert failed,
# we let the user explicitly log in to AAD with MSAL.
"Please explicitly log in with:\n{}" if error.get('error') == 'broker_error'
else "Interactive authentication is needed. Please run:\n{}").format(login_command)

from azure.cli.core.azclierror import AuthenticationError
raise AuthenticationError(error_description, msal_error=error, recommendation=login_message)
raise AuthenticationError(error_description, msal_error=error, recommendation=recommendation)


def _generate_login_command(scopes=None, claims=None):
Expand Down
4 changes: 2 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,8 +21,8 @@
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.
[WARNING] `--password` no longer accepts a service principal certificate.
Use `--certificate` to pass a service principal certificate.
To log in with a service principal, specify --service-principal.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,7 @@ 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(
secret_or_certificate=password,
client_secret=password,
certificate=certificate, use_cert_sn_issuer=use_cert_sn_issuer,
client_assertion=client_assertion)

Expand Down
Loading