Fix dangerous forget-all operation which drops all snapshots by schoettl · Pull Request #4568 · restic/restic

schoettl · 2023-11-18T23:12:15Z

What does this PR change? What problem does it solve?

restic forget --keep-tag=SomeUnusedTag --dry-run

This command would drop all snapshots. This is very dangerous and probably unwanted.

A suggested fix (see forum) is to only allow --keep-tag together with other --keep-* options.

While working on this, I noticed that another if case seem to have no effect, see comment in diff.

Was the change previously discussed in an issue or on the forum?

See also https://forum.restic.net/t/delete-all-snapshots-in-one-command-is-this-feature-intentional/6923/3

Fixes #4569

Checklist

I have read the contribution guidelines.
I have enabled maintainer edits.
I have added tests for all code changes.
I have added documentation for relevant changes (in the manual).
There's a new file in changelog/unreleased/ that describes the changes for our users (see template).
I have run gofmt on the code in all commits.
All commit messages are formatted in the same style as the other commits in the repo.
I'm done! This pull request is ready for review.

arberg · 2023-11-19T19:18:47Z

Hi Jakob,

Thank you for working on this. I'm the original issue OP. I've never looked at restic code before, but I have a few comments, though of cause they may be misplaced, as I have only looked at this tiny part of the code.

lines 148-152 looks like it contains a bug (which would be a prevous bug), namely that it is missing withinWeekly in the check, both the old and your new copy. Probably called d.Weekly. Also there's the --keep-within duration, though it possible the duration type cannot be negative.
line 159, the ending if, should also allow the --keep-within duration which isn't matched by your two new allKeep*Undefined. Or add keep-within to the allKeepWithinXUndefined
It might make sense to keep the initial if-check even if handled elsewhere as well, or maybe convert it into some kind of assert. That makes this method self-contained in its requirement of valid options. I mean just by reading this small method, one can verify that regardless of what goes on elsewhere in the sourcecode, this method does not allow --keep-* with negative values or with missing positives.

Best Alex

schoettl · 2023-11-19T21:48:05Z

Hey Alex, thanks for the great feedback and the catched bugs! :-)

I'll fix that in the next commits on this PR. But today it's too late.

I'm happy to get more feedback later on! It's also my first time contributing to restic and first time using the go language ^^

In case you're not familiar to github PRs, note that you can also add comments to the code in the "Files changed" tab.

schoettl · 2023-11-22T21:56:24Z

cmd/restic/cmd_forget.go

 		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 {


@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.

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

schoettl · 2023-11-22T22:03:47Z

cmd/restic/cmd_forget.go

+	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 {


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?

schoettl · 2023-11-22T22:25:54Z

I should try to add some tests in cmd_forget_*test.og...

schoettl · 2023-11-25T17:13:30Z

cmd/restic/cmd_forget_test.go

 		{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},


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

schoettl · 2023-11-29T17:58:09Z

@MichaelEischer @fd0 Do you want to review and merge or give feedback on this bugfix?

Edit: Sorry, I forgot some of the checkboxes above. I'll need to rebase again, run gofmt and update the changelog.

MichaelEischer · 2023-12-02T22:23:51Z

@schoettl Unfortunately, I won't have time to take a look at the PR before christmas, sorry.

schoettl · 2023-12-25T17:53:00Z

Happy Christmas! May I just ping on this PR? Maybe someone has time for review over the holidays :)

MichaelEischer

Enforcing that --keep-tag is always used together with another keep option feels wrong. In particular, it will lead unwanted side effects if the snapshots with the specified tags are not the latest snapshots in the snapshot group. Then both the latest snapshot and the tagged snapshots in the snapshot group would be kept, which breaks the expected semantics.

I'd like to propose a different approach:

Fix the current behavior by adding a check that forget refuses to work, if any snapshot group would end up with zero snapshots. This check would necessarily have to happen at runtime of the command, and cannot be checked before hand.
Add an --unsafe-allow-remove-all option for the forget command. This will disable the above check.
To prevent the worst cases of fat fingering, --unsafe-allow-remove-all must refuse to work unless a host/path/tag filter is specified or any other --keep-* option. This adds a new feature: users can then delete all snapshots from a specific host using e.g. restic forget --host old-host --unsafe-allow-remove-all.

Are you still interested in adapting and finishing this PR or should I take over?

MichaelEischer · 2024-03-29T13:37:25Z

cmd/restic/cmd_forget.go

 	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`.")


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

schoettl · 2024-04-03T20:42:22Z

Oh, that sounds like a more complicated fix. I'd be happy if you take over, I'm afraid I don't have the expertise and time, currently.

arberg · 2024-04-07T17:10:34Z

Michael I also like that option, and as I can recall from the forum, some would indeed appreciate that delete-all option. I think its fine with the '--unsafe-allow-remove-all', though adding the extra requirement of path/host/tag also sounds good.

I also won't have time at the moment and have yet evolved my go-mastery.

MichaelEischer · 2024-04-15T20:28:39Z

I've opened #4764 as a replacement for this PR. Different from my previous suggestion that PR currently always forbids the complete deletion of a whole snapshot group if any --keep-* policy was specified. This means that forget --keep-tag invalid will always result in an error, even if --unsafe-allow-remove-all was specified.

Suggest more accurate option description for --keep-tag

0dd1f0a

schoettl commented Nov 22, 2023

View reviewed changes

schoettl added 2 commits November 23, 2023 09:16

Add check against dangerous forget-all operation

63bb3dc

Add comment to condition that normally does not match

1248332

schoettl force-pushed the fix/keep-tag branch from 01e3474 to 1248332 Compare November 23, 2023 08:20

Add test for --keep-tag

a03d2a1

schoettl marked this pull request as ready for review November 25, 2023 17:12

schoettl commented Nov 25, 2023

View reviewed changes

Add changelog file

fff1476

MichaelEischer requested changes Apr 3, 2024

View reviewed changes

MichaelEischer mentioned this pull request Apr 15, 2024

Prevent unsafe uses of forget --keep-tag #4764

Merged

8 tasks

MichaelEischer closed this Apr 15, 2024

BrewTestBot mentioned this pull request Jul 26, 2024

restic 0.17.0 Homebrew/homebrew-core#178554

Merged

Conversation

schoettl commented Nov 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR change? What problem does it solve?

Was the change previously discussed in an issue or on the forum?

Checklist

Uh oh!

arberg commented Nov 19, 2023

Uh oh!

schoettl commented Nov 19, 2023

Uh oh!

schoettl Nov 22, 2023

Choose a reason for hiding this comment

Uh oh!

schoettl Nov 22, 2023

Choose a reason for hiding this comment

Uh oh!

schoettl Nov 22, 2023

Choose a reason for hiding this comment

Uh oh!

schoettl commented Nov 22, 2023

Uh oh!

schoettl Nov 25, 2023

Choose a reason for hiding this comment

Uh oh!

schoettl commented Nov 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MichaelEischer commented Dec 2, 2023

Uh oh!

schoettl commented Dec 25, 2023

Uh oh!

MichaelEischer left a comment

Choose a reason for hiding this comment

Uh oh!

MichaelEischer Mar 29, 2024

Choose a reason for hiding this comment

Uh oh!

schoettl commented Apr 3, 2024

Uh oh!

arberg commented Apr 7, 2024

Uh oh!

MichaelEischer commented Apr 15, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

schoettl commented Nov 18, 2023 •

edited

Loading

schoettl commented Nov 29, 2023 •

edited

Loading