-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Add AzureSasCredential #17636
Changes from 4 commits
a4c5ebc
5fdb953
bb719f2
61c3839
2d9fb26
6375606
2cbe659
212e464
b246125
a1bffb4
89cc867
3d76464
8944925
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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 signature without creating a new client. | ||||
/// </summary> | ||||
public class AzureSasCredential | ||||
{ | ||||
private string _signature; | ||||
|
||||
/// <summary> | ||||
/// 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">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 signature. | ||||
/// This is intended to be used when you've regenerated your signature | ||||
/// and want to update long lived clients. | ||||
/// </summary> | ||||
/// <param name="signature">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)); | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I borrowed that from here
|
||||
Signature = signature; | ||||
} | ||||
} | ||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,40 @@ | ||
// Copyright (c) Microsoft Corporation. All rights reserved. | ||
// Licensed under the MIT License. | ||
|
||
using System; | ||
using Azure.Core.Pipeline; | ||
|
||
namespace Azure.Core | ||
{ | ||
internal class AzureSasCredentialPolicy : HttpPipelineSynchronousPolicy | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
{ | ||
base.OnSendingRequest(message); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit - consider doing this last? It feels like changing the URI should happen before whatever goes on in |
||
string query = message.Request.Uri.Query; | ||
string signature = _credential.Signature; | ||
if (signature.StartsWith("?", StringComparison.InvariantCulture)) | ||
{ | ||
signature = signature.Substring(1); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would be better to use StringBuilder and Spans to minimize allocations in the pipeline if possible. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ouch. In Java There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I changed the code. However, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks like we won't get any better with .NET version we're targeting (see comments below). I brought back original code. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
} | ||
if (!query.Contains(signature)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can the There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
{ | ||
query = string.IsNullOrEmpty(query) ? '?' + signature : query + '&' + signature; | ||
message.Request.Uri.Query = query; | ||
} | ||
} | ||
} | ||
} |
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); | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit - let's use "Shared access signature" in all the doc comments. The property/param names are fine though.