diff --git a/internal/config/migrations/003relativescheduling.go b/internal/config/migrations/003relativescheduling.go index 574ea018..546368fc 100644 --- a/internal/config/migrations/003relativescheduling.go +++ b/internal/config/migrations/003relativescheduling.go @@ -5,7 +5,7 @@ import ( "go.uber.org/zap" ) -var migration003RelativeScheduling = func(config *v1.Config) { +var migration003RelativeScheduling = func(config *v1.Config) error { zap.L().Info("applying config migration 003: relative scheduling") // loop over plans and examine prune policy's for _, repo := range config.Repos { @@ -22,4 +22,5 @@ var migration003RelativeScheduling = func(config *v1.Config) { schedule.Clock = v1.Schedule_CLOCK_LAST_RUN_TIME } } + return nil } diff --git a/internal/config/migrations/004repoguid.go b/internal/config/migrations/004repoguid.go index b55f2f0c..bd9aded3 100644 --- a/internal/config/migrations/004repoguid.go +++ b/internal/config/migrations/004repoguid.go @@ -7,7 +7,7 @@ import ( v1 "github.com/garethgeorge/backrest/gen/go/v1" ) -var migration004RepoGuid = func(config *v1.Config) { +var migration004RepoGuid = func(config *v1.Config) error { for _, repo := range config.Repos { if repo.Guid != "" { continue @@ -16,4 +16,5 @@ var migration004RepoGuid = func(config *v1.Config) { h.Write([]byte(repo.Id)) repo.Guid = hex.EncodeToString(h.Sum(nil)) } + return nil } diff --git a/internal/config/migrations/005checkrepopasswords.go b/internal/config/migrations/005checkrepopasswords.go new file mode 100644 index 00000000..bd0f3fc5 --- /dev/null +++ b/internal/config/migrations/005checkrepopasswords.go @@ -0,0 +1,127 @@ +package migrations + +import ( + "fmt" + "os" + "strings" + + v1 "github.com/garethgeorge/backrest/gen/go/v1" + "go.uber.org/zap" +) + +var conflictingEnvVars = []string{ + "RESTIC_PASSWORD", + "RESTIC_PASSWORD_FILE", + "RESTIC_PASSWORD_COMMAND", +} + +// migration005CheckRepoPasswords detects repos whose restic password may have +// been set incorrectly due to a bug fixed in this release +// (https://github.com/garethgeorge/backrest/issues/1139). +// +// The bug: RESTIC_PASSWORD, RESTIC_PASSWORD_FILE, and RESTIC_PASSWORD_COMMAND +// inherited from the process environment previously took precedence over the +// password configured in Backrest's UI, because WithEnviron() was appended +// after the config password. Repos initialized or used under those conditions +// were encrypted with the env var's password, not the one the user entered. +// +// On first run after upgrading, if any repo has a config password AND one of +// the above env vars is set, this migration returns an error and refuses to +// start, printing instructions for the user. +// +// If ISSUE_1139_FIX_PASSWORDS=1 is set in the environment, the migration +// instead applies the following automatic fixes to each affected repo and +// writes the corrected config to disk before startup continues: +// +// - RESTIC_PASSWORD: the env var's value is written into repo.Password, +// replacing the (likely wrong) value the user had entered in the UI. +// +// - RESTIC_PASSWORD_FILE or RESTIC_PASSWORD_COMMAND: repo.Password is +// cleared and the env var (name=value) is appended to repo.Env, so +// restic continues to resolve the password via the same mechanism it +// was using before. The env var can then be removed from the Backrest +// process environment, as the password source is now explicit in the +// repo config. +// +// After this migration runs successfully (whether or not a fix was needed) +// the config version is bumped and it will not run again on future starts. +var migration005CheckRepoPasswords = func(config *v1.Config) error { + // Find repos that have a password configured in the UI. + var affectedRepos []*v1.Repo + for _, repo := range config.Repos { + if repo.GetPassword() != "" { + affectedRepos = append(affectedRepos, repo) + } + } + if len(affectedRepos) == 0 { + return nil + } + + // Check which conflicting env vars are set. + var setEnvVars []string + for _, envVar := range conflictingEnvVars { + if os.Getenv(envVar) != "" { + setEnvVars = append(setEnvVars, envVar) + } + } + if len(setEnvVars) == 0 { + return nil + } + + // There is a potential conflict. Check if the user has acknowledged it. + if os.Getenv("ISSUE_1139_FIX_PASSWORDS") != "" { + for _, repo := range affectedRepos { + for _, envVar := range setEnvVars { + val := os.Getenv(envVar) + switch envVar { + case "RESTIC_PASSWORD": + zap.S().Warnf("repo %q: overwriting config password with value of RESTIC_PASSWORD env var (issue 1139 fix)", repo.Id) + repo.Password = val + case "RESTIC_PASSWORD_FILE", "RESTIC_PASSWORD_COMMAND": + zap.S().Warnf("repo %q: clearing config password and adding %s=%s to repo env (issue 1139 fix)", repo.Id, envVar, val) + repo.Password = "" + repo.Env = append(repo.Env, envVar+"="+val) + } + } + } + return nil + } + + // Block startup with a detailed error message. + var b strings.Builder + b.WriteString("IMPORTANT: Backrest detected a potential password conflict affecting your restic repos (see https://github.com/garethgeorge/backrest/issues/1139).\n") + b.WriteString("\n") + b.WriteString("What happened:\n") + b.WriteString(" Backrest was using the wrong password for restic repos. The environment variables listed\n") + b.WriteString(" below were inherited by Backrest's process and previously took precedence over the password\n") + b.WriteString(" you configured in the UI. Your repos may have been encrypted with a different password than\n") + b.WriteString(" what your Backrest config says.\n") + b.WriteString("\n") + b.WriteString("Conflicting environment variables found:\n") + for _, envVar := range setEnvVars { + fmt.Fprintf(&b, " - %s\n", envVar) + } + b.WriteString("\n") + b.WriteString("Potentially affected repos:\n") + for _, repo := range affectedRepos { + fmt.Fprintf(&b, " - %s\n", repo.Id) + } + b.WriteString("\n") + b.WriteString("How to fix:\n") + b.WriteString(" Rerun Backrest with ISSUE_1139_FIX_PASSWORDS=1 set in the environment.\n") + b.WriteString(" Backrest will automatically update each affected repo's config as follows,\n") + b.WriteString(" then write the corrected config to disk and start normally:\n") + b.WriteString("\n") + b.WriteString(" RESTIC_PASSWORD: the env var's value is written into the repo's\n") + b.WriteString(" password field, replacing the value entered in the UI.\n") + b.WriteString("\n") + b.WriteString(" RESTIC_PASSWORD_FILE / the repo's password field is cleared and the env var\n") + b.WriteString(" RESTIC_PASSWORD_COMMAND: (name=value) is appended to the repo's env list, so\n") + b.WriteString(" restic continues to resolve the password the same way.\n") + b.WriteString("\n") + b.WriteString(" Once the fix has been applied you can remove the conflicting environment variable\n") + b.WriteString(" from your Backrest process environment — the password source will be stored\n") + b.WriteString(" explicitly in the repo config from that point on.\n") + + return fmt.Errorf("%s", b.String()) +} diff --git a/internal/config/migrations/005checkrepopasswords_test.go b/internal/config/migrations/005checkrepopasswords_test.go new file mode 100644 index 00000000..e8b31825 --- /dev/null +++ b/internal/config/migrations/005checkrepopasswords_test.go @@ -0,0 +1,199 @@ +package migrations + +import ( + "strings" + "testing" + + v1 "github.com/garethgeorge/backrest/gen/go/v1" +) + +func TestMigration005CheckRepoPasswords(t *testing.T) { + // Cannot use t.Parallel() because subtests use t.Setenv. + + tests := []struct { + name string + config func() *v1.Config // factory so each run gets a fresh config + envVars map[string]string // env vars to set for the test + ackEnvVar bool // whether to set ISSUE_1139_FIX_PASSWORDS=1 + wantErr bool + errContains string // substring that must appear in the error + checkConfig func(*testing.T, *v1.Config) // optional post-migration assertions + }{ + { + name: "no repos with passwords - no conflict", + config: func() *v1.Config { + return &v1.Config{Repos: []*v1.Repo{{Id: "r", Uri: "/tmp/r"}}} + }, + envVars: map[string]string{"RESTIC_PASSWORD": "somevalue"}, + wantErr: false, + }, + { + name: "repos with passwords but no conflicting env vars - no conflict", + config: func() *v1.Config { + return &v1.Config{Repos: []*v1.Repo{{Id: "r", Uri: "/tmp/r", Password: "secret"}}} + }, + envVars: map[string]string{}, + wantErr: false, + }, + { + name: "RESTIC_PASSWORD conflict blocks startup without ack", + config: func() *v1.Config { + return &v1.Config{Repos: []*v1.Repo{{Id: "my-repo", Uri: "/tmp/r", Password: "wrong"}}} + }, + envVars: map[string]string{"RESTIC_PASSWORD": "envpassword"}, + wantErr: true, + errContains: "RESTIC_PASSWORD", + }, + { + name: "RESTIC_PASSWORD_FILE conflict blocks startup without ack", + config: func() *v1.Config { + return &v1.Config{Repos: []*v1.Repo{{Id: "r", Uri: "/tmp/r", Password: "wrong"}}} + }, + envVars: map[string]string{"RESTIC_PASSWORD_FILE": "/run/secrets/restic"}, + wantErr: true, + errContains: "RESTIC_PASSWORD_FILE", + }, + { + name: "RESTIC_PASSWORD_COMMAND conflict blocks startup without ack", + config: func() *v1.Config { + return &v1.Config{Repos: []*v1.Repo{{Id: "r", Uri: "/tmp/r", Password: "wrong"}}} + }, + envVars: map[string]string{"RESTIC_PASSWORD_COMMAND": "cat /secrets/pw"}, + wantErr: true, + errContains: "RESTIC_PASSWORD_COMMAND", + }, + { + name: "error message names the affected repo", + config: func() *v1.Config { + return &v1.Config{Repos: []*v1.Repo{{Id: "my-repo", Uri: "/tmp/r", Password: "wrong"}}} + }, + envVars: map[string]string{"RESTIC_PASSWORD": "envpassword"}, + wantErr: true, + errContains: "my-repo", + }, + { + name: "error message references ISSUE_1139_FIX_PASSWORDS", + config: func() *v1.Config { + return &v1.Config{Repos: []*v1.Repo{{Id: "r", Uri: "/tmp/r", Password: "wrong"}}} + }, + envVars: map[string]string{"RESTIC_PASSWORD": "envpassword"}, + wantErr: true, + errContains: "ISSUE_1139_FIX_PASSWORDS", + }, + { + name: "RESTIC_PASSWORD: ack inlines value into repo.Password", + config: func() *v1.Config { + return &v1.Config{Repos: []*v1.Repo{{Id: "r", Uri: "/tmp/r", Password: "wrong"}}} + }, + envVars: map[string]string{"RESTIC_PASSWORD": "real-password"}, + ackEnvVar: true, + wantErr: false, + checkConfig: func(t *testing.T, cfg *v1.Config) { + t.Helper() + if got := cfg.Repos[0].Password; got != "real-password" { + t.Errorf("expected repo.Password = %q, got %q", "real-password", got) + } + }, + }, + { + name: "RESTIC_PASSWORD_FILE: ack moves to repo.Env and clears Password", + config: func() *v1.Config { + return &v1.Config{Repos: []*v1.Repo{{Id: "r", Uri: "/tmp/r", Password: "wrong"}}} + }, + envVars: map[string]string{"RESTIC_PASSWORD_FILE": "/run/secrets/pw"}, + ackEnvVar: true, + wantErr: false, + checkConfig: func(t *testing.T, cfg *v1.Config) { + t.Helper() + repo := cfg.Repos[0] + if repo.Password != "" { + t.Errorf("expected repo.Password to be cleared, got %q", repo.Password) + } + wantEnv := "RESTIC_PASSWORD_FILE=/run/secrets/pw" + for _, e := range repo.Env { + if e == wantEnv { + return + } + } + t.Errorf("expected repo.Env to contain %q, got %v", wantEnv, repo.Env) + }, + }, + { + name: "RESTIC_PASSWORD_COMMAND: ack moves to repo.Env and clears Password", + config: func() *v1.Config { + return &v1.Config{Repos: []*v1.Repo{{Id: "r", Uri: "/tmp/r", Password: "wrong"}}} + }, + envVars: map[string]string{"RESTIC_PASSWORD_COMMAND": "cat /secrets/pw"}, + ackEnvVar: true, + wantErr: false, + checkConfig: func(t *testing.T, cfg *v1.Config) { + t.Helper() + repo := cfg.Repos[0] + if repo.Password != "" { + t.Errorf("expected repo.Password to be cleared, got %q", repo.Password) + } + wantEnv := "RESTIC_PASSWORD_COMMAND=cat /secrets/pw" + for _, e := range repo.Env { + if e == wantEnv { + return + } + } + t.Errorf("expected repo.Env to contain %q, got %v", wantEnv, repo.Env) + }, + }, + { + name: "only repos with passwords are considered affected", + config: func() *v1.Config { + return &v1.Config{Repos: []*v1.Repo{ + {Id: "no-pass", Uri: "/tmp/r1"}, + {Id: "with-pass", Uri: "/tmp/r2", Password: "wrong"}, + }} + }, + envVars: map[string]string{"RESTIC_PASSWORD": "envpassword"}, + wantErr: true, + errContains: "with-pass", + }, + { + name: "no repos at all - no conflict", + config: func() *v1.Config { + return &v1.Config{} + }, + envVars: map[string]string{"RESTIC_PASSWORD": "envpassword"}, + wantErr: false, + }, + } + + for _, tc := range tests { + tc := tc + t.Run(tc.name, func(t *testing.T) { + for k, v := range tc.envVars { + t.Setenv(k, v) + } + // Ensure conflicting env vars not in tc.envVars are unset. + for _, k := range conflictingEnvVars { + if _, ok := tc.envVars[k]; !ok { + t.Setenv(k, "") + } + } + if tc.ackEnvVar { + t.Setenv("ISSUE_1139_FIX_PASSWORDS", "1") + } else { + t.Setenv("ISSUE_1139_FIX_PASSWORDS", "") + } + + cfg := tc.config() + err := migration005CheckRepoPasswords(cfg) + if (err != nil) != tc.wantErr { + t.Errorf("migration005CheckRepoPasswords() error = %v, wantErr %v", err, tc.wantErr) + } + if tc.errContains != "" && err != nil { + if !strings.Contains(err.Error(), tc.errContains) { + t.Errorf("expected error to contain %q, got: %s", tc.errContains, err.Error()) + } + } + if tc.checkConfig != nil { + tc.checkConfig(t, cfg) + } + }) + } +} diff --git a/internal/config/migrations/migrations.go b/internal/config/migrations/migrations.go index 89a6fcf3..41a63b86 100644 --- a/internal/config/migrations/migrations.go +++ b/internal/config/migrations/migrations.go @@ -8,11 +8,12 @@ import ( "google.golang.org/protobuf/proto" ) -var migrations = []*func(*v1.Config){ +var migrations = []*func(*v1.Config) error{ &noop, // migration001PrunePolicy is deprecated &noop, // migration002Schedules is deprecated &migration003RelativeScheduling, &migration004RepoGuid, + &migration005CheckRepoPasswords, } var CurrentVersion = int32(len(migrations)) @@ -39,13 +40,16 @@ func ApplyMigrations(config *v1.Config) error { if m == &noop { return fmt.Errorf("config version %d is too old to migrate, please try first upgrading to backrest 1.4.0 which is the last version that may be compatible with your config", config.Version) } - (*m)(config) + if err := (*m)(config); err != nil { + return err + } } config.Version = CurrentVersion return nil } -var noop = func(config *v1.Config) { +var noop = func(config *v1.Config) error { // do nothing + return nil } diff --git a/internal/orchestrator/repo/repo_test.go b/internal/orchestrator/repo/repo_test.go index 35b03bab..42ccbe8a 100644 --- a/internal/orchestrator/repo/repo_test.go +++ b/internal/orchestrator/repo/repo_test.go @@ -371,6 +371,48 @@ func initRepoHelper(t *testing.T, config *v1.Config, repo *v1.Repo) *RepoOrchest return orchestrator } +// TestConfigPasswordPrecedence verifies that a password set in the Backrest +// repo config takes precedence over RESTIC_PASSWORD (and related env vars) set +// in the process environment. This is a regression test for +// https://github.com/garethgeorge/backrest/issues/1139. +func TestConfigPasswordPrecedence(t *testing.T) { + // Cannot use t.Parallel() because t.Setenv is used. + + repoDir := t.TempDir() + configPassword := "config-password" + wrongEnvPassword := "env-password-should-be-ignored" + + repo := &v1.Repo{ + Id: "test", + Uri: repoDir, + Password: configPassword, + Flags: []string{"--no-cache"}, + } + + // Initialize the repo using the config password. + orchestrator := initRepoHelper(t, configForTest, repo) + + // Now set RESTIC_PASSWORD in the environment to a *different* value. + // NewRepoOrchestrator should still use the config password, not this one. + t.Setenv("RESTIC_PASSWORD", wrongEnvPassword) + t.Setenv("RESTIC_PASSWORD_FILE", "") + t.Setenv("RESTIC_PASSWORD_COMMAND", "") + + // Re-create the orchestrator so it picks up the current environment. + orchestrator2, err := NewRepoOrchestrator(configForTest, repo, helpers.ResticBinary(t)) + if err != nil { + t.Fatalf("failed to create repo orchestrator: %v", err) + } + + // Listing snapshots exercises the repo unlock path and will fail with + // "wrong password" if the env var is incorrectly taking precedence. + if _, err := orchestrator2.Snapshots(context.Background()); err != nil { + t.Fatalf("Snapshots() failed — config password did not take precedence over RESTIC_PASSWORD env var: %v", err) + } + + _ = orchestrator // suppress unused warning +} + func TestRestoreAmbiguity(t *testing.T) { t.Parallel() repoDir := t.TempDir()