support local creds probing #119

yugangw-msft · 2018-10-23T05:48:45Z

For #118.
The probe sequence:

Manager service identity
only "system assigned identity" is probed, as user assigned identity would require extra arguments, and under that context users are better off using strong type credentials
Environment variable AZURE_CONNECTION_STRING with sdk auth file content in json, which is proposed in the design review meeting in June.
e.g. {AZURE_CLIENT_ID='...', AZURE_CLIENT_SECRET='...', AZURE_TENANT_ID='...', AZURE_CLOUD_NAME='...'}
AZURE_CLOUD_NAME is optional. We default to public cloud
Individual environment variables for a service principal
Like 2, rather using individual environment variables
Azure Authentication File (originating from java sdk)
Azure CLI credentials

Subscription id will be optional as the chainer will find them
Per suggestion, the example code includes both management plane and data plane client codes

yugangw-msft · 2018-10-23T05:59:55Z

//cc: @lmazuel @jianghaolu

yugangw-msft · 2018-10-23T17:08:59Z

cc: @jianghaolu @lmazuel @jhendrixMSFT @shahabhijeet @joshgav

lmazuel · 2018-10-23T17:34:45Z

msrestazure/azure_local_creds_prober.py

+            program_files_folder = os.environ.get('ProgramFiles(x86)') or os.environ.get('ProgramFiles')
+            probing_paths = [os.path.join(program_files_folder, 'Microsoft SDKs', 'Azure', 'CLI2', 'wbin', 'az.cmd')]
+        else:
+            probing_paths = ['/usr/bin/az', '/usr/local/bin/az']


This won't work if CLI was installed by pip. You could subprocess "which az", or use "shutil.which('az')" starting Python 3 (probably the right approach is "shutil.which('az')" on Python 3 and fallback to subprocess otherwise).
https://docs.python.org/3/library/shutil.html?highlight=.which#shutil.which

Can we use the presence of a ${HOME}/.azure directory as the hint to try the CLI creds?

...or, alternatively, use the ${PATH} environment variable...

lmazuel · 2018-10-23T17:37:34Z

msrestazure/azure_local_creds_prober.py

+
+    def signed_session(self, session=None):
+        # Token cache is handled by the VM extension, call each time to avoid expiration
+        self.set_token()


Should, the CLI be called each request to refresh the token?

I assume you meant sub-shelling is kind of slow? Yes, I can optimize by leveraging the expiresOn information returned from the get-access-token command

lmazuel · 2018-10-23T17:39:00Z

tests/test_local_creds_prober.py

+from msrestazure.azure_local_creds_prober import AzureLocalCredentialProber
+
+prober = AzureLocalCredentialProber()
+client = StorageManagementClient(prober, prober.subscription_id)


I would prefer to use the factory style I have right now:

from azure.common.client_factory import get_client_from_cli_profile from azure.mgmt.compute import ComputeManagementClient client = get_client_from_cli_profile(ComputeManagementClient)

but something more generic like:

from azure.common.client_factory import get_client from azure.mgmt.compute import ComputeManagementClient client = get_client(ComputeManagementClient)

johanste · 2018-10-23T18:58:25Z

msrestazure/azure_local_creds_prober.py

+class AzureLocalCredentialProber(object):
+    '''
+    Probing logics:
+    1. Managed service identity


The progression should be from more specific to less specific. This is the way I'd define the specificity of the different sources of information:

Environment variables > local file > local endpoint available.

Following this, does this: 2, 3, 1, 4 make sense?

I started with the most secure option, then follow up with SDK home made ones. But you have a point, so I will adjust. Also it is unlikely people would set env variable in a VM with identity

joshgav · 2018-10-23T22:32:03Z

Explicit environment variables should come before managed identity like @johanste says cause we should prefer explicit intent. That's what the specs say too :).

Rather than a global "Prober", could each credentials type include a static Probe() method which returns a boolean? That would grant more flexibility to the provider and allow us to add new providers and probes in the future.

Can we document the names of keys in the connection string and names of environment variables? We should use the same names in all SDKs.

joshgav · 2018-10-23T22:40:19Z

We will need to get tokens for resources other than AzureRM, e.g. Storage and Key Vault. Can the constructor be parameterized to take a resource URL and default to "https://management.azure.com"?

yugangw-msft · 2018-10-24T03:16:31Z

We will need to get tokens for resources other than AzureRM, e.g. Storage and Key Vault

Each specific SDK has the resource uri defined inside, e.g. go-sdk , so this is not a concern.
Since we are at it, let us see how to get sovereign clouds work. In Python's autorest runtime, those endpoints are pre-defined as a set of instances. I believe Java does too (cc: @jianghaolu). If other language SDKs have the similar supports, then the probing can default to public azure while client app can override with an enum.

yugangw-msft · 2018-10-26T04:34:42Z

Hi folks, i have addressed all feedback, please take another look.
The sample code wrapped in the unit test works for both storage management plane and data plane. Note the new storage data plan involves a highly unique requirement on client applications on how to supply the access token. I will cross check with my CLI colleague for whether or not I am doing the most optimal way.
The method now exposes the resource keyword arguments with the defaulting value on arm.

lmazuel · 2018-11-01T23:19:08Z

msrestazure/azure_local_creds_prober.py

+    def probe(self):
+        creds = None
+        if self.enabled and os.environ.get('AZURE_CONNECTION_STRING'):
+            auth_info = json.loads(os.environ.get('AZURE_CONNECTION_STRING'))


Is the connection string JSON valid? I thought it was something like:
AZURE_CONNECTION_STRING="client_id=1234;client_secret=2345;tenant_id=3456;subscription_id=4567;"

lmazuel · 2018-11-01T23:19:44Z

msrestazure/azure_local_creds_prober.py

+        return None
+
+    def probe_subscription(self, creds):
+        return json.loads(os.environ.get('AZURE_CONNECTION_STRING'))['subscriptionId']


Same question about JSON, and I thought too it should be subscription_id

lmazuel · 2018-11-01T23:19:55Z

msrestazure/azure_local_creds_prober.py

+            else:
+                cloud_environment = AZURE_PUBLIC_CLOUD
+            resource = self.resource or cloud_environment.endpoints.management
+            creds = ServicePrincipalCredentials(client_id=auth_info['clientId'],


lmazuel · 2018-11-01T23:21:23Z

msrestazure/azure_local_creds_prober.py

+        try:
+            creds = MSIAuthentication()
+             # MSI is not yet supported in sovereigns
+            setattr(creds, 'cloud_environment', AZURE_PUBLIC_CLOUD)


1/ PUBLIC AZURE is the default anyway 2/ we read cloud_environemnent from the kwargs, so should be passed to the init if we want to

lmazuel · 2018-11-01T23:21:54Z

msrestazure/azure_local_creds_prober.py

+            creds = MSIAuthentication()
+             # MSI is not yet supported in sovereigns
+            setattr(creds, 'cloud_environment', AZURE_PUBLIC_CLOUD)
+            creds.resource = self.resource or AZURE_PUBLIC_CLOUD.endpoints.management


Currently default is:
self.resource = kwargs.get('resource', self.cloud_environment.endpoints.active_directory_resource_id)

Is it wrong?

lmazuel · 2018-11-01T23:24:24Z

msrestazure/azure_local_creds_prober.py

+    def probe_subscription(self, creds):  # pylint: disable=no-self-use
+        subscription_id = None
+        try:
+            from azure.mgmt.resource.subscriptions import SubscriptionClient


I don't want to assume resource client is available. Especially the code here will use latest API version, which is a bad idea for stack.
I would prefer to hard code this one with a requests call, like I did for auto-registration of RP

lmazuel · 2018-11-01T23:25:10Z

msrestazure/azure_local_creds_prober.py

+                    _LOGGER.warning('You also have accesses to a few other subscriptions "%S".'
+                                    ' You can supply subscription_id on invoking probers')
+        except ImportError:  # should be rare
+            _LOGGER.warning('Failed to load azure.mgmt.resource.subscriptions to find the default'


This problem will disapear with a direct call to azure using requests

lmazuel · 2018-11-01T23:26:23Z

msrestazure/azure_local_creds_prober.py

+            else:
+                cloud_environment = AZURE_PUBLIC_CLOUD
+
+            resource = self.resource or cloud_environment.endpoints.management


Right now I use
resource = self._cloud_environment.endpoints.active_directory_resource_id

Is it wrong?

lmazuel · 2018-11-01T23:27:34Z

msrestazure/azure_local_creds_prober.py

+                process = Popen(['which', 'az'], stdout=PIPE, stderr=PIPE)
+                stdout, stderr = process.communicate()
+                process.wait()
+                if not stderr:


If stderr, log it as warning

lmazuel · 2018-11-01T23:28:11Z

msrestazure/azure_local_creds_prober.py

+                        _LOGGER.warning('More than one Azure CLI are installed at "%s"'
+                                        ' Pick the 1st one.', ', '.join(installed_clis))
+
+        if not cli_path:


We should provide a cli_path optional kwargs to cover weird scenario

lmazuel · 2018-11-01T23:30:14Z

msrestazure/azure_local_creds_prober.py

+    Detect CLI installations
+    '''
+    def probe(self):  # pylint: disable=no-self-use
+        uname = platform.uname()


I would take a different approach:

Let try to call "az"

If it fails, try to find it and retry

It's more Pythonic in the design (EAFP), and if "az" is in the Path it might work out of the box anyway.

lmazuel · 2018-11-01T23:42:21Z

msrestazure/azure_local_creds_prober.py

+        a. app service
+        b. virtual machine
+    4. Azure CLI, through "az account get-access-token"
+    '''


Add the authentication file:
https://docs.microsoft.com/en-us/python/azure/python-sdk-azure-authenticate?view=azure-python#mgmt-auth-file

lmazuel · 2018-11-01T23:43:28Z

msrestazure/azure_local_creds_prober.py

+
+def get_creds_through_local_probing(**kwargs):
+    '''
+    Probing logics:


Should we have the concept of critical failure? If we recognize some env variables and something is wrong, is this enough to fail completly and not fallback?
@johanste @joshgav

lmazuel · 2019-09-16T17:44:05Z

Will be done in azure-identity but not msrestazure

yugangw-msft added 3 commits October 22, 2018 22:25

POC

215f213

add tests

8c327c4

add license header

356ee6b

lmazuel added the enhancement label Oct 23, 2018

yugangw-msft added 2 commits October 23, 2018 09:48

capture more errors

4bee205

add docstring

7ad7583

more comments

3c61142

lmazuel reviewed Oct 23, 2018

View reviewed changes

johanste reviewed Oct 23, 2018

View reviewed changes

address review feedback on probe type, sequences

13bcc41

yugangw-msft force-pushed the chainer branch from 79ef430 to 13bcc41 Compare October 25, 2018 02:40

yugangw-msft added 4 commits October 25, 2018 14:48

optimization for cli creds handling

5f81ada

cli probing on linux and add storage examples

1cbd14e

fix CI error

5d223e5

add docstrings

17dbd90

yugangw-msft added 2 commits October 25, 2018 21:37

revert an unnecessary change

b0c48f8

finalize all

dda7a7f

yugangw-msft force-pushed the chainer branch from 53b570c to dda7a7f Compare October 28, 2018 05:37

integrate azure cloud

421d0a3

lmazuel reviewed Nov 1, 2018

View reviewed changes

add msi detecting

451a97e

yugangw-msft force-pushed the chainer branch from 717ff62 to 451a97e Compare November 2, 2018 06:01

lmazuel closed this Sep 16, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support local creds probing #119

support local creds probing #119

yugangw-msft commented Oct 23, 2018 •

edited

yugangw-msft commented Oct 23, 2018

yugangw-msft commented Oct 23, 2018

lmazuel Oct 23, 2018 •

edited

johanste Oct 23, 2018 •

edited

johanste Oct 23, 2018

lmazuel Oct 23, 2018

yugangw-msft Oct 24, 2018

lmazuel Oct 23, 2018

johanste Oct 23, 2018

yugangw-msft Oct 24, 2018

joshgav commented Oct 23, 2018

joshgav commented Oct 23, 2018

yugangw-msft commented Oct 24, 2018

yugangw-msft commented Oct 26, 2018

lmazuel Nov 1, 2018

lmazuel Nov 1, 2018

lmazuel Nov 1, 2018

lmazuel Nov 1, 2018

lmazuel Nov 1, 2018

lmazuel Nov 1, 2018 •

edited

lmazuel Nov 1, 2018

lmazuel Nov 1, 2018

lmazuel Nov 1, 2018

lmazuel Nov 1, 2018

lmazuel Nov 1, 2018

lmazuel Nov 1, 2018

lmazuel Nov 1, 2018

johanste Jan 10, 2019

lmazuel commented Sep 16, 2019

support local creds probing #119

support local creds probing #119

Conversation

yugangw-msft commented Oct 23, 2018 • edited

yugangw-msft commented Oct 23, 2018

yugangw-msft commented Oct 23, 2018

lmazuel Oct 23, 2018 • edited

Choose a reason for hiding this comment

johanste Oct 23, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshgav commented Oct 23, 2018

joshgav commented Oct 23, 2018

yugangw-msft commented Oct 24, 2018

yugangw-msft commented Oct 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lmazuel Nov 1, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lmazuel commented Sep 16, 2019

yugangw-msft commented Oct 23, 2018 •

edited

lmazuel Oct 23, 2018 •

edited

johanste Oct 23, 2018 •

edited

lmazuel Nov 1, 2018 •

edited