Skip to content

Conversation

@mccoyp
Copy link
Member

@mccoyp mccoyp commented Dec 16, 2020

Part of #15118.

This also includes some (very) minor fixes to the azure-keyvault-keys README.

@mccoyp mccoyp added KeyVault Client This issue points to a problem in the data-plane of the library. labels Dec 16, 2020
@mccoyp mccoyp added this to the MQ-2020 milestone Dec 16, 2020
@mccoyp mccoyp requested review from chlowell and schaabs December 16, 2020 01:05
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
You can also create a `CryptographyClient` to enable cryptographic operations (encrypt/decrypt, wrap/unwrap, sign/verify) using a particular key.
You can also create a `CryptographyClient` to perform cryptographic operations (encrypt/decrypt, wrap/unwrap, sign/verify) using a particular key.

Comment on lines 134 to 135
Copy link
Member

Choose a reason for hiding this comment

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

"oct" will get an error from a standard Key Vault. Also may be useful to show the KeyType enum here:

Suggested change
# create a key with specified type
key = key_client.create_key(name="key-name", key_type="oct")
from azure.keyvault.keys import KeyType
# create a key with specified type
key = key_client.create_key(name="key-name", key_type=KeyType.ec)

Copy link
Member

Choose a reason for hiding this comment

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

Suggest showing KeyCurveName here

Copy link
Member

Choose a reason for hiding this comment

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

This isn't mentioned in the text or used in the example?

Copy link
Member Author

Choose a reason for hiding this comment

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

This variable isn't used directly, but I split up the process of fetching the key version because I wanted to clearly show how the KeyId was being used. I don't think condensing lines 159/160 into key_version = KeyId(key_item.kid).version would be too complicated though, if you think that's neater

Copy link
Member

Choose a reason for hiding this comment

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

Ah, my bad, it's used in the sense I meant on the next line, it's fine like this 🤓

Copy link
Member

Choose a reason for hiding this comment

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

Another difference is that CryptographyClient performs operations locally when it has or can get the key material whereas every KeyVaultClient crypto operation is performed by Key Vault.

ciphertext = operation_result.result
```

Now in `azure-keyvault-keys` you can perform these cryptographic operations by using a `CryptographyClient`. The key used to create the client will be used for these operations. Cryptographic operations are now performed locally by the client, rather than remotely by Key Vault.
Copy link
Member

Choose a reason for hiding this comment

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

This implies CryptographyClient does everything locally, but actually it falls back to Key Vault when it can't get the key material.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm, that makes sense. Is there documentation of the CryptographyClient's behavior that I could refer/link to, or would this have to be gleaned from the source code?

Copy link
Member

Choose a reason for hiding this comment

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

Good question. It should be documented but is not: #15859

@mccoyp mccoyp merged commit 185c0f1 into Azure:master Dec 17, 2020
@mccoyp mccoyp deleted the migration-keys branch December 17, 2020 23:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Client This issue points to a problem in the data-plane of the library. KeyVault

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants