Secure SMB implementation for Ontap-Nas-Eco

Author: alloydsa

storage_drivers/ontap/ontap_common.go

@@ -4402,16 +4402,35 @@ func ConstructOntapNASQTreeVolumePath(
 		}
 	case sa.SMB:
 		var smbSharePath string
-		if smbShare != "" {
+		if smbShare != "" && !volConfig.SecureSMBEnabled {
 			smbSharePath = smbShare + tridentconfig.WindowsPathSeparator
 		}
+		// In Secure SMB mode, Trident creates a unique SMB share for each qtree. Therefore, we use internalName (the qtree name) to construct the SMB path, instead of flexvol.
 		if volConfig.ReadOnlyClone {
-			completeVolumePath = fmt.Sprintf("\\%s%s\\%s\\%s\\%s", smbSharePath, flexvol,
-				volConfig.CloneSourceVolumeInternal, "~snapshot", volConfig.CloneSourceSnapshot)
+			if volConfig.SecureSMBEnabled {
+				// Secure SMB, read-only clone:
+				// Use for SMB client access to a snapshot of a qtree.
+				// Example: \qtree1\~snapshot\snap1
+				completeVolumePath = fmt.Sprintf("\\%s\\%s\\%s", volConfig.InternalName, "~snapshot", volConfig.CloneSourceSnapshot)
+			} else {
+				// Non-secure SMB, read-only clone:
+				// Use for SMB client access to a snapshot of a qtree via a shared SMB path.
+				// Example: \share1\flexvol1\sourceQtree\~snapshot\snap1
+				completeVolumePath = fmt.Sprintf("\\%s%s\\%s\\%s\\%s", smbSharePath, flexvol, volConfig.CloneSourceVolumeInternal, "~snapshot", volConfig.CloneSourceSnapshot)
+			}
 		} else {
-			completeVolumePath = fmt.Sprintf("\\%s%s\\%s", smbSharePath, flexvol, volConfig.InternalName)
+			if volConfig.SecureSMBEnabled {
+				// Secure SMB :
+				// Use for SMB client access to a qtree.
+				// Example: \qtree1
+				completeVolumePath = fmt.Sprintf("\\%s", volConfig.InternalName)
+			} else {
+				// Non-secure SMB,:
+				// Use for SMB client access to a qtree via a shared SMB path.
+				// Example: \share1\flexvol1\qtree1
+				completeVolumePath = fmt.Sprintf("\\%s%s\\%s", smbSharePath, flexvol, volConfig.InternalName)
+			}
 		}
-
 		// Replace unix styled path separator, if exists
 		completeVolumePath = strings.Replace(completeVolumePath, tridentconfig.UnixPathSeparator,
 			tridentconfig.WindowsPathSeparator,
@@ -5007,3 +5026,11 @@ func purgeRecoveryQueueVolume(ctx context.Context, api api.OntapAPI, volumeName
 			volumeName).Errorf("error purging volume from ONTAP recovery queue: %v", err)
 	}
 }
+
+// getSMBShareNamePath constructs the SMB share name and path based on the provided parameters.
+func getSMBShareNamePath(flexvol, name string, secureSMBEnabled bool) (string, string) {
+	if secureSMBEnabled {
+		return name, "/" + flexvol + "/" + name
+	}
+	return flexvol, "/" + flexvol
+}

storage_drivers/ontap/ontap_common_test.go

@@ -2636,6 +2636,82 @@ func TestConstructOntapNASQTreeVolumePath(t *testing.T) {
 	}
 }
 
+func TestConstructOntapNASQTreeVolumePath_SecureSMBEnabled(t *testing.T) {
+	ctx := context.Background()
+
+	tests := []struct {
+		smbShare     string
+		flexvol      string
+		volConfig    *storage.VolumeConfig
+		protocol     string
+		expectedPath string
+	}{
+		{
+			"test_share",
+			"flex-vol",
+			&storage.VolumeConfig{
+				Name:                      "pvc-vol",
+				InternalName:              "trident_pvc_vol",
+				CloneSourceVolumeInternal: "cloneSourceInternal",
+				CloneSourceSnapshot:       "sourceSnapShot",
+				ReadOnlyClone:             false,
+				SecureSMBEnabled:          true,
+			},
+			sa.SMB,
+			"\\trident_pvc_vol",
+		},
+		{
+			"",
+			"flex-vol",
+			&storage.VolumeConfig{
+				Name:                      "volumeConfig",
+				InternalName:              "trident_pvc_vol",
+				CloneSourceVolumeInternal: "cloneSourceInternal",
+				CloneSourceSnapshot:       "sourceSnapShot",
+				ReadOnlyClone:             false,
+				SecureSMBEnabled:          true,
+			},
+			sa.SMB,
+			"\\trident_pvc_vol",
+		},
+		{
+			"test_share",
+			"flex-vol",
+			&storage.VolumeConfig{
+				Name:                      "volumeConfig",
+				InternalName:              "trident_pvc_vol",
+				CloneSourceVolumeInternal: "cloneSourceInternal",
+				CloneSourceSnapshot:       "sourceSnapShot",
+				ReadOnlyClone:             true,
+				SecureSMBEnabled:          true,
+			},
+			sa.SMB,
+			"\\trident_pvc_vol\\~snapshot\\sourceSnapShot",
+		},
+		{
+			"",
+			"flex-vol",
+			&storage.VolumeConfig{
+				Name:                      "volumeConfig",
+				InternalName:              "trident_pvc_vol",
+				CloneSourceVolumeInternal: "cloneSourceInternal",
+				CloneSourceSnapshot:       "sourceSnapShot",
+				ReadOnlyClone:             true,
+				SecureSMBEnabled:          true,
+			},
+			sa.SMB,
+			"\\trident_pvc_vol\\~snapshot\\sourceSnapShot",
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.smbShare, func(t *testing.T) {
+			result := ConstructOntapNASQTreeVolumePath(ctx, test.smbShare, "flex-vol", test.volConfig, test.protocol)
+			assert.Equal(t, test.expectedPath, result, "unable to construct Ontap-NAS-QTree SMB volume path")
+		})
+	}
+}
+
 func TestEnsureNodeAccess(t *testing.T) {
 	// Test 1 - Positive flow
 
@@ -9530,3 +9606,41 @@ func TestCloneASAvol(t *testing.T) {
 		})
 	}
 }
+
+func TestGetSMBShareNamePath(t *testing.T) {
+	// Define test cases
+	tests := []struct {
+		name             string
+		flexvol          string
+		inputName        string
+		secureSMBEnabled bool
+		expectedName     string
+		expectedPath     string
+	}{
+		{
+			name:             "secureSMB disabled, valid flexvol",
+			flexvol:          "vol1",
+			inputName:        "share1",
+			secureSMBEnabled: false,
+			expectedName:     "vol1",
+			expectedPath:     "/vol1",
+		},
+		{
+			name:             "secureSMB enabled, valid flexvol and name",
+			flexvol:          "vol1",
+			inputName:        "share1",
+			secureSMBEnabled: true,
+			expectedName:     "share1",
+			expectedPath:     "/vol1/share1",
+		},
+	}
+
+	// Run test cases
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			shareName, sharePath := getSMBShareNamePath(tt.flexvol, tt.inputName, tt.secureSMBEnabled)
+			assert.Equal(t, tt.expectedName, shareName, "shareName mismatch for flexvol=%q, name=%q, secureSMBEnabled=%v", tt.flexvol, tt.inputName, tt.secureSMBEnabled)
+			assert.Equal(t, tt.expectedPath, sharePath, "sharePath mismatch for flexvol=%q, name=%q, secureSMBEnabled=%v", tt.flexvol, tt.inputName, tt.secureSMBEnabled)
+		})
+	}
+}

storage_drivers/ontap/ontap_nas_qtree.go

@@ -391,6 +391,7 @@ func (d *NASQtreeStorageDriver) Create(
 		spaceReserve    = collection.GetV(opts, "spaceReserve", storagePool.InternalAttributes()[SpaceReserve])
 		snapshotPolicy  = collection.GetV(opts, "snapshotPolicy", storagePool.InternalAttributes()[SnapshotPolicy])
 		snapshotReserve = storagePool.InternalAttributes()[SnapshotReserve]
+		adAdminUser     = collection.GetV(opts, "adAdminUser", storagePool.InternalAttributes()[ADAdminUser])
 		snapshotDir     = collection.GetV(opts, "snapshotDir", storagePool.InternalAttributes()[SnapshotDir])
 		encryption      = collection.GetV(opts, "encryption", storagePool.InternalAttributes()[Encryption])
 
@@ -499,7 +500,14 @@ func (d *NASQtreeStorageDriver) Create(
 		}
 
 		if d.Config.NASType == sa.SMB {
-			if err = d.EnsureSMBShare(ctx, flexvol); err != nil {
+			if adAdminUser != "" {
+				if _, exists := volConfig.SMBShareACL[adAdminUser]; !exists {
+					volConfig.SMBShareACL[adAdminUser] = ADAdminUserPermission
+				}
+			}
+
+			shareName, sharePath := getSMBShareNamePath(flexvol, name, volConfig.SecureSMBEnabled)
+			if err = d.EnsureSMBShare(ctx, shareName, sharePath, volConfig.SMBShareACL, volConfig.SecureSMBEnabled); err != nil {
 				return err
 			}
 		}
@@ -513,7 +521,7 @@ func (d *NASQtreeStorageDriver) Create(
 
 // CreateClone creates a volume clone
 func (d *NASQtreeStorageDriver) CreateClone(
-	ctx context.Context, sourceVolConfig, cloneVolConfig *storage.VolumeConfig, _ storage.Pool,
+	ctx context.Context, sourceVolConfig, cloneVolConfig *storage.VolumeConfig, storagePool storage.Pool,
 ) error {
 	name := cloneVolConfig.InternalName
 	source := cloneVolConfig.CloneSourceVolumeInternal
@@ -548,6 +556,32 @@ func (d *NASQtreeStorageDriver) CreateClone(
 			return fmt.Errorf("snapshot directory access is set to %t and readOnly clone is set to %t ",
 				*storageVolume.SnapshotDir, cloneVolConfig.ReadOnlyClone)
 		}
+
+		// If the NAS type is SMB and Secure SMB is enabled, configure SMB share and ACLs for the clone.
+		if d.Config.NASType == sa.SMB && cloneVolConfig.SecureSMBEnabled {
+			adAdminUser := d.Config.ADAdminUser
+
+			// If a storage pool is set, prefer its AD admin user if available.
+			if !storage.IsStoragePoolUnset(storagePool) {
+				if spAdminUser := storagePool.InternalAttributes()[ADAdminUser]; spAdminUser != "" {
+					adAdminUser = spAdminUser
+				}
+			}
+
+			// Add the AD admin user to the SMB share ACL with the necessary permissions if not already present.
+			if adAdminUser != "" {
+				if _, exists := cloneVolConfig.SMBShareACL[adAdminUser]; !exists {
+					cloneVolConfig.SMBShareACL[adAdminUser] = ADAdminUserPermission
+				}
+			}
+
+			// Determine the SMB share path to create new SMB Share.
+			_, sharePath := getSMBShareNamePath(flexvol, source, cloneVolConfig.SecureSMBEnabled)
+			if err := d.EnsureSMBShare(ctx, name, sharePath, cloneVolConfig.SMBShareACL,
+				cloneVolConfig.SecureSMBEnabled); err != nil {
+				return err
+			}
+		}
 	} else {
 		return fmt.Errorf("cloning is not supported by backend type %s", d.Name())
 	}
@@ -575,14 +609,19 @@ func (d *NASQtreeStorageDriver) Destroy(ctx context.Context, volConfig *storage.
 		"name":   name,
 	}
 
-	// If it's a RO clone, no need to delete the volume
-	if volConfig.ReadOnlyClone {
-		return nil
-	}
-
 	Logd(ctx, d.Name(), d.Config.DebugTraceFlags["method"]).WithFields(fields).Trace(">>>> Destroy")
 	defer Logd(ctx, d.Name(), d.Config.DebugTraceFlags["method"]).WithFields(fields).Trace("<<<< Destroy")
 
+	// Handle ReadOnlyClone case early
+	if volConfig.ReadOnlyClone {
+		if d.Config.NASType == sa.SMB && volConfig.SecureSMBEnabled {
+			if err := d.DestroySMBShare(ctx, name); err != nil {
+				return err
+			}
+		}
+		return nil // Return early for all ReadOnlyClone cases
+	}
+
 	// Ensure the deleted qtree reaping job doesn't interfere with this workflow
 	locks.Lock(ctx, "destroy", d.sharedLockID)
 	defer locks.Unlock(ctx, "destroy", d.sharedLockID)
@@ -639,6 +678,12 @@ func (d *NASQtreeStorageDriver) Destroy(ctx context.Context, volConfig *storage.
 		return deleteError
 	}
 
+	if d.Config.NASType == sa.SMB && volConfig.SecureSMBEnabled {
+		if err := d.DestroySMBShare(ctx, name); err != nil {
+			return err
+		}
+	}
+
 	return nil
 }
 
@@ -2542,37 +2587,58 @@ func (d NASQtreeStorageDriver) getQtreesInPool(
 }
 
 // EnsureSMBShare ensures that required SMB share is made available.
-func (d *NASQtreeStorageDriver) EnsureSMBShare(
-	ctx context.Context, name string,
+func (d NASQtreeStorageDriver) EnsureSMBShare(
+	ctx context.Context, name, path string, smbShareACL map[string]string, secureSMBEnabled bool,
 ) error {
-	if d.Config.SMBShare != "" {
-		// If user did specify SMB share, and it does not exist, create an SMB share with the specified name.
-		share, err := d.API.SMBShareExists(ctx, d.Config.SMBShare)
-		if err != nil {
+	shareName := name
+	sharePath := path
+
+	// Determine share name and path based on secureSMBEnabled and configuration
+	if !secureSMBEnabled && d.Config.SMBShare != "" {
+		shareName = d.Config.SMBShare
+		sharePath = "/"
+	}
+
+	// Check if the share exists
+	if shareExists, err := d.API.SMBShareExists(ctx, shareName); err != nil {
+		return err
+	} else if !shareExists {
+		// Create the share if it does not exist
+		if err = d.API.SMBShareCreate(ctx, shareName, sharePath); err != nil {
 			return err
 		}
+	}
 
-		// If share is not present create it.
-		if !share {
-			if err := d.API.SMBShareCreate(ctx, d.Config.SMBShare, "/"); err != nil {
-				return err
-			}
-		}
-	} else {
-		// If user did not specify SMB share in backend configuration, create an SMB share with the name passed.
-		share, err := d.API.SMBShareExists(ctx, name)
-		if err != nil {
+	// If secure SMB is enabled, configure access control
+	if secureSMBEnabled {
+		// Delete the Everyone Access Control created by default during share creation by ONTAP
+		if err := d.API.SMBShareAccessControlDelete(ctx, shareName, smbShareDeleteACL); err != nil {
 			return err
 		}
 
-		// If share is not present create it.
-		if !share {
-			if err := d.API.SMBShareCreate(ctx, name, "/"+name); err != nil {
-				return err
-			}
+		if err := d.API.SMBShareAccessControlCreate(ctx, shareName, smbShareACL); err != nil {
+			return err
 		}
 	}
+	return nil
+}
 
+// DestroySMBShare destroys an SMB share
+func (d NASQtreeStorageDriver) DestroySMBShare(
+	ctx context.Context, name string,
+) error {
+	// If the share being deleted matches with the backend config, Trident will not delete the SMB share.
+	if d.Config.SMBShare == name {
+		return nil
+	}
+
+	if shareExists, err := d.API.SMBShareExists(ctx, name); err != nil {
+		return err
+	} else if shareExists {
+		if err := d.API.SMBShareDestroy(ctx, name); err != nil {
+			return err
+		}
+	}
 	return nil
 }