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

Add AzureSasCredential #17636

Merged
merged 13 commits into from
Jan 5, 2021
7 changes: 7 additions & 0 deletions sdk/core/Azure.Core/api/Azure.Core.net461.cs
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,13 @@ public AzureKeyCredential(string key) { }
public string Key { get { throw null; } }
public void Update(string key) { }
}
public partial class AzureSasCredential
{
public AzureSasCredential(string signature) { }
[System.ComponentModel.EditorBrowsableAttribute(System.ComponentModel.EditorBrowsableState.Never)]
public string Signature { get { throw null; } }
public void Update(string signature) { }
}
[System.Runtime.InteropServices.StructLayoutAttribute(System.Runtime.InteropServices.LayoutKind.Sequential)]
public readonly partial struct ETag : System.IEquatable<Azure.ETag>
{
Expand Down
7 changes: 7 additions & 0 deletions sdk/core/Azure.Core/api/Azure.Core.net5.0.cs
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,13 @@ public AzureKeyCredential(string key) { }
public string Key { get { throw null; } }
public void Update(string key) { }
}
public partial class AzureSasCredential
{
public AzureSasCredential(string signature) { }
[System.ComponentModel.EditorBrowsableAttribute(System.ComponentModel.EditorBrowsableState.Never)]
public string Signature { get { throw null; } }
public void Update(string signature) { }
}
[System.Runtime.InteropServices.StructLayoutAttribute(System.Runtime.InteropServices.LayoutKind.Sequential)]
public readonly partial struct ETag : System.IEquatable<Azure.ETag>
{
Expand Down
7 changes: 7 additions & 0 deletions sdk/core/Azure.Core/api/Azure.Core.netstandard2.0.cs
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,13 @@ public AzureKeyCredential(string key) { }
public string Key { get { throw null; } }
public void Update(string key) { }
}
public partial class AzureSasCredential
{
public AzureSasCredential(string signature) { }
[System.ComponentModel.EditorBrowsableAttribute(System.ComponentModel.EditorBrowsableState.Never)]
public string Signature { get { throw null; } }
public void Update(string signature) { }
}
[System.Runtime.InteropServices.StructLayoutAttribute(System.Runtime.InteropServices.LayoutKind.Sequential)]
public readonly partial struct ETag : System.IEquatable<Azure.ETag>
{
Expand Down
1 change: 1 addition & 0 deletions sdk/core/Azure.Core/src/Azure.Core.csproj
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
<Compile Remove="Shared\**\*.cs" />
<Compile Include="Shared\Argument.cs" />
<Compile Include="Shared\AzureKeyCredentialPolicy.cs" />
<Compile Include="Shared\AzureSasCredentialPolicy.cs" />
<Compile Include="Shared\EventSourceEventFormatting.cs" />
<Compile Include="Shared\HashCodeBuilder.cs" />
<Compile Include="Shared\ClientDiagnostics.cs" />
Expand Down
60 changes: 60 additions & 0 deletions sdk/core/Azure.Core/src/AzureSasCredential.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.

using System.ComponentModel;
using System.Threading;
using Azure.Core;

namespace Azure
{
/// <summary>
/// Shared access signature credential used to authenticate to an Azure Service.
/// It provides the ability to update the shared access signature without creating a new client.
/// </summary>
public class AzureSasCredential
{
private string _signature;

/// <summary>
/// Shared access signature used to authenticate to an Azure service.
/// </summary>
[EditorBrowsable(EditorBrowsableState.Never)]
public string Signature
{
get => Volatile.Read(ref _signature);
private set => Volatile.Write(ref _signature, value);
}

/// <summary>
/// Initializes a new instance of the <see cref="AzureSasCredential"/> class.
/// </summary>
/// <param name="signature">Shared access signature to use to authenticate with the Azure service.</param>
/// <exception cref="System.ArgumentNullException">
/// Thrown when the <paramref name="signature"/> is null.
/// </exception>
/// <exception cref="System.ArgumentException">
/// Thrown when the <paramref name="signature"/> is empty.
/// </exception>
#pragma warning disable CS8618 // Non-nullable field is uninitialized. Consider declaring as nullable.
public AzureSasCredential(string signature) => Update(signature);
#pragma warning restore CS8618 // Non-nullable field is uninitialized. Consider declaring as nullable.

/// <summary>
/// Updates the shared access signature.
/// This is intended to be used when you've regenerated your shared access signature
/// and want to update long lived clients.
/// </summary>
/// <param name="signature">Shared access signature to authenticate the service against.</param>
/// <exception cref="System.ArgumentNullException">
/// Thrown when the <paramref name="signature"/> is null.
/// </exception>
/// <exception cref="System.ArgumentException">
/// Thrown when the <paramref name="signature"/> is empty.
/// </exception>
public void Update(string signature)
{
Argument.AssertNotNullOrEmpty(signature, nameof(signature));
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I borrowed that from here

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

Signature = signature;
}
}
}
44 changes: 44 additions & 0 deletions sdk/core/Azure.Core/src/Shared/AzureSasCredentialPolicy.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.

using System;
using System.Text;
using Azure.Core.Pipeline;

namespace Azure.Core
{
internal class AzureSasCredentialPolicy : HttpPipelineSynchronousPolicy
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 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.

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

{
private readonly AzureSasCredential _credential;

/// <summary>
/// Initializes a new instance of the <see cref="AzureSasCredentialPolicy"/> class.
/// </summary>
/// <param name="credential">The <see cref="AzureSasCredentialPolicy"/> used to authenticate requests.</param>
public AzureSasCredentialPolicy(AzureSasCredential credential)
{
Argument.AssertNotNull(credential, nameof(credential));
_credential = credential;
}

/// <inheritdoc/>
public override void OnSendingRequest(HttpMessage message)
{
string query = message.Request.Uri.Query;
string signature = _credential.Signature;
if (signature.StartsWith("?", StringComparison.InvariantCulture))
{
signature = signature.AsSpan().Slice(1).ToString();
Copy link
Contributor

Choose a reason for hiding this comment

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

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

}
if (!query.Contains(signature))
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Can the query be null?

Copy link
Contributor Author

@kasobol-msft kasobol-msft Dec 27, 2020

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

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

@pakrym pakrym Jan 4, 2021

Choose a reason for hiding this comment

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

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.

}

base.OnSendingRequest(message);
}
}
}
77 changes: 77 additions & 0 deletions sdk/core/Azure.Core/tests/AzureSasCredentialPolicyTests.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.

using System.Threading;
using System.Threading.Tasks;
using Azure.Core.Pipeline;
using Azure.Core.TestFramework;
using NUnit.Framework;

namespace Azure.Core.Tests
{
public class AzureSasCredentialPolicyTests : PolicyTestBase
{
[TestCase("sig=test_signature_value")]
[TestCase("?sig=test_signature_value")]
public async Task SetsSignatureEmptyQuery(string signatureValue)
{
var transport = new MockTransport(new MockResponse(200));
var sasPolicy = new AzureSasCredentialPolicy(new AzureSasCredential(signatureValue));

await SendGetRequest(transport, sasPolicy);

Assert.AreEqual("?sig=test_signature_value", transport.SingleRequest.Uri.Query);
}

[TestCase("sig=test_signature_value")]
[TestCase("?sig=test_signature_value")]
public async Task SetsSignatureNonEmptyQuery(string signatureValue)
{
var transport = new MockTransport(new MockResponse(200));
var sasPolicy = new AzureSasCredentialPolicy(new AzureSasCredential(signatureValue));
string query = "?foo=bar";

await SendGetRequest(transport, sasPolicy, query: query);

Assert.AreEqual($"?foo=bar&sig=test_signature_value", transport.SingleRequest.Uri.Query);
}

[TestCase("sig=test_signature_value")]
[TestCase("?sig=test_signature_value")]
public async Task VerifyRetryEmptyQuery(string signatureValue)
{
var transport = new MockTransport(new MockResponse(200), new MockResponse(200));
var sasPolicy = new AzureSasCredentialPolicy(new AzureSasCredential(signatureValue));

using (Request request = transport.CreateRequest())
{
request.Method = RequestMethod.Get;
var pipeline = new HttpPipeline(transport, new[] { sasPolicy });
await pipeline.SendRequestAsync(request, CancellationToken.None);
await pipeline.SendRequestAsync(request, CancellationToken.None);
}

Assert.AreEqual("?sig=test_signature_value", transport.Requests[0].Uri.Query);
}

[TestCase("sig=test_signature_value")]
[TestCase("?sig=test_signature_value")]
public async Task VerifyRetryNonEmptyQuery(string signatureValue)
{
var transport = new MockTransport(new MockResponse(200), new MockResponse(200));
var sasPolicy = new AzureSasCredentialPolicy(new AzureSasCredential(signatureValue));
string query = "?foo=bar";

using (Request request = transport.CreateRequest())
{
request.Method = RequestMethod.Get;
request.Uri.Query = query;
var pipeline = new HttpPipeline(transport, new[] { sasPolicy });
await pipeline.SendRequestAsync(request, CancellationToken.None);
await pipeline.SendRequestAsync(request, CancellationToken.None);
}

Assert.AreEqual("?foo=bar&sig=test_signature_value", transport.Requests[0].Uri.Query);
}
}
}
3 changes: 2 additions & 1 deletion sdk/core/Azure.Core/tests/PolicyTestBase.cs
Original file line number Diff line number Diff line change
Expand Up @@ -11,14 +11,15 @@ namespace Azure.Core.Tests
{
public abstract class PolicyTestBase
{
protected static async Task<Response> SendGetRequest(HttpPipelineTransport transport, HttpPipelinePolicy policy, ResponseClassifier responseClassifier = null)
protected static async Task<Response> SendGetRequest(HttpPipelineTransport transport, HttpPipelinePolicy policy, ResponseClassifier responseClassifier = null, string query = null)
{
Assert.IsInstanceOf<HttpPipelineSynchronousPolicy>(policy, "Use SyncAsyncPolicyTestBase base type for non-sync policies");

using (Request request = transport.CreateRequest())
{
request.Method = RequestMethod.Get;
request.Uri.Reset(new Uri("http://example.com"));
request.Uri.Query = query;
var pipeline = new HttpPipeline(transport, new[] { policy }, responseClassifier);
return await pipeline.SendRequestAsync(request, CancellationToken.None);
}
Expand Down