[Storage] SAS Credential in Storage. #17646

kasobol-msft · 2020-12-18T18:00:19Z

In this PR:

Usage of AzureSasCredential in Storage

Remarks

This PR builds on the top of Add AzureSasCredential #17636 and must wait until referenced PR goes first. Please post comments related to SAS credentials in that PR.

…t much APIs that would work with sas there.

tg-msft · 2020-12-19T02:23:01Z

sdk/storage/Azure.Storage.Blobs.ChangeFeed/src/BlobChangeFeedClient.cs

@@ -111,6 +111,28 @@ public BlobChangeFeedClient(Uri serviceUri, StorageSharedKeyCredential credentia
            _blobServiceClient = new BlobServiceClient(serviceUri, credential, options);
        }

+        /// <summary>
+        /// Initializes a new instance of the <see cref="BlobChangeFeedClient"/>
+        /// class.


We should add a blurb to all these doc comments telling people to only use this overload if they need to roll SAS signatures.

added remarks.

tg-msft · 2020-12-19T02:31:27Z

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

+            => new ArgumentException($"The resource URI {uri} must not contain SAS if AzureSasCredential is used." +
+                $" You can use BlobUriBuilder to strip SAS from the resource URI before instantiating clients.");


What about something like?

Suggested change

=> new ArgumentException($"The resource URI {uri} must not contain SAS if AzureSasCredential is used." +

$" You can use BlobUriBuilder to strip SAS from the resource URI before instantiating clients.");

=> new ArgumentException(

$"You cannot use {nameof(AzureSasCredential)} when the resource URI also contains a Shared Access Signature: {uri}\n" +

$"You can remove the shared access signature by creating a {nameof(BlobUriBuilder)}, setting {nameof(BlobUriBuilder.Sas)} to null, and calling {nameof(BlobUriBuilder.ToUri)}.");

Anyone getting this exception is probably going to be confused. I'd like to put the URI after the core of the message so it's not lost behind a very long string.

Sounds Good.

tg-msft · 2020-12-19T02:37:23Z

sdk/storage/Azure.Storage.Common/src/Shared/StorageClientOptions.cs

+        /// <returns>An authentication policy.</returns>
+        public static HttpPipelinePolicy AsPolicy(this AzureSasCredential credential, Uri serviceUri)
+        {
+            var queryParameters = serviceUri.GetQueryParameters();


Do you need to check if serviceUri is null? I think this will get called before validation in base(...) for some .ctors. Consider also taking in the name of the URI param so you can throw the most helpful exception.

I could. However, after writing test for that and wrapping this piece of code in null check I found that NullReferenceException is thrown here.
Maybe we should add Argument.AssertNotNull in both AsPolicy and ctros ?

Added validations.

tg-msft · 2020-12-19T02:41:10Z

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

@@ -61,7 +61,24 @@ public static void AssertExpectedException<T>(Action action, T expectedException

        public static void AssertExpectedException<T>(Action action, Func<T, bool> predicate = null)
            where T : Exception
-            => AssertExpectedException(action, default, GetDefaultExceptionAssertion<T>((_, a) => predicate(a)));
+        {


Should you flip it around and make the other version call this then with a predicate: _ => true?

Which other version do you mean? The one that takes expectedException?

The problem here was that existing implementation of this overload ended up in this line attempting to assert exception message, which was deemed to always fail... This overload of AssertExpectedException had zero callers because of that problem.

...torage.Scenario.Tests/tests/Microsoft.Azure.WebJobs.Extensions.Storage.Scenario.Tests.csproj

seanmcc-msft

What about Blob Batch?

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

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

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

kasobol-msft · 2021-01-05T19:56:16Z

What about Blob Batch?

The Batch should work without change as long as underlying service client is created with AzureSASCredential. Added test for that.

pakrym · 2021-01-06T00:33:36Z

sdk/storage/Azure.Storage.Files.DataLake/api/Azure.Storage.Files.DataLake.netstandard2.0.cs

@@ -228,6 +231,7 @@ public partial class DataLakePathClient
        public DataLakePathClient(string connectionString, string fileSystemName, string path) { }
        public DataLakePathClient(string connectionString, string fileSystemName, string path, Azure.Storage.Files.DataLake.DataLakeClientOptions options) { }
        public DataLakePathClient(System.Uri pathUri) { }
+        public DataLakePathClient(System.Uri pathUri, Azure.AzureSasCredential credential, Azure.Storage.Files.DataLake.DataLakeClientOptions options = null) { }


We have a different ctor parameter pattern for the TokenCredential (3 overloads instead of defaulted options). Is this intentional?

Less code. I was trying to understand why do we have such pattern in place, but nobody would recall why. From user POV there shouldn't be a difference (unless I'm missing something subtle here).

You can observe similar inconsistencies in APIs implementations. Some would come with multiple overloads, some will use optional parameters. I think those with multiple overloads approach are older.

I think the goal of all of these is to make mainline APIs look simpler. We've noticed that customers get scared of methods with many parameters even if they are all optional so opted for having overloads in important scenarios.

Gotcha. I'll make it consistent then.

pakrym

LGTM

KrzysztofCwalina

In general looks good. Just a couple of nits and a question about the impact of using generic methods (where we don't have to) on AOT scenarios.

KrzysztofCwalina · 2021-01-06T18:35:08Z

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

@@ -47,6 +47,12 @@ public static InvalidOperationException StreamMustBeAtPosition0()
        public static InvalidOperationException TokenCredentialsRequireHttps()
            => new InvalidOperationException("Use of token credentials requires HTTPS");

+        public static ArgumentException SasCredentialRequiresUriWithoutSas<TUriBuilder>(Uri uri)


@jkotas, how does it affect AOT scenarios? Would it be better to pass the type as a method parameter (object or Type)?

Generic arguments are better than passing Type around. It is not just AOT, it is also a for IL trimming. Passing System.Type around is a potential problem for IL trimming and may sometime need annotations to work well with it.

In this specific case, it should not really matter since the typeof usage is trivial and the method is small. https://github.com/dotnet/runtime/blob/master/src/libraries/System.Private.CoreLib/src/System/ThrowHelper.cs in the CoreLib uses both patterns.

Great. so this stays I guess.

KrzysztofCwalina · 2021-01-06T18:37:28Z

sdk/storage/Azure.Storage.Common/src/Shared/StorageClientOptions.cs

+                throw Errors.SasCredentialRequiresUriWithoutSas<TUriBuilder>(resourceUri);
+            }
+            return new AzureSasCredentialSynchronousPolicy(
+                credential ?? throw Errors.ArgumentNull(nameof(credential)));


I think it's better to check preconditions at the beginning of the method body.

KrzysztofCwalina · 2021-01-06T18:42:56Z

sdk/storage/Azure.Storage.Files.DataLake/src/DataLakeDirectoryClient.cs

+        /// A <see cref="Uri"/> referencing the directory that includes the
+        /// name of the account, the name of the file system, and the path of the
+        /// directory.
+        /// Must not contain shared access signature.


I would add ", which should be passed in the second (following) parameter" ... or something like that.

…-storage

* Add AzureSasCredential * corner case. * api. * core as project ref (todo undo this) * first client. * hack azure core in webjobs for now. * api. * constructors. * blob tests. * datalake tests. * share tests + take out sas on share and service clients as there isn't much APIs that would work with sas there. * queues tests. * well that works. nvm. * remarks. * error message. * predicate shouldn't be optional. * post-merge tweaks. * message about right UriBuilder. * added uri validation. * changelog. * user delegation sas change. * batch. * this test doesn't work well in playback mode. * pr feedback. * comments. * validation. * undo project references workaround. * undo webjobs project ref workaround.

kasobol-msft added 5 commits December 17, 2020 13:56

Add AzureSasCredential

a4c5ebc

corner case.

5fdb953

Merge remote-tracking branch 'upstream/master' into sas-credential

bb719f2

api.

61c3839

core as project ref (todo undo this)

2504f78

ghost added Azure.Core Storage Storage Service (Queues, Blobs, Files) labels Dec 18, 2020

kasobol-msft added 9 commits December 18, 2020 10:48

first client.

fa6536d

hack azure core in webjobs for now.

2bccb52

api.

bd20b54

constructors.

fd86385

blob tests.

4b38c1e

datalake tests.

8becf87

share tests + take out sas on share and service clients as there isn'…

e117feb

…t much APIs that would work with sas there.

queues tests.

f35672e

well that works. nvm.

454586c

kasobol-msft requested review from tg-msft, seanmcc-msft and amnguye December 18, 2020 23:17

tg-msft reviewed Dec 19, 2020

View reviewed changes

kasobol-msft added 8 commits December 28, 2020 11:55

remarks.

1740da6

error message.

d68dd4e

predicate shouldn't be optional.

0f83c9b

merge master

0aa98f0

post-merge tweaks.

01c538b

message about right UriBuilder.

730a3b5

added uri validation.

31d9c96

changelog.

3389e39

kasobol-msft marked this pull request as ready for review January 5, 2021 17:30

kasobol-msft requested a review from amishra-dev as a code owner January 5, 2021 17:30

kasobol-msft requested review from fabiocav, KrzysztofCwalina and mathewc as code owners January 5, 2021 17:30

kasobol-msft requested a review from pakrym January 5, 2021 17:31

kasobol-msft added APIChange and removed Azure.Core labels Jan 5, 2021

seanmcc-msft reviewed Jan 5, 2021

View reviewed changes

kasobol-msft added 2 commits January 5, 2021 11:25

user delegation sas change.

39d059b

batch.

43dcaf6

kasobol-msft added 2 commits January 5, 2021 12:00

merge master.

f8ab972

this test doesn't work well in playback mode.

e33ca4c

seanmcc-msft approved these changes Jan 5, 2021

View reviewed changes

pakrym reviewed Jan 6, 2021

View reviewed changes

pakrym approved these changes Jan 6, 2021

View reviewed changes

pr feedback.

f108a76

KrzysztofCwalina reviewed Jan 6, 2021

View reviewed changes

kasobol-msft added 2 commits January 6, 2021 11:49

comments.

c35d5e2

validation.

5975ee9

KrzysztofCwalina approved these changes Jan 6, 2021

View reviewed changes

kasobol-msft added 3 commits January 11, 2021 14:03

Merge remote-tracking branch 'upstream/master' into sas-credential-in…

f68c029

…-storage

undo project references workaround.

4deba35

undo webjobs project ref workaround.

4b59f79

kasobol-msft merged commit e2fad94 into Azure:master Jan 11, 2021

amnguye mentioned this pull request Jan 25, 2021

[FEATURE REQ] Support updating SasToken in storage clients #17596

Closed

johnthcall mentioned this pull request Feb 2, 2021

[FEATURE REQ] Support SAS credentials in Azure.Data.Tables #18360

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Storage] SAS Credential in Storage. #17646

[Storage] SAS Credential in Storage. #17646

kasobol-msft commented Dec 18, 2020 •

edited

Loading

tg-msft Dec 19, 2020

kasobol-msft Jan 5, 2021

tg-msft Dec 19, 2020

kasobol-msft Dec 28, 2020 •

edited

Loading

tg-msft Dec 19, 2020

kasobol-msft Dec 28, 2020

kasobol-msft Jan 5, 2021

tg-msft Dec 19, 2020

kasobol-msft Dec 28, 2020

seanmcc-msft left a comment

kasobol-msft commented Jan 5, 2021

pakrym Jan 6, 2021

kasobol-msft Jan 6, 2021

pakrym Jan 6, 2021

kasobol-msft Jan 6, 2021

pakrym left a comment

KrzysztofCwalina left a comment

KrzysztofCwalina Jan 6, 2021

jkotas Jan 6, 2021

jkotas Jan 6, 2021

kasobol-msft Jan 6, 2021

KrzysztofCwalina Jan 6, 2021

kasobol-msft Jan 6, 2021

KrzysztofCwalina Jan 6, 2021

kasobol-msft Jan 6, 2021

		=> new ArgumentException($"The resource URI {uri} must not contain SAS if AzureSasCredential is used." +
		$" You can use BlobUriBuilder to strip SAS from the resource URI before instantiating clients.");

[Storage] SAS Credential in Storage. #17646

[Storage] SAS Credential in Storage. #17646

Conversation

kasobol-msft commented Dec 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kasobol-msft Dec 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanmcc-msft left a comment

Choose a reason for hiding this comment

kasobol-msft commented Jan 5, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pakrym left a comment

Choose a reason for hiding this comment

KrzysztofCwalina left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kasobol-msft commented Dec 18, 2020 •

edited

Loading

kasobol-msft Dec 28, 2020 •

edited

Loading