userdel: --force: Allow the flag to be specified twice, for more granularity

[alejandro-colomar]

Previously, with a single --force, one could only chose between not forcing, and forcing (which could destroy one's system entirely).

Now one can pass --force, with only a small danger, to skip some checks, and a double --force --force with the meaning of the previous --force, which will possibly destroy the entire system.

Closes: #1050
Reported-by: @wolfsage
Cc: @poettering
Cc: @hallyn

---

I have NOT tested this. Please test.

---

Revisions:

v2

• Reword documentation

$ git range-diff shadow/master gh/ff ff 
1:  5d8ddfa5 ! 1:  11c0e8b5 userdel: --force: Allow the flag to be specified twice, for more granularity
    @@ man/userdel.8.xml
     +    </para>
     +    <para>
     +      If specified once,
    -+      it doesn't check if the user is still logged in.
    ++      users are removed
    ++      even if they're still logged in,
    ++      and groups are removed
    ++      even if they are the primary group of a user.
     +      If specified twice,
     +      it skips all safety checks.
          </para>

v2b

• Reword paragraph.

$ git range-diff shadow/master gh/ff ff 
1:  11c0e8b5 ! 1:  cb580580 userdel: --force: Allow the flag to be specified twice, for more granularity
    @@ man/userdel.8.xml
     +    </para>
     +    <para>
     +      If specified once,
    -+      users are removed
    -+      even if they're still logged in,
    -+      and groups are removed
    -+      even if they are the primary group of a user.
    ++      a user is removed
    ++      even if it's still logged in,
    ++      and its primary group is removed
    ++      even if it's the primary group of another user.
     +      If specified twice,
     +      it skips all safety checks.
          </para>

v2c

• Rebase

$ git range-diff gh/master..gh/ff master..ff 
1:  cb580580 = 1:  72af7d32 userdel: --force: Allow the flag to be specified twice, for more granularity

v2d

• Rebase

$ git range-diff master..gh/ff shadow/master..ff 
1:  72af7d32 = 1:  372fdbe2 userdel: --force: Allow the flag to be specified twice, for more granularity

v2e

• Rebase

$ git range-diff alx/master..gh/ff master..ff 
1:  372fdbe2 = 1:  d6d8095d userdel: --force: Allow the flag to be specified twice, for more granularity

v2f

• Rebase

$ git range-diff alx/master..gh/ff shadow/master..ff 
1:  d6d8095d = 1:  cd0f9edb userdel: --force: Allow the flag to be specified twice, for more granularity

v2g

• Rebase

$ git range-diff master..gh/ff shadow/master..ff 
1:  cd0f9edb = 1:  da377a98 userdel: --force: Allow the flag to be specified twice, for more granularity

v2h

• Rebase

$ git range-diff master..gh/ff shadow/master..ff 
1:  da377a98 = 1:  49d5a39f userdel: --force: Allow the flag to be specified twice, for more granularity

v2i

• Rebase

$ git range-diff master..gh/ff shadow/master..ff 
1:  49d5a39f = 1:  8ed1c0cb userdel: --force: Allow the flag to be specified twice, for more granularity

v2j

• Rebase

$ git range-diff master..gh/ff shadow/master..ff 
1:  8ed1c0cb = 1:  2ba0f96a userdel: --force: Allow the flag to be specified twice, for more granularity

v2k

• Rebase

$ git range-diff master..gh/ff shadow/master..ff 
1:  2ba0f96a ! 1:  6d8a7a22 userdel: --force: Allow the flag to be specified twice, for more granularity
    @@ src/userdel.c: int main (int argc, char **argv)
     @@ src/userdel.c: int main (int argc, char **argv)
         * a cron job may be started on her behalf, etc.
         */
    -   if ((prefix[0] == '\0') && !Rflg && user_busy (user_name, user_id) != 0) {
    +   if (streq(prefix, "") && !Rflg && user_busy(user_name, user_id) != 0) {
     -          if (!fflg) {
     +          if (fflg < 1) {
      #ifdef WITH_AUDIT

[alejandro-colomar]

Does this distinction make sense? Should the group thing go with -ff?

[alejandro-colomar]

I think it's ok, but I ask just in case.

[alejandro-colomar]

@wolfsage, can you please check if this looks good to you? Also, would you please test it?

[ikerexxe]

I don't like this change and I think it'll be problematic to some users.

[alejandro-colomar]

I don't like this change and I think it'll be problematic to some users.

Yep, we have problems either way. This is half broken, so users will complain about it being quite unsafe. Yet, scripts may rely on that brokenness, so fixing it may break scripts.

I'm in favour of fixing it and potentially breaking some random scripts. Any other opinions?

[ikerexxe]

From my perspective, the force' option implies deleting the user regardless of the consequences. This option should be used with care, because as you mention, it can have fatal consequences for the system. The double use of the force' option is problematic and will cause more problems than it intends to solve. I would try to keep it as simple as possible and avoid such a complication.

[alejandro-colomar]

From my perspective, the force' option implies deleting the user regardless of the consequences. This option should be used with care, because as you mention, it can have fatal consequences for the system. The double use of the force' option is problematic and will cause more problems than it intends to solve. I would try to keep it as simple as possible and avoid such a complication.

There's prior art in other software for this schema of asking for double -ff for more dangerous stuff.

This is for example from git-clean(1):

     -f, --force
         If the Git configuration variable clean.requireForce is not
         set to false, git clean will refuse to delete files or
         directories unless given -f. Git will refuse to modify
         untracked nested git repositories (directories with a .git
         subdirectory) unless a second -f is given.

The only thing that might be a problem is existing scripts that use this (or interactive users that might not read the new docs if they remember well the old docs; but if I were them, I'd probably re-read the docs just in case). But for interactive use, or for new scripts, I don't think requiring the option twice would be problematic.

[hallyn]

Alex, thank you for continuing to try to make this better.

Please don't make any changes yet based on what I say here, I'm just thinking aloud :)

First, there's a difference between "home is not owned by the user" and "home is / and we're all gonna die!". Shadow is not that "generic" of a package. I think it would be ok to add a list of "do not touch" paths, and always refuse to delete those no matter what: /, /usr, /etc, and /var at least. I suppose we could even make the list configurable through /etc/login.defs, so distros can add their own entries.

Then, for the "home not owned by user" case, we just need to draw line between not breaking any current users and complicating code. The main options are: 1. change nothing (which if we implement the never-delete list above becomes far less scary), 2. add a default-off DELETE_UNOWNED_HOME option to login.defs, 3. add -ff like you have, and 4. both 2 and 3.

[alejandro-colomar]

Shadow is not that "generic" of a package. I think it would be ok to add a list of "do not touch" paths, and always refuse to delete those no matter what: /, /usr, /etc, and /var at least.

How about refusing to add users with such homedirs in the first place?

(Just thinking aloud too.)

[alejandro-colomar]

Then, for the "home not owned by user" case, we just need to draw line between not breaking any current users and complicating code. The main options are: [...] 2. add a default-off DELETE_UNOWNED_HOME option to login.defs. [...]

I tend to dislike configurability of programs. It adds complexity to both sysadmins, and the program itself. I think I would prefer a hardcoded list of directories that should not be removed.

[hallyn]

Shadow is not that "generic" of a package. I think it would be ok to add a list of "do not touch" paths, and always refuse to delete those no matter what: /, /usr, /etc, and /var at least.

How about refusing to add users with such homedirs in the first place?

(Just thinking aloud too.)

The problem with that is that a mistaken or malicious /etc/passwd edit could easily change it afterwards.

[alejandro-colomar]

Shadow is not that "generic" of a package. I think it would be ok to add a list of "do not touch" paths, and always refuse to delete those no matter what: /, /usr, /etc, and /var at least.

How about refusing to add users with such homedirs in the first place?
(Just thinking aloud too.)

The problem with that is that a mistaken or malicious /etc/passwd edit could easily change it afterwards.

I think -ff is good in the long term (even if we may want to do other changes too); even if we want to forbid removing /, I'd still hide removing other non-owned dirs under -ff.

What's your opinion about breaking existing scripts (or hardcoded brains)? I don't know how much people would rely on -f for removing non-owned dirs, and other stuff we're hiding under -ff.

[hallyn]

What's your opinion about breaking existing scripts (or hardcoded brains)? I don't know how much people would rely on -f for removing non-owned dirs, and other stuff we're hiding under -ff.

Well actually, I would imagine there are a lot of places that have end-of-semester or whatever scripts that nuke the out-going students' accounts. For those, having -f return error and -ff succeed would be a problem - if they ever have a situation like this. It might be better if -f just skipped the deletion and returned success. But that has its own problems.

[alejandro-colomar]

What's your opinion about breaking existing scripts (or hardcoded brains)? I don't know how much people would rely on -f for removing non-owned dirs, and other stuff we're hiding under -ff.

Well actually, I would imagine there are a lot of places that have end-of-semester or whatever scripts that nuke the out-going students' accounts. For those, having -f return error and -ff succeed would be a problem - if they ever have a situation like this. It might be better if -f just skipped the deletion and returned success. But that has its own problems.

But I expect such user accounts to either have an owned dir, or have something like /nonexistent that shouldn't be removed. In what case would one have a directory that isn't owned but they want to remove it anyway?

[hallyn]

But I expect such user accounts to either have an owned dir, or have something like /nonexistent that shouldn't be removed. In what case would one have a directory that isn't owned but they want to remove it anyway?

That makes sense, yeah. So system accounts pointing at some /var/lib/something owned by another system user would be more likely.

But, I was thinking based on the discussion that this patch would make it so that whereas
previously you would just not delete the homedir, how you'd return an error. But that
doesn't appear to be the case. So I'm good with this patch.

@ikerexxe do you still feel uncomfortable with this patch?

[ikerexxe]

It seems counter-intuitive to me, but I won't object if you think it's the best solution

[alejandro-colomar]

Turning into draft until after 4.17.0.