-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Respect Sas token in conn string #8940
Conversation
4eaecca
to
e468306
Compare
if ((services & AccountSasServices.Files) == AccountSasServices.Files) | ||
{ | ||
sb.Append(Constants.Sas.AccountServices.File); | ||
} | ||
if ((services & AccountSasServices.Queues) == AccountSasServices.Queues) |
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.
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.
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.
Mind adding a comment in the source explaining why the order's important?
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.
Added
44f2978
to
4c9dc28
Compare
4c9dc28
to
81ce9c0
Compare
81ce9c0
to
b650958
Compare
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.
Thank you Josh!
var sasBuilder = new AccountSasBuilder | ||
{ | ||
ExpiresOn = Recording.UtcNow.AddHours(1), | ||
Services = AccountSasServices.All, |
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.
Do we have tests that cover only Blob, Files, and Queues. Or since this is just for blobs, should test for Blobs and All?
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.
Blobs and All should be sufficient for this file. Adding additional tests in the Files and Queue test projects might be a good idea.
{ | ||
ExpiresOn = Recording.UtcNow.AddHours(1), | ||
Services = AccountSasServices.All, | ||
ResourceTypes = AccountSasResourceTypes.All, |
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.
Can we also test individually for Container, Object, Service?
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.
Added
ResourceTypes = AccountSasResourceTypes.All, | ||
Protocol = SasProtocol.Https, | ||
}; | ||
sasBuilder.SetPermissions(AccountSasPermissions.All); |
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.
If not done already would be good to have coverage for Read only and Write only.
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.
Added a Read-only
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.
And a Write, Delete
.../Azure.Storage.Blobs/tests/SessionRecords/BlobBaseClientTests/Ctor_ConnectionString_Sas.json
Outdated
Show resolved
Hide resolved
...e.Storage.Blobs/tests/SessionRecords/BlobBaseClientTests/Ctor_ConnectionString_SasAsync.json
Outdated
Show resolved
Hide resolved
/// <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) => |
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.
Should this param be of type object or restricted to an interface?
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.
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.
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.
Azure.Core didn't want to add a base Credential
type so we're stuck with object
.
{ | ||
accountName = sharedKeyCredentials.AccountName; | ||
} | ||
else |
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.
Should we use else if
here?
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.
Sure
conn._accountName = accountName; | ||
var sasToken = (storageCredentials is SharedAccessSignatureCredentials sasCredentials) ? sasCredentials.SasToken : default; | ||
|
||
var protocol = useHttps ? "https" : "http"; |
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.
Technically when discussing URIs, this part is called the scheme, not protocol.
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.
Updated
sdk/storage/Azure.Storage.Common/tests/Shared/TestExtensions.cs
Outdated
Show resolved
Hide resolved
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.
Looks good.
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.
Please get escrow approval before merging this change.
if ((services & AccountSasServices.Files) == AccountSasServices.Files) | ||
{ | ||
sb.Append(Constants.Sas.AccountServices.File); | ||
} | ||
if ((services & AccountSasServices.Queues) == AccountSasServices.Queues) |
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.
Mind adding a comment in the source explaining why the order's important?
{ | ||
var builder = new UriBuilder(endpoint) | ||
{ | ||
Query = sasToken |
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.
Do we know for sure the endpoint will never have any query params? Especially since connection strings aren't well defined anywhere?
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.
Would you recommend using HttpUtility.ParseQueryString to append sasToken?
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.
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 &
).
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.
Updated
endpointSuffix); | ||
|
||
return (new Uri(primaryUri), new Uri(secondaryUri)); | ||
return (primaryUriBuilder.Uri, secondaryUriBuilder.Uri); |
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 - is it worth sharing a single builder and just changing the host between Uri creation?
/// <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) => |
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.
Azure.Core didn't want to add a base Credential
type so we're stuck with object
.
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. |
b2fd410
to
496b609
Compare
496b609
to
076f31b
Compare
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
/// The transport pipeline used to send every request. | ||
/// </param> | ||
/// <param name="clientDiagnostics"></param> | ||
internal ShareServiceClient(Uri serviceUri, HttpPipeline pipeline, ClientDiagnostics clientDiagnostics) |
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.
This ctor is unused.
/// The transport pipeline used to send every request. | ||
/// </param> | ||
/// <param name="clientDiagnostics"></param> | ||
internal QueueServiceClient(Uri serviceUri, HttpPipeline pipeline, ClientDiagnostics clientDiagnostics) |
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.
Unused
f7c4507
to
55a0fe8
Compare
Backing this out. This needs more discussion. |
Fixes #8859