Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Port clientside encryption to mono-dll #12183

Conversation

jaschrep-msft
Copy link
Member

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
Copy link
Member Author

jaschrep-msft commented May 19, 2020

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

Copy link
Contributor

@kasobol-msft kasobol-msft left a comment

Choose a reason for hiding this comment

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

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

@@ -914,6 +914,11 @@ public partial class UserDelegationKey
}
namespace Azure.Storage.Blobs.Specialized
{
public partial class AdvancedBlobClientOptions : Azure.Storage.Blobs.BlobClientOptions
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

I like Extended

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

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

#region Utility

private byte[] LocalManualEncryption(byte[] data, byte[] key, byte[] iv)
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Independent/External/Baseline?

Manual indicates human involvement in the process.

}


#region Utility
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

It is closed, write above the first test.

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

}

/// <summary>
/// AES-CBC using a 256 bit key.
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

/// <returns>Transformed content stream and metadata.</returns>
internal virtual (Stream, Metadata) TransformContent(Stream content, Metadata metadata)
private async Task<(Stream, Metadata)> ClientSideEncryptInternal(
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

// 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);
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

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.
Copy link
Contributor

@kasobol-msft kasobol-msft left a comment

Choose a reason for hiding this comment

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

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


namespace Azure.Storage.Blobs.Tests
{
internal class AlwaysFailsKeyEncryptionKeyResolver : IKeyEncryptionKeyResolver
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

addressed in upcoming push


namespace Azure.Storage.Queues.Tests
{
internal class MockMissingClientSideEncryptionKeyListener : IMissingClientSideEncryptionKeyListener
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd use Moq for that as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

addressed in upcoming push

Copy link
Member

@gapra-msft gapra-msft left a comment

Choose a reason for hiding this comment

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

Looks good to me

Copy link
Contributor

@kasobol-msft kasobol-msft left a comment

Choose a reason for hiding this comment

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

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.

@@ -1020,6 +1028,26 @@ public BlobClient(Uri blobUri, TokenCredential credential, BlobClientOptions opt
bool async = true,
CancellationToken cancellationToken = default)
{
// TODO uncomment when upload from file gets its own implementation
Copy link
Contributor

Choose a reason for hiding this comment

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

to be addressed in this PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Comment on lines 716 to 717
stream = await new BlobClientSideDecryptor(ClientSideEncryption)
.ClientSideDecryptInternal(stream, response.Value.Metadata, requestedRange, response.Value.ContentRange, async, cancellationToken).ConfigureAwait(false);
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Comment on lines 953 to 954
(content, metadata) = await new BlobClientSideEncryptor(ClientSideEncryption)
.ClientSideEncryptInternal(content, metadata, async, cancellationToken).ConfigureAwait(false);
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar feedback here as for Decryptor.

Comment on lines 64 to 67
//Stream seekableCiphertext = new RollingBufferStream(
// nonSeekableCiphertext,
// EncryptionConstants.DefaultRollingBufferSize,
// GetExpectedCryptoStreamLength(originalLength));
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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 was old code that got moved around in the refactors. Is obsolete and will be removed.

/// <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(
Copy link
Contributor

Choose a reason for hiding this comment

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

similar feedback here.

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

Choose a reason for hiding this comment

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

does this have to be public ?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's on an internal class.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

@tg-msft tg-msft left a comment

Choose a reason for hiding this comment

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

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

@@ -989,6 +989,10 @@ public partial class BlobBaseClient
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
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

@@ -1037,6 +1041,11 @@ public partial class BlockBlobClient : Azure.Storage.Blobs.Specialized.BlobBaseC
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
Copy link
Member

Choose a reason for hiding this comment

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

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?

@@ -13,6 +13,9 @@
<CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>
</None>
<PackageReference Include="Azure.Identity" />
<PackageReference Include="Azure.Security.KeyVault.Keys" />
<PackageReference Include="Microsoft.Azure.KeyVault.Core" />
<PackageReference Include="Microsoft.Azure.Storage.Queue" />
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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
Copy link
Member Author

/azp run net - storage - ci

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Member

@tg-msft tg-msft left a comment

Choose a reason for hiding this comment

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

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.

clientDiagnostics,
customerProvidedKey,
clientSideEncryption: default,
encryptionScope)
{
}
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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


private static void AssertNoClientSideEncryption(BlobClientOptions options)
{
if (options._clientSideEncryptionOptions != default)
Copy link
Member

Choose a reason for hiding this comment

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

Could options ever be null when you get here?

@@ -35,6 +38,9 @@
<Compile Include="$(AzureCoreSharedSources)TaskExtensions.cs" Link="Shared\Core\%(RecursiveDir)\%(Filename)%(Extension)" />
</ItemGroup>
<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" />
Copy link
Member

Choose a reason for hiding this comment

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

This shouldn't be checked in.

{
private const string WildcardMarker = "*";

public struct RangeUnit
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Nit - maybe make this a readonly struct.

/// </summary>
internal virtual ClientSideEncryptionOptions ClientSideEncryption => _clientSideEncryption;

internal bool UsingClientSideEncryption => ClientSideEncryption != default;
Copy link
Member

Choose a reason for hiding this comment

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

Nit - this feels a little gratuitous.

}
public enum ClientSideEncryptionVersion
{
V1_0 = 0,
Copy link
Member

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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);
}
Copy link
Member

Choose a reason for hiding this comment

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

This should be an abstract class instead of an interface

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

@tg-msft tg-msft Jun 4, 2020

Choose a reason for hiding this comment

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

@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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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.

{
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);
Copy link
Member

Choose a reason for hiding this comment

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

The property name should just be DecryptionFailureListener

}
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; }
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

@tg-msft tg-msft left a comment

Choose a reason for hiding this comment

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

Adding a few more comments

{
internal class EncryptedMessage
{
public string EncryptedMessageContents { get; set; }
Copy link
Member

Choose a reason for hiding this comment

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

Should this be EncryptedMessageText to map to QueueMessage.MessageText?


namespace Azure.Storage.Queues.Specialized.Models
{
internal static class EncryptedMessageSerializer
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

No, just merging the two classes together.

}
catch (Exception e) when (_listener != default)
{
if (async)
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

}
}
}
return filteredMessages.ToArray();
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

The alternatives as far as we've explored are to

  1. Return the serialized encrypted message schema and let the user write the code to discover they failed to decrypt.
  2. 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.

/// </summary>
internal virtual ClientSideEncryptionOptions ClientSideEncryption => _clientSideEncryption;

private readonly IClientSideDecryptionFailureListener _missingClientSideEncryptionKeyListener;
Copy link
Member

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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

Copy link
Member

@tg-msft tg-msft left a comment

Choose a reason for hiding this comment

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

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.

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

Choose a reason for hiding this comment

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

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

{
internal static class ClientSideEncryptionOptionsExtensions
{
public static ClientSideEncryptionOptions Clone(this ClientSideEncryptionOptions options)
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

@tg-msft tg-msft left a comment

Choose a reason for hiding this comment

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

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


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

Choose a reason for hiding this comment

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

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.

/// 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; }
Copy link
Member

Choose a reason for hiding this comment

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

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

Comment on lines 43 to 79
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;
}
}
Copy link
Member

Choose a reason for hiding this comment

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

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;
}

Copy link
Member Author

Choose a reason for hiding this comment

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

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
Copy link
Member Author

/azp run net - storage - ci

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

…nto feature/storage/integrated-clientside-encryption
@tg-msft
Copy link
Member

tg-msft commented Jun 5, 2020

/check-enforcer reset

@tg-msft
Copy link
Member

tg-msft commented Jun 5, 2020

/check-enforcer evaluate

@jaschrep-msft jaschrep-msft merged commit 639e185 into Azure:master Jun 8, 2020
danieljurek added a commit that referenced this pull request Jun 15, 2020
* [Storage] Add test case for accesstier (#12340)

* [Datafactory] Enable Excel SDK (#12282)

* Enable Excel SDK

* update

Co-authored-by: zhanyu2014 <zhanyu@microsoft.com>

* update Azure.ClientSdk.Analyzers to 20200529.1 (#12370)

* make it required (#12354)

* Tables: Query support for dictionary entities and CreateFilter (#12366)

* Support for dictionary entities and CreateFilter

* add comment to CreateFilter

* take the new Azure.ClientSdk.Analyzers and remove pragmas

* remove redundant .ToList() calls in tests

* make CreateFilter less prominant on the client

* Initial commit to master for Azure Digital Twins .Net SDK. (#12398)

* Update Batch SDK regeneration instructions (#12347)

* K8sConfiguration .Net SDK - Create (#12227)

* K8sConfiguration .Net SDK - Create

* include k8s in CI

* Project version and dependencies update

Co-authored-by: nanthi <nanthi@NANTHI01>
Co-authored-by: Yeming Liu <Yeming.Liu@microsoft.com>

* Support Sku capabilities. Bump to 7.3.0 (#12328)

* Support Sku capabilities. Bump to 7.3.0

* Generate from AutoRest

* Removed color and imageType from the VisualFeature enum (#12375)

* Removed color and imageType from the VisualFeature enum

* Modified test data and removed changes to generated code (so the deprecated enum still exists in there)

* Regenerated CreateSkillsetReturnsCorrectDefinitionImageAnalysisKeyPhrase.json on dogfood

* Generate new .NET SDK based on #9377 (#12364)

* Generate new .NET SDK based on #9377

* Update package version

* [Event Hubs Client] Migration Guide Updates (#12402)

The focus of these changes is to enhance the migration guide with additional
information about the client hierarchy and discuss the limitations around
processor types consuming legacy checkpoint data.

* [FormRecognizer] Remove Operation type constructors (#12419)

* Release prep (#12422)

* Prepare Storage for release

* pr feedback.

* PR feedback.

* pr feedback.

* Release preparation

* Update GitIgnore (#12368)

* Enable return type and banned assembly analyzers (#12418)

* Increment version for storage releases (#12423)

* Increment package version after release of Azure.Storage.Common

* Increment package version after release of Azure.Storage.Files.Shares

* Increment package version after release of Azure.Storage.Blobs

* Increment package version after release of Azure.Storage.Files.DataLake

* Increment package version after release of Azure.Storage.Queues

* Increment version for storage releases (#12425)

* Increment package version after release of Azure.Storage.Common

* Increment package version after release of Azure.Storage.Files.Shares

* Increment package version after release of Azure.Storage.Blobs

* Increment package version after release of Azure.Storage.Files.DataLake

* Increment package version after release of Azure.Storage.Queues

* feat(Samples): Add Telemetry Sample (#12407)

feat(Samples): Add Telemetry Sample

* add some unit tests for expression parsing (#12421)

* add some unit tests for expression parsing

* refactor for TestCaseSource

* fixup naming

* samples(adt): Update ctor snippets, and update sample options (#12409)

* fix(e2e): Fix replay tests for differnt OS(s) (#12424)

* fix(adt): fix BasicDigitalTwin and use in an example (#12428)

* Update root readme file (#12430)

* samples(adt): add a sample for using a custom twin type for serialization (#12432)

* Update swagger_to_sdk_config.json (#12412)

@microsoft.azure/autorest.csharp@2.3.84 is a preview version. Going back to 82

* Fix Incomplete Doc Upload (#12442)

* ensure that child directory items are copied

* bump template version so it can actually pass through the version checks + integration feed

* Update AutoRest C# version to https://github.com/Azure/autorest.csharp/releases/download/3.0.0-dev.20200602.1/autorest-csharp-v3-3.0.0-dev.20200602.1.tgz (#12435)

* Fix json serialization of geometries (#12440)

* samples(adt): add snippets for get and deserialize digital twin (#12441)

* fix(adt): Updated comments to generted more meaningful API documentation (#12443)

* Escaping % (percent sign) segment in Uri.AppendToPath  (#12369)

* Escaping % percent sign in segment in AppendToPath

* webdocs(adt): test seealso, and adding links for more resources (#12471)

* Make collection properties read-only (#12459)

* Make collection properties read-only

Fixes #11712. Removes the setters per .NET guidelines.

* Resolve PR feedback

* feat(adt): Adding see-also section for sync methods (#12470)

* comments(adt): removing inaccurate comment (#12474)

* Remove FieldBuilder (#12464)

* Remove FieldBuilder

Resolves #12460

* Update public APIs

* feat(test): Adding tests to ensure that the test mode values are not changed (#12481)

* Ignore Azure.Management.KeyVault.Tests (#12483)

Works around #12211 by ignoring tests that fail sporadically for both
live and playback tests.

* Default to more Newtonsoft-like response encoding in TestFramework (#12482)

* samples(adt): misc minor clean-up from Azure SDK review (#12488)

* feat(adt): Add exception tag to comments (#12475)

* Add SessionReceiverOptions/SessionProcessorOptions (#12383)

* Changelogs and version updates for June release (#12507)

* test(PublishTelemetry): Add E2E test for Publish Telemetry API (#12479)

* Increment version for core releases (#12516)

* Update versions of Azure.Core and Azure.Core.Experimental (#12520)

* Update AutoRest C# version (#12525)

Co-authored-by: Pavel Krymets <pavel@krymets.com>

* Release Prep (#12526)

* Prepare Storage for release

* pr feedback.

* PR feedback.

* pr feedback.

* release prep

* samples(adt): Update component property to be an integer (#12515)

* samples(adt): Update component property to be an integer

* samples(adt): Update dt property to be an integer

Co-authored-by: Abhipsa Misra <abhipsa.misra@microsoft.com>

* Increment version for storage releases (#12529)

* Increment package version after release of Azure.Storage.Queues

* Increment package version after release of Azure.Storage.Common

* Increment package version after release of Azure.Storage.Files.Shares

* Increment package version after release of Azure.Storage.Blobs

* Increment package version after release of Azure.Storage.Files.DataLake

* samples(adt): more misc changes per Azure SDK review (#12532)

* feat(samples): Add an introduction section to the samples page (#12524)

* Added xml dataset & fixed version issue (#12458)

Co-authored-by: Shawn Xiao (IM) <shawnxia@microsoft.com>

* Batch Management SDK 11.0.0 (#12393)

* 2020-05-01 generated sdk and recordings

* update changelog

* fix description typo

* undo change to changelog

* update AzSdk.RP.props

* update assemblyinfo

* GuestConfiguration SDK update to support ARC senario (#12373)

* GuestConfiguration SDK update to support ARC senario

* add unit tests

* add unit tests2

* [no change] trigger a new build

* [no change] trigger a new build2

* migrate tests to subscription: MGMT GCRP Package Test 001, re-record the tests

Co-authored-by: ge huang <gehuan@microsoft.com>

* Rename AnalyzeRequest to AnalyzeTextOptions (#12489)

* Rename AnalyzeRequest to AnalyzeTextOptions

Fixes #12486
Fixes #12530

* Rename some properties, parameters for consistency

Fixes #12536

* Removed the SynonymMap.Format property

Only "solr" is supported, and this makes .NET consistent with the other languages.

* Resolve PR feedback

* Implement the management client (#12484)

ATOM based management operations

* Making developer credential types public (#12501)

* Making developer credential types public

* update api listing

* docs(adt): add link to samples (#12533)

* Increment version for extensions releases (#12547)

* feat(samples): Use DefaultAzureCredential in samples using app id login (#12535)

* [FormRecognizer] Preview 3: moving to new Receipt design (#12523)

* Identity Adding Authenticate APIs to UsernamePasswordCredential (#12502)

* Adding Authenticate APIs to UsernamePasswordCredential

* updating test recordings

* updating api listing

* update changelog

* [TA] Drop preview and change readme links to absolute (#12541)

* drop preview and links absolute

* updates

* add azure-cognitive-services to our samples

* change package version

* Consistency operation failed status management in LRO  (#12453)

* failures in LROs

* changelog

* PR Feedback

* don't overwrite history....

* improve race condition comment

* [FR] Absolute links and bump version in Readme (#12540)

* absolute links and version

* add azure-cognitive-services to samples readme

* Add changelog format template - bump nuget version for next release - Update nuget tag  (#12552)

* Update release info

* refactor(adt): move QueryChargeHelper to root namespace (#12569)

* Add sdk directory name check (#12556)

* [Synapse] - Build track 2 data plane SDK (#12275)

* [Synapse] - Build track 2 SDK

* Update ci.yml

* Remove unused gitignore

* Resolve compilation error

* Fix build error

* Update README

* Update code snippets and README

* Update README

* Add file header for code snippet

* Remove template api

* Rename Azure.Analytics.Synapse.Development to Azure.Analytics.Synapse.Artifacts

* Re-generate to the latest Swagger spec

* Regenerate with the latest code generator

* Resolve comments

* Make VersionString internal and remove setter

* Sync up with master, consume official Swagger repo and regenerate code

Co-authored-by: Dongwei Wang <dongwwa@microsoft.com>

* update change log for KeyVault track 2 mgmt sdk (#12272)

* update change log for KeyVault track 2 mgmt sdk

* update change log

* refine change log

* refine code sample

* update change logs for track 2 mgmt sdks for preview (#12391)

* update change logs for track 2 mgmt sdks

* update code sample

* [App Service] Azure .NET SDK update for Powershell (#12380)

* [Datafactory] Add source/sink linked service/schema linked service support for Data Flow (#12528)

* SDK changes for source/sink linked service/schema LS

* update

* renaming operation-ids for Cluster put and patch as per guidelines (#12554)

* renaming operation-ids for Cluster put and patch as per guidelines

* updated the version

* updated the major version to 3.0.0

* Revert "updated the major version to 3.0.0"

This reverts commit f689e1a.

* [Event Hubs Client] Fix Test Environment Typo (#12596)

The focus of these changes is to fix a typo on the constructor of the
StorageTestEnvironment, which initializes the base class with an incorrect
service directory name.

* Port clientside encryption to mono-dll (#12183)

* Port clientside encryption to mono-dll

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.

* Removed crypto package from ci.yml

* regenerate/export-api

* Ported clientside encryption for queues

* Queues handles partial decryption errors.

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.

* Removed nonexistent project from sln

* Change in handling key resolution

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

* Many PR comments addressed; key-substitution added

* Refactors; Mocking some tests

* Refactors and bug fixes

* Export-API

* Constant/error/assertion refactors

* Refactors

* Revert deletion of an api.cs file

* Minor Crypto API Adjustments

* Export API

* Queue listener changed to events.

Other minor PR feedback.

* Export api

* Testing for update message encryption

* Minor event API adjustments

* export api

* fix(tests,samples)do not send modelid in component metadata (#12573)

* docs(adt): update RequestOptions documentation (#12571)

* [FormRecognizer] Remove <inheritdoc> from documentation (#12570)

* Fix Tables access policies test and configure related methods (#12567)

* fixed accesspolicies test but still errors with recordedmode

* test to default, api export, and sessions

Co-authored-by: Allison Kim <t-allkim@microsoft.com>

* fix(samples):remove unnecessary lines from sample (#12606)

* Update for Preview 3 (#12608)

* [FormRecognizer] Preview 3: updating changelog

* Update DigitalTwin lifecycle to use ETag and re-record (#12603)

* Update changelog (#12612)

* Update changelog

* Updated changelogs for preview release (#12616)

* Increment package version after release of Azure.Messaging.ServiceBus (#12619)

* readme(adt): update readme (#12618)

* Tables: Fixup diagnostic scopes (#12621)

* Fixup diagnostic scopes

* fix lambda scopes

* fix random failure by changing from referencing helper project to contain source code directly (#12498)

* Supporting specifying test run frequency (#12454)

* Add RunFrequencyAttribute to support live test run frequency

* remove workaround file sdk/core/service.projects

* fix(comments): Fix xml comments for API surface (#12635)

* Search cross-language consistency API changes (#12615)

* Search cross-language consistency API changes

Fixes #11162

* Add strongly typed FacetResult helpers for Search (#12620)

* Add strongly typed FacetResult helpers for Search

Fixes #10613.  We kept this open for a long time hoping we'd be able to do
something nifty using type info, but there's just not a great answer so we're
stealing the Track 1 experience.

* [Synapse] - Fix test failure (#12625)

* Fix test failure

* Correct test name

* Remove Ignore annotation and use random guid

* change namespace to Azure.ResourceManager for track 2 mgmt sdk (#12598)

* change namespace to Azure.ResourceManager. for track 2 mgmt sdk

* update readmes

* update Azure.Core.All.sln

* add resource group clean up policy for management test

* Support node reboot features (#12641)

Co-authored-by: Zhenyu Zhou <zhezhou@microsoft.com>

* Throw an AggregateException for Search IndexDocuments batch errors (#12630)

* Throw an AggregateException for Search IndexDocuments batch errors

Fixes #10594

* Add SearchModelFactory (#12626)

* Add SearchModelFactory

Fixes #10610

* [FormRecognizer] Remove long-running operation transform (#12652)

* Adding SearchClientBuilderExtensions (#12628)

* Adding SearchClientBuilderExtensions
* Updating Changelog

* Increment package version after release of Azure.DigitalTwins.Core (#12617)

* [Event Hubs Client] Migration Guide Tweaks (#12656)

The focus of these changes is to incorporate some feedback from other languages
around phrasing and content for the messaging around legacy checkpoint support.

* fix setup script (#12661)

* Fix SearchDocument (#12610)

* Fix SearchDocument
* PR Feedback
* PR Feedback - Change from Array to IReadOnlyList

* Update README.mds for extensions (#12658)

* Increment package version after release of Azure.AI.TextAnalytics (#12666)

* updated mappings for new Microsoft.Web event types in the azure-sdk-for-net repo (#12171)

Co-authored-by: Elle Tojaroon <patojaro@microsoft.com>

* Increment package version after release of Azure.Search.Documents (#12671)

* Sync eng/common directory with azure-sdk-tools repository (#12665)

* Event Grid: Regenerate new .NET SDK and record tests for new GA service API version 2020-06-01 (#12576)

* regenerate .NET SDK for Event Grid 2020-06-01 API version

* re-record the tests

* add changelog.md and update note release

* add more history to changelog.md

* fix text

* update version in azSdk.RP.props and assymblyinfo.cs

Co-authored-by: Ashraf Hamad <ahamad@ntdev.microsoft.com>

* [Azure Cognitive Search] Creating new SDK version to match new management-plane API Version 2020-03-13  (#12607)

* Generated for api 2020-03

* Removing unused dependency.

* Updated session records of data plane tests.

* merge upstream

* Updated SDK version information.

* Generated off of API Specs Master.

* merge upstream

* Remove unused method.

* updated session record.

* Skipping broken test.

Co-authored-by: Jacob Hill <thhill@microsoft.com>

* [Microsoft.StorageSync] Update SDK to use 2020-03-01 API version (#12560)

* autorest code generation for 2020-03-01 API version

* update changelog, SDK version, update tests

* Updating API tag in AzSdk.props

* Azure Digital Twins 0.1.0 SDK (#12538)

* DigitalTwins SDK and Tests

* New swagger and tests updated

* enable CI

* Update version

Co-authored-by: Yeming Liu <felix_liu@outlook.com>

Co-authored-by: Ryan Kelly <rykelly@microsoft.com>
Co-authored-by: Yeming Liu <yeliu@microsoft.com>
Co-authored-by: Yeming Liu <felix_liu@outlook.com>

* [FormRecognizer] Changing test location to West Central US + Live tests fix (#12677)

* [FormRecognizer] Preview 3: update changelogs (#12687)

* Update CRUD samples (#12693)

* Identity updating changelog for 1.2.0-preview.4 release (#12696)

* Remove unhealthy pump (#12691)

* Up version to 4.2.1 for stress testing pipeline

* Remove pump if not at running or not opening states.

* enable apicompat (#12669)

* Increment package version after release of Azure.Identity (#12697)

* Increment package version after release of Azure.AI.FormRecognizer (#12698)

* [Event Hubs Client] Documentation Updates (#12690)

The focus of these changes is to refine some documentation to correct a missed
update to `Azure.Identity` types and improve context around storage expectations
when creating an `EventProcessorClient`.

* Increase Key Vault test timeout to 120mins (#12700)

* Increase Key Vault test timeout to 90mins

* Further increase timeout

* Update sample readmes to have absolute links to GitHub (#12568)

* Network SDK release for API version 2020-05-01 (#12685)

* Re-generate Network SDK

* fix sdk breaking change with firewall policy rule collection group rename (#12381)

* Update generated code

* SDK

* Update SDK code

* Re-generate Network SDK

* Update ChangeLog

* Update SDK version as 20.0.0-preview already exists

Co-authored-by: gimotwanMSFT <36463144+gimotwanMSFT@users.noreply.github.com>

* DiskRp changes for 2020-05-01 (#12553)

* DiskRP sdk changes for api version 2020-05-01

* changing sdk versions

* taking in latest changes from swagger pr

* fix tests and generate sdk from latest swagger

* taking in latest swagger changes and running the failed test

* regenerate sdk from merged swagger

* fixing autorest, updating azsdk.props, updating validation in des tests

* CRR update (#12650)

* CRR update

* Update

* ADT C# SDK Samples Improvements (#12683)

* fix(samples): improve on the snippet comments

* Create a relationship samples

* Initial AccessControlClient for Azure.Security.KeyVault.Administration (#12480)

* poc of generated client

* cleanup

* implement the rest

* export api

* tests

* recorded tests

* regen client

* fix version

* xml comments

* pr comments

* pr comments

* diagnostic scopes

* model factory

* adjust diagnostic scopes

* change assignment name to Guid

* make RoleAssignmentListResult internal

* rename file

* pr comments

* remove commented shared import

* add xml docs for ArgumentNullExceptions

* refactor(samples): Refactor the delete digital twin sample snippet (#12716)

* Update AutoRest C# version (#12692)

Co-authored-by: Shivangi Reja <shreja@microsoft.com>

* Generate API (#12720)

* Add initial samples for review (#12664)

* init samples

* learning about clieeents

* CreateTable should work

* Sample template finished with create + delete table working

* first sample files

* added delete nonexistent table

* revised based on feedback + first readme

* revised sample1 readme and file

Co-authored-by: Allison Kim <t-allkim@microsoft.com>

* Refresh generated code for KVAdmin library (#12725)

* Simplify delete relationship snippet (#12717)

* simplify delete relationship snippet

* Api compat changes (#12703)

* Update how ApiCompat checks are ran

- Now include the ApiCompatVersion in the project
- Update the ApiCompatVersion everytime we ship a GA
- Remove EnableApiCompat as to disable now remove the ApiCompatVersion

* Remove EnableApiCompat from everywhere.

* Add ApiCompatVersion to all GA packages

* PR feedback

Fix bad appconfig test
Add ApiCompatVersion for textanalytics

* Remove apicompat project from solutions

* Remove more EnableApiCompat properties

* Add reboot parameters for multiple replicas per master. (#12712)

* Add reboot parameters for multiple replicas per master.

* Update the preview version in csproj.

* Reenable registered server tests, update recordings (#12724)

* Update AutoRest C# version (#12739)

Co-authored-by: Pavel Krymets <pavel@krymets.com>

* Fix links in extensions README.mds (#12689)

* doc(adt): update prereq readme instructions (#12743)

* Sanitize json body (#12707)

* sanitize body

* PR feedback

* update to master

* no var

* Use US sovereign region

* Use sovereign KV domain suffix

* Update KV tests.yml to use new approach

* Unique naming of jobs

* Unique display names

* Set SubscriptionConfiguraiton back to default

* Name Job directly

* use valid characters for job name

* Parameterize KV SKU and adjust Azure China Cloud deployment

* add AZURE_AUTHORITY_HOST

Co-authored-by: Wei Wei <weiwei@microsoft.com>
Co-authored-by: yuzhangyi <44692931+yuzhangyi@users.noreply.github.com>
Co-authored-by: zhanyu2014 <zhanyu@microsoft.com>
Co-authored-by: Christopher Scott <chriss@microsoft.com>
Co-authored-by: Mariana Rios Flores <mariari@microsoft.com>
Co-authored-by: Azad Abbasi <azabbasi@microsoft.com>
Co-authored-by: Jacob Freck <jafreck@microsoft.com>
Co-authored-by: NarayanThiru <nanthi@microsoft.com>
Co-authored-by: nanthi <nanthi@NANTHI01>
Co-authored-by: Yeming Liu <Yeming.Liu@microsoft.com>
Co-authored-by: Yang Yuan <yangyuan@users.noreply.github.com>
Co-authored-by: Jennifer Marsman <jennifermarsman@users.noreply.github.com>
Co-authored-by: Junbo Wang <juniwang@microsoft.com>
Co-authored-by: Jesse Squire <jesse.squire@gmail.com>
Co-authored-by: kinelski <camaiaor@microsoft.com>
Co-authored-by: Kamil Sobol <61715331+kasobol-msft@users.noreply.github.com>
Co-authored-by: Chidozie Ononiwu <31145988+chidozieononiwu@users.noreply.github.com>
Co-authored-by: Pavel Krymets <pavel@krymets.com>
Co-authored-by: Azure SDK Bot <53356347+azure-sdk@users.noreply.github.com>
Co-authored-by: Basel Rustum <barustum@microsoft.com>
Co-authored-by: David R. Williamson <drwill@microsoft.com>
Co-authored-by: vinagesh <vinagesh@outlook.com>
Co-authored-by: Scott Beddall <45376673+scbedd@users.noreply.github.com>
Co-authored-by: Amanda Nguyen <48961492+amnguye@users.noreply.github.com>
Co-authored-by: Heath Stewart <heaths@microsoft.com>
Co-authored-by: JoshLove-msft <54595583+JoshLove-msft@users.noreply.github.com>
Co-authored-by: Abhipsa Misra <22563986+abhipsaMisra@users.noreply.github.com>
Co-authored-by: Abhipsa Misra <abhipsa.misra@microsoft.com>
Co-authored-by: shawnxzq <zhiqiang.xiao@gmail.com>
Co-authored-by: Shawn Xiao (IM) <shawnxia@microsoft.com>
Co-authored-by: Ge Huang <gehuang.dut@gmail.com>
Co-authored-by: ge huang <gehuan@microsoft.com>
Co-authored-by: ShivangiReja <45216704+ShivangiReja@users.noreply.github.com>
Co-authored-by: Scott Schaab <sschaab@microsoft.com>
Co-authored-by: Dongwei Wang <bean0idear@163.com>
Co-authored-by: Dongwei Wang <dongwwa@microsoft.com>
Co-authored-by: erich-wang <eriwan@microsoft.com>
Co-authored-by: Shubham Dhond <sdhond.sd@gmail.com>
Co-authored-by: Richard Zhang <rizh@microsoft.com>
Co-authored-by: Ajit Navasare <v-ajnav@microsoft.com>
Co-authored-by: James <41338290+jaschrep-msft@users.noreply.github.com>
Co-authored-by: Allison Kim <am.kim327@gmail.com>
Co-authored-by: Allison Kim <t-allkim@microsoft.com>
Co-authored-by: tg-msft <tg-msft@users.noreply.github.com>
Co-authored-by: aim-for-better <zhenyu.zhou@microsoft.com>
Co-authored-by: Zhenyu Zhou <zhezhou@microsoft.com>
Co-authored-by: Paviya (Elle) Tojaroon <elle.tojaroon@gmail.com>
Co-authored-by: Elle Tojaroon <patojaro@microsoft.com>
Co-authored-by: Ashraf Hamad <ahamad-MS@users.noreply.github.com>
Co-authored-by: Ashraf Hamad <ahamad@ntdev.microsoft.com>
Co-authored-by: Jacob Hill <tjhill_94@hotmail.com>
Co-authored-by: Jacob Hill <thhill@microsoft.com>
Co-authored-by: Andre Pinto <31374863+anpint@users.noreply.github.com>
Co-authored-by: Ryan K <ryan.k@outlook.com>
Co-authored-by: Ryan Kelly <rykelly@microsoft.com>
Co-authored-by: Yeming Liu <yeliu@microsoft.com>
Co-authored-by: Yeming Liu <felix_liu@outlook.com>
Co-authored-by: Serkant Karaca <serkantkaraca@users.noreply.github.com>
Co-authored-by: Anton Evseev <v-anevse@microsoft.com>
Co-authored-by: gimotwanMSFT <36463144+gimotwanMSFT@users.noreply.github.com>
Co-authored-by: chetmaddula <57730350+chetmaddula@users.noreply.github.com>
Co-authored-by: Pratima Upadhyay <pratima.upadhyay02@gmail.com>
Co-authored-by: Shivangi Reja <shreja@microsoft.com>
Co-authored-by: Wes Haggard <weshaggard@users.noreply.github.com>
Co-authored-by: Lakshmi Priya Sekar <Priya91@users.noreply.github.com>
Co-authored-by: Daniel Jurek <djurek@microsoft.com>
@jaschrep-msft jaschrep-msft deleted the feature/storage/integrated-clientside-encryption branch August 3, 2022 15:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants