Fix azure pre-signed url for blob from different account (#6594)

treeverse · Sep 14, 2023 · aa755c0 · aa755c0
1 parent 0b3302d
commit aa755c0
Show file tree

Hide file tree

Showing 2 changed files with 17 additions and 13 deletions.
diff --git a/pkg/block/azure/adapter.go b/pkg/block/azure/adapter.go
@@ -2,7 +2,6 @@ package azure
 
 import (
 	"context"
-	"errors"
 	"fmt"
 	"io"
 	"net/http"
@@ -21,18 +20,16 @@ import (
 	"github.com/treeverse/lakefs/pkg/logging"
 )
 
-var ErrNotImplemented = errors.New("not implemented")
-
 const (
 	sizeSuffix = "_size"
 	idSuffix   = "_id"
 	_1MiB      = 1024 * 1024
 	MaxBuffers = 1
 	// udcCacheSize - Arbitrary number: exceeding this number means that in the expiry timeframe we requested pre-signed urls from
-	// more the 5000 different accounts which is highly unlikely
+	// more the 5000 different accounts, which is highly unlikely
 	udcCacheSize = 5000
 
-	URLTemplate = "https://%s.blob.core.windows.net/"
+	BlobEndpointFormat = "https://%s.blob.core.windows.net/"
 )
 
 type Adapter struct {
@@ -92,7 +89,7 @@ func ResolveBlobURLInfoFromURL(pathURL *url.URL) (BlobURLInfo, error) {
 		return qk, err
 	}
 
-	// In azure the first part of the path is part of the storage namespace
+	// In azure, the first part of the path is part of the storage namespace
 	trimmedPath := strings.Trim(pathURL.Path, "/")
 	pathParts := strings.Split(trimmedPath, "/")
 	if len(pathParts) == 0 {
@@ -222,9 +219,9 @@ func (a *Adapter) GetPreSignedURL(ctx context.Context, obj block.ObjectPointer,
 			Write: true,
 		}
 	}
-	url, err := a.getPreSignedURL(ctx, obj, permissions)
+	preSignedURL, err := a.getPreSignedURL(ctx, obj, permissions)
 	// TODO(#6347): Report expiry.
-	return url, time.Time{}, err
+	return preSignedURL, time.Time{}, err
 }
 
 func (a *Adapter) getPreSignedURL(ctx context.Context, obj block.ObjectPointer, permissions sas.BlobPermissions) (string, error) {
@@ -253,18 +250,25 @@ func (a *Adapter) getPreSignedURL(ctx context.Context, obj block.ObjectPointer,
 	}
 
 	// Create Blob Signature Values with desired permissions and sign with user delegation credential
-	sasQueryParams, err := sas.BlobSignatureValues{
+	blobSignatureValues := sas.BlobSignatureValues{
 		Protocol:      sas.ProtocolHTTPS,
 		ExpiryTime:    urlExpiry,
 		Permissions:   to.Ptr(permissions).String(),
 		ContainerName: qualifiedKey.ContainerName,
 		BlobName:      qualifiedKey.BlobURL,
-	}.SignWithUserDelegation(udc)
+	}
+	sasQueryParams, err := blobSignatureValues.SignWithUserDelegation(udc)
 	if err != nil {
 		return "", err
 	}
-	u := fmt.Sprintf("%s/%s?%s", qualifiedKey.ContainerURL, qualifiedKey.BlobURL, sasQueryParams.Encode())
 
+	// format blob URL with signed SAS query params
+	accountEndpoint := fmt.Sprintf(BlobEndpointFormat, qualifiedKey.StorageAccountName)
+	u, err := url.JoinPath(accountEndpoint, qualifiedKey.ContainerName, qualifiedKey.BlobURL)
+	if err != nil {
+		return "", err
+	}
+	u += "?" + sasQueryParams.Encode()
 	return u, nil
 }
 
@@ -541,7 +545,7 @@ func (a *Adapter) copyPartRange(ctx context.Context, sourceObj, destinationObj b
 }
 
 func (a *Adapter) AbortMultiPartUpload(_ context.Context, _ block.ObjectPointer, _ string) error {
-	// Azure has no abort, in case of commit, uncommitted parts are erased, otherwise staged data is erased after 7 days
+	// Azure has no abort. In case of commit, uncommitted parts are erased. Otherwise, staged data is erased after 7 days
 	return nil
 }
 

diff --git a/pkg/block/azure/client_cache.go b/pkg/block/azure/client_cache.go
@@ -122,7 +122,7 @@ func BuildAzureServiceClient(params params.Azure) (*service.Client, error) {
 	if params.TestEndpointURL != "" { // For testing purposes - override default url template
 		url = params.TestEndpointURL
 	} else {
-		url = fmt.Sprintf(URLTemplate, params.StorageAccount)
+		url = fmt.Sprintf(BlobEndpointFormat, params.StorageAccount)
 	}
 
 	options := service.ClientOptions{ClientOptions: azcore.ClientOptions{Retry: policy.RetryOptions{TryTimeout: params.TryTimeout}}}