From 00b82a4413000f3fd01a350918a092a28a37e6ae Mon Sep 17 00:00:00 2001 From: Philipp Stotz Date: Thu, 20 Jul 2023 13:36:32 +0200 Subject: [PATCH 1/5] [azblob][sas] Fix SignWithSharedKey if stored access policy is used --- sdk/storage/azblob/sas/service.go | 5 ++- sdk/storage/azblob/sas/service_test.go | 50 ++++++++++++++++++++++++++ 2 files changed, 54 insertions(+), 1 deletion(-) diff --git a/sdk/storage/azblob/sas/service.go b/sdk/storage/azblob/sas/service.go index b900e2b06d33..50435dcb3394 100644 --- a/sdk/storage/azblob/sas/service.go +++ b/sdk/storage/azblob/sas/service.go @@ -16,6 +16,9 @@ import ( "github.com/Azure/azure-sdk-for-go/sdk/storage/azblob/internal/exported" ) +type SharedKeyCredentials interface { +} + // BlobSignatureValues is used to generate a Shared Access Signature (SAS) for an Azure Storage container or blob. // For more information on creating service sas, see https://docs.microsoft.com/rest/api/storageservices/constructing-a-service-sas // For more information on creating user delegation sas, see https://docs.microsoft.com/rest/api/storageservices/create-user-delegation-sas @@ -51,7 +54,7 @@ func getDirectoryDepth(path string) string { // SignWithSharedKey uses an account's SharedKeyCredential to sign this signature values to produce the proper SAS query parameters. func (v BlobSignatureValues) SignWithSharedKey(sharedKeyCredential *SharedKeyCredential) (QueryParameters, error) { - if v.ExpiryTime.IsZero() || v.Permissions == "" { + if v.Identifier == "" && (v.ExpiryTime.IsZero() || v.Permissions == "") { return QueryParameters{}, errors.New("service SAS is missing at least one of these: ExpiryTime or Permissions") } diff --git a/sdk/storage/azblob/sas/service_test.go b/sdk/storage/azblob/sas/service_test.go index b5fa3900a84e..65e29cc9e3d1 100644 --- a/sdk/storage/azblob/sas/service_test.go +++ b/sdk/storage/azblob/sas/service_test.go @@ -7,8 +7,11 @@ package sas import ( + "errors" + "github.com/Azure/azure-sdk-for-go/sdk/storage/azblob/internal/exported" "github.com/stretchr/testify/require" "testing" + "time" ) func TestContainerPermissions_String(t *testing.T) { @@ -257,3 +260,50 @@ func TestGetDirectoryDepth(t *testing.T) { require.Equal(t, c.expected, getDirectoryDepth(c.input)) } } + +func TestBlobSignatureValues_SignWithSharedKey(t *testing.T) { + validSharedKeyCredential, err := exported.NewSharedKeyCredential("fakeaccountname", "AKIAIOSFODNN7EXAMPLE") + require.Nil(t, err, "error creating valid shared key credentials.") + + expiryDate, err := time.Parse("2006-01-02", "2023-07-20") + require.Nil(t, err, "error creating valid expiry date.") + + testdata := []struct { + object BlobSignatureValues + input *SharedKeyCredential + expected QueryParameters + expectedError error + }{ + { + object: BlobSignatureValues{ContainerName: "fakestoragecontainer", Permissions: "a", ExpiryTime: expiryDate}, + input: validSharedKeyCredential, + expected: QueryParameters{version: "2020-02-10", permissions: "a", expiryTime: expiryDate, resource: "c"}, + expectedError: nil, + }, + { + object: BlobSignatureValues{ContainerName: "fakestoragecontainer", Permissions: "", ExpiryTime: expiryDate}, + input: validSharedKeyCredential, + expected: QueryParameters{}, + expectedError: errors.New("service SAS is missing at least one of these: ExpiryTime or Permissions"), + }, + { + object: BlobSignatureValues{ContainerName: "fakestoragecontainer", Permissions: "a", ExpiryTime: *new(time.Time)}, + input: validSharedKeyCredential, + expected: QueryParameters{}, + expectedError: errors.New("service SAS is missing at least one of these: ExpiryTime or Permissions"), + }, + { + object: BlobSignatureValues{ContainerName: "fakestoragecontainer", Permissions: "", ExpiryTime: *new(time.Time), Identifier: "fakepolicyname"}, + input: validSharedKeyCredential, + expected: QueryParameters{version: "2020-02-10", resource: "c", identifier: "fakepolicyname"}, + expectedError: nil, + }, + } + for _, c := range testdata { + act, err := c.object.SignWithSharedKey(c.input) + // ignore signature value + act.signature = "" + require.Equal(t, c.expected, act) + require.Equal(t, c.expectedError, err) + } +} From 999b74e927f46eda8722815af7c39c8149eabdb9 Mon Sep 17 00:00:00 2001 From: Philipp Stotz Date: Thu, 20 Jul 2023 14:01:09 +0200 Subject: [PATCH 2/5] [azblob][sas] Fix SignWithSharedKey if stored access policy is used --- sdk/storage/azblob/sas/service.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/sdk/storage/azblob/sas/service.go b/sdk/storage/azblob/sas/service.go index 50435dcb3394..6c3ae263fc7a 100644 --- a/sdk/storage/azblob/sas/service.go +++ b/sdk/storage/azblob/sas/service.go @@ -16,9 +16,6 @@ import ( "github.com/Azure/azure-sdk-for-go/sdk/storage/azblob/internal/exported" ) -type SharedKeyCredentials interface { -} - // BlobSignatureValues is used to generate a Shared Access Signature (SAS) for an Azure Storage container or blob. // For more information on creating service sas, see https://docs.microsoft.com/rest/api/storageservices/constructing-a-service-sas // For more information on creating user delegation sas, see https://docs.microsoft.com/rest/api/storageservices/create-user-delegation-sas From c895a6173f7313594bc4494f9c6ea56fe24b7640 Mon Sep 17 00:00:00 2001 From: Sourav Gupta Date: Fri, 21 Jul 2023 12:30:05 +0530 Subject: [PATCH 3/5] Adding test and changelog --- sdk/storage/azblob/CHANGELOG.md | 3 +- sdk/storage/azblob/container/client_test.go | 57 +++++++++++++++++++++ 2 files changed, 59 insertions(+), 1 deletion(-) diff --git a/sdk/storage/azblob/CHANGELOG.md b/sdk/storage/azblob/CHANGELOG.md index 235250a0b328..f77c2ce88043 100644 --- a/sdk/storage/azblob/CHANGELOG.md +++ b/sdk/storage/azblob/CHANGELOG.md @@ -8,6 +8,8 @@ ### Bugs Fixed +* Fixed service SAS creation where expiry time or permissions can be omitted when stored access policy is used. Fixes [#21229](https://github.com/Azure/azure-sdk-for-go/issues/21229). + ### Other Changes ## 1.1.0 (2023-07-13) @@ -25,7 +27,6 @@ * Fixed time formatting for the conditional request headers. Fixes [#20475](https://github.com/Azure/azure-sdk-for-go/issues/20475). * Fixed an issue where passing a blob tags map of length 0 would result in the x-ms-tags header to be sent to the service with an empty string as value. - * Fixed block size and number of blocks calculation in `UploadBuffer` and `UploadFile`. Fixes [#20735](https://github.com/Azure/azure-sdk-for-go/issues/20735). ### Other Changes diff --git a/sdk/storage/azblob/container/client_test.go b/sdk/storage/azblob/container/client_test.go index 21261c4e4931..2bc3ac73eae2 100644 --- a/sdk/storage/azblob/container/client_test.go +++ b/sdk/storage/azblob/container/client_test.go @@ -3027,3 +3027,60 @@ func (s *ContainerUnrecordedTestsSuite) TestContainerBlobBatchErrors() { _, err = cntClient.SubmitBatch(context.Background(), nil, nil) _require.Error(err) } + +func (s *ContainerUnrecordedTestsSuite) TestContainerSASUsingAccessPolicy() { + _require := require.New(s.T()) + testName := s.T().Name() + + cred, err := testcommon.GetGenericSharedKeyCredential(testcommon.TestAccountDefault) + _require.NoError(err) + + svcClient, err := testcommon.GetServiceClient(s.T(), testcommon.TestAccountDefault, nil) + _require.NoError(err) + + // Creating container client + containerName := testcommon.GenerateContainerName(testName) + cntClient := testcommon.CreateNewContainer(context.Background(), _require, containerName, svcClient) + defer testcommon.DeleteContainer(context.Background(), _require, cntClient) + + id := "testAccessPolicy" + signedIdentifiers := make([]*container.SignedIdentifier, 0) + signedIdentifiers = append(signedIdentifiers, &container.SignedIdentifier{ + AccessPolicy: &container.AccessPolicy{ + Expiry: to.Ptr(time.Now().Add(1 * time.Hour)), + Start: to.Ptr(time.Now()), + Permission: to.Ptr("racwd"), + }, + ID: &id, + }) + + _, err = cntClient.SetAccessPolicy(context.Background(), &container.SetAccessPolicyOptions{ + ContainerACL: signedIdentifiers, + }) + _require.NoError(err) + + gResp, err := cntClient.GetAccessPolicy(context.Background(), nil) + _require.NoError(err) + _require.Len(gResp.SignedIdentifiers, 1) + + time.Sleep(30 * time.Second) + + sasQueryParams, err := sas.BlobSignatureValues{ + Protocol: sas.ProtocolHTTPS, + Identifier: id, + ContainerName: containerName, + }.SignWithSharedKey(cred) + _require.NoError(err) + + cntSAS := cntClient.URL() + "?" + sasQueryParams.Encode() + cntClientSAS, err := container.NewClientWithNoCredential(cntSAS, nil) + _require.NoError(err) + + bbClient := testcommon.CreateNewBlockBlob(context.Background(), _require, testcommon.GenerateBlobName(testName), cntClientSAS) + + _, err = bbClient.GetProperties(context.Background(), nil) + _require.NoError(err) + + _, err = bbClient.Delete(context.Background(), nil) + _require.NoError(err) +} From fcc74ce8160688cf8f0b3a84fddc9b4f1b53764c Mon Sep 17 00:00:00 2001 From: Sourav Gupta Date: Fri, 21 Jul 2023 14:46:35 +0530 Subject: [PATCH 4/5] Test fix --- sdk/storage/azblob/container/client_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sdk/storage/azblob/container/client_test.go b/sdk/storage/azblob/container/client_test.go index 2bc3ac73eae2..566a7a38fe64 100644 --- a/sdk/storage/azblob/container/client_test.go +++ b/sdk/storage/azblob/container/client_test.go @@ -1548,7 +1548,7 @@ func (s *ContainerRecordedTestsSuite) TestContainerGetPermissionsPublicAccessNot _require.Equal(*resp.BlobPublicAccess, container.PublicAccessTypeBlob) } -func (s *ContainerRecordedTestsSuite) TestContainerSetPermissionsPublicAccessNone() { +func (s *ContainerUnrecordedTestsSuite) TestContainerSetPermissionsPublicAccessNone() { // Test the basic one by making an anonymous request to ensure it's actually doing it and also with GetPermissions // For all the others, can just use GetPermissions since we've validated that it at least registers on the server correctly _require := require.New(s.T()) From f7cd9a82d82473d4dfee97c411966f2b4a31ce5f Mon Sep 17 00:00:00 2001 From: Sourav Gupta Date: Fri, 21 Jul 2023 15:47:40 +0530 Subject: [PATCH 5/5] small change in UT --- sdk/storage/azblob/sas/service_test.go | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/sdk/storage/azblob/sas/service_test.go b/sdk/storage/azblob/sas/service_test.go index 65e29cc9e3d1..60951ee5999e 100644 --- a/sdk/storage/azblob/sas/service_test.go +++ b/sdk/storage/azblob/sas/service_test.go @@ -262,7 +262,7 @@ func TestGetDirectoryDepth(t *testing.T) { } func TestBlobSignatureValues_SignWithSharedKey(t *testing.T) { - validSharedKeyCredential, err := exported.NewSharedKeyCredential("fakeaccountname", "AKIAIOSFODNN7EXAMPLE") + cred, err := exported.NewSharedKeyCredential("fakeaccountname", "AKIAIOSFODNN7EXAMPLE") require.Nil(t, err, "error creating valid shared key credentials.") expiryDate, err := time.Parse("2006-01-02", "2023-07-20") @@ -270,37 +270,32 @@ func TestBlobSignatureValues_SignWithSharedKey(t *testing.T) { testdata := []struct { object BlobSignatureValues - input *SharedKeyCredential expected QueryParameters expectedError error }{ { object: BlobSignatureValues{ContainerName: "fakestoragecontainer", Permissions: "a", ExpiryTime: expiryDate}, - input: validSharedKeyCredential, - expected: QueryParameters{version: "2020-02-10", permissions: "a", expiryTime: expiryDate, resource: "c"}, + expected: QueryParameters{version: Version, permissions: "a", expiryTime: expiryDate, resource: "c"}, expectedError: nil, }, { object: BlobSignatureValues{ContainerName: "fakestoragecontainer", Permissions: "", ExpiryTime: expiryDate}, - input: validSharedKeyCredential, expected: QueryParameters{}, expectedError: errors.New("service SAS is missing at least one of these: ExpiryTime or Permissions"), }, { object: BlobSignatureValues{ContainerName: "fakestoragecontainer", Permissions: "a", ExpiryTime: *new(time.Time)}, - input: validSharedKeyCredential, expected: QueryParameters{}, expectedError: errors.New("service SAS is missing at least one of these: ExpiryTime or Permissions"), }, { object: BlobSignatureValues{ContainerName: "fakestoragecontainer", Permissions: "", ExpiryTime: *new(time.Time), Identifier: "fakepolicyname"}, - input: validSharedKeyCredential, - expected: QueryParameters{version: "2020-02-10", resource: "c", identifier: "fakepolicyname"}, + expected: QueryParameters{version: Version, resource: "c", identifier: "fakepolicyname"}, expectedError: nil, }, } for _, c := range testdata { - act, err := c.object.SignWithSharedKey(c.input) + act, err := c.object.SignWithSharedKey(cred) // ignore signature value act.signature = "" require.Equal(t, c.expected, act)