Skip to content

Commit

Permalink
fix: generate sas token when secret is used in volume cloning
Browse files Browse the repository at this point in the history
  • Loading branch information
@andyzhangx
andyzhangx committed Jan 4, 2024
1 parent 3146637 commit f2d6459
Show file tree
Hide file tree
Showing 2 changed files with 55 additions and 116 deletions.
78 changes: 41 additions & 37 deletions pkg/blob/controllerserver.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -421,11 +421,11 @@ func (d *Driver) CreateVolume(ctx context.Context, req *csi.CreateVolumeRequest)
}

if req.GetVolumeContentSource() != nil {
var accountSASToken string
if accountSASToken, err = d.getSASToken(ctx, accountName, accountKey, storageEndpointSuffix, accountOptions, secrets, secretName, secretNamespace); err != nil {
return nil, status.Errorf(codes.Internal, "failed to getSASToken on account(%s) rg(%s), error: %v", accountOptions.Name, accountOptions.ResourceGroup, err)
accountSASToken, authAzcopyEnv, err := d.getAzcopyAuth(ctx, accountName, accountKey, storageEndpointSuffix, accountOptions, secrets, secretName, secretNamespace)
if err != nil {
return nil, status.Errorf(codes.Internal, "failed to getAzcopyAuth on account(%s) rg(%s), error: %v", accountOptions.Name, accountOptions.ResourceGroup, err)
}
if err := d.copyVolume(ctx, req, accountSASToken, validContainerName, storageEndpointSuffix); err != nil {
if err := d.copyVolume(req, accountSASToken, authAzcopyEnv, validContainerName, storageEndpointSuffix); err != nil {
return nil, err
}
} else {
Expand Down Expand Up @@ -720,7 +720,7 @@ func (d *Driver) DeleteBlobContainer(ctx context.Context, subsID, resourceGroupN
}

// CopyBlobContainer copies a blob container in the same storage account
func (d *Driver) copyBlobContainer(_ context.Context, req *csi.CreateVolumeRequest, accountSasToken, dstContainerName, storageEndpointSuffix string) error {
func (d *Driver) copyBlobContainer(req *csi.CreateVolumeRequest, accountSasToken string, authAzcopyEnv []string, dstContainerName, storageEndpointSuffix string) error {
var sourceVolumeID string
if req.GetVolumeContentSource() != nil && req.GetVolumeContentSource().GetVolume() != nil {
sourceVolumeID = req.GetVolumeContentSource().GetVolume().GetVolumeId()
Expand All @@ -734,13 +734,6 @@ func (d *Driver) copyBlobContainer(_ context.Context, req *csi.CreateVolumeReque
return fmt.Errorf("srcContainerName(%s) or dstContainerName(%s) is empty", srcContainerName, dstContainerName)
}

var authAzcopyEnv []string
if accountSasToken == "" {
if authAzcopyEnv, err = d.authorizeAzcopyWithIdentity(); err != nil {
return err
}
}

timeAfter := time.After(waitForCopyTimeout)
timeTick := time.Tick(waitForCopyInterval)
srcPath := fmt.Sprintf("https://%s.blob.%s/%s%s", accountName, storageEndpointSuffix, srcContainerName, accountSasToken)
Expand Down Expand Up @@ -781,18 +774,19 @@ func (d *Driver) copyBlobContainer(_ context.Context, req *csi.CreateVolumeReque
}

// copyVolume copies a volume form volume or snapshot, snapshot is not supported now
func (d *Driver) copyVolume(ctx context.Context, req *csi.CreateVolumeRequest, accountSASToken, dstContainerName, storageEndpointSuffix string) error {
func (d *Driver) copyVolume(req *csi.CreateVolumeRequest, accountSASToken string, authAzcopyEnv []string, dstContainerName, storageEndpointSuffix string) error {
vs := req.VolumeContentSource
switch vs.Type.(type) {
case *csi.VolumeContentSource_Snapshot:
return status.Errorf(codes.InvalidArgument, "copy volume from volumeSnapshot is not supported")
case *csi.VolumeContentSource_Volume:
return d.copyBlobContainer(ctx, req, accountSASToken, dstContainerName, storageEndpointSuffix)
return d.copyBlobContainer(req, accountSASToken, authAzcopyEnv, dstContainerName, storageEndpointSuffix)
default:
return status.Errorf(codes.InvalidArgument, "%v is not a proper volume source", vs)
}
}

// authorizeAzcopyWithIdentity returns auth env for azcopy using cluster identity
func (d *Driver) authorizeAzcopyWithIdentity() ([]string, error) {
azureAuthConfig := d.cloud.Config.AzureAuthConfig
var authAzcopyEnv []string
Expand Down Expand Up @@ -822,45 +816,55 @@ func (d *Driver) authorizeAzcopyWithIdentity() ([]string, error) {
return []string{}, fmt.Errorf("service principle or managed identity are both not set")
}

// getSASToken will only generate sas token for azcopy in following conditions:
// getAzcopyAuth will only generate sas token for azcopy in following conditions:
// 1. secrets is not empty
// 2. driver is not using managed identity and service principal
// 3. azcopy returns AuthorizationPermissionMismatch error when using service principal or managed identity
func (d *Driver) getSASToken(ctx context.Context, accountName, accountKey, storageEndpointSuffix string, accountOptions *azure.AccountOptions, secrets map[string]string, secretName, secretNamespace string) (string, error) {
authAzcopyEnv, _ := d.authorizeAzcopyWithIdentity()
func (d *Driver) getAzcopyAuth(ctx context.Context, accountName, accountKey, storageEndpointSuffix string, accountOptions *azure.AccountOptions, secrets map[string]string, secretName, secretNamespace string) (string, []string, error) {
var authAzcopyEnv []string
useSasToken := false
if len(authAzcopyEnv) > 0 {
// search in cache first
cache, err := d.azcopySasTokenCache.Get(accountName, azcache.CacheReadTypeDefault)
if len(secrets) == 0 && len(secretName) == 0 {
var err error
authAzcopyEnv, err = d.authorizeAzcopyWithIdentity()
if err != nil {
return "", fmt.Errorf("get(%s) from azcopySasTokenCache failed with error: %v", accountName, err)
}
if cache != nil {
klog.V(2).Infof("use sas token for account(%s) since this account is found in azcopySasTokenCache", accountName)
useSasToken = true
klog.Warningf("failed to authorize azcopy with identity, error: %v", err)
} else {
out, testErr := d.azcopy.TestListJobs(accountName, storageEndpointSuffix, authAzcopyEnv)
if testErr != nil {
return "", fmt.Errorf("azcopy list command failed with error(%v): %v", testErr, out)
}
if strings.Contains(out, authorizationPermissionMismatch) {
klog.Warningf("azcopy list failed with AuthorizationPermissionMismatch error, should assign \"Storage Blob Data Contributor\" role to controller identity, fall back to use sas token, original output: %v", out)
d.azcopySasTokenCache.Set(accountName, "")
useSasToken = true
if len(authAzcopyEnv) > 0 {
// search in cache first
cache, err := d.azcopySasTokenCache.Get(accountName, azcache.CacheReadTypeDefault)
if err != nil {
return "", nil, fmt.Errorf("get(%s) from azcopySasTokenCache failed with error: %v", accountName, err)
}
if cache != nil {
klog.V(2).Infof("use sas token for account(%s) since this account is found in azcopySasTokenCache", accountName)
useSasToken = true
} else {
out, testErr := d.azcopy.TestListJobs(accountName, storageEndpointSuffix, authAzcopyEnv)
if testErr != nil {
return "", nil, fmt.Errorf("azcopy list command failed with error(%v): %v", testErr, out)
}
if strings.Contains(out, authorizationPermissionMismatch) {
klog.Warningf("azcopy list failed with AuthorizationPermissionMismatch error, should assign \"Storage Blob Data Contributor\" role to controller identity, fall back to use sas token, original output: %v", out)
d.azcopySasTokenCache.Set(accountName, "")
useSasToken = true
}
}
}
}
}
if len(secrets) > 0 || len(authAzcopyEnv) == 0 || useSasToken {

if len(secrets) > 0 || len(secretName) > 0 || len(authAzcopyEnv) == 0 || useSasToken {
var err error
if accountKey == "" {
if _, accountKey, err = d.GetStorageAccesskey(ctx, accountOptions, secrets, secretName, secretNamespace); err != nil {
return "", err
return "", nil, err
}
}
klog.V(2).Infof("generate sas token for account(%s)", accountName)
return generateSASToken(accountName, accountKey, storageEndpointSuffix, d.sasTokenExpirationMinutes)
sasToken, err := generateSASToken(accountName, accountKey, storageEndpointSuffix, d.sasTokenExpirationMinutes)
return sasToken, nil, err
}
return "", nil
return "", authAzcopyEnv, nil
}

// isValidVolumeCapabilities validates the given VolumeCapability array is valid
Expand Down
93 changes: 14 additions & 79 deletions pkg/blob/controllerserver_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1555,10 +1555,8 @@ func TestCopyVolume(t *testing.T) {
VolumeContentSource: &volumecontensource,
}

ctx := context.Background()

expectedErr := status.Errorf(codes.InvalidArgument, "copy volume from volumeSnapshot is not supported")
err := d.copyVolume(ctx, req, "", "", "core.windows.net")
err := d.copyVolume(req, "", nil, "", "core.windows.net")
if !reflect.DeepEqual(err, expectedErr) {
t.Errorf("Unexpected error: %v", err)
}
Expand Down Expand Up @@ -1587,10 +1585,8 @@ func TestCopyVolume(t *testing.T) {
VolumeContentSource: &volumecontensource,
}

ctx := context.Background()

expectedErr := status.Errorf(codes.NotFound, "error parsing volume id: \"unit-test\", should at least contain two #")
err := d.copyVolume(ctx, req, "", "dstContainer", "core.windows.net")
err := d.copyVolume(req, "", nil, "dstContainer", "core.windows.net")
if !reflect.DeepEqual(err, expectedErr) {
t.Errorf("Unexpected error: %v", err)
}
Expand Down Expand Up @@ -1619,10 +1615,8 @@ func TestCopyVolume(t *testing.T) {
VolumeContentSource: &volumecontensource,
}

ctx := context.Background()

expectedErr := fmt.Errorf("srcContainerName() or dstContainerName(dstContainer) is empty")
err := d.copyVolume(ctx, req, "", "dstContainer", "core.windows.net")
err := d.copyVolume(req, "", nil, "dstContainer", "core.windows.net")
if !reflect.DeepEqual(err, expectedErr) {
t.Errorf("Unexpected error: %v", err)
}
Expand Down Expand Up @@ -1651,43 +1645,8 @@ func TestCopyVolume(t *testing.T) {
VolumeContentSource: &volumecontensource,
}

ctx := context.Background()

expectedErr := fmt.Errorf("srcContainerName(fileshare) or dstContainerName() is empty")
err := d.copyVolume(ctx, req, "", "", "core.windows.net")
if !reflect.DeepEqual(err, expectedErr) {
t.Errorf("Unexpected error: %v", err)
}
},
},
{
name: "AADClientSecret shouldn't be nil or useManagedIdentityExtension must be set to true when accountSASToken is empty",
testFunc: func(t *testing.T) {
d := NewFakeDriver()
d.cloud = &azure.Cloud{}
mp := map[string]string{}

volumeSource := &csi.VolumeContentSource_VolumeSource{
VolumeId: "vol_1#f5713de20cde511e8ba4900#fileshare#",
}
volumeContentSourceVolumeSource := &csi.VolumeContentSource_Volume{
Volume: volumeSource,
}
volumecontensource := csi.VolumeContentSource{
Type: volumeContentSourceVolumeSource,
}

req := &csi.CreateVolumeRequest{
Name: "unit-test",
VolumeCapabilities: stdVolumeCapabilities,
Parameters: mp,
VolumeContentSource: &volumecontensource,
}

ctx := context.Background()

expectedErr := fmt.Errorf("service principle or managed identity are both not set")
err := d.copyVolume(ctx, req, "", "dstContainer", "core.windows.net")
err := d.copyVolume(req, "", nil, "", "core.windows.net")
if !reflect.DeepEqual(err, expectedErr) {
t.Errorf("Unexpected error: %v", err)
}
Expand Down Expand Up @@ -1728,10 +1687,8 @@ func TestCopyVolume(t *testing.T) {

d.azcopy.ExecCmd = m

ctx := context.Background()

var expectedErr error
err := d.copyVolume(ctx, req, "sastoken", "dstContainer", "core.windows.net")
err := d.copyVolume(req, "sastoken", nil, "dstContainer", "core.windows.net")
if !reflect.DeepEqual(err, expectedErr) {
t.Errorf("Unexpected error: %v", err)
}
Expand Down Expand Up @@ -1773,10 +1730,8 @@ func TestCopyVolume(t *testing.T) {

d.azcopy.ExecCmd = m

ctx := context.Background()

var expectedErr error
err := d.copyVolume(ctx, req, "sastoken", "dstContainer", "core.windows.net")
err := d.copyVolume(req, "sastoken", nil, "dstContainer", "core.windows.net")
if !reflect.DeepEqual(err, expectedErr) {
t.Errorf("Unexpected error: %v", err)
}
Expand Down Expand Up @@ -1995,7 +1950,7 @@ func TestAuthorizeAzcopyWithIdentity(t *testing.T) {
}
}

func TestGetSASToken(t *testing.T) {
func TestGetAzcopyAuth(t *testing.T) {
testCases := []struct {
name string
testFunc func(t *testing.T)
Expand All @@ -2014,14 +1969,14 @@ func TestGetSASToken(t *testing.T) {
ctx := context.Background()
expectedAccountSASToken := ""
expectedErr := fmt.Errorf("could not find accountkey or azurestorageaccountkey field in secrets")
accountSASToken, err := d.getSASToken(ctx, "accountName", "", "core.windows.net", &azure.AccountOptions{}, secrets, "secretsName", "secretsNamespace")
accountSASToken, _, err := d.getAzcopyAuth(ctx, "accountName", "", "core.windows.net", &azure.AccountOptions{}, secrets, "secretsName", "secretsNamespace")
if !reflect.DeepEqual(err, expectedErr) || !reflect.DeepEqual(accountSASToken, expectedAccountSASToken) {
t.Errorf("Unexpected accountSASToken: %s, Unexpected error: %v", accountSASToken, err)
}
},
},
{
name: "failed to test azcopy list command",
name: "generate sas token using account key",
testFunc: func(t *testing.T) {
d := NewFakeDriver()
d.cloud = &azure.Cloud{
Expand All @@ -2033,21 +1988,10 @@ func TestGetSASToken(t *testing.T) {
}
secrets := map[string]string{
defaultSecretAccountName: "accountName",
defaultSecretAccountKey: "YWNjb3VudGtleQo=",
}
ctrl := gomock.NewController(t)
defer ctrl.Finish()

m := util.NewMockEXEC(ctrl)
listStr := "error"
m.EXPECT().RunCommand(gomock.Any(), gomock.Any()).Return(listStr, fmt.Errorf("error"))

d.azcopy.ExecCmd = m

ctx := context.Background()
expectedAccountSASToken := ""
expectedErr := fmt.Errorf("azcopy list command failed with error(%v): %v", fmt.Errorf("error"), "error")
accountSASToken, err := d.getSASToken(ctx, "accountName", "", "core.windows.net", &azure.AccountOptions{}, secrets, "secretsName", "secretsNamespace")
if !reflect.DeepEqual(err, expectedErr) || !reflect.DeepEqual(accountSASToken, expectedAccountSASToken) {
accountSASToken, _, err := d.getAzcopyAuth(context.Background(), "accountName", "", "core.windows.net", &azure.AccountOptions{}, secrets, "secretsName", "secretsNamespace")
if !reflect.DeepEqual(err, nil) || !strings.Contains(accountSASToken, "?se=") {
t.Errorf("Unexpected accountSASToken: %s, Unexpected error: %v", accountSASToken, err)
}
},
Expand All @@ -2067,19 +2011,10 @@ func TestGetSASToken(t *testing.T) {
defaultSecretAccountName: "accountName",
defaultSecretAccountKey: "fakeValue",
}
ctrl := gomock.NewController(t)
defer ctrl.Finish()

m := util.NewMockEXEC(ctrl)
listStr := "RESPONSE 403: 403 This request is not authorized to perform this operation using this permission.\nERROR CODE: AuthorizationPermissionMismatch"
m.EXPECT().RunCommand(gomock.Any(), gomock.Any()).Return(listStr, nil)

d.azcopy.ExecCmd = m

ctx := context.Background()
expectedAccountSASToken := ""
expectedErr := status.Errorf(codes.Internal, fmt.Sprintf("failed to generate sas token in creating new shared key credential, accountName: %s, err: %s", "accountName", "decode account key: illegal base64 data at input byte 8"))
accountSASToken, err := d.getSASToken(ctx, "accountName", "", "core.windows.net", &azure.AccountOptions{}, secrets, "secretsName", "secretsNamespace")
accountSASToken, _, err := d.getAzcopyAuth(context.Background(), "accountName", "", "core.windows.net", &azure.AccountOptions{}, secrets, "secretsName", "secretsNamespace")
if !reflect.DeepEqual(err, expectedErr) || !reflect.DeepEqual(accountSASToken, expectedAccountSASToken) {
t.Errorf("Unexpected accountSASToken: %s, Unexpected error: %v", accountSASToken, err)
}
Expand All @@ -2100,7 +2035,7 @@ func TestGetSASToken(t *testing.T) {
ctx := context.Background()
expectedAccountSASToken := ""
expectedErr := status.Errorf(codes.Internal, fmt.Sprintf("failed to generate sas token in creating new shared key credential, accountName: %s, err: %s", "accountName", "decode account key: illegal base64 data at input byte 8"))
accountSASToken, err := d.getSASToken(ctx, "accountName", "", "core.windows.net", &azure.AccountOptions{}, secrets, "secretsName", "secretsNamespace")
accountSASToken, _, err := d.getAzcopyAuth(ctx, "accountName", "", "core.windows.net", &azure.AccountOptions{}, secrets, "secretsName", "secretsNamespace")
if !reflect.DeepEqual(err, expectedErr) || !reflect.DeepEqual(accountSASToken, expectedAccountSASToken) {
t.Errorf("Unexpected accountSASToken: %s, Unexpected error: %v", accountSASToken, err)
}
Expand Down

0 comments on commit f2d6459

Please sign in to comment.