Add AzureSasCredential #17636

kasobol-msft · 2020-12-17T22:04:07Z

In this PR:

Add AzureSasCredential per Board Review: Add SharedKey/SAS Credential types to Azure.Core (all languages) azure-sdk#1954
AzureSasCredential is the name that has been settled on the end of discussion.
Add AzureSasCredentialPolicy (internal shared source) that appends SAS to query

Remarks:

Some service (like storage in the Portal) present SAS with leading "?". This has to be stripped before appending
The validation if serviceUri already contain sas (mentioned here) will be responsibility of service clients:
- the format varies between services (i.e. Event Grid SAS and Storage SAS are vastly different)
- it would be good to fail fast (at client creation) rather than late (at request send).

pakrym

LGTM, please get an architect to sign off on this PR

tg-msft · 2020-12-19T02:07:19Z

sdk/core/Azure.Core/src/AzureSasCredential.cs

+        private string _signature;
+
+        /// <summary>
+        /// Signature used to authenticate to an Azure service.


Nit - let's use "Shared access signature" in all the doc comments. The property/param names are fine though.

tg-msft · 2020-12-19T02:11:47Z

sdk/core/Azure.Core/src/Shared/AzureSasCredentialPolicy.cs

+            string signature = _credential.Signature;
+            if (signature.StartsWith("?", StringComparison.InvariantCulture))
+            {
+                signature = signature.Substring(1);


It would be better to use StringBuilder and Spans to minimize allocations in the pipeline if possible.

Ouch. In Java substring would share internal array with parent. Good to know.

I changed the code. However, StringBuilder.Append that takes ReadOnlySpan<string> isn't available in both netstandard20 and net461 that we target :/ So it isn't as good as it could be in net5... unless there's better way.

Looks like we won't get any better with .NET version we're targeting (see comments below). I brought back original code.

Have you considered using https://docs.microsoft.com/en-us/dotnet/api/system.text.stringbuilder.append?view=net-5.0#System_Text_StringBuilder_Append_System_String_System_Int32_System_Int32_

tg-msft · 2020-12-19T02:12:05Z

sdk/core/Azure.Core/src/Shared/AzureSasCredentialPolicy.cs

+            {
+                signature = signature.Substring(1);
+            }
+            if (!query.Contains(signature))


Why is this check needed? Won't we validate no SAS URI on the client .ctor?

Can the query be null?

It's not related to presence of the SAS in client's URI.
I used AzureKeyCredentialPolicy as a blueprint for this one and found this test. That test checks idempotency of the policy if request is retried. I created similar tests for SAS policy and discovered that we'd be appending SAS on each retry. So this is to prevent that.

In storage auth policy is per-retry in all languages. We had issues where requests were so long that retry could end up with expired token.

Query is empty string in worst case.

Java SDK handles this problem in an interesting way. Its retry policy makes a copy of its HttpMessage equivalent before calling rest of pipeline chain, so that retries won't step on each other. Maybe doing something like that in .NET would be right solution to this problem.

tg-msft · 2020-12-19T02:18:28Z

sdk/core/Azure.Core/src/Shared/AzureSasCredentialPolicy.cs

+        /// <inheritdoc/>
+        public override void OnSendingRequest(HttpMessage message)
+        {
+            base.OnSendingRequest(message);


Nit - consider doing this last? It feels like changing the URI should happen before whatever goes on in base, but it's a super nit thing because base is empty and probably always will be.

tg-msft · 2020-12-19T02:51:47Z

sdk/core/Azure.Core/src/Shared/AzureSasCredentialPolicy.cs

+
+namespace Azure.Core
+{
+    internal class AzureSasCredentialPolicy : HttpPipelineSynchronousPolicy


We should consider deriving from the base policy class so we can eventually:

While there's not a scenario that requires it today, we can think about supporting automatic refresh in a future release. The pipeline policy would listen for 403s on the way back, check if the SAS token expired, raise an event (probably sync or async depending on the path), allow the event to compute/fetch a new SAS token, and mark the message as retriable so customers never notice the expiration. This could be added without breaking.

Pedantically, by REST conventions, retries should only be performed on a 401. A 403 should be considered terminal and indicating "we know who you are and you'll never be allowed to do this."

Do we have services not returning 401 for expired credentials?

Since AzureSasCredentialPolicy is internal class - can this be deferred until it's actually implemented? We'd be doing same thing synchronous policy does in SAS policy.

I think it's fine. I would just rename this class to AzureSasCredentialSynchronousPolicy. It's shared source and it might be easier to transition to a base policy by adding a new subclass and removing this one overtime.

pakrym · 2021-01-04T16:57:25Z

sdk/core/Azure.Core/src/Shared/AzureSasCredentialPolicy.cs

+            string signature = _credential.Signature;
+            if (signature.StartsWith("?", StringComparison.InvariantCulture))
+            {
+                signature = signature.AsSpan().Slice(1).ToString();


nit: Substring would result in the same amount of allocations.

pakrym · 2021-01-04T17:01:45Z

sdk/core/Azure.Core/src/Shared/AzureSasCredentialPolicy.cs

+                var newQuery = new StringBuilder(query, query.Length + signature.Length + 1);
+                newQuery.Append(string.IsNullOrEmpty(query) ? '?' : '&');
+                newQuery.Append(signature);
+                message.Request.Uri.Query = newQuery.ToString();


The StringBuilder version is actually more allocating than the code you had before.

var s = string.IsNullOrEmpty(query) ? '?' + signature : query + '&' + signature;

is compiled into

string s= (string.IsNullOrEmpty(text) ? string.Concat("?", text2) : string.Concat(text, "&", text2));

The string.Concat call is a single allocation. While StringBuilder + internal array + ToString call is 3 allocations.

This reverts commit 6375606.

KrzysztofCwalina · 2021-01-04T22:13:13Z

sdk/core/Azure.Core/src/AzureSasCredential.cs

+        /// </exception>
+        public void Update(string signature)
+        {
+            Argument.AssertNotNullOrEmpty(signature, nameof(signature));


Should we be using IsNullOrWhitespace for these? I think we use OrEmpty in other key credentails, but it seems like OrWhitespace would be better. @schaabs?

I borrowed that from here

azure-sdk-for-net/sdk/core/Azure.Core/src/AzureKeyCredential.cs

Line 56 in 2b59433

Argument.AssertNotNullOrEmpty(key, nameof(key));

KrzysztofCwalina · 2021-01-04T22:17:59Z

sdk/core/Azure.Core/src/Shared/AzureSasCredentialPolicy.cs

+
+namespace Azure.Core
+{
+    internal class AzureSasCredentialPolicy : HttpPipelineSynchronousPolicy


I think it's fine. I would just rename this class to AzureSasCredentialSynchronousPolicy. It's shared source and it might be easier to transition to a base policy by adding a new subclass and removing this one overtime.

KrzysztofCwalina · 2021-01-04T22:19:56Z

sdk/core/Azure.Core/src/Shared/AzureSasCredentialPolicy.cs

+            string signature = _credential.Signature;
+            if (signature.StartsWith("?", StringComparison.InvariantCulture))
+            {
+                signature = signature.Substring(1);


Have you considered using https://docs.microsoft.com/en-us/dotnet/api/system.text.stringbuilder.append?view=net-5.0#System_Text_StringBuilder_Append_System_String_System_Int32_System_Int32_

This reverts commit 89cc867.

kasobol-msft · 2021-01-04T23:26:31Z

It seems that allocation optimizations must wait till we target higher .NET.

check-enforcer · 2021-01-04T23:41:30Z

This pull request is protected by Check Enforcer.

What is Check Enforcer?

Check Enforcer helps ensure all pull requests are covered by at least one check-run (typically an Azure Pipeline). When all check-runs associated with this pull request pass then Check Enforcer itself will pass.

Why am I getting this message?

You are getting this message because Check Enforcer did not detect any check-runs being associated with this pull request within five minutes. This may indicate that your pull request is not covered by any pipelines and so Check Enforcer is correctly blocking the pull request being merged.

What should I do now?

If the check-enforcer check-run is not passing and all other check-runs associated with this PR are passing (excluding license-cla) then you could try telling Check Enforcer to evaluate your pull request again. You can do this by adding a comment to this pull request as follows:
/check-enforcer evaluate
Typically evaulation only takes a few seconds. If you know that your pull request is not covered by a pipeline and this is expected you can override Check Enforcer using the following command:
/check-enforcer override
Note that using the override command triggers alerts so that follow-up investigations can occur (PRs still need to be approved as normal).

What if I am onboarding a new service?

Often, new services do not have validation pipelines associated with them, in order to bootstrap pipelines for a new service, you can issue the following command as a pull request comment:
/azp run prepare-pipelines
This will run a pipeline that analyzes the source tree and creates the pipelines necessary to build and validate your pull request. Once the pipeline has been created you can trigger the pipeline using the following comment:
/azp run net - [service] - ci

kasobol-msft · 2021-01-04T23:49:13Z

/azp run net - core - ci

azure-pipelines · 2021-01-04T23:49:22Z

Azure Pipelines failed to run 1 pipeline(s).

kasobol-msft · 2021-01-04T23:51:08Z

/azp run net - core - ci

azure-pipelines · 2021-01-04T23:51:19Z

Azure Pipelines successfully started running 1 pipeline(s).

**In this PR:** - Add `AzureSasCredential` per Azure/azure-sdk#1954 - `AzureSasCredential` is the name that has been settled on the end of discussion. - Add `AzureSasCredentialPolicy` that appends SAS to query **Remarks:** - Some service (like storage in the Portal) present SAS with leading "?". This has to be stripped before appending - The validation if serviceUri already contain sas (mentioned [here](Azure/azure-sdk#1954 (comment))) will be responsibility of service clients: - the format varies between services (i.e. Event Grid SAS and Storage SAS are vastly different) - it would be good to fail fast (at client creation) rather than late (at request send). **References** - [.NET PR](Azure/azure-sdk-for-net#17636)

* Add AzureSasCredential * corner case. * api. * doc update. * less allocations. * call base last. * Revert "less allocations." This reverts commit 6375606. * validation feedback. * policy rename. * another attempt to reduce allocations. * Revert "another attempt to reduce allocations." This reverts commit 89cc867. * changelog.

**In this PR:** - Add `AzureSasCredential` per Azure/azure-sdk#1954 - `AzureSasCredential` is the name that has been settled on the end of discussion. - Add `AzureSasCredentialPolicy` that appends SAS to query **Remarks:** - Some service (like storage in the Portal) present SAS with leading "?". This has to be stripped before appending - The validation if serviceUri already contain sas (mentioned [here](Azure/azure-sdk#1954 (comment))) will be responsibility of service clients: - the format varies between services (i.e. Event Grid SAS and Storage SAS are vastly different) - it would be good to fail fast (at client creation) rather than late (at request send). **References** - [.NET PR](Azure/azure-sdk-for-net#17636)

Add AzureSasCredential

a4c5ebc

ghost added the Azure.Core label Dec 17, 2020

corner case.

5fdb953

kasobol-msft marked this pull request as ready for review December 17, 2020 22:12

kasobol-msft requested review from KrzysztofCwalina and pakrym as code owners December 17, 2020 22:12

kasobol-msft requested a review from tg-msft December 17, 2020 22:12

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

bb719f2

pakrym added the APIChange label Dec 17, 2020

pakrym reviewed Dec 17, 2020

View reviewed changes

api.

61c3839

kasobol-msft mentioned this pull request Dec 18, 2020

[Storage] SAS Credential in Storage. #17646

Merged

tg-msft reviewed Dec 19, 2020

View reviewed changes

kasobol-msft added 3 commits December 26, 2020 21:37

doc update.

2d9fb26

less allocations.

6375606

call base last.

2cbe659

This was referenced Dec 28, 2020

Add SAS credential and policy. Azure/azure-sdk-for-java#18380

Merged

Add AzureSasCredential Azure/azure-sdk-for-python#15946

Merged

pakrym reviewed Jan 4, 2021

View reviewed changes

Revert "less allocations."

212e464

This reverts commit 6375606.

KrzysztofCwalina reviewed Jan 4, 2021

View reviewed changes

KrzysztofCwalina approved these changes Jan 4, 2021

View reviewed changes

kasobol-msft added 4 commits January 4, 2021 14:47

validation feedback.

b246125

policy rename.

a1bffb4

another attempt to reduce allocations.

89cc867

Revert "another attempt to reduce allocations."

3d76464

This reverts commit 89cc867.

changelog.

8944925

kasobol-msft merged commit b3e61dd into Azure:master Jan 5, 2021

JoshLove-msft mentioned this pull request Jan 5, 2021

Use AzureSasCredential once available #17761

Closed

amnguye mentioned this pull request Jan 25, 2021

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

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add AzureSasCredential #17636

Add AzureSasCredential #17636

kasobol-msft commented Dec 17, 2020 •

edited

Loading

pakrym left a comment

tg-msft Dec 19, 2020

tg-msft Dec 19, 2020

kasobol-msft Dec 27, 2020

kasobol-msft Dec 27, 2020

kasobol-msft Jan 4, 2021

KrzysztofCwalina Jan 4, 2021

tg-msft Dec 19, 2020

tg-msft Dec 19, 2020

kasobol-msft Dec 27, 2020 •

edited

Loading

kasobol-msft Dec 31, 2020

tg-msft Dec 19, 2020

tg-msft Dec 19, 2020

jsquire Dec 19, 2020

kasobol-msft Dec 27, 2020

KrzysztofCwalina Jan 4, 2021

pakrym Jan 4, 2021

pakrym Jan 4, 2021 •

edited

Loading

KrzysztofCwalina Jan 4, 2021

kasobol-msft Jan 4, 2021

KrzysztofCwalina Jan 4, 2021

KrzysztofCwalina Jan 4, 2021

kasobol-msft commented Jan 4, 2021

check-enforcer bot commented Jan 4, 2021

kasobol-msft commented Jan 4, 2021

azure-pipelines bot commented Jan 4, 2021

kasobol-msft commented Jan 4, 2021

azure-pipelines bot commented Jan 4, 2021

Add AzureSasCredential #17636

Add AzureSasCredential #17636

Conversation

kasobol-msft commented Dec 17, 2020 • edited Loading

pakrym 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 Dec 27, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pakrym Jan 4, 2021 • 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

kasobol-msft commented Jan 4, 2021

check-enforcer bot commented Jan 4, 2021

What is Check Enforcer?

Why am I getting this message?

What should I do now?

What if I am onboarding a new service?

kasobol-msft commented Jan 4, 2021

azure-pipelines bot commented Jan 4, 2021

kasobol-msft commented Jan 4, 2021

azure-pipelines bot commented Jan 4, 2021

kasobol-msft commented Dec 17, 2020 •

edited

Loading

kasobol-msft Dec 27, 2020 •

edited

Loading

pakrym Jan 4, 2021 •

edited

Loading