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

Fix dangerous forget-all operation which drops all snapshots #4568

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
11 changes: 11 additions & 0 deletions changelog/unreleased/issue-4569
@@ -0,0 +1,11 @@
Bugfix: Prevent restic forget from deleting all snapshots accidentally

The command `restic forget --keep-tag=SomeUnusedTag --dry-run`
used to drop all snapshots. This would be very dangerous and probably
unwanted.

Restic now only allows --keep-tag together with other --keep-* options.

https://github.com/restic/restic/issues/4569
https://github.com/restic/restic/pull/4568
https://forum.restic.net/t/delete-all-snapshots-in-one-command-is-this-feature-intentional/6923/3
14 changes: 13 additions & 1 deletion cmd/restic/cmd_forget.go
Expand Up @@ -115,7 +115,7 @@ func init() {
f.VarP(&forgetOptions.WithinWeekly, "keep-within-weekly", "", "keep weekly snapshots that are newer than `duration` (eg. 1y5m7d2h) relative to the latest snapshot")
f.VarP(&forgetOptions.WithinMonthly, "keep-within-monthly", "", "keep monthly snapshots that are newer than `duration` (eg. 1y5m7d2h) relative to the latest snapshot")
f.VarP(&forgetOptions.WithinYearly, "keep-within-yearly", "", "keep yearly snapshots that are newer than `duration` (eg. 1y5m7d2h) relative to the latest snapshot")
f.Var(&forgetOptions.KeepTags, "keep-tag", "keep snapshots with this `taglist` (can be specified multiple times)")
f.Var(&forgetOptions.KeepTags, "keep-tag", "keep snapshots with this `taglist` (can be specified multiple times, `taglist`s will be combined into one). this is a \"has all\" condition, i.e. a snapshot is only kept when it has all tags in `taglist`.")
Copy link
Member

Choose a reason for hiding this comment

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

A snapshot must match all tags within a single taglist (AND), but the taglists are combined using OR.


initMultiSnapshotFilter(f, &forgetOptions.SnapshotFilter, false)
f.StringArrayVar(&forgetOptions.Hosts, "hostname", nil, "only consider snapshots with the given `hostname` (can be specified multiple times)")
Expand All @@ -138,14 +138,26 @@ func init() {
func verifyForgetOptions(opts *ForgetOptions) error {
if opts.Last < -1 || opts.Hourly < -1 || opts.Daily < -1 || opts.Weekly < -1 ||
opts.Monthly < -1 || opts.Yearly < -1 {
// This condition is already catched in ForgetPolicyCount's Set(s string)
return errors.Fatal("negative values other than -1 are not allowed for --keep-*")
}

var allKeepWithinXUndefined = true
for _, d := range []restic.Duration{opts.Within, opts.WithinHourly, opts.WithinDaily,
opts.WithinMonthly, opts.WithinWeekly, opts.WithinYearly} {
if d.Hours < 0 || d.Days < 0 || d.Months < 0 || d.Years < 0 {
return errors.Fatal("durations containing negative values are not allowed for --keep-within*")
}
if d.Hours != 0 || d.Days != 0 || d.Months != 0 || d.Years != 0 {
Copy link
Author

Choose a reason for hiding this comment

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

@arberg regarding 1.: these lines should be correct. d is a Duration which is defined as

type Duration struct {
	Hours, Days, Months, Years int
}

The duration specification has no week.

Copy link
Author

Choose a reason for hiding this comment

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

... and each opts.Within* option has such a duration specification as argument. That's my understanding.

allKeepWithinXUndefined = false
}
}

allKeepXUndefined := opts.Last == 0 && opts.Hourly == 0 && opts.Daily == 0 && opts.Weekly == 0 &&
opts.Monthly == 0 && opts.Yearly == 0

if len(opts.KeepTags) > 0 && allKeepXUndefined && allKeepWithinXUndefined {
Copy link
Author

Choose a reason for hiding this comment

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

Regarding 2.: Why do you think it doesn't cover --keep-within? It should be covered by allKeepWithinXUndefined which takes opts.Within into account.

	f.VarP(&forgetOptions.Within, "keep-within", "", "keep snapshots that are newer than `duration` (eg. 1y5m7d2h) relative to the latest snapshot")

Should be fine, right?

return errors.Fatal("--keep-tags can only be used together with other --keep-* options.")
}

return nil
Expand Down
2 changes: 2 additions & 0 deletions cmd/restic/cmd_forget_test.go
Expand Up @@ -40,6 +40,7 @@ func TestForgetPolicyValues(t *testing.T) {
func TestForgetOptionValues(t *testing.T) {
const negValErrorMsg = "Fatal: negative values other than -1 are not allowed for --keep-*"
const negDurationValErrorMsg = "Fatal: durations containing negative values are not allowed for --keep-within*"
const keepTagsAloneMsg = "Fatal: --keep-tags can only be used together with other --keep-* options."
testCases := []struct {
input ForgetOptions
errorMsg string
Expand Down Expand Up @@ -80,6 +81,7 @@ func TestForgetOptionValues(t *testing.T) {
{ForgetOptions{WithinWeekly: restic.ParseDurationOrPanic("1y2m3d-3h")}, negDurationValErrorMsg},
{ForgetOptions{WithinMonthly: restic.ParseDurationOrPanic("-2y4m6d8h")}, negDurationValErrorMsg},
{ForgetOptions{WithinYearly: restic.ParseDurationOrPanic("2y-4m6d8h")}, negDurationValErrorMsg},
{ForgetOptions{KeepTags: []restic.TagList{[]string{"tag1"}}}, keepTagsAloneMsg},
Copy link
Author

Choose a reason for hiding this comment

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

Maybe the TagLists can be created in a more concise way? I don't know go lang.

}

for _, testCase := range testCases {
Expand Down