From d4b0d21199c6bdc2b44b1cdc7551af7928e8c623 Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Sat, 18 May 2024 18:25:09 +0200 Subject: [PATCH 1/3] key add/passwd: deduplicate options setup and remove globals The current pattern of using a global options variable is problematic. --- cmd/restic/cmd_key_add.go | 25 ++++++++++++++----------- cmd/restic/cmd_key_passwd.go | 14 +++++--------- 2 files changed, 19 insertions(+), 20 deletions(-) diff --git a/cmd/restic/cmd_key_add.go b/cmd/restic/cmd_key_add.go index 306754627..c87a99a5e 100644 --- a/cmd/restic/cmd_key_add.go +++ b/cmd/restic/cmd_key_add.go @@ -9,6 +9,7 @@ import ( "github.com/restic/restic/internal/errors" "github.com/restic/restic/internal/repository" "github.com/spf13/cobra" + "github.com/spf13/pflag" ) var cmdKeyAdd = &cobra.Command{ @@ -23,26 +24,28 @@ EXIT STATUS Exit status is 0 if the command is successful, and non-zero if there was any error. `, DisableAutoGenTag: true, - RunE: func(cmd *cobra.Command, args []string) error { - return runKeyAdd(cmd.Context(), globalOptions, keyAddOpts, args) - }, } type KeyAddOptions struct { - NewPasswordFile string - Username string - Hostname string + NewPasswordFile string + Username string + Hostname string } -var keyAddOpts KeyAddOptions +func (opts *KeyAddOptions) Add(flags *pflag.FlagSet) { + flags.StringVarP(&opts.NewPasswordFile, "new-password-file", "", "", "`file` from which to read the new password") + flags.StringVarP(&opts.Username, "user", "", "", "the username for new key") + flags.StringVarP(&opts.Hostname, "host", "", "", "the hostname for new key") +} func init() { cmdKey.AddCommand(cmdKeyAdd) - flags := cmdKeyAdd.Flags() - flags.StringVarP(&keyAddOpts.NewPasswordFile, "new-password-file", "", "", "`file` from which to read the new password") - flags.StringVarP(&keyAddOpts.Username, "user", "", "", "the username for new key") - flags.StringVarP(&keyAddOpts.Hostname, "host", "", "", "the hostname for new key") + var keyAddOpts KeyAddOptions + keyAddOpts.Add(cmdKeyAdd.Flags()) + cmdKeyAdd.RunE = func(cmd *cobra.Command, args []string) error { + return runKeyAdd(cmd.Context(), globalOptions, keyAddOpts, args) + } } func runKeyAdd(ctx context.Context, gopts GlobalOptions, opts KeyAddOptions, args []string) error { diff --git a/cmd/restic/cmd_key_passwd.go b/cmd/restic/cmd_key_passwd.go index 0836c4cfe..32822a0ba 100644 --- a/cmd/restic/cmd_key_passwd.go +++ b/cmd/restic/cmd_key_passwd.go @@ -22,24 +22,20 @@ EXIT STATUS Exit status is 0 if the command is successful, and non-zero if there was any error. `, DisableAutoGenTag: true, - RunE: func(cmd *cobra.Command, args []string) error { - return runKeyPasswd(cmd.Context(), globalOptions, keyPasswdOpts, args) - }, } type KeyPasswdOptions struct { KeyAddOptions } -var keyPasswdOpts KeyPasswdOptions - func init() { cmdKey.AddCommand(cmdKeyPasswd) - flags := cmdKeyPasswd.Flags() - flags.StringVarP(&keyPasswdOpts.NewPasswordFile, "new-password-file", "", "", "`file` from which to read the new password") - flags.StringVarP(&keyPasswdOpts.Username, "user", "", "", "the username for new key") - flags.StringVarP(&keyPasswdOpts.Hostname, "host", "", "", "the hostname for new key") + var keyPasswdOpts KeyPasswdOptions + keyPasswdOpts.KeyAddOptions.Add(cmdKeyPasswd.Flags()) + cmdKeyPasswd.RunE = func(cmd *cobra.Command, args []string) error { + return runKeyPasswd(cmd.Context(), globalOptions, keyPasswdOpts, args) + } } func runKeyPasswd(ctx context.Context, gopts GlobalOptions, opts KeyPasswdOptions, args []string) error { From 1d2277b4c32d8f2259d0960870497aed17751004 Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Sat, 18 May 2024 18:59:29 +0200 Subject: [PATCH 2/3] Add --insecure-no-password option This also includes two derived options `--from-insecure-no-password` used for commands that require specifying a source repository. And `--new-insecure-no-password` for the `key add` and `key passwd` commands. Specifying `--insecure-no-password` disabled the password prompt and immediately uses an empty password. Passing a password via CLI option or environment variable at the same time is an error. --- cmd/restic/cmd_backup.go | 2 +- cmd/restic/cmd_backup_integration_test.go | 14 +++++++ cmd/restic/cmd_copy_integration_test.go | 25 ++++++++++++- cmd/restic/cmd_key_add.go | 24 ++++++++++-- cmd/restic/cmd_key_integration_test.go | 39 ++++++++++++++++++++ cmd/restic/cmd_key_passwd.go | 2 +- cmd/restic/global.go | 45 ++++++++++++++--------- cmd/restic/global_test.go | 13 +++++++ cmd/restic/secondary_repo.go | 17 ++++++--- 9 files changed, 150 insertions(+), 31 deletions(-) diff --git a/cmd/restic/cmd_backup.go b/cmd/restic/cmd_backup.go index 19b96e9b0..4890f82ff 100644 --- a/cmd/restic/cmd_backup.go +++ b/cmd/restic/cmd_backup.go @@ -257,7 +257,7 @@ func readFilenamesRaw(r io.Reader) (names []string, err error) { // Check returns an error when an invalid combination of options was set. func (opts BackupOptions) Check(gopts GlobalOptions, args []string) error { - if gopts.password == "" { + if gopts.password == "" && !gopts.InsecureNoPassword { if opts.Stdin { return errors.Fatal("cannot read both password and data from stdin") } diff --git a/cmd/restic/cmd_backup_integration_test.go b/cmd/restic/cmd_backup_integration_test.go index 75de1341c..f7372851f 100644 --- a/cmd/restic/cmd_backup_integration_test.go +++ b/cmd/restic/cmd_backup_integration_test.go @@ -627,3 +627,17 @@ func TestStdinFromCommandFailNoOutputAndExitCode(t *testing.T) { testRunCheck(t, env.gopts) } + +func TestBackupEmptyPassword(t *testing.T) { + // basic sanity test that empty passwords work + env, cleanup := withTestEnvironment(t) + defer cleanup() + + env.gopts.password = "" + env.gopts.InsecureNoPassword = true + + testSetupBackupData(t, env) + testRunBackup(t, filepath.Dir(env.testdata), []string{"testdata"}, BackupOptions{}, env.gopts) + testListSnapshots(t, env.gopts, 1) + testRunCheck(t, env.gopts) +} diff --git a/cmd/restic/cmd_copy_integration_test.go b/cmd/restic/cmd_copy_integration_test.go index 1c8837690..704615870 100644 --- a/cmd/restic/cmd_copy_integration_test.go +++ b/cmd/restic/cmd_copy_integration_test.go @@ -13,10 +13,12 @@ func testRunCopy(t testing.TB, srcGopts GlobalOptions, dstGopts GlobalOptions) { gopts := srcGopts gopts.Repo = dstGopts.Repo gopts.password = dstGopts.password + gopts.InsecureNoPassword = dstGopts.InsecureNoPassword copyOpts := CopyOptions{ secondaryRepoOptions: secondaryRepoOptions{ - Repo: srcGopts.Repo, - password: srcGopts.password, + Repo: srcGopts.Repo, + password: srcGopts.password, + InsecureNoPassword: srcGopts.InsecureNoPassword, }, } @@ -134,3 +136,22 @@ func TestCopyUnstableJSON(t *testing.T) { testRunCheck(t, env2.gopts) testListSnapshots(t, env2.gopts, 1) } + +func TestCopyToEmptyPassword(t *testing.T) { + env, cleanup := withTestEnvironment(t) + defer cleanup() + env2, cleanup2 := withTestEnvironment(t) + defer cleanup2() + env2.gopts.password = "" + env2.gopts.InsecureNoPassword = true + + testSetupBackupData(t, env) + testRunBackup(t, "", []string{filepath.Join(env.testdata, "0", "0", "9")}, BackupOptions{}, env.gopts) + + testRunInit(t, env2.gopts) + testRunCopy(t, env.gopts, env2.gopts) + + testListSnapshots(t, env.gopts, 1) + testListSnapshots(t, env2.gopts, 1) + testRunCheck(t, env2.gopts) +} diff --git a/cmd/restic/cmd_key_add.go b/cmd/restic/cmd_key_add.go index c87a99a5e..9e50aa67d 100644 --- a/cmd/restic/cmd_key_add.go +++ b/cmd/restic/cmd_key_add.go @@ -28,12 +28,14 @@ Exit status is 0 if the command is successful, and non-zero if there was any err type KeyAddOptions struct { NewPasswordFile string + InsecureNoPassword bool Username string Hostname string } func (opts *KeyAddOptions) Add(flags *pflag.FlagSet) { flags.StringVarP(&opts.NewPasswordFile, "new-password-file", "", "", "`file` from which to read the new password") + flags.BoolVar(&opts.InsecureNoPassword, "new-insecure-no-password", false, "add an empty password for the repository (insecure)") flags.StringVarP(&opts.Username, "user", "", "", "the username for new key") flags.StringVarP(&opts.Hostname, "host", "", "", "the hostname for new key") } @@ -63,7 +65,7 @@ func runKeyAdd(ctx context.Context, gopts GlobalOptions, opts KeyAddOptions, arg } func addKey(ctx context.Context, repo *repository.Repository, gopts GlobalOptions, opts KeyAddOptions) error { - pw, err := getNewPassword(ctx, gopts, opts.NewPasswordFile) + pw, err := getNewPassword(ctx, gopts, opts.NewPasswordFile, opts.InsecureNoPassword) if err != nil { return err } @@ -86,19 +88,35 @@ func addKey(ctx context.Context, repo *repository.Repository, gopts GlobalOption // testKeyNewPassword is used to set a new password during integration testing. var testKeyNewPassword string -func getNewPassword(ctx context.Context, gopts GlobalOptions, newPasswordFile string) (string, error) { +func getNewPassword(ctx context.Context, gopts GlobalOptions, newPasswordFile string, insecureNoPassword bool) (string, error) { if testKeyNewPassword != "" { return testKeyNewPassword, nil } + if insecureNoPassword { + if newPasswordFile != "" { + return "", fmt.Errorf("only either --new-password-file or --new-insecure-no-password may be specified") + } + return "", nil + } + if newPasswordFile != "" { - return loadPasswordFromFile(newPasswordFile) + password, err := loadPasswordFromFile(newPasswordFile) + if err != nil { + return "", err + } + if password == "" { + return "", fmt.Errorf("an empty password is not allowed by default. Pass the flag `--new-insecure-no-password` to restic to disable this check") + } + return password, nil } // Since we already have an open repository, temporary remove the password // to prompt the user for the passwd. newopts := gopts newopts.password = "" + // empty passwords are already handled above + newopts.InsecureNoPassword = false return ReadPasswordTwice(ctx, newopts, "enter new password: ", diff --git a/cmd/restic/cmd_key_integration_test.go b/cmd/restic/cmd_key_integration_test.go index 16cc1bdad..0b4533887 100644 --- a/cmd/restic/cmd_key_integration_test.go +++ b/cmd/restic/cmd_key_integration_test.go @@ -3,6 +3,8 @@ package main import ( "bufio" "context" + "os" + "path/filepath" "regexp" "strings" "testing" @@ -109,6 +111,43 @@ func TestKeyAddRemove(t *testing.T) { testRunKeyAddNewKeyUserHost(t, env.gopts) } +func TestKeyAddInvalid(t *testing.T) { + env, cleanup := withTestEnvironment(t) + defer cleanup() + testRunInit(t, env.gopts) + + err := runKeyAdd(context.TODO(), env.gopts, KeyAddOptions{ + NewPasswordFile: "some-file", + InsecureNoPassword: true, + }, []string{}) + rtest.Assert(t, strings.Contains(err.Error(), "only either"), "unexpected error message, got %q", err) + + pwfile := filepath.Join(t.TempDir(), "pwfile") + rtest.OK(t, os.WriteFile(pwfile, []byte{}, 0o666)) + + err = runKeyAdd(context.TODO(), env.gopts, KeyAddOptions{ + NewPasswordFile: pwfile, + }, []string{}) + rtest.Assert(t, strings.Contains(err.Error(), "an empty password is not allowed by default"), "unexpected error message, got %q", err) +} + +func TestKeyAddEmpty(t *testing.T) { + env, cleanup := withTestEnvironment(t) + // must list keys more than once + env.gopts.backendTestHook = nil + defer cleanup() + testRunInit(t, env.gopts) + + rtest.OK(t, runKeyAdd(context.TODO(), env.gopts, KeyAddOptions{ + InsecureNoPassword: true, + }, []string{})) + + env.gopts.password = "" + env.gopts.InsecureNoPassword = true + + testRunCheck(t, env.gopts) +} + type emptySaveBackend struct { backend.Backend } diff --git a/cmd/restic/cmd_key_passwd.go b/cmd/restic/cmd_key_passwd.go index 32822a0ba..1a1200109 100644 --- a/cmd/restic/cmd_key_passwd.go +++ b/cmd/restic/cmd_key_passwd.go @@ -53,7 +53,7 @@ func runKeyPasswd(ctx context.Context, gopts GlobalOptions, opts KeyPasswdOption } func changePassword(ctx context.Context, repo *repository.Repository, gopts GlobalOptions, opts KeyPasswdOptions) error { - pw, err := getNewPassword(ctx, gopts, opts.NewPasswordFile) + pw, err := getNewPassword(ctx, gopts, opts.NewPasswordFile, opts.InsecureNoPassword) if err != nil { return err } diff --git a/cmd/restic/global.go b/cmd/restic/global.go index c954a4270..9671f2a26 100644 --- a/cmd/restic/global.go +++ b/cmd/restic/global.go @@ -52,22 +52,23 @@ type backendWrapper func(r backend.Backend) (backend.Backend, error) // GlobalOptions hold all global options for restic. type GlobalOptions struct { - Repo string - RepositoryFile string - PasswordFile string - PasswordCommand string - KeyHint string - Quiet bool - Verbose int - NoLock bool - RetryLock time.Duration - JSON bool - CacheDir string - NoCache bool - CleanupCache bool - Compression repository.CompressionMode - PackSize uint - NoExtraVerify bool + Repo string + RepositoryFile string + PasswordFile string + PasswordCommand string + KeyHint string + Quiet bool + Verbose int + NoLock bool + RetryLock time.Duration + JSON bool + CacheDir string + NoCache bool + CleanupCache bool + Compression repository.CompressionMode + PackSize uint + NoExtraVerify bool + InsecureNoPassword bool backend.TransportOptions limiter.Limits @@ -125,6 +126,7 @@ func init() { f.BoolVar(&globalOptions.NoCache, "no-cache", false, "do not use a local cache") f.StringSliceVar(&globalOptions.RootCertFilenames, "cacert", nil, "`file` to load root certificates from (default: use system certificates or $RESTIC_CACERT)") f.StringVar(&globalOptions.TLSClientCertKeyFilename, "tls-client-cert", "", "path to a `file` containing PEM encoded TLS client certificate and private key (default: $RESTIC_TLS_CLIENT_CERT)") + f.BoolVar(&globalOptions.InsecureNoPassword, "insecure-no-password", false, "use an empty password for the repository, must be passed to every restic command (insecure)") f.BoolVar(&globalOptions.InsecureTLS, "insecure-tls", false, "skip TLS certificate verification when connecting to the repository (insecure)") f.BoolVar(&globalOptions.CleanupCache, "cleanup-cache", false, "auto remove old cache directories") f.Var(&globalOptions.Compression, "compression", "compression mode (only available for repository format version 2), one of (auto|off|max) (default: $RESTIC_COMPRESSION)") @@ -327,6 +329,13 @@ func readPasswordTerminal(ctx context.Context, in *os.File, out *os.File, prompt // variable RESTIC_PASSWORD or prompts the user. If the context is canceled, // the function leaks the password reading goroutine. func ReadPassword(ctx context.Context, opts GlobalOptions, prompt string) (string, error) { + if opts.InsecureNoPassword { + if opts.password != "" { + return "", errors.Fatal("--insecure-no-password must not be specified together with providing a password via a cli option or environment variable") + } + return "", nil + } + if opts.password != "" { return opts.password, nil } @@ -348,7 +357,7 @@ func ReadPassword(ctx context.Context, opts GlobalOptions, prompt string) (strin } if len(password) == 0 { - return "", errors.Fatal("an empty password is not a password") + return "", errors.Fatal("an empty password is not allowed by default. Pass the flag `--insecure-no-password` to restic to disable this check") } return password, nil @@ -445,7 +454,7 @@ func OpenRepository(ctx context.Context, opts GlobalOptions) (*repository.Reposi } passwordTriesLeft := 1 - if stdinIsTerminal() && opts.password == "" { + if stdinIsTerminal() && opts.password == "" && !opts.InsecureNoPassword { passwordTriesLeft = 3 } diff --git a/cmd/restic/global_test.go b/cmd/restic/global_test.go index 4f5c29e9a..ce59bba49 100644 --- a/cmd/restic/global_test.go +++ b/cmd/restic/global_test.go @@ -1,8 +1,10 @@ package main import ( + "context" "os" "path/filepath" + "strings" "testing" rtest "github.com/restic/restic/internal/test" @@ -50,3 +52,14 @@ func TestReadRepo(t *testing.T) { t.Fatal("must not read repository path from invalid file path") } } + +func TestReadEmptyPassword(t *testing.T) { + opts := GlobalOptions{InsecureNoPassword: true} + password, err := ReadPassword(context.TODO(), opts, "test") + rtest.OK(t, err) + rtest.Equals(t, "", password, "got unexpected password") + + opts.password = "invalid" + _, err = ReadPassword(context.TODO(), opts, "test") + rtest.Assert(t, strings.Contains(err.Error(), "must not be specified together with providing a password via a cli option or environment variable"), "unexpected error message, got %v", err) +} diff --git a/cmd/restic/secondary_repo.go b/cmd/restic/secondary_repo.go index 2afd36a81..9a3eb5fe2 100644 --- a/cmd/restic/secondary_repo.go +++ b/cmd/restic/secondary_repo.go @@ -11,11 +11,12 @@ import ( type secondaryRepoOptions struct { password string // from-repo options - Repo string - RepositoryFile string - PasswordFile string - PasswordCommand string - KeyHint string + Repo string + RepositoryFile string + PasswordFile string + PasswordCommand string + KeyHint string + InsecureNoPassword bool // repo2 options LegacyRepo string LegacyRepositoryFile string @@ -49,6 +50,7 @@ func initSecondaryRepoOptions(f *pflag.FlagSet, opts *secondaryRepoOptions, repo f.StringVarP(&opts.PasswordFile, "from-password-file", "", "", "`file` to read the source repository password from (default: $RESTIC_FROM_PASSWORD_FILE)") f.StringVarP(&opts.KeyHint, "from-key-hint", "", "", "key ID of key to try decrypting the source repository first (default: $RESTIC_FROM_KEY_HINT)") f.StringVarP(&opts.PasswordCommand, "from-password-command", "", "", "shell `command` to obtain the source repository password from (default: $RESTIC_FROM_PASSWORD_COMMAND)") + f.BoolVar(&opts.InsecureNoPassword, "from-insecure-no-password", false, "use an empty password for the source repository, must be passed to every restic command (insecure)") opts.Repo = os.Getenv("RESTIC_FROM_REPOSITORY") opts.RepositoryFile = os.Getenv("RESTIC_FROM_REPOSITORY_FILE") @@ -63,7 +65,7 @@ func fillSecondaryGlobalOpts(ctx context.Context, opts secondaryRepoOptions, gop } hasFromRepo := opts.Repo != "" || opts.RepositoryFile != "" || opts.PasswordFile != "" || - opts.KeyHint != "" || opts.PasswordCommand != "" + opts.KeyHint != "" || opts.PasswordCommand != "" || opts.InsecureNoPassword hasRepo2 := opts.LegacyRepo != "" || opts.LegacyRepositoryFile != "" || opts.LegacyPasswordFile != "" || opts.LegacyKeyHint != "" || opts.LegacyPasswordCommand != "" @@ -85,6 +87,7 @@ func fillSecondaryGlobalOpts(ctx context.Context, opts secondaryRepoOptions, gop dstGopts.PasswordFile = opts.PasswordFile dstGopts.PasswordCommand = opts.PasswordCommand dstGopts.KeyHint = opts.KeyHint + dstGopts.InsecureNoPassword = opts.InsecureNoPassword pwdEnv = "RESTIC_FROM_PASSWORD" repoPrefix = "source" @@ -98,6 +101,8 @@ func fillSecondaryGlobalOpts(ctx context.Context, opts secondaryRepoOptions, gop dstGopts.PasswordFile = opts.LegacyPasswordFile dstGopts.PasswordCommand = opts.LegacyPasswordCommand dstGopts.KeyHint = opts.LegacyKeyHint + // keep existing bevhaior for legacy options + dstGopts.InsecureNoPassword = false pwdEnv = "RESTIC_PASSWORD2" } From 130506250f04c86f8487819eaf801c40f1d4990b Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Sat, 18 May 2024 19:20:04 +0200 Subject: [PATCH 3/3] document insecure-no-password --- changelog/unreleased/issue-1786 | 19 +++++++++++++++++++ doc/030_preparing_a_new_repo.rst | 23 +++++++++++++++++++++++ 2 files changed, 42 insertions(+) create mode 100644 changelog/unreleased/issue-1786 diff --git a/changelog/unreleased/issue-1786 b/changelog/unreleased/issue-1786 new file mode 100644 index 000000000..bdd0d48c3 --- /dev/null +++ b/changelog/unreleased/issue-1786 @@ -0,0 +1,19 @@ +Enhancement: Support repositories with empty password + +Restic refused to create or operate on repositories with an emtpy password. +Using the new option `--insecure-no-password` it is now possible to disable +this check. Restic will not prompt for a password when using this option. +For security reasons, the option must always be specified when operating on +repositories with an empty password. + +Specifying `--insecure-no-password` while also passing a password to restic +via a CLI option or via environment variable results in an error. + +The `init` and `copy` command also support the option `--from-insecure-no-password` +which applies to the source repository. The `key add` and `key passwd` comands +include the `--new-insecure-no-password` option to add or set an emtpy password. + +https://github.com/restic/restic/issues/1786 +https://github.com/restic/restic/issues/4326 +https://github.com/restic/restic/pull/4698 +https://github.com/restic/restic/pull/4808 diff --git a/doc/030_preparing_a_new_repo.rst b/doc/030_preparing_a_new_repo.rst index ee0a0df5e..5f3f3ff15 100644 --- a/doc/030_preparing_a_new_repo.rst +++ b/doc/030_preparing_a_new_repo.rst @@ -852,3 +852,26 @@ and then grants read/write permissions for group access. .. note:: To manage who has access to the repository you can use ``usermod`` on Linux systems, to change which group controls repository access ``chgrp -R`` is your friend. + + +Repositories with empty password +******************************** + +Restic by default refuses to create or operate on repositories that use an +empty password. Since restic 0.17.0, the option ``--insecure-no-password`` allows +disabling this check. Restic will not prompt for a password when using this option. +Specifying ``--insecure-no-password`` while also passing a password to restic +via a CLI option or via environment variable results in an error. + +For security reasons, the option must always be specified when operating on +repositories with an empty password. For example to create a new repository +with an empty password, use the following command. + +.. code-block:: console + + restic init --insecure-no-password + + +The ``init`` and ``copy`` command also support the option ``--from-insecure-no-password`` +which applies to the source repository. The ``key add`` and ``key passwd`` comands +include the ``--new-insecure-no-password`` option to add or set and emtpy password.