Merge pull request #12 from JoshVanL/storage-write-files-fsUser

Allow storage implementations to optionally specifiy an fsGroup GID when writing files.

Author: jetstack-bot

deploy/example/example-app.yaml

@@ -51,6 +51,9 @@ spec:
       labels:
         app: my-csi-app
     spec:
+      securityContext:
+        runAsGroup: 1000
+        runAsUser: 2000
       containers:
         - name: my-frontend
           image: busybox
@@ -67,3 +70,4 @@ spec:
                   csi.cert-manager.io/issuer-name: ca-issuer
                   csi.cert-manager.io/dns-names: my-service.sandbox.svc.cluster.local
                   csi.cert-manager.io/uri-sans: spiffe://my-service.sandbox.cluster.local
+                  csi.cert-manager.io/fs-group: "1000"

example/main.go

@@ -64,6 +64,10 @@ const (
 
 	RenewBeforeKey  string = "csi.cert-manager.io/renew-before"
 	ReusePrivateKey string = "csi.cert-manager.io/reuse-private-key"
+
+	// fs-group is used to optionally set the GID ownership of the volume's
+	// files. Useful when running containers with a specified user and group.
+	FsGroupKey string = "csi.cert-manager.io/fs-group"
 )
 
 var (
@@ -97,6 +101,8 @@ func main() {
 		panic("failed to setup filesystem: " + err.Error())
 	}
 
+	store.FSGroupVolumeAttributeKey = FsGroupKey
+
 	d, err := driver.New(*endpoint, log, driver.Options{
 		DriverName:    "csi.cert-manager.io",
 		DriverVersion: "v0.0.1",
@@ -250,7 +256,7 @@ func (w *writer) writeKeypair(meta metadata.Metadata, key crypto.PrivateKey, cha
 		return fmt.Errorf("calculating next issuance time: %w", err)
 	}
 
-	if err := w.store.WriteFiles(meta.VolumeID, map[string][]byte{
+	if err := w.store.WriteFiles(meta, map[string][]byte{
 		pkFile:  keyPEM,
 		crtFile: chain,
 		caFile:  ca,

storage/filesystem.go

@@ -24,6 +24,7 @@ import (
 	"io/ioutil"
 	"os"
 	"path/filepath"
+	"strconv"
 
 	"github.com/go-logr/logr"
 	apiequality "k8s.io/apimachinery/pkg/api/equality"
@@ -47,6 +48,18 @@ type Filesystem struct {
 
 	// used by the 'read only' methods
 	fs fs.FS
+
+	// FixedFSGroup is an optional field which will set the gid ownership of all
+	// volume's data directories to this value.
+	// If this value is set, FSGroupVolumeAttributeKey has no effect.
+	FixedFSGroup *int64
+
+	// FSGroupVolumeAttributeKey is an optional well-known key in the volume
+	// attributes. If this attribute is present in the context when writing
+	// 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
 }
 
 // Ensure the Filesystem implementation is fully featured
@@ -176,18 +189,53 @@ func (f *Filesystem) RegisterMetadata(meta metadata.Metadata) (bool, error) {
 	return false, nil
 }
 
-func (f *Filesystem) WriteFiles(volumeID string, files map[string][]byte) error {
-	if err := os.MkdirAll(f.dataPathForVolumeID(volumeID), 0644); err != nil {
+// WriteFiles writes the given data to filesystem files within the volume's
+// data directory. Filesystem supports changing ownership of the data directory
+// to a custom gid.
+func (f *Filesystem) WriteFiles(meta metadata.Metadata, files map[string][]byte) error {
+	// Data directory should be read and execute only to the fs user and group.
+	if err := os.MkdirAll(f.dataPathForVolumeID(meta.VolumeID), 0550); err != nil {
 		return err
 	}
 
-	writer, err := util.NewAtomicWriter(f.dataPathForVolumeID(volumeID), fmt.Sprintf("volumeID %v", volumeID))
+	fsGroup, err := f.fsGroupForMetadata(meta)
+	if err != nil {
+		return err
+	}
+
+	// If a fsGroup is defined, Chown the directory to that group.
+	if fsGroup != nil {
+		if err := os.Chown(f.dataPathForVolumeID(meta.VolumeID), -1, int(*fsGroup)); err != nil {
+			return fmt.Errorf("failed to chown data dir to gid %v: %w", *fsGroup, err)
+		}
+	}
+
+	writer, err := util.NewAtomicWriter(f.dataPathForVolumeID(meta.VolumeID), fmt.Sprintf("volumeID %v", meta.VolumeID))
 	if err != nil {
 		return err
 	}
 
 	payload := makePayload(files)
-	return writer.Write(payload)
+	if err := writer.Write(payload); err != nil {
+		return err
+	}
+
+	// If a fsGroup is defined, Chown all files just written.
+	if fsGroup != nil {
+		// "..data" is the well-known location where the atomic writer links to the
+		// latest directory containing the files just written. Chown these real
+		// files.
+		dirName := filepath.Join(f.dataPathForVolumeID(meta.VolumeID), "..data")
+
+		for filename := range files {
+			// Set the uid to -1 which means don't change ownership in Go.
+			if err := os.Chown(filepath.Join(dirName, filename), -1, int(*fsGroup)); err != nil {
+				return err
+			}
+		}
+	}
+
+	return nil
 }
 
 // ReadFile reads the named file within the volume's data directory.
@@ -228,9 +276,44 @@ func makePayload(in map[string][]byte) map[string]util.FileProjection {
 	for name, data := range in {
 		out[name] = util.FileProjection{
 			Data: data,
-			// read-only for user + group (TODO: set fsUser/fsGroup)
 			Mode: readOnlyUserAndGroupFileMode,
 		}
 	}
 	return out
 }
+
+// fsGroupForMetadata returns the gid that ownership of the volume data
+// directory should be changed to. Returns nil if ownership should not be
+// changed.
+func (f *Filesystem) fsGroupForMetadata(meta metadata.Metadata) (*int64, error) {
+	// FixedFSGroup takes precedence over attribute key.
+	if f.FixedFSGroup != nil {
+		return f.FixedFSGroup, nil
+	}
+
+	// If the FSGroupVolumeAttributeKey is not defined, no ownership can change.
+	if len(f.FSGroupVolumeAttributeKey) == 0 {
+		return nil, nil
+	}
+
+	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)
+	}
+
+	// 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 Chown.
+	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

@@ -117,3 +117,115 @@ func TestFilesystem_ListVolumes(t *testing.T) {
 		t.Errorf("expected only entry to be 'fake-volume' but got: %s", vols[0])
 	}
 }
+
+func Test_fsGroupForMetadata(t *testing.T) {
+	intPtr := func(i int64) *int64 {
+		return &i
+	}
+
+	tests := map[string]struct {
+		fixedFSGroup              *int64
+		fsGroupVolumeAttributeKey string
+		volumeContext             map[string]string
+
+		expGID *int64
+		expErr bool
+	}{
+		"FixedFSGroup=nil FSGroupVolumeAttributeKey='', should return nil gid": {
+			fixedFSGroup:              nil,
+			fsGroupVolumeAttributeKey: "",
+			volumeContext:             map[string]string{},
+			expGID:                    nil,
+			expErr:                    false,
+		},
+		"FixedFSGroup=10 FSGroupVolumeAttributeKey='', should return 10": {
+			fixedFSGroup:              intPtr(10),
+			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: "fs-gid",
+			volumeContext:             map[string]string{},
+			expGID:                    nil,
+			expErr:                    false,
+		},
+		"FixedFSGroup=nil FSGroupVolumeAttributeKey=defined and present in context, should return 20": {
+			fixedFSGroup:              nil,
+			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: "fs-gid",
+			volumeContext: map[string]string{
+				"fs-gid": "bad-value",
+			},
+			expGID: nil,
+			expErr: true,
+		},
+		"FixedFSGroup=10 FSGroupVolumeAttributeKey=defined and present in context, should return superseding FixedFSGroup (10)": {
+			fixedFSGroup:              intPtr(10),
+			fsGroupVolumeAttributeKey: "fs-gid",
+			volumeContext: map[string]string{
+				"fs-gid": "20",
+			},
+			expGID: intPtr(10),
+			expErr: false,
+		},
+	}
+
+	for name, test := range tests {
+		t.Run(name, func(t *testing.T) {
+			f := Filesystem{
+				FixedFSGroup:              test.fixedFSGroup,
+				FSGroupVolumeAttributeKey: test.fsGroupVolumeAttributeKey,
+			}
+
+			gid, err := f.fsGroupForMetadata(metadata.Metadata{
+				VolumeContext: test.volumeContext,
+			})
+
+			if (err != nil) != test.expErr {
+				t.Errorf("unexpected error, exp=%t got=%v", test.expErr, err)
+			}
+
+			if !reflect.DeepEqual(gid, test.expGID) {
+				t.Errorf("unexpected gid, exp=%v got=%v", test.expGID, gid)
+			}
+		})
+	}
+}

storage/interfaces.go

@@ -71,5 +71,5 @@ type MetadataWriter interface {
 // DataWriter is used to write data (e.g. certificate and private keys) to the
 // storage backend.
 type DataWriter interface {
-	WriteFiles(volumeID string, files map[string][]byte) error
+	WriteFiles(meta metadata.Metadata, files map[string][]byte) error
 }

storage/memory.go

@@ -126,10 +126,10 @@ func (m *MemoryFS) RegisterMetadata(meta metadata.Metadata) (bool, error) {
 	return true, nil
 }
 
-func (m *MemoryFS) WriteFiles(volumeID string, files map[string][]byte) error {
+func (m *MemoryFS) WriteFiles(meta metadata.Metadata, files map[string][]byte) error {
 	m.lock.Lock()
 	defer m.lock.Unlock()
-	vol, ok := m.files[volumeID]
+	vol, ok := m.files[meta.VolumeID]
 	if !ok {
 		return ErrNotFound
 	}

test/integration/issuance_test.go

@@ -57,7 +57,7 @@ func TestIssuesCertificate(t *testing.T) {
 			return []byte{}, nil
 		},
 		WriteKeypair: func(meta metadata.Metadata, key crypto.PrivateKey, chain []byte, ca []byte) error {
-			store.WriteFiles(meta.VolumeID, map[string][]byte{
+			store.WriteFiles(meta, map[string][]byte{
 				"ca":   ca,
 				"cert": chain,
 			})
@@ -106,6 +106,7 @@ func TestIssuesCertificate(t *testing.T) {
 func TestManager_CleansUpOldRequests(t *testing.T) {
 	store := storage.NewMemoryFS()
 	clock := fakeclock.NewFakeClock(time.Now())
+
 	opts, cl, stop := testutil.RunTestDriver(t, testutil.DriverOptions{
 		Store:                store,
 		Clock:                clock,
@@ -117,7 +118,7 @@ func TestManager_CleansUpOldRequests(t *testing.T) {
 			}, nil
 		},
 		WriteKeypair: func(meta metadata.Metadata, key crypto.PrivateKey, chain []byte, ca []byte) error {
-			store.WriteFiles(meta.VolumeID, map[string][]byte{
+			store.WriteFiles(meta, map[string][]byte{
 				"ca":   ca,
 				"cert": chain,
 			})