diff --git a/internal/storage/file.go b/internal/storage/file.go index 7bb74d1..83769a6 100644 --- a/internal/storage/file.go +++ b/internal/storage/file.go @@ -120,9 +120,10 @@ func (f *FileStorer) Delete(_ context.Context, key string) error { return nil } -// List returns all keys with the given prefix. It fails when the -// destination directory is missing; a missing prefix under it is an -// empty listing. +// List returns all keys with the given prefix, reading through symlinked +// directories. It fails when the destination directory is missing, or +// when the prefix or a directory above it is a symlink whose target is +// missing; a prefix that is truly missing is an empty listing. func (f *FileStorer) List(ctx context.Context, prefix string) ([]string, error) { var keys []string @@ -133,8 +134,7 @@ func (f *FileStorer) List(ctx context.Context, prefix string) ([]string, error) basePath := f.fullPath(prefix) - // Check if the prefix exists - exists, err := afero.Exists(f.fs, basePath) + exists, err := f.prefixExists(prefix) if err != nil { return nil, fmt.Errorf("checking path: %w", err) } @@ -143,7 +143,7 @@ func (f *FileStorer) List(ctx context.Context, prefix string) ([]string, error) return keys, nil // Empty list for non-existent prefix } - err = afero.Walk(f.fs, basePath, func(path string, info os.FileInfo, err error) error { + err = f.walk(basePath, func(path string, info os.FileInfo, err error) error { if err != nil { return err } @@ -176,7 +176,9 @@ func (f *FileStorer) List(ctx context.Context, prefix string) ([]string, error) } // ListStream returns a channel of ObjectInfo for large result sets. Like -// List, it sends an error when the destination directory is missing. +// List, it reads through symlinked directories and sends an error when +// the destination directory is missing, or when the prefix or a directory +// above it is a symlink whose target is missing. func (f *FileStorer) ListStream(ctx context.Context, prefix string) <-chan ObjectInfo { ch := make(chan ObjectInfo) @@ -192,8 +194,7 @@ func (f *FileStorer) ListStream(ctx context.Context, prefix string) <-chan Objec basePath := f.fullPath(prefix) - // Check if the prefix exists - exists, err := afero.Exists(f.fs, basePath) + exists, err := f.prefixExists(prefix) if err != nil { ch <- ObjectInfo{Err: fmt.Errorf("checking path: %w", err)} @@ -204,7 +205,7 @@ func (f *FileStorer) ListStream(ctx context.Context, prefix string) <-chan Objec return // Empty channel for non-existent prefix } - _ = afero.Walk(f.fs, basePath, func(path string, info os.FileInfo, err error) error { + _ = f.walk(basePath, func(path string, info os.FileInfo, err error) error { // Check context cancellation select { case <-ctx.Done(): @@ -243,11 +244,12 @@ func (f *FileStorer) ListStream(ctx context.Context, prefix string) <-chan Objec } // DeletePartialUploads removes every file under prefix whose name ends in -// tempSuffix. A missing prefix has none to remove. +// tempSuffix. Like List, it reads through symlinked directories and fails +// on one whose target is missing. A missing prefix has none to remove. func (f *FileStorer) DeletePartialUploads(ctx context.Context, prefix string) error { basePath := f.fullPath(prefix) - exists, err := afero.Exists(f.fs, basePath) + exists, err := f.prefixExists(prefix) if err != nil { return fmt.Errorf("checking path: %w", err) } @@ -256,7 +258,7 @@ func (f *FileStorer) DeletePartialUploads(ctx context.Context, prefix string) er return nil } - err = afero.Walk(f.fs, basePath, func(path string, info os.FileInfo, err error) error { + err = f.walk(basePath, func(path string, info os.FileInfo, err error) error { if err != nil { return err } @@ -352,6 +354,54 @@ func (f *FileStorer) writeAtomic( return nil } +// walk is afero.Walk, except that it reads through a symlinked directory +// the way Get and Stat do, where afero.Walk reports the link as one file. +// afero.Walk uses Lstat, which does not follow a link, whenever the +// filesystem has one; the struct below carries only the afero.Fs methods, +// so the walk uses Stat instead. A link back up the tree is followed until +// the operating system refuses the path for too many levels of links, and +// the walk then fails. +func (f *FileStorer) walk(root string, fn filepath.WalkFunc) error { + return afero.Walk(struct{ afero.Fs }{f.fs}, root, fn) +} + +// prefixExists reports whether prefix exists under the destination +// directory. It checks each directory on the way down to prefix in turn, +// and the first one Stat cannot find decides: if it is not there at all, +// prefix is missing; if it is a symlink whose target is missing, such as +// a link to a volume that is not mounted, prefixExists fails, since +// listing it as empty would let prune delete every blob. +func (f *FileStorer) prefixExists(prefix string) (bool, error) { + lstater, canLstat := f.fs.(afero.Lstater) + path := f.basePath + + for name := range strings.SplitSeq(prefix, "/") { + path = filepath.Join(path, name) + + _, err := f.fs.Stat(path) + if err == nil { + continue + } + + if !os.IsNotExist(err) { + return false, err + } + + // Lstat does not follow a symlink at path, so it finds the link + // that Stat could not follow. + if canLstat { + _, _, lstatErr := lstater.LstatIfPossible(path) + if lstatErr == nil { + return false, err + } + } + + return false, nil + } + + return true, nil +} + // fullPath returns the full filesystem path for a key. func (f *FileStorer) fullPath(key string) string { return filepath.Join(f.basePath, key) diff --git a/internal/storage/file_backend_test.go b/internal/storage/file_backend_test.go index f92331e..795ce77 100644 --- a/internal/storage/file_backend_test.go +++ b/internal/storage/file_backend_test.go @@ -4,7 +4,10 @@ import ( "context" "errors" "io/fs" + "os" "path/filepath" + "reflect" + "strings" "testing" "sneak.berlin/go/vaultik/internal/storage" @@ -60,3 +63,225 @@ func TestFileStorerListMissingDestination(t *testing.T) { t.Errorf("ListStream error = %v, want a not-exist error", streamErr) } } + +// moveBehindSymlink moves base/dir to a directory elsewhere and puts a +// symlink to it in its place. +func moveBehindSymlink(t *testing.T, base, dir string) { + t.Helper() + + linkPath := filepath.Join(base, dir) + movedPath := filepath.Join(t.TempDir(), dir) + + err := os.Rename(linkPath, movedPath) + if err != nil { + t.Fatalf("moving %s away: %v", dir, err) + } + + err = os.Symlink(movedPath, linkPath) + if err != nil { + t.Fatalf("linking %s: %v", dir, err) + } +} + +// TestFileStorerListsThroughSymlinkedDirectory checks that List and +// ListStream read through a symlinked metadata/ or blobs/ directory, as Get +// and Stat do, instead of listing the link as one key. Listing the whole +// destination meets the link part-way through the walk; listing its prefix +// starts the walk at the link. A prefix missing behind the link lists as +// empty. +func TestFileStorerListsThroughSymlinkedDirectory(t *testing.T) { + t.Parallel() + + for _, dir := range []string{"metadata", "blobs"} { + t.Run(dir, func(t *testing.T) { + t.Parallel() + + base := t.TempDir() + key := dir + "/aa/bb/object" + + s, err := storage.NewFileStorer(base) + if err != nil { + t.Fatalf("NewFileStorer: %v", err) + } + + err = s.Put(context.Background(), key, strings.NewReader("data")) + if err != nil { + t.Fatalf("Put: %v", err) + } + + moveBehindSymlink(t, base, dir) + + want := []string{key} + + for _, prefix := range []string{"", dir + "/"} { + got := listSorted(t, s, prefix) + if !reflect.DeepEqual(got, want) { + t.Errorf("List(%q) = %v, want %v", prefix, got, want) + } + + got = listStreamKeys(t, s, prefix) + if !reflect.DeepEqual(got, want) { + t.Errorf("ListStream(%q) = %v, want %v", prefix, got, want) + } + } + + absent := dir + "/absent/" + + if got := listSorted(t, s, absent); len(got) != 0 { + t.Errorf("List(%q) = %v, want empty", absent, got) + } + + if got := listStreamKeys(t, s, absent); len(got) != 0 { + t.Errorf("ListStream(%q) = %v, want empty", absent, got) + } + }) + } +} + +// TestFileStorerListFailsOnSymlinkWithMissingTarget checks that List and +// ListStream fail on a symlinked metadata/ or blobs/ directory whose target +// is missing, such as a link to a volume that is not mounted, as they do on +// a missing destination directory, instead of listing it as empty. That +// holds for a prefix below the link too. +func TestFileStorerListFailsOnSymlinkWithMissingTarget(t *testing.T) { + t.Parallel() + + ctx := context.Background() + + for _, dir := range []string{"metadata", "blobs"} { + t.Run(dir, func(t *testing.T) { + t.Parallel() + + base := t.TempDir() + + s, err := storage.NewFileStorer(base) + if err != nil { + t.Fatalf("NewFileStorer: %v", err) + } + + unmounted := filepath.Join(t.TempDir(), "unmounted") + + err = os.Symlink(unmounted, filepath.Join(base, dir)) + if err != nil { + t.Fatalf("Symlink: %v", err) + } + + for _, prefix := range []string{"", dir + "/", dir + "/aa/"} { + keys, err := s.List(ctx, prefix) + if !errors.Is(err, fs.ErrNotExist) { + t.Errorf("List(%q) = %v, %v; want a not-exist error", + prefix, keys, err) + } + + var streamErr error + + for object := range s.ListStream(ctx, prefix) { + if object.Err != nil { + streamErr = object.Err + } + } + + if !errors.Is(streamErr, fs.ErrNotExist) { + t.Errorf("ListStream(%q) error = %v, want a not-exist error", + prefix, streamErr) + } + } + }) + } +} + +// TestFileStorerListFailsOnSymlinkLoop checks that a symlink pointing back +// at the destination makes List and ListStream fail instead of walking it +// forever. +func TestFileStorerListFailsOnSymlinkLoop(t *testing.T) { + t.Parallel() + + ctx := context.Background() + base := t.TempDir() + + s, err := storage.NewFileStorer(base) + if err != nil { + t.Fatalf("NewFileStorer: %v", err) + } + + err = s.Put(ctx, testBlobKey, strings.NewReader("data")) + if err != nil { + t.Fatalf("Put: %v", err) + } + + err = os.Symlink(base, filepath.Join(base, "blobs", "loop")) + if err != nil { + t.Fatalf("Symlink: %v", err) + } + + keys, err := s.List(ctx, "blobs/") + if err == nil { + t.Errorf("List = %d keys and no error, want an error", len(keys)) + } + + var streamErr error + + for object := range s.ListStream(ctx, "blobs/") { + if object.Err != nil { + streamErr = object.Err + } + } + + if streamErr == nil { + t.Error("ListStream sent no error, want one") + } +} + +// TestFileStorerDeletePartialUploadsThroughSymlinkedDirectory checks that +// DeletePartialUploads, which remote nuke uses, removes a leftover temp +// file behind a symlinked metadata/ or blobs/ directory and keeps the +// object at the real key. +func TestFileStorerDeletePartialUploadsThroughSymlinkedDirectory(t *testing.T) { + t.Parallel() + + ctx := context.Background() + + for _, dir := range []string{"metadata", "blobs"} { + t.Run(dir, func(t *testing.T) { + t.Parallel() + + base := t.TempDir() + key := dir + "/aa/bb/object" + + s, err := storage.NewFileStorer(base) + if err != nil { + t.Fatalf("NewFileStorer: %v", err) + } + + err = s.Put(ctx, key, strings.NewReader("data")) + if err != nil { + t.Fatalf("Put: %v", err) + } + + leftover := filepath.Join(base, key+"-123456.partial") + + err = os.WriteFile(leftover, []byte("half"), 0o600) + if err != nil { + t.Fatalf("writing leftover temp file: %v", err) + } + + moveBehindSymlink(t, base, dir) + + err = s.DeletePartialUploads(ctx, dir+"/") + if err != nil { + t.Fatalf("DeletePartialUploads: %v", err) + } + + // leftover runs through the link. + _, err = os.Stat(leftover) + if !os.IsNotExist(err) { + t.Errorf("leftover temp file was not removed: %v", err) + } + + _, err = s.Stat(ctx, key) + if err != nil { + t.Errorf("Stat of the real key: %v", err) + } + }) + } +} diff --git a/internal/vaultik/symlinked_destination_test.go b/internal/vaultik/symlinked_destination_test.go new file mode 100644 index 0000000..934ca00 --- /dev/null +++ b/internal/vaultik/symlinked_destination_test.go @@ -0,0 +1,147 @@ +package vaultik_test + +import ( + "context" + "io/fs" + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "sneak.berlin/go/vaultik/internal/log" + "sneak.berlin/go/vaultik/internal/vaultik" +) + +// TestPruneDeletesNothingThroughSymlinkedDirectory checks that prune keeps +// every blob and the local snapshot record when the metadata/ or blobs/ +// directory of a file:// destination is a symlink to a directory elsewhere +// (https://git.eeqj.de/sneak/vaultik/issues/296). A backup writes through +// such a link, so prune has to list through it too. +func TestPruneDeletesNothingThroughSymlinkedDirectory(t *testing.T) { + log.Initialize(log.Config{}) + t.Parallel() + + for _, dir := range []string{"metadata", "blobs"} { + t.Run(dir, func(t *testing.T) { + t.Parallel() + + ctx := context.Background() + storeDir := filepath.Join(t.TempDir(), "store") + v, repos, _ := backUpToFileDestination(ctx, t, storeDir) + blobFiles := blobFilesIn(t, storeDir) + moveBehindSymlink(t, storeDir, dir) + + require.NoError(t, v.Prune(&vaultik.PruneOptions{Force: true})) + + // When blobs/ is the link, these paths run through it. + for _, blobFile := range blobFiles { + assert.FileExists(t, blobFile, "prune must delete no blob") + } + + snapshots, err := repos.Snapshots.ListRecent(ctx, listRecentTestLimit) + require.NoError(t, err) + assert.Len(t, snapshots, 1, "prune must delete no local snapshot record") + }) + } +} + +// TestPruneFailsOnSymlinkedDirectoryWithMissingTarget checks that prune +// fails, deleting no blob and keeping the local snapshot record, when the +// metadata/ or blobs/ directory of a file:// destination is a symlink +// whose target is missing, as a link to a volume that is not mounted is. +func TestPruneFailsOnSymlinkedDirectoryWithMissingTarget(t *testing.T) { + log.Initialize(log.Config{}) + t.Parallel() + + for _, dir := range []string{"metadata", "blobs"} { + t.Run(dir, func(t *testing.T) { + t.Parallel() + + ctx := context.Background() + storeDir := filepath.Join(t.TempDir(), "store") + v, repos, _ := backUpToFileDestination(ctx, t, storeDir) + blobFiles := blobFilesIn(t, storeDir) + targetPath := moveBehindSymlink(t, storeDir, dir) + + // Unmount the volume the link points to, and mount it again + // once prune has run. + unmountedPath := targetPath + "-unmounted" + require.NoError(t, os.Rename(targetPath, unmountedPath)) + + err := v.Prune(&vaultik.PruneOptions{Force: true}) + require.ErrorIs(t, err, fs.ErrNotExist) + + require.NoError(t, os.Rename(unmountedPath, targetPath)) + + // When blobs/ is the link, these paths run through it. + for _, blobFile := range blobFiles { + assert.FileExists(t, blobFile, "prune must delete no blob") + } + + snapshots, err := repos.Snapshots.ListRecent(ctx, listRecentTestLimit) + require.NoError(t, err) + assert.Len(t, snapshots, 1, "prune must delete no local snapshot record") + }) + } +} + +// TestRemoveSnapshotWarnsOnSymlinkedMetadataWithMissingTarget checks that +// snapshot remove warns that the metadata could not be removed from the +// destination when metadata/ is a symlink whose target is missing, as it +// does when the destination directory is missing, instead of finding no +// metadata to remove. +func TestRemoveSnapshotWarnsOnSymlinkedMetadataWithMissingTarget(t *testing.T) { + log.Initialize(log.Config{}) + t.Parallel() + + ctx := context.Background() + storeDir := filepath.Join(t.TempDir(), "store") + v, repos, out := backUpToFileDestination(ctx, t, storeDir) + targetPath := moveBehindSymlink(t, storeDir, "metadata") + require.NoError(t, os.Rename(targetPath, targetPath+"-unmounted")) + + result, err := v.RemoveSnapshot(localSnapshotID(ctx, t, repos, "first"), + &vaultik.RemoveOptions{Force: true}) + require.NoError(t, err) + + assert.False(t, result.RemoteRemoved) + assert.Contains(t, out.String(), + "Could not remove snapshot metadata from remote") + assert.NotContains(t, out.String(), + "Removed snapshot metadata from remote storage") +} + +// blobFilesIn returns the paths of the blob files under storeDir/blobs, and +// fails the test when there are none. +func blobFilesIn(t *testing.T, storeDir string) []string { + t.Helper() + + var blobFiles []string + + err := filepath.WalkDir(filepath.Join(storeDir, "blobs"), + func(path string, entry fs.DirEntry, err error) error { + if err == nil && !entry.IsDir() { + blobFiles = append(blobFiles, path) + } + + return err + }) + require.NoError(t, err) + require.NotEmpty(t, blobFiles) + + return blobFiles +} + +// moveBehindSymlink moves storeDir/dir to a directory elsewhere, puts a +// symlink to it in its place, and returns where it went. +func moveBehindSymlink(t *testing.T, storeDir, dir string) string { + t.Helper() + + linkPath := filepath.Join(storeDir, dir) + targetPath := filepath.Join(t.TempDir(), dir) + require.NoError(t, os.Rename(linkPath, targetPath)) + require.NoError(t, os.Symlink(targetPath, linkPath)) + + return targetPath +}