Skip to content

Comments

tmpfiles: disable --purge#33353

Closed
keszybz wants to merge 1 commit intosystemd:mainfrom
keszybz:tmpfiles-disable-purge
Closed

tmpfiles: disable --purge#33353
keszybz wants to merge 1 commit intosystemd:mainfrom
keszybz:tmpfiles-disable-purge

Conversation

@keszybz
Copy link
Member

@keszybz keszybz commented Jun 15, 2024

This effectively reverts 81a1838. Note that the actual implementation was retained, but there is no way to invoke it once the commandline switch is removed. I expect that the switch will be restored in a slightly different form, so it doesn't make sense to create churn by removing all the code.

As discussed in #33349, --purge provides a footgun for users by allowing them to easily remove contents of /home. At least one user has done this, which is one too many. Let's disable the command before this hits stable distributions and more users have a chance to destroy their data.

Closes #33349.

Copy link
Member

@YHNdnzj YHNdnzj left a comment

Choose a reason for hiding this comment

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

Need to be removed from documentation too?

@YHNdnzj
Copy link
Member

YHNdnzj commented Jun 15, 2024

Reverting a feature is unfortunate, but in this particular case I think it's the right thing to do. The feature is too much of a footgun right now, and has caused real damage. I agree that the whole design choices need to be reconsidered for the next version.

@YHNdnzj YHNdnzj added good-to-merge/with-minor-suggestions and removed please-review PR is ready for (re-)review by a maintainer labels Jun 15, 2024
@NekkoDroid
Copy link
Contributor

I don't know how realistic/wanted this is, but maybe defaulting sd-tmpfiles to --dry-run=true (similar to repart) for next version might help with preventing unintentional invocations.

And specifically for the current issue: maybe calling it --delete-everything= might be obvious enough if it required explicitly with a truthy value (in combination with the first suggestion).

This effectively reverts 81a1838.
Note that the actual implementation was retained, but there is no way to invoke
it once the commandline switch is removed. I expect that the switch will be
restored in a slightly different form, so it doesn't make sense to create churn
by removing all the code.

As discussed in systemd#33349, --purge
provides a footgun for users by allowing them to easily remove contents of
/home. At least one user has done this, which is one too many. Let's disable
the command before this hits stable distributions and more users have a
chance to destroy their data.

Closes systemd#33349.
@keszybz keszybz force-pushed the tmpfiles-disable-purge branch from 31538e6 to 8c09661 Compare June 15, 2024 15:33
@keszybz
Copy link
Member Author

keszybz commented Jun 15, 2024

Need to be removed from documentation too?

Arrghs, I had an unsaved change. The docs are removed now too.

@keszybz
Copy link
Member Author

keszybz commented Jun 15, 2024

maybe defaulting sd-tmpfiles to --dry-run=true

I don't think we can do this easily, because it'd break backwards compat. We could do that conditionally with --purge.

@keszybz keszybz added good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed and removed good-to-merge/with-minor-suggestions labels Jun 15, 2024
@keszybz
Copy link
Member Author

keszybz commented Jun 15, 2024

I upgraded the green label because comments have been addressed, but let's wait for a review from @bluca too.

@daandemeyer
Copy link
Collaborator

daandemeyer commented Jun 15, 2024

What about an isatty check? Maybe even imply dry run when run in an interactive shell.

@keszybz
Copy link
Member Author

keszybz commented Jun 15, 2024

What about an isatty check? Maybe even imply dry run when run in an interactive shell.

Dunno, such things are always very fragile. You run a command in a script and it works, then copy it into the terminal and it doesn't? Or run the script from a terminal and it's broken? But pipe it to less and it works again. I think it's fine when we enable/disable coloring and output details based on such checks, but I wouldn't apply it to basic (and dangerous) rules of behaviour.

@bluca
Copy link
Member

bluca commented Jun 15, 2024

Let's not overreact over social media drama from well-known trolls. This is already in use in Debian/Ubuntu packages (edit: here https://salsa.debian.org/debian/debhelper/-/blob/main/autoscripts/postrm-init-tmpfiles-purge ). It's fine to make this act only on input (stdin or explicit config file), and add another flag like --factor-reset, perhaps not documented, for the global behaviour. None of this is ever intended to be ran manually anyway, there's no documentation that I am aware of that even hints of doing that, it's for timed services and packaging scriptlets. Changing documentation is of course also fine to do, if you like. It is true that tmpfiles.d has long since not been only about temporary files, and for years has taken care of /etc/ for example, and the docs could use some freshing up.

@bluca bluca added reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks and removed good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed labels Jun 15, 2024
@keszybz
Copy link
Member Author

keszybz commented Jun 15, 2024

My reasoning is not based on social media reports, but on the github issue and analysis of the interface.

This is already in use in Debian/Ubuntu packages (edit: here https://salsa.debian.org/debian/debhelper/-/blob/main/autoscripts/postrm-init-tmpfiles-purge ).

This sounds dangerous to use in an automatic fashion. I wouldn't unleash this on my users. But actually this use is compatible with the removal, since the script checks for --purge in help output.

It's fine to make this act only on input (stdin or explicit config file), and add another flag like --factor-reset, perhaps not documented, for the global behaviour.

I think that's something to discuss before reenabling this option.

None of this is ever intended to be ran manually anyway, there's no documentation that I am aware of that even hints of doing that, it's for timed services and packaging scriptlets.

I guess the intent was not, but our intent is that important. What matters is that users clearly are using this in unintended ways. And the tools is intended to be used in scripts and various special ways. The tool has a user interface and --help and is a normal user-level tool, just not in $PATH.


Do you have any actual comments on the patch as written? Or a suggestion how to resolve the issue in a different way? But all that must be actionable now. We have a stable release that in principle we consider appropriate for users and I want to remove this footgun before another user looses data.

@bluca
Copy link
Member

bluca commented Jun 15, 2024

My reasoning is not based on social media reports, but on the github issue and analysis of the interface.

Sure, but if it wasn't for a couple of trolls on social media, nobody would have cared in the first place.

This is already in use in Debian/Ubuntu packages (edit: here https://salsa.debian.org/debian/debhelper/-/blob/main/autoscripts/postrm-init-tmpfiles-purge ).

This sounds dangerous to use in an automatic fashion. I wouldn't unleash this on my users. But actually this use is compatible with the removal, since the script checks for --purge in help output.

It's not dangerous at all, as it wired up to run only on the specified input. The purpose of the purge operation is to remove any trace that a package might have left behind, and that's exactly what it does. Removal is not compatible with it, as it regresses: leftovers will be left around. Sorry, such a regression not acceptable.

It's fine to make this act only on input (stdin or explicit config file), and add another flag like --factor-reset, perhaps not documented, for the global behaviour.

I think that's something to discuss before reenabling this option.

There's no need to rush anything. The proposal to change the default to act only on explicit input is compatible with current use so it's acceptable to me, and it will shut up the trolls on social media too.

None of this is ever intended to be ran manually anyway, there's no documentation that I am aware of that even hints of doing that, it's for timed services and packaging scriptlets.

I guess the intent was not, but our intent is that important. What matters is that users clearly are using this in unintended ways. And the tools is intended to be used in scripts and various special ways. The tool has a user interface and --help and is a normal user-level tool, just not in $PATH.

Personally I am actually very, very skeptical actual users are really doing anything of the sort. Even the bug report I find highly dubious: why would anybody who has never heard of this tool and has never used it, think of running it just to remove something from /var/cache as reported, and with this option instead of the more obvious clean/remove on top of that? There is no documentation nor hint nor help nor manpage anywhere suggesting to do that. There's no mention of /var/cache anywhere that I can find. I find it extremely weird. This all started on IRC first and then moved to social media, so I wouldn't be surprised if what really happened is that someone read NEWS, found the entry and decided to have a bit of fun.

Do you have any actual comments on the patch as written? Or a suggestion how to resolve the issue in a different way? But all that must be actionable now. We have a stable release that in principle we consider appropriate for users and I want to remove this footgun before another user looses data.

Yes, as already mentioned, Lennart's suggestion sound fine to me: make the switch act only on the provided input. This is actionable now too, and doesn't sound too hard. Updating the documentation is also actionable now.

@evverx
Copy link
Contributor

evverx commented Jun 16, 2024

trolls

It's concerning that people raising valid concerns are called trolls.

the bug report I find highly dubious

This reminds me of #25269 (and I thought nothing could top it honestly). In the end it was of course a badly designed feature with sloppy UX that someone else had to fix as usual.

@keszybz as far as I can see the majority are fine with merging this so I wonder why it's still being discussed? Whoever wants to keep it downstream can apply patches there.

@poettering
Copy link
Member

the --purge stuff is about ownership of files. I.e. that stuff "owned" by some specific package can be removed nicely when the package goes away. Given we know this usecase very clearly I am pretty sure we should just require that a config snippet is specified on the invocation (either by filename on cmdline or via data on stdin), and if it isn't we refuse to do anything, because it simply makes no sense to "delete everything owned", because that necessarily destroys the whole system.

I think it's quite a high bar against accidental invocation if we require people to specify valid tmpfiles.d/ drop-in filenames or valid definitions on stdin before we do anything. Hence I think that should really be enough as a protection.

@poettering
Copy link
Member

My alternative fix: #33383 ptal.

@github-actions github-actions bot removed the reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks label Jun 18, 2024
@bluca
Copy link
Member

bluca commented Jun 18, 2024

replaced by #33383

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

refuse systemd-tmpfiles --purge invocation without config file specified on cmdline

7 participants