[Key Vault] Add local-only mode to CryptographyClient #16565

mccoyp · 2021-02-05T16:59:53Z

Resolves #16619.

Aligns Python's CryptographyClient with other languages, in providing a local-only mode by means of a factory method.

sdk/keyvault/azure-keyvault-keys/azure/keyvault/keys/crypto/_client.py

sdk/keyvault/azure-keyvault-keys/azure/keyvault/keys/crypto/aio/_client.py

sdk/keyvault/azure-keyvault-keys/azure/keyvault/keys/crypto/_client.py

sdk/keyvault/azure-keyvault-keys/tests/test_crypto_client.py

sdk/keyvault/azure-keyvault-keys/azure/keyvault/keys/crypto/_client.py

chlowell · 2021-03-04T17:28:44Z

sdk/keyvault/azure-keyvault-keys/tests/test_crypto_client.py

+
+    mock_client = mock.Mock()
+    jwk = mock.Mock(spec=JsonWebKey)
+    client = CryptographyClient.from_jwk(jwk=jwk)


We expect no request, so I would use a mock transport that raises when the client tries to send e.g.

mock.Mock(send=mock.Mock(side_effect=Exception("client shouldn't send a request"))

(That is, if it would be a bug for the client to send a request in local-only mode.)

This is one that I've made a note of to revisit -- I'm not too familiar with Mock and wasn't able to randomly turn enough dials to get a better understanding. I figure that asserting that the underlying client doesn't call get_key is sufficient for the time being

sdk/keyvault/azure-keyvault-keys/azure/keyvault/keys/crypto/_client.py

sdk/keyvault/azure-keyvault-keys/azure/keyvault/keys/_models.py

sdk/keyvault/azure-keyvault-keys/azure/keyvault/keys/crypto/_client.py

chlowell · 2021-03-05T21:45:09Z

sdk/keyvault/azure-keyvault-keys/azure/keyvault/keys/crypto/_client.py

@@ -106,16 +107,20 @@ def __init__(self, key, credential, **kwargs):
            self._key = None
            self._key_id = parse_key_vault_id(key)
            self._keys_get_forbidden = None  # type: Optional[bool]
+        elif self._jwk:
+            self._key_id = key.get("kid", "")
+            self._key = KeyVaultKey(None, jwk=key, _local_only=True)


Everything we need for local crypto is in the JWK. Do we really need to jam that into KeyVaultKey?

All the local crypto providers and operations assume that they're working with a KeyVaultKey, so we'd have to do so unless we refactor them to accept a JsonWebKey (unless I'm mistaken)

I don't think it's unreasonable by any means to make local providers accept JWKs, but it seems like a design question of whether we do that (and preserve the current definition of a KeyVaultKey) or make a smaller tweak to accept KeyVaultKeys intended for local-only use

Right, that's what I'm getting at. Their expectation of KeyVaultKey appears to be driving additional complexity here but that expectation isn't a requirement. Actually they only need the JWK. If they took that up front, maybe it wouldn't be necessary to complicate KeyVaultKey?

sdk/keyvault/azure-keyvault-keys/azure/keyvault/keys/crypto/_client.py

sdk/keyvault/azure-keyvault-keys/azure/keyvault/keys/crypto/_providers/__init__.py

sdk/keyvault/azure-keyvault-keys/azure/keyvault/keys/crypto/_key_validity.py

sdk/keyvault/azure-keyvault-keys/azure/keyvault/keys/crypto/_client.py

sdk/keyvault/azure-keyvault-keys/tests/test_examples_crypto.py

chlowell · 2021-03-08T23:35:51Z

sdk/keyvault/azure-keyvault-keys/azure/keyvault/keys/crypto/_providers/local_provider.py

        """The full identifier of the provider's key.

        :rtype: str
        """
-        return self._key.id
+        return self._key_id


This will be either a valid Key Vault identifier or whatever the user gave us:

Suggested change

return self._key_id

return self._key.get("kid")

Does that remove the need to ask a caller to give us _key_id separately?

My thinking here is that a KeyVaultKey provided to CryptographyClient could have a valid Key Vault identifier for its id, but that the JsonWebKey could have a different kid. I assumed it made more sense to return the Key Vault identifier in the operation result if possible, which is why I added the _key_id keyword argument

That would happen only when someone does KeyVaultKey(key_id, jwk=jwk) with key_id != jwk["kid"]. Granted, that is possible, but seems unlikely and given KeyVaultKey's docs ask for a Key Vault identifier for key_id, it seems reasonable not to special case it. My $0.02 🤑

sdk/keyvault/azure-keyvault-keys/azure/keyvault/keys/crypto/_client.py

chlowell · 2021-03-09T18:30:06Z

sdk/keyvault/azure-keyvault-keys/azure/keyvault/keys/crypto/_providers/local_provider.py

        """The full identifier of the provider's key.

        :rtype: str
        """
-        return self._key.id
+        return self._key_id


That would happen only when someone does KeyVaultKey(key_id, jwk=jwk) with key_id != jwk["kid"]. Granted, that is possible, but seems unlikely and given KeyVaultKey's docs ask for a Key Vault identifier for key_id, it seems reasonable not to special case it. My $0.02 🤑

sdk/keyvault/azure-keyvault-keys/azure/keyvault/keys/crypto/aio/_client.py

sdk/keyvault/azure-keyvault-keys/tests/test_examples_crypto.py

sdk/keyvault/azure-keyvault-keys/azure/keyvault/keys/crypto/aio/_client.py

chlowell · 2021-03-09T23:31:01Z

sdk/keyvault/azure-keyvault-keys/tests/test_crypto_client.py

@@ -276,6 +276,23 @@ def test_encrypt_local(self, azure_keyvault_url, **kwargs):
            result = crypto_client.decrypt(result.algorithm, result.ciphertext)
            self.assertEqual(result.plaintext, self.plaintext)

+    @KeyVaultPreparer()
+    def test_encrypt_local_from_jwk(self, azure_keyvault_url, **kwargs):


It's also possible to test private operations locally, using an imported key for public operations (the neighboring keys.py has suitable keys). Doesn't need to be in this PR but it's a test gap to fill before the next stable release. Also, need to cover EC keys as well.

chlowell · 2021-03-09T23:40:28Z

.../azure-keyvault-keys/tests/recordings/test_crypto_client_async.test_wrap_local_from_jwk.yaml

+    status:
+      code: 401
+      message: Unauthorized
+    url: https://mcpatinokv.vault.azure.net/keys/livekvtestwrap-localfe02144b/create?api-version=7.2-preview


Do you know why we've stopped replacing the vault name?

Vault names aren't being replaced in responses with the PowerShellPreparer -- this is another item on my list of things to investigate 🕵️‍♀️

Fix Swagger for SecurityInsights - Add teamInformation to IncidentProperties (Azure#16565)

mccoyp added KeyVault Client This issue points to a problem in the data-plane of the library. labels Feb 5, 2021

mccoyp force-pushed the feature/localclient-pr branch 3 times, most recently from f3ef95b to 22e32c4 Compare February 25, 2021 00:28

mccoyp commented Feb 25, 2021

View reviewed changes

sdk/keyvault/azure-keyvault-keys/azure/keyvault/keys/crypto/_client.py Outdated Show resolved Hide resolved

First prototype of from_jwk

13dd7a2

mccoyp force-pushed the feature/localclient-pr branch from 22e32c4 to 1ee77f7 Compare February 25, 2021 01:59

Add tests

41ef114

mccoyp force-pushed the feature/localclient-pr branch from 1ee77f7 to 41ef114 Compare February 25, 2021 23:39

mccoyp marked this pull request as ready for review February 25, 2021 23:42

mccoyp requested review from chlowell and schaabs as code owners February 25, 2021 23:42

mccoyp added this to the [2021] March milestone Feb 25, 2021

chlowell reviewed Feb 26, 2021

View reviewed changes

Incorporate feedback

fad4bc0

mccoyp requested a review from chlowell March 4, 2021 00:23

chlowell reviewed Mar 4, 2021

View reviewed changes

mccoyp added 4 commits March 4, 2021 10:08

Log before raising

9ecdf21

Accept JsonWebKey object

b801f12

Allow None key_id, try JWK's .kid

ae2967b

Use JsonWebKey in tests for .kid

0d2519f

mccoyp force-pushed the feature/localclient-pr branch from cd63ab5 to 219f32f Compare March 5, 2021 02:45

Add .from_jwk example

218a312

mccoyp force-pushed the feature/localclient-pr branch from 219f32f to 218a312 Compare March 5, 2021 02:47

Address pylint

8a602c1

chlowell reviewed Mar 5, 2021

View reviewed changes

sdk/keyvault/azure-keyvault-keys/azure/keyvault/keys/crypto/_client.py Outdated Show resolved Hide resolved

sdk/keyvault/azure-keyvault-keys/azure/keyvault/keys/_models.py Outdated Show resolved Hide resolved

Pass JWK to crypto client

8345eb6

mccoyp commented Mar 5, 2021

View reviewed changes

chlowell reviewed Mar 5, 2021

View reviewed changes

Add JWK support for local providers

ec7afaf

mccoyp requested a review from chlowell March 5, 2021 23:34

chlowell reviewed Mar 6, 2021

View reviewed changes

Local crypto only needs JWK

a4dd8f8

mccoyp requested a review from chlowell March 8, 2021 19:39

mccoyp added 2 commits March 8, 2021 11:52

JsonWebKey imports only for type checking

e6ae519

Appease pylint

a2d8c87

chlowell reviewed Mar 8, 2021

View reviewed changes

mccoyp force-pushed the feature/localclient-pr branch from e670d05 to 801ba6d Compare March 9, 2021 01:01

Thanks, Charles!

fb36a25

mccoyp force-pushed the feature/localclient-pr branch from 801ba6d to fb36a25 Compare March 9, 2021 01:08

mccoyp requested a review from chlowell March 9, 2021 01:52

chlowell reviewed Mar 9, 2021

View reviewed changes

Address feedback

5ca931d

mccoyp force-pushed the feature/localclient-pr branch from 6c843b8 to 5ca931d Compare March 9, 2021 23:01

mccoyp requested a review from chlowell March 9, 2021 23:12

Satisfy frozen six requirement

08f5935

chlowell approved these changes Mar 9, 2021

View reviewed changes

Remove unused import

8afbfe2

mccoyp merged commit 95035c7 into Azure:master Mar 10, 2021

mccoyp deleted the feature/localclient-pr branch March 10, 2021 00:54

mccoyp mentioned this pull request Mar 10, 2021

[Key Vault] Test enhancements for local-only cryptography #17227

Closed

3 tasks

openapi-sdkautomation bot pushed a commit to AzureSDKAutomation/azure-sdk-for-python that referenced this pull request Nov 9, 2021

CodeGen from PR 16565 in Azure/azure-rest-api-specs

69184af

Fix Swagger for SecurityInsights - Add teamInformation to IncidentProperties (Azure#16565)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Key Vault] Add local-only mode to CryptographyClient #16565

[Key Vault] Add local-only mode to CryptographyClient #16565

mccoyp commented Feb 5, 2021 •

edited

Loading

chlowell Mar 4, 2021

chlowell Mar 9, 2021

mccoyp Mar 9, 2021

chlowell Mar 5, 2021

mccoyp Mar 5, 2021 •

edited

Loading

mccoyp Mar 5, 2021

chlowell Mar 5, 2021

chlowell Mar 8, 2021

mccoyp Mar 9, 2021

chlowell Mar 9, 2021

chlowell Mar 9, 2021

chlowell Mar 9, 2021

chlowell Mar 9, 2021

mccoyp Mar 10, 2021

[Key Vault] Add local-only mode to CryptographyClient #16565

[Key Vault] Add local-only mode to CryptographyClient #16565

Conversation

mccoyp commented Feb 5, 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

mccoyp Mar 5, 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

mccoyp commented Feb 5, 2021 •

edited

Loading

mccoyp Mar 5, 2021 •

edited

Loading