Add AzureNamedKeyCredential #17548

rakshith91 · 2021-03-24T22:55:25Z

Summary

When using a shared key for authorization, some services require that the name of the key be used as a component of the algorithm to form the authorization token. For these cases, AzureKeyCredential and AzureSasCredential aren't adequate as they track only a single value. A new credential type, AzureNamedKeyCredential has been approved to address this gap.

Goals

Offer a token credential suitable for working with any service needing a named key that follows patterns established by the other credential types offered in Azure.Core and Azure.Identity.
Developers should be able to construct the credential by providing a string-based name and key.
Developers should be able to update the name and key after construction to support rolling credentials or using a new access policy. Updating credential information should be done in-place and not require creating a new credential instance.
Developers should be able to read the name of they key, in order to support caching within an application so that it can be easily identified when updates are needed.
The Azure SDKs should be able to read the name and key, ensuring they reflect the most current values.
Support updates and reads concurrently with consistent behavior. Developers should be able to update the credential at any time without concern for whether the SDK is reading from it.

Non-Goals

Providing a friendly and discoverable means for developers to read the key from the credential; it is preferable to hide the key or discourage use.
Allowing the name or key to be updated independently; both elements will be required when updating a credential.

Success Criteria

An AzureNamedKeyCredential type has been implemented, following language guidelines and idioms.
The tests necessary for its validation have been created and pass reliably.
The existing test suite continues to produce deterministic results and pass reliably.

xiangyan99 · 2021-03-24T23:00:11Z

The code looks good.

Please talk to Johan & Anna for the architect perspective feedback.

swathipil

LGTM

sdk/core/azure-core/CHANGELOG.md

Co-authored-by: swathipil <76007337+swathipil@users.noreply.github.com>

johanste · 2021-03-26T17:00:31Z

sdk/core/azure-core/azure/core/credentials.py

+        """
+        return self._key
+
+    def update(self, name, key):


Do we have scenarios where you want to update/change the name of the key?

I think one scenario could be that users want to use a new share access policy which has new name and key.

Can user update key only?

technically - they are allowed to provide the same name - but they must provide a name

I think the most common scenario is the name is fixed after creation, but user may modify the key value?

@yunhaoling any thoughts on this?

If we know that users may normally not modify the name, I'll incline towards making name kwarg only

actually on second thought, i think we should let this be - as much as i like kwargs, it doesn't seem like they'll grow later in this case.

also, if the user thinks they wont need to change name for their use case, AzureKeyCredential serves their purpose

...unless they need to initially specify the name?

hmm, right - still doesn't change tht kwargs would not grow in this case - so will stick to it

johanste · 2021-03-26T17:01:50Z

sdk/core/azure-core/azure/core/credentials.py

+        """
+        if not isinstance(name, six.string_types) or not isinstance(key, six.string_types):
+            raise TypeError("Both name and key must be Strings.")
+        self._name = name


The reason for a single update method rather than two read/write properties is (per my understanding) to make the assignment atomic. This is not an atomic assignment. Is there a specific reason we have an non-atomic update method?

How common user creates an AzureNamedKeyCredential then modify the name?

It seems to me maybe

def update(self, key, **kwarg):

makes more sense?

@johanste

the update method is to

be consistent with other creds in python (aka AzureKeyCredential)

be consistent with other languages when it comes to AzureNamedKeyCredential

should we have two read/write properties instead of an update method?

No. The reason for the update method is to be atomic. You cannot do that unless you expect callers to lock the instance when reading values.

johanste · 2021-03-26T17:05:19Z

sdk/core/azure-core/azure/core/pipeline/policies/_authentication.py

+    def __init__(self, credential, **kwargs):  # pylint: disable=unused-argument
+        # type: (AzureNamedKeyCredential, **Any) -> None
+        super(AzureNamedKeyCredentialPolicy, self).__init__()
+        self._key = credential.key


What happens when a client is created with a given NamedKeyCredential and later call update on the NamedKeyCredential? The whole reason for the update method is to be able to roll the key.

johanste · 2021-04-15T21:06:44Z

sdk/core/azure-core/azure/core/credentials.py

+        Individual attributes cannot be updated.
+        """
+        if not isinstance(name, six.string_types) or not isinstance(key, six.string_types):
+            raise TypeError("Both name and key must be Strings.")


why Strings (with capital S)?

johanste · 2021-04-15T21:13:01Z

sdk/core/azure-core/azure/core/credentials.py

+        self._credential = AzureNamedKey(name, key)
+
+    @property
+    def credential(self):


It feels odd to have a credential property on a credential object. Is this the right name?

changed to named_key

johanste · 2021-04-15T21:17:28Z

sdk/core/azure-core/azure/core/pipeline/policies/_authentication.py

+    :type credential: ~azure.core.credentials.AzureNamedKeyCredential
+    :raises: ValueError or TypeError
+    """
+    def __init__(self, credential, name, **kwargs):  # pylint: disable=unused-argument


Who is consuming the name of the AzureNamedKeyCredential?

sdk/core/azure-core/azure/core/pipeline/policies/_authentication.py

…on.py

sdk/core/azure-core/azure/core/pipeline/policies/__init__.py

xiangyan99 · 2021-04-16T18:00:36Z

sdk/core/azure-core/CHANGELOG.md


+### New Features
+
+- Added `azure.core.credentials.AzureNamedKeyCredential` credential and its respective policy #17548.


I don't see the policy in your PR?

updted the change log

johanste · 2021-04-23T19:02:04Z

sdk/core/azure-core/azure/core/credentials.py

+        """
+        return self._key
+
+    def update(self, name, key):


...unless they need to initially specify the name?

johanste · 2021-04-23T19:04:28Z

sdk/core/azure-core/tests/test_authentication.py

+
+    cred.update("newname", "newkey")
+    assert cred.named_key.name == "newname"
+    assert cred.named_key.key == "newkey"


We should have a test to make sure that we are returning a tuple as well...

added - for clarification, we don't really return a tuple, we update the cred with a new tuple

Feature/cplat 2021 10 01 (Azure#17548) * Feature/cplat 2021 10 03 (Azure#16512) * update new api version branch with base * change 2021-10-03 to 2021-10-01 * Update readme.md * fix folder structure * add community gallery * Feature/cplat 2021 10 03 (Azure#16512) * update new api version branch with base * change 2021-10-03 to 2021-10-01 * Update readme.md * fix folder structure * add community gallery * remove sharedGallery and communityGallery * Gallery 10-01 change (community gallery, CVM) (Azure#16824) * add new api version 2020-09-30 for gallery.json * add support for api change, sharing profile related * update, change post to patch * fix model validation error * fix typo * add shared gallery api * update * fix typo * update * update * update * chagne new api version to preview * update readme.md and nit * remove some required field and make groups readonly * add swagger support for Grace's and Tim's work * error fix * error fix * change api version name to 2020-09-30 withour 'preview' * update * fix typo * address pr comment * set modelAsString to true for gallery sharing-related enum * update, fix merge * remove duplicate entry * shared gallery, change id to identifier.uniqueId * fix typo * update, remove x-ms-azure-resource for shared gallery * change name of Permissions to avoid SDK code build error * update * add new line * correct reset * update * save * update * fix * update * update * update * resolve CI error * update * Edge zone (Azure#17097) * added target extended locations to the caps gallery schema and added an example file * fixed a comma * Update readme.md * Rename specification/compute/resource-manager/Microsoft.Compute/stable/2021-10-01/gallery/CreateOrUpdateASimpleGalleryImageVersionWithTargetExtendedLocations.json to specification/compute/resource-manager/Microsoft.Compute/stable/2021-10-01/examples/gallery/CreateOrUpdateASimpleGalleryImageVersionWithTargetExtendedLocations.json * Update readme.md * update * address comment * resolve CI Co-authored-by: Andrew Sager <asager@microsoft.com> Co-authored-by: Theodore Chang <thchan@microsoft.com> * save (Azure#17600) Co-authored-by: kangsun-ctrl <69279251+kangsun-ctrl@users.noreply.github.com> Co-authored-by: Andrew Sager <asager@microsoft.com>

Add AzureNamedKeyCredential

7f70520

rakshith91 requested review from lmazuel and xiangyan99 as code owners March 24, 2021 22:55

ghost added the Azure.Core label Mar 24, 2021

rakshith91 requested review from yunhaoling, swathipil and johanste March 24, 2021 22:55

rakshith91 requested a review from annatisch March 24, 2021 23:00

rakshith91 and others added 2 commits March 24, 2021 17:12

lint

3db2ef2

Merge branch 'master' into named_key

be51628

swathipil reviewed Mar 26, 2021

View reviewed changes

sdk/core/azure-core/CHANGELOG.md Show resolved Hide resolved

Update sdk/core/azure-core/CHANGELOG.md

87530e9

Co-authored-by: swathipil <76007337+swathipil@users.noreply.github.com>

johanste reviewed Mar 26, 2021

View reviewed changes

rakshith91 and others added 6 commits April 14, 2021 15:48

some changes

8be7ce5

oops

8b21514

Merge branch 'master' into named_key

75ecf46

version

05c4205

mypy fix

d8ba6de

lint

1555f1d

johanste reviewed Apr 15, 2021

View reviewed changes

rakshith91 added 2 commits April 16, 2021 10:50

remove policy

6ab090c

doc

6a98f0b

rakshith91 commented Apr 16, 2021

View reviewed changes

sdk/core/azure-core/azure/core/pipeline/policies/_authentication.py Outdated Show resolved Hide resolved

Update sdk/core/azure-core/azure/core/pipeline/policies/_authenticati…

5f94691

…on.py

rakshith91 commented Apr 16, 2021

View reviewed changes

sdk/core/azure-core/azure/core/pipeline/policies/__init__.py Outdated Show resolved Hide resolved

Update sdk/core/azure-core/azure/core/pipeline/policies/__init__.py

53fab2e

rakshith91 commented Apr 16, 2021

View reviewed changes

sdk/core/azure-core/azure/core/pipeline/policies/__init__.py Outdated Show resolved Hide resolved

Update sdk/core/azure-core/azure/core/pipeline/policies/__init__.py

0865f90

xiangyan99 reviewed Apr 16, 2021

View reviewed changes

rakshith91 added 3 commits April 16, 2021 11:41

changelog

4a6736f

update tests

4c2143d

fix

71f0b35

rakshith91 closed this Apr 20, 2021

rakshith91 reopened this Apr 20, 2021

johanste approved these changes Apr 23, 2021

View reviewed changes

update test

6bc4c0a

rakshith91 merged commit 844e16d into Azure:master Apr 25, 2021

yunhaoling mentioned this pull request Apr 26, 2021

Eh named key #18292

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add AzureNamedKeyCredential #17548

Add AzureNamedKeyCredential #17548

rakshith91 commented Mar 24, 2021 •

edited

Loading

xiangyan99 commented Mar 24, 2021

swathipil left a comment

johanste Mar 26, 2021

yunhaoling Mar 26, 2021

xiangyan99 Apr 16, 2021

rakshith91 Apr 16, 2021

xiangyan99 Apr 16, 2021

rakshith91 Apr 16, 2021

rakshith91 Apr 16, 2021

rakshith91 Apr 16, 2021

johanste Apr 23, 2021

rakshith91 Apr 25, 2021

johanste Mar 26, 2021

xiangyan99 Mar 26, 2021

rakshith91 Mar 29, 2021 •

edited

Loading

johanste Apr 16, 2021

johanste Mar 26, 2021

johanste Apr 15, 2021

johanste Apr 15, 2021

rakshith91 Apr 16, 2021

johanste Apr 15, 2021

xiangyan99 Apr 16, 2021

rakshith91 Apr 16, 2021

johanste Apr 23, 2021

johanste Apr 23, 2021

rakshith91 Apr 25, 2021 •

edited

Loading


		### New Features

		- Added `azure.core.credentials.AzureNamedKeyCredential` credential and its respective policy #17548.

Add AzureNamedKeyCredential #17548

Add AzureNamedKeyCredential #17548

Conversation

rakshith91 commented Mar 24, 2021 • edited Loading

Summary

Goals

Non-Goals

Success Criteria

xiangyan99 commented Mar 24, 2021

swathipil left a 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

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

rakshith91 Mar 29, 2021 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rakshith91 Apr 25, 2021 • edited Loading

Choose a reason for hiding this comment

rakshith91 commented Mar 24, 2021 •

edited

Loading

rakshith91 Mar 29, 2021 •

edited

Loading

rakshith91 Apr 25, 2021 •

edited

Loading