[Identity] azure.identity.aio.AzureCliCredential unusable due to malformed command line

[cdunklau]

• Package Name: azure-identity
• Package Version: 1.4.0b5
• Operating System: macOS Catalina 10.15.5
• Python Version: 3.7.5

Describe the bug

The async flavor of AzureCliCredential doesn't properly construct the args for its az account get-access-token call, so /bin/sh (bash on my system) barfs:

<snip>

  File "/.../venv/lib/python3.7/site-packages/azure/identity/aio/_credentials/azure_cli.py", line 48, in get_token
    output = await _run_command(COMMAND_LINE.format(resource))
  File "/.../venv/lib/python3.7/site-packages/azure/identity/aio/_credentials/azure_cli.py", line 96, in _run_command
    raise ClientAuthenticationError(message=message)
azure.core.exceptions.ClientAuthenticationError: /bin/sh: - : invalid option
Usage:  /bin/sh [GNU long option] [option] ...
        /bin/sh [GNU long option] [option] script-file ...
GNU long options:
        --debug
        --debugger
        --dump-po-strings

<snip>

It looks like this is the culprit:

azure-sdk-for-python/sdk/identity/azure-identity/azure/identity/aio/_credentials/azure_cli.py

Lines 61 to 65 in a7ac22a

	async def _run_command(command):
	if sys.platform.startswith("win"):
	args = ("cmd", "/c " + command)
	else:
	args = ("/bin/sh", "-c " + command)

I'm not intimately familiar with how Windows parses CLAs but that's definitely wrong on *nix, it should be args = ("/bin/sh", "-c", command). Changing to that fixes the problem for me.

Side note, it seems odd to use the shell at all. Is there a particular reason for that, as opposed to just giving the az command line directly, e.g. args = ('az', 'account', 'get-access-token', '--output', 'json', '--resource' 'https://vault.azure.net')? This would also remove the need for the platform-specific code.

To Reproduce

Change the vault name and secret name below, and run it on Mac. Linux probably suffers as well, but I'm not sure if Windows is affected:

import sys
import logging
import asyncio

from azure.identity.aio import AzureCliCredential
from azure.keyvault.secrets.aio import SecretClient


logging.basicConfig(level=logging.DEBUG, stream=sys.stderr)

async def run():
    async with AzureCliCredential() as credential:

        async with SecretClient(
            vault_url="https://REPLACEME.vault.azure.net/",
            credential=credential,
        ) as secret_client:

            secret = await secret_client.get_secret('REPLACEME-SECRET-NAME')
            print(secret.name)
            print(secret.value)

asyncio.run(run())

Observe azure.core.exceptions.ClientAuthenticationError: /bin/sh: - : invalid option

Expected behavior

The above should print the name and value for the secret.

[chlowell]

Thanks for reporting this! It will be fixed in the next release.

Side note, it seems odd to use the shell at all. Is there a particular reason for that, as opposed to just giving the az command line directly, e.g. args = ('az', 'account', 'get-access-token', '--output', 'json', '--resource' 'https://vault.azure.net')?

Yes: to search the path for az rather than execute something named az in the working directory.

[cdunklau]

Hi @chlowell, thanks for the speedy response!

The fix (#12056) seems to only address *nix, but I wonder if the bug exists on Windows too. Do you have the ability to test there? The synchronous client builds the args consistently for both platforms:

azure-sdk-for-python/sdk/identity/azure-identity/azure/identity/_credentials/azure_cli.py

Lines 100 to 103 in 485ee38

	if sys.platform.startswith("win"):
	args = ["cmd", "/c", command]
	else:
	args = ["/bin/sh", "-c", command]

...but they're different in the async client (since your PR's merge):

azure-sdk-for-python/sdk/identity/azure-identity/azure/identity/aio/_credentials/azure_cli.py

Lines 62 to 65 in 485ee38

	if sys.platform.startswith("win"):
	args = ("cmd", "/c " + command)
	else:
	args = ("/bin/sh", "-c", command)

Note args = ("cmd", "/c " + command)... maybe that's fine on Windows, I'm not sure, but I suspect it would be worth being consistent with the synchronous implementation, regardless. What do you think?

Side note, it seems odd to use the shell at all. Is there a particular reason for that, as opposed to just giving the az command line directly, e.g. args = ('az', 'account', 'get-access-token', '--output', 'json', '--resource' 'https://vault.azure.net')?

Yes: to search the path for az rather than execute something named az in the working directory.

Aha, so on *nix, that's not a concern; to execute something not in PATH, you must specify the filesystem path, e.g. ./az... but IIRC Windows does the implicit cwd-in-PATH thing, right? Does that also occur with Python's subprocess module?

[chlowell]

The fix (#12056) seems to only address *nix, but I wonder if the bug exists on Windows too. Do you have the ability to test there?

Yes, it works on Windows.

Note args = ("cmd", "/c " + command)... maybe that's fine on Windows, I'm not sure, but I suspect it would be worth being consistent with the synchronous implementation, regardless. What do you think?

I agree it would be better to be consistent with the sync version, if only so no one else wonders whether it works on Windows 😊

Aha, so on *nix, that's not a concern; to execute something not in PATH, you must specify the filesystem path, e.g. ./az... but IIRC Windows does the implicit cwd-in-PATH thing, right? Does that also occur with Python's subprocess module?

I haven't tested this behavior of subprocess. Even if it does the thing we want, we'd still invoke a shell here because doing so consistently across all our SDKs (.NET, Java, JavaScript) makes the implementation easier for us to reason about.

[cdunklau]

Thanks for the detailed clarification :)