Navigation Menu

Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Force restic to ask the password when adding a key. #1133

Merged
merged 3 commits into from Jul 26, 2017
Merged

Force restic to ask the password when adding a key. #1133

merged 3 commits into from Jul 26, 2017

Conversation

middelink
Copy link
Member

As restic key add uses the same ReadPasswordTwice() as the
rest of restic, it is sensitive to the environment variable
RESTIC_PASSWORD or --password-file= override.

When asking for the new key, temporary remove these 2 overrides, forcing
the password to be asked.

Closes #1132

As `restic key add` uses the same `ReadPasswordTwice()` as the
rest of restic, it is sensitive to the environment variable
RESTIC_PASSWORD or --password-file= override.

When asking for the new key, temporary remove these 2 overrides, forcing
the password to be asked.
@codecov-io
Copy link

codecov-io commented Jul 24, 2017

Codecov Report

Merging #1133 into master will decrease coverage by 3.34%.
The diff coverage is 18.18%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #1133      +/-   ##
=========================================
- Coverage   52.84%   49.5%   -3.35%     
=========================================
  Files         121     121              
  Lines       11397   11406       +9     
=========================================
- Hits         6023    5646     -377     
- Misses       4667    5078     +411     
+ Partials      707     682      -25
Impacted Files Coverage Δ
cmd/restic/main.go 7.54% <0%> (-1.15%) ⬇️
cmd/restic/cmd_backup.go 25.89% <0%> (ø) ⬆️
cmd/restic/cmd_key.go 53.78% <0%> (-1.4%) ⬇️
cmd/restic/global.go 23.05% <21.42%> (+2.59%) ⬆️
cmd/restic/cmd_init.go 53.12% <60%> (+10.7%) ⬆️
internal/backend/b2/b2.go 0% <0%> (-79.32%) ⬇️
internal/backend/swift/swift.go 0% <0%> (-73.9%) ⬇️
internal/backend/swift/config.go 34.37% <0%> (-56.25%) ⬇️
internal/archiver/archiver.go 64.9% <0%> (+0.16%) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 608adf1...c95e2b0. Read the comment docs.

Pauline Middelink added 2 commits July 24, 2017 23:05
Instead of determining the password lazily during ReadPassword(), do so now in
cobra.PersistentPreRunE() so we can store the result in the globalOptions and
reuse/override when applicable without having to worry about the environment
or flag options interfering.
@pvgoran
Copy link

pvgoran commented Jul 24, 2017

Sounds like this solution will not allow non-interactive invocation of this subcommand.

Possibly a more universal solution would be to use a different environment variable and a different command line option for password that is added by restic key add. Like RESTIC_NEW_PASSWORD and --new-password-file. Moreover, these new environment variable/command line option could be also used by restic init; this way, a password to open the repository will be always RESTIC_PASSWORD/--password-file, and a "new" password (for new repository or new key) will be always RESTIC_NEW_PASSWORD/--new-password-file.

@middelink
Copy link
Member Author

Can you please file a new feature request for this? I'm just trying to fix the broken existing behavior (==bug) here and would be happy to discuss (and share my concerns ^^) on your feature request.

Copy link
Member

@fd0 fd0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thanks!

@fd0 fd0 merged commit c95e2b0 into restic:master Jul 26, 2017
fd0 added a commit that referenced this pull request Jul 26, 2017
Force restic to ask the password when adding a key.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants