Adding interfaces for storage client side encryption to Azure.Core #7601

schaabs · 2019-09-13T16:43:45Z

Adding IKeyEncryptionKey and IKeyEncryptionKeyResolver to Azure.Core.Cryptography
Updated CryptographyClient to implement IKeyEncryptionKey explicitly
Updated key vault algorithm enums to follow the extensible enum pattern used throughout the key vault client libraries

sdk/keyvault/Azure.Security.KeyVault.Keys/src/Cryptography/EncryptionAlgorithm.cs

sdk/core/Azure.Core/src/Cryptography/IKeyEncryptionKey.cs

sdk/keyvault/Azure.Security.KeyVault.Keys/src/Cryptography/SignatureAlgorithm.cs

danieljurek · 2019-09-13T17:10:15Z

/azp run net - keyvault - ci

azure-pipelines · 2019-09-13T17:10:25Z

Azure Pipelines successfully started running 1 pipeline(s).

pakrym · 2019-09-13T17:12:43Z

@danieljurek why did the pipeline fail so you had to restart it?

danieljurek · 2019-09-13T17:31:13Z

@danieljurek why did the pipeline fail so you had to restart it?

I'm not yet sure what's causing this. I'm having a look locally and tracking this in #7604

pakrym · 2019-09-13T17:37:43Z

I'm trying to get us to the point where we track all the issues that make builds flaky so if possible when restarting the pipeline reference the issues that's tracking the failure you are seeing (flaky test/build flakiness etc.) for we can prioritize fixing them in the right order.

pakrym · 2019-09-13T17:47:13Z

sdk/core/Azure.Core/src/Cryptography/IKeyEncryptionKey.cs

+    /// <summary>
+    /// A key which is used to encrypt, or wrap, another key
+    /// </summary>
+    public interface IKeyEncryptionKey


Wouldn't a IKeyEncryptor be a better name?

That's what I originally was going to call it. However, when this is fed to storage it's referred to as keyEncryptionKey so I felt this would more explicitly link the two together.

IKeyEncryptionKey does look strange. I think storage should still call the parameter whatever it think is best, but the type name could be what we think is best. BTW, is the interface specific to wrapping and unwrapping key? Or would it work on any byte array? If the later, we should call it without reffering to "key" at all.

This interface is specific to encrypting keys, in that it is only able to encrypt a single block as it doesn't use a nonce or IV. Also the algorithm specified is expected to be a key wrap algorithm, not just any encryption algorithm. I personally think that IKeyEncryptionKey is an appropriate name as it's an abstraction of a key which is able to wrap another key. While the name might seem odd without prior knowledge of cryptography concepts, it is a well known term in cryptography and key hierarchy management. https://csrc.nist.gov/glossary/term/Key_Encryption_Key

tg-msft

This looks awesome and is much nicer than another package. Thanks Scott!

tg-msft · 2019-09-13T17:46:11Z

sdk/core/Azure.Core/src/Cryptography/IKeyEncryptionKey.cs

+using System.Threading;
+using System.Threading.Tasks;
+
+namespace Azure.Core.Cryptography


So we'll use this from Azure.Storage.Blobs.Cryptography and you'll implement it in Azure.Security.KeyVault.Cryptography? That's a good namespace choice then.

tg-msft · 2019-09-13T17:51:45Z

sdk/core/Azure.Core/src/Cryptography/IKeyEncryptionKey.cs

+    /// <summary>
+    /// A key which is used to encrypt, or wrap, another key
+    /// </summary>
+    public interface IKeyEncryptionKey


I'm not an expert in this area by any stretch so I find this name a little jarring. It's not really a key but a thing that encrypts keys, right? Would IKeyEncryptionClient make sense since it follows most of our other client patterns? Disregard if this is a well known concept/pattern that should keep using this name.

I don't think that this should contain the word "Client" as quite often implementations of this would not call a service.

sdk/keyvault/Azure.Security.KeyVault.Keys/src/Cryptography/KeyWrapAlgorithm.cs

tg-msft · 2019-09-13T17:58:15Z

sdk/core/Azure.Core/src/Cryptography/IKeyEncryptionKeyResolver.cs

+        /// <param name="keyId">The key idenitifier of the key encryption key to retrieve</param>
+        /// <param name="cancellationToken">A <see cref="CancellationToken"/> controlling the request lifetime.</param>
+        /// <returns>The key encryption key corresponding to the specified keyId</returns>
+        IKeyEncryptionKey Resolve(string keyId, CancellationToken cancellationToken);


Nit - what happens when it's not resolved? Do we return null or throw something? It might be helpful to document that here.

heaths · 2019-09-16T16:57:45Z

sdk/core/Azure.Core/src/Cryptography/IKeyEncryptionKey.cs

+        /// <param name="key">The key to be encrypted</param>
+        /// <param name="cancellationToken">A <see cref="CancellationToken"/> controlling the request lifetime.</param>
+        /// <returns>The encrypted key bytes</returns>
+        byte[] WrapKey(string algorithm, byte[] key, CancellationToken cancellationToken);


If clients will be interacting with implementations via the interface, cancellationToken should be defaulted so the compiler emits the default if otherwise unspecified.

sdk/core/Azure.Core/src/Cryptography/IKeyEncryptionKeyResolver.cs

heaths · 2019-09-16T17:03:49Z

sdk/keyvault/Azure.Security.KeyVault.Keys/src/Cryptography/CryptographyClient.cs

+        /// <returns>The encrypted key</returns>
+        async Task<byte[]> IKeyEncryptionKey.WrapKeyAsync(string algorithm, byte[] key, CancellationToken cancellationToken)
+        {
+            return (await WrapKeyAsync(new KeyWrapAlgorithm(algorithm), key, cancellationToken).ConfigureAwait(false)).EncryptedKey;


Nit: breaking apart complex statements makes it easier to debug later, i.e. make this two separate statements. IMO, makes it more readable as well.

schaabs · 2019-09-16T17:46:41Z

/azp run net - keyvault - ci

azure-pipelines · 2019-09-16T17:46:51Z

Azure Pipelines successfully started running 1 pipeline(s).

schaabs · 2019-09-16T17:52:12Z

restarted pipeline for issue #7490

KrzysztofCwalina · 2019-09-16T23:44:53Z

sdk/core/Azure.Core/src/Cryptography/IKeyEncryptionKey.cs

+    /// <summary>
+    /// A key which is used to encrypt, or wrap, another key
+    /// </summary>
+    public interface IKeyEncryptionKey


IKeyEncryptionKey does look strange. I think storage should still call the parameter whatever it think is best, but the type name could be what we think is best. BTW, is the interface specific to wrapping and unwrapping key? Or would it work on any byte array? If the later, we should call it without reffering to "key" at all.

KrzysztofCwalina · 2019-09-16T23:50:31Z

sdk/core/Azure.Core/src/Cryptography/IKeyEncryptionKey.cs

+        /// <param name="key">The key to be encrypted</param>
+        /// <param name="cancellationToken">A <see cref="CancellationToken"/> controlling the request lifetime.</param>
+        /// <returns>The encrypted key bytes</returns>
+        byte[] WrapKey(string algorithm, byte[] key, CancellationToken cancellationToken = default);


Should take ReadOnlyMemory instead of byte[]?

KrzysztofCwalina · 2019-09-16T23:51:49Z

sdk/keyvault/Azure.Security.KeyVault.Keys/src/Cryptography/EncryptionAlgorithm.cs

+        public static implicit operator string(EncryptionAlgorithm value) => value._value;
+
+        /// <inheritdoc/>
+        public override bool Equals(object obj) => obj is EncryptionAlgorithm other && Equals(other);


This should have EditorBrowsable false

I added this to Equals, GetHashCode and ToString for the added enum like structs.

Adding interfaces for storage client side encryption to Azure.Core

97884aa

schaabs requested review from tg-msft, heaths, pakrym and KrzysztofCwalina September 13, 2019 16:43

tg-msft requested a review from jaschrep-msft September 13, 2019 16:44

pakrym reviewed Sep 13, 2019

View reviewed changes

sdk/keyvault/Azure.Security.KeyVault.Keys/src/Cryptography/EncryptionAlgorithm.cs Outdated Show resolved Hide resolved

pakrym reviewed Sep 13, 2019

View reviewed changes

sdk/core/Azure.Core/src/Cryptography/IKeyEncryptionKey.cs Outdated Show resolved Hide resolved

minor cleanup

c23326a

pakrym reviewed Sep 13, 2019

View reviewed changes

sdk/keyvault/Azure.Security.KeyVault.Keys/src/Cryptography/SignatureAlgorithm.cs Outdated Show resolved Hide resolved

making 'extensible enum' structs readonly

dffaca8

pakrym reviewed Sep 13, 2019

View reviewed changes

tg-msft approved these changes Sep 13, 2019

View reviewed changes

heaths reviewed Sep 16, 2019

View reviewed changes

sdk/core/Azure.Core/src/Cryptography/IKeyEncryptionKeyResolver.cs Show resolved Hide resolved

heaths reviewed Sep 16, 2019

View reviewed changes

heaths approved these changes Sep 16, 2019

View reviewed changes

making interface cancellationToken arguments optional

2471f4e

KrzysztofCwalina reviewed Sep 16, 2019

View reviewed changes

updates addressing PR feedback

8a47593

schaabs merged commit 7d13263 into Azure:master Sep 17, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding interfaces for storage client side encryption to Azure.Core #7601

Adding interfaces for storage client side encryption to Azure.Core #7601

schaabs commented Sep 13, 2019

danieljurek commented Sep 13, 2019

azure-pipelines bot commented Sep 13, 2019

pakrym commented Sep 13, 2019

danieljurek commented Sep 13, 2019

pakrym commented Sep 13, 2019

pakrym Sep 13, 2019

schaabs Sep 13, 2019

KrzysztofCwalina Sep 16, 2019

schaabs Sep 17, 2019

tg-msft left a comment

tg-msft Sep 13, 2019

tg-msft Sep 13, 2019

schaabs Sep 13, 2019

tg-msft Sep 13, 2019

heaths Sep 16, 2019

heaths Sep 16, 2019

schaabs commented Sep 16, 2019

azure-pipelines bot commented Sep 16, 2019

schaabs commented Sep 16, 2019

KrzysztofCwalina Sep 16, 2019

KrzysztofCwalina Sep 16, 2019

KrzysztofCwalina Sep 16, 2019

schaabs Sep 17, 2019

Adding interfaces for storage client side encryption to Azure.Core #7601

Adding interfaces for storage client side encryption to Azure.Core #7601

Conversation

schaabs commented Sep 13, 2019

danieljurek commented Sep 13, 2019

azure-pipelines bot commented Sep 13, 2019

pakrym commented Sep 13, 2019

danieljurek commented Sep 13, 2019

pakrym commented Sep 13, 2019

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

tg-msft 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

schaabs commented Sep 16, 2019

azure-pipelines bot commented Sep 16, 2019

schaabs commented Sep 16, 2019

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