diff --git a/storage/filesystem.go b/storage/filesystem.go index f82d736..c80ef05 100644 --- a/storage/filesystem.go +++ b/storage/filesystem.go @@ -34,6 +34,14 @@ import ( "github.com/cert-manager/csi-lib/third_party/util" ) +// These variables can be overridden by tests, this makes it easier to test +// the filesystem implementation as a non-root user. +var ( + parentFolderMode fs.FileMode = 0700 + volumeFolderMode fs.FileMode = 0644 + volumeDataFolderMode fs.FileMode = 0550 +) + const ( readWriteUserFileMode = 0600 readOnlyUserAndGroupFileMode = 0440 @@ -42,12 +50,9 @@ const ( type Filesystem struct { log logr.Logger - // baseDir is the absolute path to a directory used to store all metadata + // basePath is the absolute path to a directory used to store all metadata // about mounted volumes and mount points. - baseDir string - - // used by the 'read only' methods - fs fs.StatFS + basePath string // FixedFSGroup is an optional field which will set the gid ownership of all // volume's data directories to this value. @@ -65,30 +70,44 @@ type Filesystem struct { // Ensure the Filesystem implementation is fully featured var _ Interface = &Filesystem{} -func NewFilesystem(log logr.Logger, baseDir string) (*Filesystem, error) { - f := &Filesystem{ - log: log, - baseDir: baseDir, - // Use the rootfs as the DirFS so that paths passed to both read & - // write methods on this struct use a consistent root. - fs: os.DirFS("/").(fs.StatFS), +func NewFilesystem(log logr.Logger, basePath string) (*Filesystem, error) { + inmemfsPath := filepath.Join(basePath, "inmemfs") + + f, err := NewFilesystemOnDisk(log, inmemfsPath) + if err != nil { + return nil, err } - isMnt, err := mount.New("").IsMountPoint(f.tempfsPath()) + isMnt, err := mount.New("").IsMountPoint(inmemfsPath) if err != nil { - if !os.IsNotExist(err) { - return nil, err - } - if err := os.MkdirAll(f.tempfsPath(), 0700); err != nil { - return nil, err - } + return nil, err } if !isMnt { - if err := mount.New("").Mount("tmpfs", f.tempfsPath(), "tmpfs", []string{}); err != nil { + if err := mount.New("").Mount("tmpfs", inmemfsPath, "tmpfs", []string{}); err != nil { return nil, fmt.Errorf("mounting tmpfs: %w", err) } - log.Info("Mounted new tmpfs", "path", f.tempfsPath()) + log.Info("Mounted new tmpfs", "path", inmemfsPath) + } + + return f, nil +} + +// WARNING: should be used only for testing purposes only, as we don't want to store +// any sensitive data on disk in production. +func NewFilesystemOnDisk(log logr.Logger, basePath string) (*Filesystem, error) { + if !filepath.IsAbs(basePath) { + return nil, fmt.Errorf("baseDir must be an absolute path") + } + + f := &Filesystem{ + log: log, + basePath: basePath, + } + + // Create the base directory if it does not exist. + if err := os.MkdirAll(basePath, parentFolderMode); err != nil { + return nil, err } return f, nil @@ -99,18 +118,18 @@ func (f *Filesystem) PathForVolume(volumeID string) string { } func (f *Filesystem) RemoveVolume(volumeID string) error { - return os.RemoveAll(filepath.Join(f.tempfsPath(), volumeID)) + return os.RemoveAll(f.volumePath(volumeID)) } func (f *Filesystem) ListVolumes() ([]string, error) { - dirs, err := fs.ReadDir(f.fs, f.tempfsPath()) + dirs, err := os.ReadDir(f.basePath) if err != nil { return nil, fmt.Errorf("listing volumes: %w", err) } var vols []string for _, dir := range dirs { - _, err := f.fs.Stat(f.metadataPathForVolumeID(dir.Name())) + _, err := os.Stat(f.metadataPathForVolumeID(dir.Name())) switch { case errors.Is(err, fs.ErrNotExist): f.log.Info("Directory exists but does not contain a metadata file - deleting directory and its contents", "volume_id", dir.Name()) @@ -133,7 +152,7 @@ func (f *Filesystem) ListVolumes() ([]string, error) { // Errors wrapping ErrNotFound will be returned if metadata for the ID cannot // be found. func (f *Filesystem) ReadMetadata(volumeID string) (metadata.Metadata, error) { - file, err := f.fs.Open(f.metadataPathForVolumeID(volumeID)) + file, err := os.Open(f.metadataPathForVolumeID(volumeID)) if err != nil { // don't leak through error types from fs.Open - wrap with ErrNotFound // if calling Open fails, as this indicates an invalid path @@ -164,6 +183,11 @@ func (f *Filesystem) ReadMetadata(volumeID string) (metadata.Metadata, error) { } func (f *Filesystem) WriteMetadata(volumeID string, meta metadata.Metadata) error { + // Ensure the the volume directory exists. + if err := f.ensureVolumeDirectory(volumeID); err != nil { + return err + } + metaBytes, err := json.Marshal(meta) if err != nil { // if it's an error type we don't recognise, wrap it with %v to prevent @@ -177,11 +201,6 @@ func (f *Filesystem) WriteMetadata(volumeID string, meta metadata.Metadata) erro func (f *Filesystem) RegisterMetadata(meta metadata.Metadata) (bool, error) { existingMeta, err := f.ReadMetadata(meta.VolumeID) if errors.Is(err, ErrNotFound) { - // Ensure directory structure for the volume exists - if err := f.ensureVolumeDirectory(meta.VolumeID); err != nil { - return false, err - } - if err := f.WriteMetadata(meta.VolumeID, meta); err != nil { return false, err } @@ -191,13 +210,6 @@ func (f *Filesystem) RegisterMetadata(meta metadata.Metadata) (bool, error) { // If the volume context has changed, should write updated metadata if !apiequality.Semantic.DeepEqual(existingMeta.VolumeContext, meta.VolumeContext) { - // Ensure directory structure for the volume exists - this will probably do - // nothing, but it helps avoid any weird edge cases we could find ourselves in & - // is an inexpensive operation. - if err := f.ensureVolumeDirectory(meta.VolumeID); err != nil { - return false, err - } - f.log.WithValues("volume_id", meta.VolumeID).Info("volume context changed, updating file system metadata") existingMeta.VolumeContext = meta.VolumeContext if err := f.WriteMetadata(existingMeta.VolumeID, existingMeta); err != nil { @@ -213,29 +225,14 @@ func (f *Filesystem) RegisterMetadata(meta metadata.Metadata) (bool, error) { // ensureVolumeDirectory ensures the directory structure for the volume exists. // If the directories already exist, it will do nothing. func (f *Filesystem) ensureVolumeDirectory(volumeID string) error { - if err := os.MkdirAll(f.volumePath(volumeID), 0644); err != nil { - return err - } - - // Data directory should be read and execute only to the fs user and group. - if err := os.MkdirAll(f.dataPathForVolumeID(volumeID), 0550); err != nil { - return err - } - - return nil + return os.MkdirAll(f.volumePath(volumeID), volumeFolderMode) } // 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 { - // Ensure the full directory structure for the volume exists. - // This already happens in RegisterMetadata, however, when a driver starts up and reads - // the metadata files from the existing tmpfs to re-populate the manager, RegisterMetadata - // is not called again (it is only invoked by driver/nodeserver.go when a pod is first processed - // during NodePublishVolume). - // There is a very slim chance we could end out in a weird situation where the metadata - // file exists but the data directory does not, so re-run ensureVolumeDirectory just to be safe. + // Ensure the the volume directory exists. if err := f.ensureVolumeDirectory(meta.VolumeID); err != nil { return err } @@ -245,9 +242,14 @@ func (f *Filesystem) WriteFiles(meta metadata.Metadata, files map[string][]byte) return err } + // Data directory should be read and execute only to the fs user and group. + if err := os.MkdirAll(f.dataPathForVolumeID(meta.VolumeID), volumeDataFolderMode); 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 { + if err := os.Lchown(f.dataPathForVolumeID(meta.VolumeID), -1, int(*fsGroup)); err != nil { return fmt.Errorf("failed to chown data dir to gid %v: %w", *fsGroup, err) } } @@ -266,7 +268,7 @@ func (f *Filesystem) WriteFiles(meta metadata.Metadata, files map[string][]byte) // If a fsGroup is defined, Chown all files in the timestamped directory. for filename := range files { // Set the uid to -1 which means don't change ownership in Go. - if err := os.Chown(filepath.Join(f.dataPathForVolumeID(meta.VolumeID), tsDirName, filename), -1, int(*fsGroup)); err != nil { + if err := os.Lchown(filepath.Join(f.dataPathForVolumeID(meta.VolumeID), tsDirName, filename), -1, int(*fsGroup)); err != nil { return err } } @@ -282,7 +284,7 @@ func (f *Filesystem) WriteFiles(meta metadata.Metadata, files map[string][]byte) // ReadFile reads the named file within the volume's data directory. func (f *Filesystem) ReadFile(volumeID, name string) ([]byte, error) { - file, err := f.fs.Open(filepath.Join(f.dataPathForVolumeID(volumeID), name)) + file, err := os.Open(filepath.Join(f.dataPathForVolumeID(volumeID), name)) if err != nil { // don't leak through error types from fs.Open - wrap with ErrNotFound // if calling Open fails, as this indicates an invalid path @@ -306,11 +308,7 @@ func (f *Filesystem) dataPathForVolumeID(id string) string { } func (f *Filesystem) volumePath(id string) string { - return filepath.Join(f.tempfsPath(), id) -} - -func (f *Filesystem) tempfsPath() string { - return filepath.Join(f.baseDir, "inmemfs") + return filepath.Join(f.basePath, id) } func makePayload(in map[string][]byte) map[string]util.FileProjection { diff --git a/storage/filesystem_test.go b/storage/filesystem_test.go index 45b5a7c..f95ce75 100644 --- a/storage/filesystem_test.go +++ b/storage/filesystem_test.go @@ -18,18 +18,44 @@ package storage import ( "errors" + "os" + "path/filepath" "reflect" + "syscall" "testing" - "testing/fstest" "github.com/cert-manager/csi-lib/metadata" + "github.com/go-logr/logr" + "k8s.io/utils/ptr" ) +func setupTestFolder(t *testing.T, files map[string][]byte) string { + t.Helper() + + testDir := t.TempDir() + for name, data := range files { + path := filepath.Join(testDir, name) + + if err := os.MkdirAll(filepath.Dir(path), 0755); err != nil { + t.Fatalf("failed to create directory %q: %v", filepath.Dir(path), err) + } + + if err := os.WriteFile(path, data, 0644); err != nil { + t.Fatalf("failed to write file %q: %v", name, err) + } + } + + return testDir +} + func TestFilesystem_ReadFile(t *testing.T) { - backend := &Filesystem{ - fs: fstest.MapFS{ - "inmemfs/fake-volume/data/file": &fstest.MapFile{Data: []byte("hello world")}, - }, + folder := setupTestFolder(t, map[string][]byte{ + "fake-volume/data/file": []byte("hello world"), + }) + + backend, err := NewFilesystemOnDisk(logr.Discard(), folder) + if err != nil { + t.Fatalf("failed to create filesystem: %v", err) } d, err := backend.ReadFile("fake-volume", "file") @@ -41,48 +67,135 @@ func TestFilesystem_ReadFile(t *testing.T) { } } +func TestFilesystem_WriteFiles(t *testing.T) { + parentFolderMode = 0755 + volumeFolderMode = 0755 + volumeDataFolderMode = 0755 + + folder := setupTestFolder(t, map[string][]byte{}) + + backend, err := NewFilesystemOnDisk(logr.Discard(), folder) + if err != nil { + t.Fatalf("failed to create filesystem: %v", err) + } + + if err := backend.WriteFiles(metadata.Metadata{ + VolumeID: "fake-volume", + }, map[string][]byte{ + "file": []byte("hello world"), + }); err != nil { + t.Errorf("unexpected error: %v", err) + } + + d, err := backend.ReadFile("fake-volume", "file") + if err != nil { + t.Errorf("unexpected error: %v", err) + } + if string(d) != "hello world" { + t.Errorf("expected contents 'hello world' but got: %v", string(d)) + } +} + +func TestFilesystem_WriteFiles_with_FixedFSGroup(t *testing.T) { + parentFolderMode = 0755 + volumeFolderMode = 0755 + volumeDataFolderMode = 0755 + + folder := setupTestFolder(t, map[string][]byte{}) + + backend, err := NewFilesystemOnDisk(logr.Discard(), folder) + if err != nil { + t.Fatalf("failed to create filesystem: %v", err) + } + + backend.FixedFSGroup = ptr.To(int64(1000)) + + if err := backend.WriteFiles(metadata.Metadata{ + VolumeID: "fake-volume", + }, map[string][]byte{ + "file": []byte("hello world"), + }); err != nil { + t.Errorf("unexpected error: %v", err) + } + + d, err := backend.ReadFile("fake-volume", "file") + if err != nil { + t.Errorf("unexpected error: %v", err) + } + if string(d) != "hello world" { + t.Errorf("expected contents 'hello world' but got: %v", string(d)) + } + + // Check the file has the correct group + info, err := os.Stat(filepath.Join(folder, "fake-volume", "data", "file")) + if err != nil { + t.Fatalf("failed to stat file: %v", err) + } + + stat, ok := info.Sys().(*syscall.Stat_t) + if !ok { + t.Fatalf("failed to get syscall.Stat_t from file info") + } + + if stat.Gid != 1000 { + t.Errorf("expected file to have GID 1000 but got: %d", stat.Gid) + } +} + func TestFilesystem_ReadFile_NotFound(t *testing.T) { - backend := &Filesystem{ - fs: fstest.MapFS{ - "inmemfs/fake-volume/data/file": &fstest.MapFile{Data: []byte("hello world")}, - }, + folder := setupTestFolder(t, map[string][]byte{ + "fake-volume/data/file": []byte("hello world"), + }) + + backend, err := NewFilesystemOnDisk(logr.Discard(), folder) + if err != nil { + t.Fatalf("failed to create filesystem: %v", err) } - _, err := backend.ReadFile("fake-volume", "file2") + _, err = backend.ReadFile("fake-volume", "file2") if !errors.Is(err, ErrNotFound) { t.Errorf("expected %v but got: %v", ErrNotFound, err) } } func TestFilesystem_MetadataForVolume_NotFound(t *testing.T) { - backend := &Filesystem{ - fs: fstest.MapFS{}, + folder := setupTestFolder(t, map[string][]byte{}) + + backend, err := NewFilesystemOnDisk(logr.Discard(), folder) + if err != nil { + t.Fatalf("failed to create filesystem: %v", err) } - _, err := backend.ReadMetadata("fake-volume") + _, err = backend.ReadMetadata("fake-volume") if !errors.Is(err, ErrNotFound) { t.Errorf("expected %v but got: %v", ErrNotFound, err) } } func TestFilesystem_MetadataForVolume_InvalidJSON(t *testing.T) { - backend := &Filesystem{ - fs: fstest.MapFS{ - "inmemfs/fake-volume/metadata.json": &fstest.MapFile{Data: []byte("{")}, - }, + folder := setupTestFolder(t, map[string][]byte{ + "fake-volume/metadata.json": []byte("{"), + }) + + backend, err := NewFilesystemOnDisk(logr.Discard(), folder) + if err != nil { + t.Fatalf("failed to create filesystem: %v", err) } - _, err := backend.ReadMetadata("fake-volume") + _, err = backend.ReadMetadata("fake-volume") if !errors.Is(err, ErrInvalidJSON) { t.Errorf("expected %v but got: %v", ErrInvalidJSON, err) } } func TestFilesystem_MetadataForVolume(t *testing.T) { - backend := &Filesystem{ - fs: fstest.MapFS{ - "inmemfs/fake-volume/metadata.json": &fstest.MapFile{Data: []byte(`{"volumeID": "fake-volume", "targetPath": "/fake-volume", "volumeContext": {"a": "b"}}`)}, - }, + folder := setupTestFolder(t, map[string][]byte{ + "fake-volume/metadata.json": []byte(`{"volumeID": "fake-volume", "targetPath": "/fake-volume", "volumeContext": {"a": "b"}}`), + }) + + backend, err := NewFilesystemOnDisk(logr.Discard(), folder) + if err != nil { + t.Fatalf("failed to create filesystem: %v", err) } meta, err := backend.ReadMetadata("fake-volume") @@ -100,10 +213,13 @@ func TestFilesystem_MetadataForVolume(t *testing.T) { } func TestFilesystem_ListVolumes(t *testing.T) { - backend := &Filesystem{ - fs: fstest.MapFS{ - "inmemfs/fake-volume/metadata.json": &fstest.MapFile{Data: []byte{}}, - }, + folder := setupTestFolder(t, map[string][]byte{ + "fake-volume/metadata.json": {}, + }) + + backend, err := NewFilesystemOnDisk(logr.Discard(), folder) + if err != nil { + t.Fatalf("failed to create filesystem: %v", err) } vols, err := backend.ListVolumes() @@ -119,11 +235,14 @@ func TestFilesystem_ListVolumes(t *testing.T) { } func TestFilesystem_ListVolumes_CleansUpCorruptVolumes(t *testing.T) { - backend := &Filesystem{ - fs: fstest.MapFS{ - "inmemfs/fake-volume/metadata.json": &fstest.MapFile{Data: []byte{}}, - "inmemfs/fake-emptyvolume/nothing": &fstest.MapFile{Data: []byte{}}, - }, + folder := setupTestFolder(t, map[string][]byte{ + "fake-volume/metadata.json": {}, + "fake-emptyvolume/nothing": {}, + }) + + backend, err := NewFilesystemOnDisk(logr.Discard(), folder) + if err != nil { + t.Fatalf("failed to create filesystem: %v", err) } vols, err := backend.ListVolumes()