From 179d09fdd5a6bd34d6ad2cf41d7f1599976ac6df Mon Sep 17 00:00:00 2001 From: Jose Diaz-Gonzalez Date: Sat, 2 May 2026 05:37:11 -0400 Subject: [PATCH] fix: chown migrated legacy storage entries to dokku `SaveEntry` and `touchMigrationFlag` wrote files via `os.WriteFile` and `os.Create` and never chowned them, so the install-time legacy-mount migration produced root-owned `legacy-*.json` files in `/var/lib/dokku/data/storage-registry/entries/`. The dokku user that runs `ps:rebuild` could not read them and every rebuild on a 0.37.x to 0.38.0 upgrade failed with permission denied. The companion `repairRegistryOwnership` pass in `TriggerInstall` rewrites ownership across the whole registry tree so installs that already ran the buggy code are fixed on the next package upgrade, since the per-app migration flag would otherwise cause `MigrateLegacyMounts` to skip the broken files forever. --- plugins/storage/entry.go | 7 ++-- plugins/storage/entry_test.go | 42 +++++++++++++++++++++++ plugins/storage/migrate.go | 6 +--- plugins/storage/migrate_test.go | 58 ++++++++++++++++++++++---------- plugins/storage/triggers.go | 30 +++++++++++++++++ plugins/storage/triggers_test.go | 40 ++++++++++++++++++++++ tests/unit/storage.bats | 9 +++++ 7 files changed, 168 insertions(+), 24 deletions(-) create mode 100644 plugins/storage/triggers_test.go diff --git a/plugins/storage/entry.go b/plugins/storage/entry.go index 470ac3781..0bb5dad73 100644 --- a/plugins/storage/entry.go +++ b/plugins/storage/entry.go @@ -148,8 +148,11 @@ func SaveEntry(entry *Entry) error { return fmt.Errorf("unable to encode storage entry %q: %w", entry.Name, err) } - path := entryPath(entry.Name) - if err := os.WriteFile(path, data, 0600); err != nil { + if err := common.WriteBytesToFile(common.WriteBytesToFileInput{ + Bytes: data, + Filename: entryPath(entry.Name), + Mode: 0600, + }); err != nil { return fmt.Errorf("unable to write storage entry %q: %w", entry.Name, err) } return nil diff --git a/plugins/storage/entry_test.go b/plugins/storage/entry_test.go index d317452dc..a09fae334 100644 --- a/plugins/storage/entry_test.go +++ b/plugins/storage/entry_test.go @@ -1,17 +1,36 @@ package storage import ( + "os" + "os/user" "path/filepath" + "strconv" "strings" + "syscall" "testing" . "github.com/onsi/gomega" ) +// withTempLibRoot points DOKKU_LIB_ROOT at a temp dir and overrides the +// permission helpers' target user/group to the current process user so +// SetPermissions is a no-op chown rather than a hard failure on dev and +// CI machines that lack a dokku system user. func withTempLibRoot(t *testing.T) string { t.Helper() dir := t.TempDir() t.Setenv("DOKKU_LIB_ROOT", dir) + + current, err := user.Current() + if err != nil { + t.Fatalf("user.Current: %v", err) + } + group, err := user.LookupGroupId(current.Gid) + if err != nil { + t.Fatalf("user.LookupGroupId: %v", err) + } + t.Setenv("DOKKU_SYSTEM_USER", current.Username) + t.Setenv("DOKKU_SYSTEM_GROUP", group.Name) return dir } @@ -203,3 +222,26 @@ func TestLegacyMountToEntry(t *testing.T) { Expect(abs.HostPath).To(Equal("/host/path")) Expect(abs.Validate()).To(Succeed()) } + +// TestSaveEntryChownsToSystemUser is a regression test for #8557: the +// install trigger runs as root, so SaveEntry must chown the file to the +// dokku user before ps:rebuild can read it. +func TestSaveEntryChownsToSystemUser(t *testing.T) { + RegisterTestingT(t) + withTempLibRoot(t) + + Expect(SaveEntry(&Entry{ + Name: "demo-data", + Scheduler: SchedulerDockerLocal, + HostPath: "/data/demo", + })).To(Succeed()) + + info, err := os.Stat(entryPath("demo-data")) + Expect(err).NotTo(HaveOccurred()) + stat, ok := info.Sys().(*syscall.Stat_t) + Expect(ok).To(BeTrue()) + + current, err := user.Current() + Expect(err).NotTo(HaveOccurred()) + Expect(strconv.Itoa(int(stat.Uid))).To(Equal(current.Uid)) +} diff --git a/plugins/storage/migrate.go b/plugins/storage/migrate.go index 62fae8d8c..07cb087a4 100644 --- a/plugins/storage/migrate.go +++ b/plugins/storage/migrate.go @@ -168,11 +168,7 @@ func migrateMount(appName string, mount string, phases []string) error { } func touchMigrationFlag(appName string) error { - f, err := os.Create(migrationFlagFile(appName)) - if err != nil { - return err - } - return f.Close() + return common.TouchFile(migrationFlagFile(appName)) } func filterMountLines(lines []string) []string { diff --git a/plugins/storage/migrate_test.go b/plugins/storage/migrate_test.go index 41188e246..415b51666 100644 --- a/plugins/storage/migrate_test.go +++ b/plugins/storage/migrate_test.go @@ -5,36 +5,26 @@ import ( "os/user" "path/filepath" "sort" + "strconv" + "syscall" "testing" "github.com/dokku/dokku/plugins/common" dockeroptions "github.com/dokku/dokku/plugins/docker-options" ) -// setupMigrationEnv points DOKKU_LIB_ROOT and DOKKU_ROOT at temp dirs and -// tells the permission helpers to chown to the current user (a no-op), -// so the test works without root and without the dokku system user. -// Mirrors the helper in docker-options/migration_test.go. +// setupMigrationEnv reuses withTempLibRoot for DOKKU_LIB_ROOT plus the +// permission-helper user/group overrides, then layers DOKKU_ROOT and +// PLUGIN_PATH on top so DokkuApps can find the staged apps. Mirrors the +// docker-options migration test fixture. func setupMigrationEnv(t *testing.T) (libRoot string, dokkuRoot string) { t.Helper() - libRoot = t.TempDir() + libRoot = withTempLibRoot(t) dokkuRoot = t.TempDir() - t.Setenv("DOKKU_LIB_ROOT", libRoot) t.Setenv("DOKKU_ROOT", dokkuRoot) t.Setenv("PLUGIN_PATH", filepath.Join(libRoot, "plugins")) - current, err := user.Current() - if err != nil { - t.Fatalf("user.Current: %v", err) - } - group, err := user.LookupGroupId(current.Gid) - if err != nil { - t.Fatalf("user.LookupGroupId: %v", err) - } - t.Setenv("DOKKU_SYSTEM_USER", current.Username) - t.Setenv("DOKKU_SYSTEM_GROUP", group.Name) - // MigrateLegacyMounts creates the flag dir before iterating apps; // migrateApp (the per-app helper) assumes it already exists. Pre- // create it so tests that call migrateApp directly don't trip. @@ -300,6 +290,40 @@ func TestMigrateAppRefusesEntryNameCollision(t *testing.T) { } } +// TestMigrateAppChownsLegacyEntryFiles is the regression for #8557: +// migrating from the install trigger (which runs as root) must produce +// dokku-owned legacy-*.json files. setupMigrationEnv overrides the +// system user/group to the current process user, so we assert against +// that uid rather than the literal dokku one. +func TestMigrateAppChownsLegacyEntryFiles(t *testing.T) { + _, dokkuRoot := setupMigrationEnv(t) + stageApp(t, dokkuRoot, "alpha", map[string][]string{ + "deploy": {"-v /var/log:/log"}, + }) + + if err := migrateApp("alpha"); err != nil { + t.Fatalf("migrateApp: %v", err) + } + + entry := LegacyMountToEntry("/var/log:/log") + info, err := os.Stat(entryPath(entry.Name)) + if err != nil { + t.Fatalf("stat entry: %v", err) + } + stat, ok := info.Sys().(*syscall.Stat_t) + if !ok { + t.Fatalf("stat.Sys is not *syscall.Stat_t") + } + + current, err := user.Current() + if err != nil { + t.Fatalf("user.Current: %v", err) + } + if got := strconv.Itoa(int(stat.Uid)); got != current.Uid { + t.Errorf("entry file uid = %s, want %s", got, current.Uid) + } +} + func equalSorted(a, b []string) bool { if len(a) != len(b) { return false diff --git a/plugins/storage/triggers.go b/plugins/storage/triggers.go index 270b00812..278b723cf 100644 --- a/plugins/storage/triggers.go +++ b/plugins/storage/triggers.go @@ -50,6 +50,10 @@ func TriggerInstall() error { } } + if err := repairRegistryOwnership(); err != nil { + return fmt.Errorf("Unable to repair storage registry ownership: %s", err.Error()) + } + if err := MigrateLegacyMounts(); err != nil { return fmt.Errorf("storage migration failed: %w", err) } @@ -73,6 +77,32 @@ func TriggerInstall() error { return nil } +// repairRegistryOwnership rewrites ownership on every regular file under +// the registry tree so the dokku user can read entry / migration-flag +// files. Earlier 0.38 builds wrote those files as root because SaveEntry +// and touchMigrationFlag bypassed the permission helpers; on an upgrade +// the per-app migration flag short-circuits MigrateLegacyMounts so a +// plain SaveEntry fix would leave the broken files in place. The walk is +// idempotent: calling SetPermissions on a file already owned by the +// target user produces a no-op chown. +func repairRegistryOwnership() error { + return filepath.Walk(RegistryDirectory(), func(path string, info os.FileInfo, err error) error { + if err != nil { + if os.IsNotExist(err) { + return nil + } + return err + } + if info.IsDir() { + return nil + } + return common.SetPermissions(common.SetPermissionInput{ + Filename: path, + Mode: info.Mode().Perm(), + }) + }) +} + // TriggerPostDelete removes the attachment store for an app that's being // destroyed. Entries are global and survive app deletion. The whole // per-app property folder goes away rather than being rewritten as diff --git a/plugins/storage/triggers_test.go b/plugins/storage/triggers_test.go new file mode 100644 index 000000000..b0f046e25 --- /dev/null +++ b/plugins/storage/triggers_test.go @@ -0,0 +1,40 @@ +package storage + +import ( + "os" + "os/user" + "strconv" + "syscall" + "testing" + + . "github.com/onsi/gomega" +) + +// TestRepairRegistryOwnership covers the install-time repair that exists +// to fix #8557 on systems that already ran the buggy 0.38.0 install. The +// repair must rewrite ownership without rewriting the file mode, and +// must tolerate a missing registry tree (clean install). +func TestRepairRegistryOwnership(t *testing.T) { + RegisterTestingT(t) + withTempLibRoot(t) + + Expect(EnsureEntriesDirectory()).To(Succeed()) + path := entryPath("legacy-deadbeef") + Expect(os.WriteFile(path, []byte(`{"name":"legacy-deadbeef"}`), 0640)).To(Succeed()) + + Expect(repairRegistryOwnership()).To(Succeed()) + + info, err := os.Stat(path) + Expect(err).NotTo(HaveOccurred()) + Expect(info.Mode().Perm()).To(Equal(os.FileMode(0640))) + + stat, ok := info.Sys().(*syscall.Stat_t) + Expect(ok).To(BeTrue()) + + current, err := user.Current() + Expect(err).NotTo(HaveOccurred()) + Expect(strconv.Itoa(int(stat.Uid))).To(Equal(current.Uid)) + + Expect(os.RemoveAll(RegistryDirectory())).To(Succeed()) + Expect(repairRegistryOwnership()).To(Succeed()) +} diff --git a/tests/unit/storage.bats b/tests/unit/storage.bats index 83af1fc59..9b2548755 100644 --- a/tests/unit/storage.bats +++ b/tests/unit/storage.bats @@ -359,6 +359,15 @@ teardown() { echo "status: $status" assert_success + # Regression for #8557: the legacy-*.json file must be readable by the + # dokku user. The migration runs as root via the install trigger, so + # without an explicit chown the file lands as root:root and ps:rebuild + # fails with permission denied. + legacy_entry_name=$(dokku storage:list-entries --format json | jq -r '.[] | select(.name | startswith("legacy-")) | .name' | head -1) + run /bin/bash -c "stat -c '%U:%G' /var/lib/dokku/data/storage-registry/entries/${legacy_entry_name}.json" + assert_success + assert_output "dokku:dokku" + # docker-options no longer holds the -v line on either phase. run /bin/bash -c "dokku docker-options:report $TEST_APP --docker-options-deploy" assert_success