Storage: Reject HTTP endpoints (#22183)

Azure · Jan 8, 2024 · 06a9327 · 06a9327
1 parent 2d017b7
commit 06a9327
Show file tree

Hide file tree

Showing 12 changed files with 161 additions and 4 deletions.
diff --git a/sdk/storage/azblob/CHANGELOG.md b/sdk/storage/azblob/CHANGELOG.md
@@ -11,6 +11,8 @@
 
 ### Bugs Fixed
 
+* Block `SharedKeyCredential` authentication mode for non TLS protected endpoints. Fixes [#21841](https://github.com/Azure/azure-sdk-for-go/issues/21841).
+
 ### Other Changes
 
 ## 1.2.1 (2023-12-13)

diff --git a/sdk/storage/azblob/assets.json b/sdk/storage/azblob/assets.json
@@ -2,5 +2,5 @@
   "AssetsRepo": "Azure/azure-sdk-assets",
   "AssetsRepoPrefixPath": "go",
   "TagPrefix": "go/storage/azblob",
-  "Tag": "go/storage/azblob_ceb9b7d6b4"
+  "Tag": "go/storage/azblob_9f40a5a13d"
 }
diff --git a/sdk/storage/azblob/internal/exported/shared_key_credential.go b/sdk/storage/azblob/internal/exported/shared_key_credential.go
@@ -11,7 +11,9 @@ import (
 	"crypto/hmac"
 	"crypto/sha256"
 	"encoding/base64"
+	"errors"
 	"fmt"
+	"github.com/Azure/azure-sdk-for-go/sdk/internal/errorinfo"
 	"net/http"
 	"net/url"
 	"sort"
@@ -195,6 +197,17 @@ func NewSharedKeyCredPolicy(cred *SharedKeyCredential) *SharedKeyCredPolicy {
 }
 
 func (s *SharedKeyCredPolicy) Do(req *policy.Request) (*http.Response, error) {
+	// skip adding the authorization header if no SharedKeyCredential was provided.
+	// this prevents a panic that might be hard to diagnose and allows testing
+	// against http endpoints that don't require authentication.
+	if s.cred == nil {
+		return req.Next()
+	}
+
+	if err := checkHTTPSForAuth(req); err != nil {
+		return nil, err
+	}
+
 	if d := getHeader(shared.HeaderXmsDate, req.Raw().Header); d == "" {
 		req.Raw().Header.Set(shared.HeaderXmsDate, time.Now().UTC().Format(http.TimeFormat))
 	}
@@ -216,3 +229,10 @@ func (s *SharedKeyCredPolicy) Do(req *policy.Request) (*http.Response, error) {
 	}
 	return response, err
 }
+
+func checkHTTPSForAuth(req *policy.Request) error {
+	if strings.ToLower(req.Raw().URL.Scheme) != "https" {
+		return errorinfo.NonRetriableError(errors.New("authenticated requests are not permitted for non TLS protected (https) endpoints"))
+	}
+	return nil
+}
diff --git a/sdk/storage/azblob/service/client_test.go b/sdk/storage/azblob/service/client_test.go
@@ -1810,6 +1810,35 @@ func (s *ServiceUnrecordedTestsSuite) TestServiceBlobBatchErrors() {
 	_require.Error(err)
 }
 
+func (s *ServiceRecordedTestsSuite) TestServiceClientRejectHTTP() {
+	_require := require.New(s.T())
+
+	cred, err := testcommon.GetGenericSharedKeyCredential(testcommon.TestAccountDefault)
+	_require.NoError(err)
+
+	svcClient, err := service.NewClientWithSharedKeyCredential("http://"+cred.AccountName()+".blob.core.windows.net/", cred, nil)
+	_require.NoError(err)
+
+	_, err = svcClient.GetProperties(context.Background(), nil)
+	_require.Error(err)
+}
+
+func (s *ServiceRecordedTestsSuite) TestServiceClientWithNilSharedKey() {
+	_require := require.New(s.T())
+
+	accountName, _ := testcommon.GetGenericAccountInfo(testcommon.TestAccountDefault)
+	_require.Greater(len(accountName), 0)
+
+	options := &service.ClientOptions{}
+	testcommon.SetClientOptions(s.T(), &options.ClientOptions)
+	svcClient, err := service.NewClientWithSharedKeyCredential("https://"+accountName+".blob.core.windows.net/", nil, options)
+	_require.NoError(err)
+
+	_, err = svcClient.GetProperties(context.Background(), nil)
+	_require.Error(err)
+	testcommon.ValidateBlobErrorCode(_require, err, bloberror.NoAuthenticationInformation)
+}
+
 func (s *ServiceRecordedTestsSuite) TestServiceClientDefaultAudience() {
 	_require := require.New(s.T())
 	testName := s.T().Name()

diff --git a/sdk/storage/azdatalake/CHANGELOG.md b/sdk/storage/azdatalake/CHANGELOG.md
@@ -12,8 +12,11 @@
 
 ### Bugs Fixed
 
+* Block `SharedKeyCredential` authentication mode for non TLS protected endpoints. Fixes [#21841](https://github.com/Azure/azure-sdk-for-go/issues/21841).
+
 ### Other Changes
 * Updated version of azblob to 1.2.1
+* Updated azcore version to `1.9.1` and azidentity version to `1.4.0`.
 
 ## 1.0.0 (2023-10-18)
 

diff --git a/sdk/storage/azdatalake/assets.json b/sdk/storage/azdatalake/assets.json
@@ -2,5 +2,5 @@
   "AssetsRepo": "Azure/azure-sdk-assets",
   "AssetsRepoPrefixPath": "go",
   "TagPrefix": "go/storage/azdatalake",
-  "Tag": "go/storage/azdatalake_abd1896b54"
-}
+  "Tag": "go/storage/azdatalake_3ae5e1441b"
+}
diff --git a/sdk/storage/azdatalake/internal/exported/shared_key_credential.go b/sdk/storage/azdatalake/internal/exported/shared_key_credential.go
@@ -11,7 +11,9 @@ import (
 	"crypto/hmac"
 	"crypto/sha256"
 	"encoding/base64"
+	"errors"
 	"fmt"
+	"github.com/Azure/azure-sdk-for-go/sdk/internal/errorinfo"
 	"github.com/Azure/azure-sdk-for-go/sdk/storage/azblob"
 	"net/http"
 	"net/url"
@@ -50,6 +52,9 @@ func (c *SharedKeyCredential) AccountName() string {
 }
 
 func ConvertToBlobSharedKey(c *SharedKeyCredential) (*azblob.SharedKeyCredential, error) {
+	if c == nil {
+		return nil, errors.New("SharedKeyCredential cannot be nil")
+	}
 	cred, err := azblob.NewSharedKeyCredential(c.accountName, c.accountKeyString)
 	if err != nil {
 		return nil, err
@@ -205,6 +210,17 @@ func NewSharedKeyCredPolicy(cred *SharedKeyCredential) *SharedKeyCredPolicy {
 }
 
 func (s *SharedKeyCredPolicy) Do(req *policy.Request) (*http.Response, error) {
+	// skip adding the authorization header if no SharedKeyCredential was provided.
+	// this prevents a panic that might be hard to diagnose and allows testing
+	// against http endpoints that don't require authentication.
+	if s.cred == nil {
+		return req.Next()
+	}
+
+	if err := checkHTTPSForAuth(req); err != nil {
+		return nil, err
+	}
+
 	if d := getHeader(shared.HeaderXmsDate, req.Raw().Header); d == "" {
 		req.Raw().Header.Set(shared.HeaderXmsDate, time.Now().UTC().Format(http.TimeFormat))
 	}
@@ -226,3 +242,11 @@ func (s *SharedKeyCredPolicy) Do(req *policy.Request) (*http.Response, error) {
 	}
 	return response, err
 }
+
+// TODO: update the azblob dependency having checks for rejecting URLs that are not HTTPS
+func checkHTTPSForAuth(req *policy.Request) error {
+	if strings.ToLower(req.Raw().URL.Scheme) != "https" {
+		return errorinfo.NonRetriableError(errors.New("authenticated requests are not permitted for non TLS protected (https) endpoints"))
+	}
+	return nil
+}
diff --git a/sdk/storage/azdatalake/service/client_test.go b/sdk/storage/azdatalake/service/client_test.go
@@ -800,3 +800,33 @@ func (s *ServiceRecordedTestsSuite) TestAccountListFilesystemsEmptyPrefix() {
 	}
 	_require.GreaterOrEqual(count, 2)
 }
+
+func (s *ServiceRecordedTestsSuite) TestServiceClientRejectHTTP() {
+	_require := require.New(s.T())
+	testName := s.T().Name()
+
+	cred, err := testcommon.GetGenericSharedKeyCredential(testcommon.TestAccountDatalake)
+	_require.NoError(err)
+
+	svcClient, err := service.NewClientWithSharedKeyCredential("http://"+cred.AccountName()+".dfs.core.windows.net/", cred, nil)
+	_require.NoError(err)
+
+	fsName := testcommon.GenerateFileSystemName(testName)
+	fileName := testcommon.GenerateFileName(testName)
+	fileClient := svcClient.NewFileSystemClient(fsName).NewFileClient(fileName)
+	_require.Equal(fileClient.DFSURL(), "http://"+cred.AccountName()+".dfs.core.windows.net/"+fsName+"/"+fileName)
+
+	_, err = fileClient.Create(context.Background(), nil)
+	_require.Error(err)
+}
+
+func (s *ServiceRecordedTestsSuite) TestServiceClientWithNilSharedKey() {
+	_require := require.New(s.T())
+
+	accountName, _ := testcommon.GetGenericAccountInfo(testcommon.TestAccountDatalake)
+	_require.Greater(len(accountName), 0)
+
+	svcClient, err := service.NewClientWithSharedKeyCredential("http://"+accountName+".dfs.core.windows.net/", nil, nil)
+	_require.Error(err)
+	_require.Nil(svcClient)
+}
diff --git a/sdk/storage/azfile/CHANGELOG.md b/sdk/storage/azfile/CHANGELOG.md
@@ -12,6 +12,7 @@
 
 ### Bugs Fixed
 
+* Block `SharedKeyCredential` authentication mode for non TLS protected endpoints. Fixes [#21841](https://github.com/Azure/azure-sdk-for-go/issues/21841).
 * Fixed a bug where `UploadRangeFromURL` using OAuth was returning error.
 
 ### Other Changes

diff --git a/sdk/storage/azfile/assets.json b/sdk/storage/azfile/assets.json
@@ -2,5 +2,5 @@
   "AssetsRepo": "Azure/azure-sdk-assets",
   "AssetsRepoPrefixPath": "go",
   "TagPrefix": "go/storage/azfile",
-  "Tag": "go/storage/azfile_f80b868396"
+  "Tag": "go/storage/azfile_8f8ed3dd66"
 }
diff --git a/sdk/storage/azfile/internal/exported/shared_key_credential.go b/sdk/storage/azfile/internal/exported/shared_key_credential.go
@@ -11,7 +11,9 @@ import (
 	"crypto/hmac"
 	"crypto/sha256"
 	"encoding/base64"
+	"errors"
 	"fmt"
+	"github.com/Azure/azure-sdk-for-go/sdk/internal/errorinfo"
 	"net/http"
 	"net/url"
 	"sort"
@@ -195,6 +197,17 @@ func NewSharedKeyCredPolicy(cred *SharedKeyCredential) *SharedKeyCredPolicy {
 }
 
 func (s *SharedKeyCredPolicy) Do(req *policy.Request) (*http.Response, error) {
+	// skip adding the authorization header if no SharedKeyCredential was provided.
+	// this prevents a panic that might be hard to diagnose and allows testing
+	// against http endpoints that don't require authentication.
+	if s.cred == nil {
+		return req.Next()
+	}
+
+	if err := checkHTTPSForAuth(req); err != nil {
+		return nil, err
+	}
+
 	if d := getHeader(shared.HeaderXmsDate, req.Raw().Header); d == "" {
 		req.Raw().Header.Set(shared.HeaderXmsDate, time.Now().UTC().Format(http.TimeFormat))
 	}
@@ -216,3 +229,10 @@ func (s *SharedKeyCredPolicy) Do(req *policy.Request) (*http.Response, error) {
 	}
 	return response, err
 }
+
+func checkHTTPSForAuth(req *policy.Request) error {
+	if strings.ToLower(req.Raw().URL.Scheme) != "https" {
+		return errorinfo.NonRetriableError(errors.New("authenticated requests are not permitted for non TLS protected (https) endpoints"))
+	}
+	return nil
+}
diff --git a/sdk/storage/azfile/service/client_test.go b/sdk/storage/azfile/service/client_test.go
@@ -630,6 +630,34 @@ func (s *ServiceRecordedTestsSuite) TestPremiumAccountListShares() {
 	}
 }
 
+func (s *ServiceRecordedTestsSuite) TestServiceClientRejectHTTP() {
+	_require := require.New(s.T())
+
+	cred, err := testcommon.GetGenericSharedKeyCredential(testcommon.TestAccountDefault)
+	_require.NoError(err)
+
+	svcClient, err := service.NewClientWithSharedKeyCredential("http://"+cred.AccountName()+".file.core.windows.net/", cred, nil)
+	_require.NoError(err)
+
+	_, err = svcClient.GetProperties(context.Background(), nil)
+	_require.Error(err)
+}
+
+func (s *ServiceRecordedTestsSuite) TestServiceClientWithNilSharedKey() {
+	_require := require.New(s.T())
+
+	accountName, _ := testcommon.GetGenericAccountInfo(testcommon.TestAccountDefault)
+	_require.Greater(len(accountName), 0)
+
+	options := &service.ClientOptions{}
+	testcommon.SetClientOptions(s.T(), &options.ClientOptions)
+	svcClient, err := service.NewClientWithSharedKeyCredential("https://"+accountName+".file.core.windows.net/", nil, options)
+	_require.NoError(err)
+
+	_, err = svcClient.GetProperties(context.Background(), nil)
+	_require.Error(err)
+}
+
 func (s *ServiceRecordedTestsSuite) TestServiceClientCustomAudience() {
 	_require := require.New(s.T())
 	testName := s.T().Name()