Port clientside encryption to mono-dll #12183

jaschrep-msft · 2020-05-19T16:29:02Z

Ported client-side encryption work from long-diverged branch. Was an inheritance approach, is now part of the main package internals.
Ported over internal WindowStream from stg73base, which allows us to prematurely end streams.
Removed internal RollingBufferStream, as uploads no longer require seekable streams as input. Upload paths that handle non-seekable streams already have their own buffer strategy.

Ported blob client-side encryption work from long-diverged branch. Was an inheritance approach, is now part of the main package internals. Ported over WindowStream from stg73base, which allows us to prematurely end streams. Removed RollingBufferStream, as uploads no longer require seekable streams as input.

jaschrep-msft · 2020-05-19T16:31:54Z

Ported over blobs. Coming up in this PR, porting queues, extra tests for blob and queue, introduction of ThrowForMissingDecryptionKey flag.

kasobol-msft

The publicly facing part looks good (I left ~2 questions for that).
I think we should work little bit on cleaning up the implementation.

kasobol-msft · 2020-05-19T22:17:36Z

sdk/storage/Azure.Storage.Blobs/api/Azure.Storage.Blobs.netstandard2.0.cs

 }
 namespace Azure.Storage.Blobs.Specialized
 {
+    public partial class AdvancedBlobClientOptions : Azure.Storage.Blobs.BlobClientOptions


I'm not 100% convinced Advanced is good prefix. Maybe Extended?
Or should we say Hidden? :D

I like Extended

I can go either way. Can we get @tg-msft's input on naming decisions?

I moved to Extended. We'll let final API review catch the name if it's unwanted.

sdk/storage/Azure.Storage.Blobs/api/Azure.Storage.Blobs.netstandard2.0.cs

sdk/storage/Azure.Storage.Blobs/src/BlobBaseClient.cs

sdk/storage/Azure.Storage.Blobs/src/BlobServiceClient.cs

kasobol-msft · 2020-05-19T23:17:15Z

sdk/storage/Azure.Storage.Blobs/tests/ClientSideEncryptionTests.cs

+
+        #region Utility
+
+        private byte[] LocalManualEncryption(byte[] data, byte[] key, byte[] iv)


nit: method naming should be verb-ish. EncryptLocally or DoLocalEncryption. noun-ish terms are reserved for types. btw. what does manual mean?

It means I'm invoking the encryption myself, instead of going through the client's automated processes. Maybe explicit is a better word than manual?

Maybe bypass may be a candidate term to consider? Is that what you're trying to do?

Independent/External/Baseline?

Manual indicates human involvement in the process.

kasobol-msft · 2020-05-19T23:17:57Z

sdk/storage/Azure.Storage.Blobs/tests/ClientSideEncryptionTests.cs

+        }
+
+
+        #region Utility


nit: I'm not sure what this region means. besides it's not closed.

It is closed, write above the first test.

Region is intended to separate tests from methods that help run the tests.

okay. I think vertical placement (tests near tests, aids near aids) is good enough. I'd avoid Utility naming if you want to keep region

kasobol-msft · 2020-05-19T23:19:37Z

sdk/storage/Azure.Storage.Blobs/src/Models/ContentRange.cs

+            }
+
+            /// <summary>
+            /// AES-CBC using a 256 bit key.


this class doesn't seem to be dependent on algorithm. this information may find better home.

Copy/paste error. Property is necessary but comment is irrelevant.

kasobol-msft · 2020-05-19T23:26:44Z

sdk/storage/Azure.Storage.Blobs/src/BlobClient.cs

+        /// </param>
        /// <returns>Transformed content stream and metadata.</returns>
-        internal virtual (Stream, Metadata) TransformContent(Stream content, Metadata metadata)
+        private async Task<(Stream, Metadata)> ClientSideEncryptInternal(


Maybe we could keep the TransformContent idea and still inject encryption just make it all internal? that way we'd keep crypto related stuff in other class/namespace without cluttering blobclient.

I've pulled out all the crypto transformations, but have left in the explicit call to an encryption method instead of a generic TransformContent. Upcoming push.

kasobol-msft · 2020-05-19T23:28:56Z

sdk/storage/Azure.Storage.Blobs/src/BlobBaseClient.cs

+                    // we already return a nonseekable stream; returning a crypto stream is fine
+                    if (UsingClientSideEncryption)
+                    {
+                        stream = await ClientSideDecryptInternal(stream, response.Value.Metadata, encryptedRange.OriginalRange, response.Value.ContentRange, async, cancellationToken).ConfigureAwait(false);


maybe we could make this extension and move out crypto code from this client ?

Created new classes to handle the translation between clients and service-agnostic encryption/decryption logic. Upcoming push.

sdk/storage/Azure.Storage.Blobs/src/BlobBaseClient.cs

sdk/storage/Azure.Storage.Blobs/src/PageBlobClient.cs

...rage/Azure.Storage.Common/src/Shared/ClientsideEncryption/Models/EncryptionDataSerializer.cs

sdk/storage/Azure.Storage.Common/src/Shared/Errors.Clients.cs

sdk/storage/Azure.Storage.Queues/src/QueueClient.cs

Queues now has a listener for when only some messages cannot be decrypted. these messages are filtered out of the response and sent to the listened instead. If no listener is provided, the whole fetch throws. Some PR comments addressed.

kasobol-msft

Looks good for shipping preview from it. We should address rest of the feedback here or before GA.

kasobol-msft · 2020-05-20T22:51:09Z

sdk/storage/Azure.Storage.Blobs/tests/AlwaysFailsKeyEncryptionKeyResolver.cs

+
+namespace Azure.Storage.Blobs.Tests
+{
+    internal class AlwaysFailsKeyEncryptionKeyResolver : IKeyEncryptionKeyResolver


nit: it doesn't always fail.

Btw. I'd use Moq for such simple case rather than creating new type. I believe we have it as dependency.

addressed in upcoming push

kasobol-msft · 2020-05-20T22:54:26Z

sdk/storage/Azure.Storage.Queues/tests/MockMissingClientSideEncryptionKeyListener.cs

+
+namespace Azure.Storage.Queues.Tests
+{
+    internal class MockMissingClientSideEncryptionKeyListener : IMissingClientSideEncryptionKeyListener


I'd use Moq for that as well.

addressed in upcoming push

Clientside decryption will now either succeed or throw. Queues can redirect their throw to a listener.

sdk/storage/Azure.Storage.Blobs/src/AppendBlobClient.cs

gapra-msft

Looks good to me

kasobol-msft

It's much easier to navigate this code now. Thank you!

Please take a look at the comment I left on QueueClient crypto extension and see if you want to do it before providing another drop.

sdk/storage/Azure.Storage.Blobs/src/AppendBlobClient.cs

kasobol-msft · 2020-06-02T20:01:30Z

sdk/storage/Azure.Storage.Blobs/src/BlobClient.cs

            bool async = true,
            CancellationToken cancellationToken = default)
        {
+            // TODO uncomment when upload from file gets its own implementation


to be addressed in this PR?

No. This is to do with future work to give upload from file its own implementation separate from upload from stream. However, such an implementation will be incompatible with this feature, and so it will need to revert to upload from stream. I can clarify this.

kasobol-msft · 2020-06-02T20:07:10Z

sdk/storage/Azure.Storage.Blobs/src/BlobBaseClient.cs

+                        stream = await new BlobClientSideDecryptor(ClientSideEncryption)
+                            .ClientSideDecryptInternal(stream, response.Value.Metadata, requestedRange, response.Value.ContentRange, async, cancellationToken).ConfigureAwait(false);


Two things to consider:

It seems that BlobClientSideDecryptor is stateless in this context. Could be a member (field) in this class instead of storing encryption options.

It's not far away from here to create IReadStreamTransformer and have BlobClientSideDecryptor : IReadStreamTransformer

not blocking here, but would be nice to have.

kasobol-msft · 2020-06-02T20:08:11Z

sdk/storage/Azure.Storage.Blobs/src/BlobClient.cs

+                (content, metadata) = await new BlobClientSideEncryptor(ClientSideEncryption)
+                    .ClientSideEncryptInternal(content, metadata, async, cancellationToken).ConfigureAwait(false);


Similar feedback here as for Decryptor.

kasobol-msft · 2020-06-02T20:09:54Z

sdk/storage/Azure.Storage.Blobs/src/BlobClientSideEncryptor.cs

+            //Stream seekableCiphertext = new RollingBufferStream(
+            //        nonSeekableCiphertext,
+            //        EncryptionConstants.DefaultRollingBufferSize,
+            //        GetExpectedCryptoStreamLength(originalLength));


Please cleanup commented out code here and there. Either we should work items for these improvements or implement them right away.

This was old code that got moved around in the refactors. Is obsolete and will be removed.

kasobol-msft · 2020-06-02T20:22:49Z

sdk/storage/Azure.Storage.Common/src/Shared/ClientsideEncryption/ClientSideEncryptor.cs

+        /// <param name="async">Whether to wrap the CEK asynchronously.</param>
+        /// <param name="cancellationToken">Cancellation token.</param>
+        /// <returns>The wrapped stream to read from and the encryption metadata for the wrapped stream.</returns>
+        public static async Task<(Stream ciphertext, EncryptionData encryptionData)> EncryptInternal(


similar feedback here.

kasobol-msft · 2020-06-02T20:24:49Z

...rage/Azure.Storage.Common/src/Shared/ClientsideEncryption/Models/EncryptionDataSerializer.cs

+        /// </summary>
+        /// <param name="data">Data to serialize.</param>
+        /// <returns>The JSON UTF8 bytes.</returns>
+        public static ReadOnlyMemory<byte> SerializeEncryptionData(EncryptionData data)


does this have to be public ?

It's on an internal class.

I mean we call it from method above. Do we call it from somewhere else? if not then we could hide it more.

sdk/storage/Azure.Storage.Queues/src/QueueClientExtensions.cs

sdk/storage/Azure.Storage.Queues/tests/EncryptedMessageSerializerTests.cs

...rage/Azure.Storage.Common/src/Shared/ClientsideEncryption/Models/EncryptionDataSerializer.cs

tg-msft

Commenting on the APIs first as they're the most important. I'll run through the rest as well.

tg-msft · 2020-06-04T17:04:31Z

sdk/storage/Azure.Storage.Blobs/api/Azure.Storage.Blobs.netstandard2.0.cs

        public virtual Azure.Storage.Blobs.Specialized.BlobBaseClient WithSnapshot(string snapshot) { throw null; }
        protected virtual Azure.Storage.Blobs.Specialized.BlobBaseClient WithSnapshotCore(string snapshot) { throw null; }
    }
+    public static partial class BlobClientExtensions


This should go in SpecializedBlobExtensions so we don't add a new type.

I'll mirror this in Queues as well. There needed to be a new type no matter what, but now it will be in line with this Blobs change.

tg-msft · 2020-06-04T17:45:47Z

sdk/storage/Azure.Storage.Blobs/api/Azure.Storage.Blobs.netstandard2.0.cs

        public new Azure.Storage.Blobs.Specialized.BlockBlobClient WithSnapshot(string snapshot) { throw null; }
        protected sealed override Azure.Storage.Blobs.Specialized.BlobBaseClient WithSnapshotCore(string snapshot) { throw null; }
    }
+    public partial class ExtendedBlobClientOptions : Azure.Storage.Blobs.BlobClientOptions


Extended is probably not the best prefix because a lot of folks have a visceral reaction to it. Could we call this SpecializedBlobClientOptions to match the Specialized namespace and terminology we've been building?

weshaggard · 2020-06-04T18:12:36Z

sdk/storage/Azure.Storage.Queues/tests/Azure.Storage.Queues.Tests.csproj

    <PackageReference Include="Azure.Identity" />
+    <PackageReference Include="Azure.Security.KeyVault.Keys" />
+    <PackageReference Include="Microsoft.Azure.KeyVault.Core" />
+    <PackageReference Include="Microsoft.Azure.Storage.Queue" />


Do we really want to reference the track1 version of these libraries?

Yes. There are compatibility tests to ensure each library can read the data encoded by the other. This is an upgrade requirement for anyone who used client-side encryption in track 1.

better safe than sorry. We can think about spinning extra test package(s) where we test track1 track2 interop aspects in long run or take it as follow up from here.

Yes I do think it is worth considering multiple test projects to help ensure we don't have any weird cross-pollination between the track 1 and track 2 library.

jaschrep-msft · 2020-06-04T21:10:03Z

/azp run net - storage - ci

azure-pipelines · 2020-06-04T21:10:15Z

Azure Pipelines successfully started running 1 pipeline(s).

tg-msft

I'm adding a bunch more comments, but only things affecting the API should be worried about for Preview. The rest can be addressed in future changes if you think they're worth doing.

tg-msft · 2020-06-04T19:36:42Z

sdk/storage/Azure.Storage.Blobs/src/AppendBlobClient.cs

+                  clientSideEncryption: default,
+                  encryptionScope)
        {
        }


Nit - might be cleaner to validate here than the in the extension method so you've mirrored the logic across all your constructors instead of splitting it between some .ctors and the extension.

There aren't any encryption options being passed in here. It's definitional that they don't exist. Are you suggesting we add the encryption options to the constructor explicitly even though we know we will never use them?

I know outside this comment you mentioned a complete refactoring of how our constructors are handled internally, which I mentioned is already an upcoming work item. Are you okay with this being addressed during that work?

I'm okay with anything but the API changes happening as future work.

tg-msft · 2020-06-04T19:37:43Z

sdk/storage/Azure.Storage.Blobs/src/AppendBlobClient.cs

+
+        private static void AssertNoClientSideEncryption(BlobClientOptions options)
+        {
+            if (options._clientSideEncryptionOptions != default)


Could options ever be null when you get here?

tg-msft · 2020-06-04T19:38:12Z

sdk/storage/Azure.Storage.Blobs/src/Azure.Storage.Blobs.csproj

  <ItemGroup>
+    <Compile Include="$(AzureStorageSharedSources)ClientsideEncryption\*.cs" Link="Shared\ClientsideEncryption\%(RecursiveDir)\%(Filename)%(Extension)" />
+    <Compile Include="$(AzureStorageSharedSources)ClientsideEncryption\Models\*.cs" Link="Shared\ClientsideEncryption\Models\%(RecursiveDir)\%(Filename)%(Extension)" />
+    <Compile Remove="C:\Repos\azure-sdk-for-net\sdk\storage\Azure.Storage.Common\src\Shared\ClientsideEncryption\Models\EncryptedBlobRange.cs" />


This shouldn't be checked in.

tg-msft · 2020-06-04T19:39:37Z

sdk/storage/Azure.Storage.Blobs/src/Models/ContentRange.cs

+    {
+        private const string WildcardMarker = "*";
+
+        public struct RangeUnit


Nit - nested types are always considered kind of weird in .NET. It would probably be nicer to pull this out into another internal strict and just call it ContentRangeUnit.

Nit - maybe make this a readonly struct.

tg-msft · 2020-06-04T19:41:27Z

sdk/storage/Azure.Storage.Blobs/src/BlobBaseClient.cs

+        /// </summary>
+        internal virtual ClientSideEncryptionOptions ClientSideEncryption => _clientSideEncryption;
+
+        internal bool UsingClientSideEncryption => ClientSideEncryption != default;


Nit - this feels a little gratuitous.

tg-msft · 2020-06-04T20:29:20Z

sdk/storage/Azure.Storage.Common/api/Azure.Storage.Common.netstandard2.0.cs

+    }
+    public enum ClientSideEncryptionVersion
+    {
+        V1_0 = 0,


Nit - start this enum at = 1 and then you can throw if someone passes default. Follow the same pattern we use for ServiceVersion.

tg-msft · 2020-06-04T20:32:20Z

sdk/storage/Azure.Storage.Common/tests/Shared/TestConfigurations.cs

+        /// method that will Assert.Inconclusive if the desired tenant wasn't
+        /// defined in this configuration.
+        /// </summary>
+        private IDictionary<string, KeyVaultConfiguration> KeyVaults { get; set; }


Might also be worth a note in https://github.com/Azure/azure-sdk-for-net/blob/master/sdk/storage/CONTRIBUTING.md#configuration

tg-msft · 2020-06-04T20:32:52Z

sdk/storage/Azure.Storage.Queues/api/Azure.Storage.Queues.netstandard2.0.cs

+        void OnFailure(Azure.Storage.Queues.Models.QueueMessage message, System.Exception exception);
+        System.Threading.Tasks.Task OnFailureAsync(Azure.Storage.Queues.Models.PeekedMessage message, System.Exception exception);
+        System.Threading.Tasks.Task OnFailureAsync(Azure.Storage.Queues.Models.QueueMessage message, System.Exception exception);
+    }


This should be an abstract class instead of an interface

I worry that we're doing something different here than we did for QuickQuery. That just took an Action.

If we believe we don't need async methods here then so be it. But we have two distinct types of QueueMessage and PeekedMessage that don't have a common base. Is accepting more than one action when the user probably wants to always supply both really the best option? I'm not sure of a better way to supply this functionality.

@KrzysztofCwalina and I talked about this a bit and came up with the following to match what we did for QuickQuery a little better here:

public class QueueClientSideEncryptionOptions : ClientSideEncryptionOptions { public event EventHandler<DecryptionFailureEventArgs> DecryptionFailed; } public class ClientSideDecryptionFailureEventArgs { public Exception Exception { get; } public ClientSideDecryptionFailureEventArgs(Exception exception); }

and you could use it like:

var options = new QueueClientSideEncryptionOptions(); options.KeyResolver = ...; options.DecryptionFailed += (object sender, ClientSideDecryptionFailureEventArgs args) => { if (sender is PeekedMessage peek) { Console.WriteLine($"Failed to peek at {peek.MessageId}"); } else if (sender is QueueMessage message) { Console.WriteLine($"Failed to receive {message.MessageId}"); } };

It's only sync for now, but so is QuickQuery's error handler and I'd wait to hear feedback from folks if they need an async event.

Do you think this would work?

Is the suggestion that SpecializedQueueClientOptions now have a property for QueueClientSideEncryptionOptions instead of the normal ClientSideEncryptionOptions, or that we still accept the base class and work it through internals like we do with client options in the first place?

I'd say leave it as the base ClientSideEncryptionOptions in SpecializedQueueClientOptions so people could pass the same value for Blobs and Queues. You'd only need to learn about QueueClientSideEncryptionOptions if you were trying to handle failures which should be an advanced scenario.

On a related note - maybe we should change the semantics to throw an exception when you can't decrypt if there's no failure handler? Then new users who aren't trying to use multiple keys/etc. will still have an easy notification that something went wrong but advanced scenarios are still possible for special partners?

And you could just do an if (options is QueueClientSideEncryptionOptions advanced) { _failureHandler = advanced.DecryptionFailed; } instead of a bunch of internal state sharing since ClientSideEncryptionOptions is in Common.

tg-msft · 2020-06-04T20:34:05Z

sdk/storage/Azure.Storage.Queues/api/Azure.Storage.Queues.netstandard2.0.cs

+    {
+        void OnFailure(Azure.Storage.Queues.Models.PeekedMessage message, System.Exception exception);
+        void OnFailure(Azure.Storage.Queues.Models.QueueMessage message, System.Exception exception);
+        System.Threading.Tasks.Task OnFailureAsync(Azure.Storage.Queues.Models.PeekedMessage message, System.Exception exception);


The property name should just be DecryptionFailureListener

tg-msft · 2020-06-04T20:35:11Z

sdk/storage/Azure.Storage.Queues/api/Azure.Storage.Queues.netstandard2.0.cs

+    }
+    public static partial class SpecializedQueueExtensions
+    {
+        public static Azure.Storage.Queues.QueueClient WithClientSideEncryptionFailureListener(this Azure.Storage.Queues.QueueClient client, Azure.Storage.Queues.Specialized.IClientSideDecryptionFailureListener listener) { throw null; }


Is this a requirement? What's the sceanrio for needing to change the failure listener of an existing queue?

It seemed to go hand in hand with replacing the key. Private drops have been given with this functionality, so we'll have to check with our partners on whether they actually use this if we think it should be removed.

If we move to the EventHandler<T> approach, hopefully the single With() method will suffice.

tg-msft

Adding a few more comments

tg-msft · 2020-06-04T21:22:11Z

sdk/storage/Azure.Storage.Queues/src/Models/EncryptedMessage.cs

+{
+    internal class EncryptedMessage
+    {
+        public string EncryptedMessageContents { get; set; }


Should this be EncryptedMessageText to map to QueueMessage.MessageText?

tg-msft · 2020-06-04T21:23:06Z

sdk/storage/Azure.Storage.Queues/src/Models/EncryptedMessageSerializer.cs

+
+namespace Azure.Storage.Queues.Specialized.Models
+{
+    internal static class EncryptedMessageSerializer


Nit - Any reason not to put these in the EncryptedMessage class?

Do you mean placing the serializer as a nested class to what it is serializing?

No, just merging the two classes together.

tg-msft · 2020-06-04T21:30:32Z

sdk/storage/Azure.Storage.Queues/src/QueueClientSideDecryptor.cs

+                }
+                catch (Exception e) when (_listener != default)
+                {
+                    if (async)


This pattern doesn't feel right. What if I implemented a sync listener becuase I only Console.WriteLine but use QueueClient.ReceiveAsync to make an async method call?

Will come back to this based on discussion of what form the listener should even take.

tg-msft · 2020-06-04T21:36:13Z

sdk/storage/Azure.Storage.Queues/src/QueueClientSideDecryptor.cs

+                    }
+                }
+            }
+            return filteredMessages.ToArray();


So you have to implement a listener if you don't want to wait for the visiblity timeout on messages you can't read? This design feels tricky to use correctly.

The alternatives as far as we've explored are to

Return the serialized encrypted message schema and let the user write the code to discover they failed to decrypt.

Silently filter out messages we couldn't decrypt.

I'm not happy with either of those. The first is undue burden on the customer, whereas through the listener we do the determination for them and they handle it how they wish. The second feels dangerous in that queue messages can just time out with no one noticing.

I suppose an option 3 could exist of overriding the failed messages to throw upon accessing the message text and just always return all the messages, but I'm not sure whether that's useful.

tg-msft · 2020-06-04T21:37:16Z

sdk/storage/Azure.Storage.Queues/src/QueueServiceClient.cs

+        /// </summary>
+        internal virtual ClientSideEncryptionOptions ClientSideEncryption => _clientSideEncryption;
+
+        private readonly IClientSideDecryptionFailureListener _missingClientSideEncryptionKeyListener;


Nit - this name doesn't match the type like the other uses.

tg-msft · 2020-06-04T21:40:58Z

sdk/storage/Azure.Storage.Common/src/Shared/ClientsideEncryption/Models/EncryptionAgent.cs

+        /// <summary>
+        /// The protocol version used for encryption.
+        /// </summary>
+        public ClientSideEncryptionVersion Protocol { get; set; }


Nit - I'd suggest naming this EncryptionVersion instead of Protocol to match the name in Options.

tg-msft

Adding a few last comments. I'm approving and it's fine if you want to merge this now and continue addressing the last API feedback in a follow up PR.

tg-msft · 2020-06-04T21:43:16Z

sdk/storage/Azure.Storage.Common/src/Shared/ClientsideEncryption/Models/EncryptionData.cs

+        {
+            Assembly assembly = typeof(EncryptionData).Assembly;
+            var platformInformation = $"({RuntimeInformation.FrameworkDescription}; {RuntimeInformation.OSDescription})";
+            return $"azsdk-net-{assembly.GetName().Name}/{assembly.GetCustomAttribute<AssemblyInformationalVersionAttribute>().InformationalVersion} {platformInformation}";


Nit - might be worth pulling this into a shared source file for TelemetryPolicy and your code.

sdk/storage/Azure.Storage.Common/src/Shared/ClientsideEncryption/Models/EncryptionData.cs

tg-msft · 2020-06-04T22:12:35Z

...zure.Storage.Common/src/Shared/ClientsideEncryption/ClientSideEncryptionOptionsExtensions.cs

+{
+    internal static class ClientSideEncryptionOptionsExtensions
+    {
+        public static ClientSideEncryptionOptions Clone(this ClientSideEncryptionOptions options)


Nit - maybe add a mainteance note in ClientSideEncryptionOptions that this msut be updated when changes are introduced there?

Other minor PR feedback.

tg-msft

There's one small thing we should change about the event source, but it otherwise looks great to me.

tg-msft · 2020-06-05T21:10:29Z

sdk/storage/Azure.Storage.Queues/src/QueueClientSideEncryptionOptions.cs

+
+        internal void OnDecryptionFailed(object message, Exception e)
+        {
+            DecryptionFailed?.Invoke(this, new ClientSideDecryptionFailureEventArgs(message, e));


We should pass message as the source instead of this (i.e., it was the message that failed to decrypt). This also kind of hides the wart of dealing with QueueMessage vs PeekedMessage behind the event handler pattern that forces object sender.

tg-msft · 2020-06-05T21:11:38Z

sdk/storage/Azure.Storage.Queues/src/QueueClientSideEncryptionOptions.cs

+        /// Message the failure occured with. Can be an instance of either
+        /// <see cref="Queues.Models.QueueMessage"/> or <see cref="Queues.Models.PeekedMessage"/>.
+        /// </summary>
+        public object Message { get; }


This should be removed as well if we're passing it as the event's sender.

tg-msft · 2020-06-05T21:15:57Z

sdk/storage/Azure.Storage.Queues/src/QueueClientSideEncryptionOptions.cs

+        private QueueClientSideEncryptionOptions CloneAsQueueClientSideEncryptionOptions()
+        {
+            // clone base class but as instance of this class
+            var newOptions = new QueueClientSideEncryptionOptions(EncryptionVersion);
+            ClientSideEncryptionOptionsExtensions.CopyOptions(this, newOptions);
+
+            // clone data specific to this subclass
+            newOptions.DecryptionFailed = DecryptionFailed;
+
+            return newOptions;
+        }
+
+        /// <summary>
+        /// Clones the given <see cref="ClientSideEncryptionOptions"/> as an instance of
+        /// <see cref="QueueClientSideEncryptionOptions"/>. If the given instance is also a
+        /// <see cref="QueueClientSideEncryptionOptions"/>, this clones it's specialty data as well.
+        /// </summary>
+        /// <returns></returns>
+        internal static QueueClientSideEncryptionOptions CloneFrom(ClientSideEncryptionOptions options)
+        {
+            if (options == default)
+            {
+                return default;
+            }
+            else if (options is QueueClientSideEncryptionOptions)
+            {
+                return ((QueueClientSideEncryptionOptions)options).CloneAsQueueClientSideEncryptionOptions();
+            }
+            else
+            {
+                // clone base class but as instance of this class
+                var newOptions = new QueueClientSideEncryptionOptions(options.EncryptionVersion);
+                ClientSideEncryptionOptionsExtensions.CopyOptions(options, newOptions);
+
+                return newOptions;
+            }
+        }


Nit - could we simplify both methods to just?

internal static QueueClientSideEncryptionOptions CloneFrom(ClientSideEncryptionOptions options) { if (options == default) { return default; } var newOptions = new QueueClientSideEncryptionOptions(options.EncryptionVersion); ClientSideEncryptionOptionsExtensions.CopyOptions(options, newOptions); if (options is QueueClientSideEncryptionOptions queueOptions) { newOptions.DecryptionFailed = queueOptions.DecryptionFailed; } return newOptions; }

Huh. Somehow that gets past the compiler restrictions on when an event can be assigned. I did something nearly identical before and it said no. Will fix.

jaschrep-msft · 2020-06-05T22:45:39Z

/azp run net - storage - ci

azure-pipelines · 2020-06-05T22:45:51Z

Azure Pipelines successfully started running 1 pipeline(s).

…nto feature/storage/integrated-clientside-encryption

tg-msft · 2020-06-05T23:52:16Z

/check-enforcer reset

tg-msft · 2020-06-05T23:52:36Z

/check-enforcer evaluate

jaschrep-msft added 3 commits May 19, 2020 09:42

Removed crypto package from ci.yml

2df8a86

regenerate/export-api

7906d8e

Ported clientside encryption for queues

4d71759

jaschrep-msft assigned rickle-msft, gapra-msft and kasobol-msft May 19, 2020

kasobol-msft reviewed May 19, 2020

View reviewed changes

gapra-msft reviewed May 20, 2020

View reviewed changes

sdk/storage/Azure.Storage.Blobs/src/BlobBaseClient.cs Outdated Show resolved Hide resolved

gapra-msft reviewed May 20, 2020

View reviewed changes

sdk/storage/Azure.Storage.Blobs/src/PageBlobClient.cs Show resolved Hide resolved

gapra-msft reviewed May 20, 2020

View reviewed changes

...rage/Azure.Storage.Common/src/Shared/ClientsideEncryption/Models/EncryptionDataSerializer.cs Show resolved Hide resolved

gapra-msft reviewed May 20, 2020

View reviewed changes

sdk/storage/Azure.Storage.Common/src/Shared/Errors.Clients.cs Show resolved Hide resolved

gapra-msft reviewed May 20, 2020

View reviewed changes

sdk/storage/Azure.Storage.Queues/src/QueueClient.cs Outdated Show resolved Hide resolved

jaschrep-msft assigned tg-msft May 20, 2020

Queues handles partial decryption errors.

5219aa5

Queues now has a listener for when only some messages cannot be decrypted. these messages are filtered out of the response and sent to the listened instead. If no listener is provided, the whole fetch throws. Some PR comments addressed.

kasobol-msft reviewed May 20, 2020

View reviewed changes

jaschrep-msft added 6 commits May 20, 2020 16:21

Removed nonexistent project from sln

295f1e8

Change in handling key resolution

df0ccc9

Clientside decryption will now either succeed or throw. Queues can redirect their throw to a listener.

Many PR comments addressed; key-substitution added

cada706

Refactors; Mocking some tests

801fb3b

Refactors and bug fixes

69be140

Export-API

e0ae520

jaschrep-msft requested review from gapra-msft and kasobol-msft June 2, 2020 17:23

gapra-msft reviewed Jun 2, 2020

View reviewed changes

sdk/storage/Azure.Storage.Blobs/src/AppendBlobClient.cs Outdated Show resolved Hide resolved

Constant/error/assertion refactors

1633ce0

gapra-msft approved these changes Jun 2, 2020

View reviewed changes

kasobol-msft reviewed Jun 2, 2020

View reviewed changes

Refactors

e32eaa6

Revert deletion of an api.cs file

55ab4cc

jaschrep-msft force-pushed the feature/storage/integrated-clientside-encryption branch from b281a88 to 55ab4cc Compare June 2, 2020 23:03

jaschrep-msft added the APIChange label Jun 2, 2020

tg-msft reviewed Jun 4, 2020

View reviewed changes

weshaggard reviewed Jun 4, 2020

View reviewed changes

jaschrep-msft added 2 commits June 4, 2020 12:19

Minor Crypto API Adjustments

9b8b321

Export API

5d2220d

tg-msft reviewed Jun 4, 2020

View reviewed changes

tg-msft approved these changes Jun 4, 2020

View reviewed changes

jaschrep-msft added 2 commits June 5, 2020 13:45

Queue listener changed to events.

b506071

Other minor PR feedback.

Export api

77c38c3

tg-msft approved these changes Jun 5, 2020

View reviewed changes

jaschrep-msft added 3 commits June 5, 2020 14:41

Testing for update message encryption

b3f5bc0

Minor event API adjustments

46eeb8b

export api

7d3ce1b

tg-msft approved these changes Jun 5, 2020

View reviewed changes

Merge branch 'master' of https://github.com/Azure/azure-sdk-for-net i…

a04f168

…nto feature/storage/integrated-clientside-encryption

kasobol-msft approved these changes Jun 6, 2020

View reviewed changes

jaschrep-msft merged commit 639e185 into Azure:master Jun 8, 2020

jaschrep-msft deleted the feature/storage/integrated-clientside-encryption branch August 3, 2022 15:41


		#region Utility

		private byte[] LocalManualEncryption(byte[] data, byte[] key, byte[] iv)

		stream = await new BlobClientSideDecryptor(ClientSideEncryption)
		.ClientSideDecryptInternal(stream, response.Value.Metadata, requestedRange, response.Value.ContentRange, async, cancellationToken).ConfigureAwait(false);

		(content, metadata) = await new BlobClientSideEncryptor(ClientSideEncryption)
		.ClientSideEncryptInternal(content, metadata, async, cancellationToken).ConfigureAwait(false);

Port clientside encryption to mono-dll #12183

Port clientside encryption to mono-dll #12183

Uh oh!

Conversation

jaschrep-msft commented May 19, 2020

Uh oh!

jaschrep-msft commented May 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kasobol-msft left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kasobol-msft left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gapra-msft left a comment

Choose a reason for hiding this comment

Uh oh!

kasobol-msft left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaschrep-msft commented May 19, 2020 •

edited

Loading