Respect Sas token in conn string #8940

JoshLove-msft · 2019-11-28T05:27:23Z

Update StorageConnectionString to include Sas token in the various Endpoint properties.
Remove ctors from StorageConnectionString that are only used in tests, and instead add corresponding factory methods to TestExtensions
Remove Table related code in StorageConnectionString and tests as this SDK doesn't support table endpoints.

JoshLove-msft · 2019-11-28T05:42:28Z

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

            if ((services & AccountSasServices.Files) == AccountSasServices.Files)
            {
                sb.Append(Constants.Sas.AccountServices.File);
            }
+            if ((services & AccountSasServices.Queues) == AccountSasServices.Queues)


Moved this down for consistency with the ordering the Portal uses when constructing the Sas signature. Otherwise we get a sig mismatch error when using a copy+pasted conn string with sas token from the portal.

Mind adding a comment in the source explaining why the order's important?

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

jongio

Thank you Josh!

jongio · 2019-12-02T00:50:55Z

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

+            var sasBuilder = new AccountSasBuilder
+            {
+                ExpiresOn = Recording.UtcNow.AddHours(1),
+                Services = AccountSasServices.All,


Do we have tests that cover only Blob, Files, and Queues. Or since this is just for blobs, should test for Blobs and All?

Blobs and All should be sufficient for this file. Adding additional tests in the Files and Queue test projects might be a good idea.

jongio · 2019-12-02T00:51:33Z

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

+            {
+                ExpiresOn = Recording.UtcNow.AddHours(1),
+                Services = AccountSasServices.All,
+                ResourceTypes = AccountSasResourceTypes.All,


Can we also test individually for Container, Object, Service?

jongio · 2019-12-02T00:52:10Z

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

+                ResourceTypes = AccountSasResourceTypes.All,
+                Protocol = SasProtocol.Https,
+            };
+            sasBuilder.SetPermissions(AccountSasPermissions.All);


If not done already would be good to have coverage for Read only and Write only.

Added a Read-only

And a Write, Delete

.../Azure.Storage.Blobs/tests/SessionRecords/BlobBaseClientTests/Ctor_ConnectionString_Sas.json

...e.Storage.Blobs/tests/SessionRecords/BlobBaseClientTests/Ctor_ConnectionString_SasAsync.json

jongio · 2019-12-02T00:55:45Z

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

+        /// <param name="storageCredentials">A StorageCredentials object.</param>
+        /// <param name="useHttps"><c>true</c> to use HTTPS to connect to storage service endpoints; otherwise, <c>false</c>.</param>
+        /// <remarks>Using HTTPS to connect to the storage services is recommended.</remarks>
+        internal static StorageConnectionString CreateStorageConnectionString(object storageCredentials, bool useHttps) =>


Should this param be of type object or restricted to an interface?

As an FYI, this and the bodies of the other new factory methods were extracted as-is from StorageConnectionString constructors, which were only used by tests.

The parameter is object, because it can be either a SharedAccessSignatureCredentials or a StorageSharedKeyCredential. We probably didn't have them inherit from a common type so as to avoid adding types that customers wouldn't actually need to use.

Azure.Core didn't want to add a base Credential type so we're stuck with object.

jongio · 2019-12-02T00:56:25Z

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

+                {
+                    accountName = sharedKeyCredentials.AccountName;
+                }
+                else


Should we use else if here?

jongio · 2019-12-02T01:02:06Z

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

+            conn._accountName = accountName;
+            var sasToken = (storageCredentials is SharedAccessSignatureCredentials sasCredentials) ? sasCredentials.SasToken : default;
+
+            var protocol = useHttps ? "https" : "http";


Technically when discussing URIs, this part is called the scheme, not protocol.

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

seanmcc-msft

Looks good.

tg-msft

Please get escrow approval before merging this change.

tg-msft · 2019-12-02T21:24:12Z

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

            if ((services & AccountSasServices.Files) == AccountSasServices.Files)
            {
                sb.Append(Constants.Sas.AccountServices.File);
            }
+            if ((services & AccountSasServices.Queues) == AccountSasServices.Queues)


Mind adding a comment in the source explaining why the order's important?

tg-msft · 2019-12-02T21:46:42Z

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

+                    {
+                        var builder = new UriBuilder(endpoint)
+                        {
+                            Query = sasToken


Do we know for sure the endpoint will never have any query params? Especially since connection strings aren't well defined anywhere?

Would you recommend using HttpUtility.ParseQueryString to append sasToken?

No, I don't think we need to parse it - I just don't want to stomp any values. I'd create the builder first without setting Query. If sasToken isn't null/empty, then we can check whether builder.Query has a value before appending (and possibly squeezing in a &).

tg-msft · 2019-12-02T21:49:34Z

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

-                endpointSuffix);
-
-            return (new Uri(primaryUri), new Uri(secondaryUri));
+            return (primaryUriBuilder.Uri, secondaryUriBuilder.Uri);


Nit - is it worth sharing a single builder and just changing the host between Uri creation?

tg-msft · 2019-12-02T21:51:21Z

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

+        /// <param name="storageCredentials">A StorageCredentials object.</param>
+        /// <param name="useHttps"><c>true</c> to use HTTPS to connect to storage service endpoints; otherwise, <c>false</c>.</param>
+        /// <remarks>Using HTTPS to connect to the storage services is recommended.</remarks>
+        internal static StorageConnectionString CreateStorageConnectionString(object storageCredentials, bool useHttps) =>


Azure.Core didn't want to add a base Credential type so we're stuck with object.

tg-msft · 2019-12-02T22:23:44Z

Also - do we need to verify HTTPS if a SAS is used via connection string now?

JoshLove-msft · 2019-12-03T00:20:57Z

Also - do we need to verify HTTPS if a SAS is used via connection string now?

We should add this for SAS in general, not just for connection string, but let's do it in a separate PR.

JoshLove-msft · 2019-12-03T05:12:09Z

Also - do we need to verify HTTPS if a SAS is used via connection string now?

We should add this for SAS in general, not just for connection string, but let's do it in a separate PR.

Ended up adding this in here.

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

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

JoshLove-msft · 2019-12-03T19:25:04Z

sdk/storage/Azure.Storage.Files.Shares/src/ShareServiceClient.cs

-        /// The transport pipeline used to send every request.
-        /// </param>
-        /// <param name="clientDiagnostics"></param>
-        internal ShareServiceClient(Uri serviceUri, HttpPipeline pipeline, ClientDiagnostics clientDiagnostics)


This ctor is unused.

JoshLove-msft · 2019-12-03T19:25:33Z

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

-        /// The transport pipeline used to send every request.
-        /// </param>
-        /// <param name="clientDiagnostics"></param>
-        internal QueueServiceClient(Uri serviceUri, HttpPipeline pipeline, ClientDiagnostics clientDiagnostics)


JoshLove-msft · 2019-12-03T19:37:07Z

Also - do we need to verify HTTPS if a SAS is used via connection string now?

We should add this for SAS in general, not just for connection string, but let's do it in a separate PR.

Ended up adding this in here.

Backing this out. This needs more discussion.

JoshLove-msft force-pushed the sas-conn-string branch from 4eaecca to e468306 Compare November 28, 2019 05:40

JoshLove-msft commented Nov 28, 2019

View reviewed changes

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

JoshLove-msft force-pushed the sas-conn-string branch 3 times, most recently from 44f2978 to 4c9dc28 Compare November 28, 2019 06:07

JoshLove-msft marked this pull request as ready for review November 28, 2019 06:12

JoshLove-msft requested review from amnguye, seanmcc-msft and tg-msft as code owners November 28, 2019 06:12

JoshLove-msft force-pushed the sas-conn-string branch from 4c9dc28 to 81ce9c0 Compare November 28, 2019 19:21

Respect Sas token in conn string

b650958

JoshLove-msft force-pushed the sas-conn-string branch from 81ce9c0 to b650958 Compare December 1, 2019 01:18

jongio requested changes Dec 2, 2019

View reviewed changes

seanmcc-msft approved these changes Dec 2, 2019

View reviewed changes

tg-msft approved these changes Dec 2, 2019

View reviewed changes

add additional tests; append sas token to Query instead of overwriting

27a1653

JoshLove-msft force-pushed the sas-conn-string branch from b2fd410 to 496b609 Compare December 3, 2019 05:18

JoshLove-msft changed the title ~~Respect Sas token in conn string~~ Respect Sas token in conn string and prevent sending Sas over HTTP Dec 3, 2019

prevent sas tokens from being sent over http

076f31b

JoshLove-msft force-pushed the sas-conn-string branch from 496b609 to 076f31b Compare December 3, 2019 07:47

tg-msft approved these changes Dec 3, 2019

View reviewed changes

sdk/storage/Azure.Storage.Common/src/Shared/StorageConnectionString.cs Outdated Show resolved Hide resolved

sdk/storage/Azure.Storage.Common/tests/Shared/TestExtensions.cs Outdated Show resolved Hide resolved

JoshLove-msft commented Dec 3, 2019

View reviewed changes

undo http change; clean up tests

55a0fe8

JoshLove-msft force-pushed the sas-conn-string branch from f7c4507 to 55a0fe8 Compare December 3, 2019 19:30

JoshLove-msft changed the title ~~Respect Sas token in conn string and prevent sending Sas over HTTP~~ Respect Sas token in conn string Dec 3, 2019

JoshLove-msft merged commit a9f3c47 into Azure:master Dec 3, 2019

JoshLove-msft deleted the sas-conn-string branch December 3, 2019 19:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Respect Sas token in conn string #8940

Respect Sas token in conn string #8940

JoshLove-msft commented Nov 28, 2019 •

edited

Loading

JoshLove-msft Nov 28, 2019 •

edited

Loading

tg-msft Dec 2, 2019

JoshLove-msft Dec 3, 2019

jongio left a comment

jongio Dec 2, 2019

seanmcc-msft Dec 2, 2019

jongio Dec 2, 2019

JoshLove-msft Dec 3, 2019

jongio Dec 2, 2019

JoshLove-msft Dec 2, 2019

JoshLove-msft Dec 2, 2019

jongio Dec 2, 2019

JoshLove-msft Dec 2, 2019

tg-msft Dec 2, 2019

jongio Dec 2, 2019

JoshLove-msft Dec 2, 2019

jongio Dec 2, 2019

JoshLove-msft Dec 2, 2019

seanmcc-msft left a comment

tg-msft left a comment

tg-msft Dec 2, 2019

tg-msft Dec 2, 2019

JoshLove-msft Dec 2, 2019

tg-msft Dec 2, 2019

JoshLove-msft Dec 3, 2019

tg-msft Dec 2, 2019

tg-msft Dec 2, 2019

tg-msft commented Dec 2, 2019

JoshLove-msft commented Dec 3, 2019

JoshLove-msft commented Dec 3, 2019

JoshLove-msft Dec 3, 2019

JoshLove-msft Dec 3, 2019

JoshLove-msft commented Dec 3, 2019

Respect Sas token in conn string #8940

Respect Sas token in conn string #8940

Conversation

JoshLove-msft commented Nov 28, 2019 • edited Loading

JoshLove-msft Nov 28, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jongio 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

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

tg-msft left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tg-msft commented Dec 2, 2019

JoshLove-msft commented Dec 3, 2019

JoshLove-msft commented Dec 3, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoshLove-msft commented Dec 3, 2019

JoshLove-msft commented Nov 28, 2019 •

edited

Loading

JoshLove-msft Nov 28, 2019 •

edited

Loading