Update filesystem to chmod the data dir and enforce gid limits

Signed-off-by: joshvanl <[email protected]>

Author: JoshVanL

storage/filesystem.go

@@ -59,7 +59,7 @@ type Filesystem struct {
 	// files, gid ownership of the volume's data directory will be changed to
 	// the value. Attribute value must be a valid int64 value.
 	// If FixedFSGroup is defined, this field has no effect.
-	FSGroupVolumeAttributeKey *string
+	FSGroupVolumeAttributeKey string
 }
 
 // Ensure the Filesystem implementation is fully featured
@@ -194,7 +194,7 @@ func (f *Filesystem) RegisterMetadata(meta metadata.Metadata) (bool, error) {
 // to a custom gid.
 func (f *Filesystem) WriteFiles(meta metadata.Metadata, files map[string][]byte) error {
 	// Data directory should be read, write and execute only to the fs user; read and executable to group
-	if err := os.MkdirAll(f.dataPathForVolumeID(meta.VolumeID), 0750); err != nil {
+	if err := os.MkdirAll(f.dataPathForVolumeID(meta.VolumeID), 0550); err != nil {
 		return err
 	}
 
@@ -215,8 +215,28 @@ func (f *Filesystem) WriteFiles(meta metadata.Metadata, files map[string][]byte)
 		return err
 	}
 
-	payload := makePayload(files, fsGroup)
-	return writer.Write(payload)
+	payload := makePayload(files)
+	if err := writer.Write(payload); err != nil {
+		return err
+	}
+
+	// If a fsGroup is defined, Chown all files within the data directory.
+	if fsGroup != nil {
+		dirName := f.dataPathForVolumeID(meta.VolumeID)
+		entries, err := os.ReadDir(dirName)
+		if err != nil {
+			return fmt.Errorf("failed to list files in data directory: %w", err)
+		}
+
+		for _, entry := range entries {
+			// Set the uid to -1 which means don't change ownership in Go.
+			if err := os.Chown(filepath.Join(dirName, entry.Name()), -1, int(*fsGroup)); err != nil {
+				return err
+			}
+		}
+	}
+
+	return nil
 }
 
 // ReadFile reads the named file within the volume's data directory.
@@ -252,13 +272,12 @@ func (f *Filesystem) tempfsPath() string {
 	return filepath.Join(f.baseDir, "inmemfs")
 }
 
-func makePayload(in map[string][]byte, fsGroup *int64) map[string]util.FileProjection {
+func makePayload(in map[string][]byte) map[string]util.FileProjection {
 	out := make(map[string]util.FileProjection, len(in))
 	for name, data := range in {
 		out[name] = util.FileProjection{
-			Data:    data,
-			FsGroup: fsGroup,
-			Mode:    readOnlyUserAndGroupFileMode,
+			Data: data,
+			Mode: readOnlyUserAndGroupFileMode,
 		}
 	}
 	return out
@@ -274,19 +293,27 @@ func (f *Filesystem) fsGroupForMetadata(meta metadata.Metadata) (*int64, error)
 	}
 
 	// If the FSGroupVolumeAttributeKey is not defined, no ownership can change.
-	if f.FSGroupVolumeAttributeKey == nil {
+	if len(f.FSGroupVolumeAttributeKey) == 0 {
 		return nil, nil
 	}
 
-	fsGroupStr, ok := meta.VolumeContext[*f.FSGroupVolumeAttributeKey]
+	fsGroupStr, ok := meta.VolumeContext[f.FSGroupVolumeAttributeKey]
 	if !ok {
 		// If the attribute has not been set, return no ownership change.
 		return nil, nil
 	}
 
 	fsGroup, err := strconv.ParseInt(fsGroupStr, 10, 64)
 	if err != nil {
-		return nil, fmt.Errorf("failed to parse %q, value must be a valid integer: %w", *f.FSGroupVolumeAttributeKey, err)
+		return nil, fmt.Errorf("failed to parse %q, value must be a valid integer: %w", f.FSGroupVolumeAttributeKey, err)
+	}
+
+	// fsGroup has to be between 1 and 4294967295 inclusive. 4294967295 is the
+	// largest gid number on most modern operating systems. If the actual maximum
+	// is smaller on the running machine, then we will simply error later during
+	// the Chmod.
+	if fsGroup <= 0 || fsGroup > 4294967295 {
+		return nil, fmt.Errorf("%q: gid value must be greater than 0 and less than 4294967295: %d", f.FSGroupVolumeAttributeKey, fsGroup)
 	}
 
 	return &fsGroup, nil

storage/filesystem_test.go

@@ -122,51 +122,75 @@ func Test_fsGroupForMetadata(t *testing.T) {
 	intPtr := func(i int64) *int64 {
 		return &i
 	}
-	strPtr := func(s string) *string {
-		return &s
-	}
 
 	tests := map[string]struct {
 		fixedFSGroup              *int64
-		fsGroupVolumeAttributeKey *string
+		fsGroupVolumeAttributeKey string
 		volumeContext             map[string]string
 
 		expGID *int64
 		expErr bool
 	}{
-		"FixedFSGroup=nil FSGroupVolumeAttributeKey=nil, should return nil gid": {
+		"FixedFSGroup=nil FSGroupVolumeAttributeKey='', should return nil gid": {
 			fixedFSGroup:              nil,
-			fsGroupVolumeAttributeKey: nil,
+			fsGroupVolumeAttributeKey: "",
 			volumeContext:             map[string]string{},
 			expGID:                    nil,
 			expErr:                    false,
 		},
-		"FixedFSGroup=10 FSGroupVolumeAttributeKey=nil, should return 10": {
+		"FixedFSGroup=10 FSGroupVolumeAttributeKey='', should return 10": {
 			fixedFSGroup:              intPtr(10),
-			fsGroupVolumeAttributeKey: nil,
+			fsGroupVolumeAttributeKey: "",
 			volumeContext:             map[string]string{},
 			expGID:                    intPtr(10),
 			expErr:                    false,
 		},
 		"FixedFSGroup=nil FSGroupVolumeAttributeKey=defined but not present in context, should return nil": {
 			fixedFSGroup:              nil,
-			fsGroupVolumeAttributeKey: strPtr("fs-gid"),
+			fsGroupVolumeAttributeKey: "fs-gid",
 			volumeContext:             map[string]string{},
 			expGID:                    nil,
 			expErr:                    false,
 		},
 		"FixedFSGroup=nil FSGroupVolumeAttributeKey=defined and present in context, should return 20": {
 			fixedFSGroup:              nil,
-			fsGroupVolumeAttributeKey: strPtr("fs-gid"),
+			fsGroupVolumeAttributeKey: "fs-gid",
 			volumeContext: map[string]string{
 				"fs-gid": "20",
 			},
 			expGID: intPtr(20),
 			expErr: false,
 		},
+		"FixedFSGroup=nil FSGroupVolumeAttributeKey=defined and present in context but value of 0, should error": {
+			fixedFSGroup:              nil,
+			fsGroupVolumeAttributeKey: "fs-gid",
+			volumeContext: map[string]string{
+				"fs-gid": "0",
+			},
+			expGID: nil,
+			expErr: true,
+		},
+		"FixedFSGroup=nil FSGroupVolumeAttributeKey=defined and present in context but value of -1, should error": {
+			fixedFSGroup:              nil,
+			fsGroupVolumeAttributeKey: "fs-gid",
+			volumeContext: map[string]string{
+				"fs-gid": "-1",
+			},
+			expGID: nil,
+			expErr: true,
+		},
+		"FixedFSGroup=nil FSGroupVolumeAttributeKey=defined and present in context but value greater than the max gid, should error": {
+			fixedFSGroup:              nil,
+			fsGroupVolumeAttributeKey: "fs-gid",
+			volumeContext: map[string]string{
+				"fs-gid": "4294967296",
+			},
+			expGID: nil,
+			expErr: true,
+		},
 		"FixedFSGroup=nil FSGroupVolumeAttributeKey=defined and present in context but with bad value, should return error": {
 			fixedFSGroup:              nil,
-			fsGroupVolumeAttributeKey: strPtr("fs-gid"),
+			fsGroupVolumeAttributeKey: "fs-gid",
 			volumeContext: map[string]string{
 				"fs-gid": "bad-value",
 			},
@@ -175,7 +199,7 @@ func Test_fsGroupForMetadata(t *testing.T) {
 		},
 		"FixedFSGroup=10 FSGroupVolumeAttributeKey=defined and present in context, should return superseding FixedFSGroup (10)": {
 			fixedFSGroup:              intPtr(10),
-			fsGroupVolumeAttributeKey: strPtr("fs-gid"),
+			fsGroupVolumeAttributeKey: "fs-gid",
 			volumeContext: map[string]string{
 				"fs-gid": "20",
 			},