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

Account bumps password set time, even when it doesnt #1510

Open
djzort opened this issue Oct 12, 2021 · 2 comments
Open

Account bumps password set time, even when it doesnt #1510

djzort opened this issue Oct 12, 2021 · 2 comments
Labels
bug Confirmed bugs

Comments

@djzort
Copy link

djzort commented Oct 12, 2021

Describe the bug

The 'account' function always bumps the password set time, even when it doesnt change anything.

Consider this simple config

    account 'mp',
        ensure => 'present',
        uid    => 1001,
        shell  => '/bin/false',
        home   => '/home/mp',
        create_home => 1,
        crypt_password => '!'; # disabled login
    account 'dc',
        ensure => 'present',
        uid    => 1002,
        shell  => '/bin/false',
        home   => '/home/dc',
        create_home => 1,
        crypt_password => '!'; # disabled login

Then i run rex ( i have etckeeper installed )

git diff /etc/shadow
diff --git a/shadow b/shadow
index 02e510e..2b472fe 100644
--- a/shadow
+++ b/shadow
@@ -22,7 +22,7 @@ systemd-network:*:18496:0:99999:7:::
 mp:!:18876:0:99999:7:::
-dc:!:18876:0:99999:7:::
+mp:!:18911:0:99999:7:::
+dc:!:18911:0:99999:7:::

Run rex again, then...

root@batou:/etc# git diff /etc/shadow
diff --git a/shadow b/shadow
index 02e510e..1053301 100644
--- a/shadow
+++ b/shadow
-mp:!:18876:0:99999:7:::
-dc:!:18876:0:99999:7:::
+mp:!:18912:0:99999:7:::
+dc:!:18912:0:99999:7:::

The "date of last password change" field changes (per man shadow)

I dont think that field should change if the password doesnt change.

YMMV on that. I can see how someone could make an argument that it should.

How to reproduce it

Create a user and run rex over and over. Diff the shadow files

Expected behavior

I dont think that field should change if the password doesnt change. Or perhaps be controllable.

Circumstances

  • Rex version: (R)?ex 1.13.4
  • Perl version: 5.32.0
  • OS running rex: Debian bookworm/sid
  • OS managed by rex: Debian bookworm/sid
  • How rex was installed: cpanm Rex

Debug log

Is this really needed?

@djzort djzort added the triage needed A potential bug that needs to be reproduced and understood label Oct 12, 2021
@ferki
Copy link
Member

ferki commented Oct 24, 2021

Thank for the report, @djzort!

In the case of account ..., crypt_password => $encrypted_password, Rex translates that to a usermod -p '$encrypted_password' call (on Linux endpoints), which in turn will set the password. I believe usermod does not check if it's the same encrypted password that is already set, just sets it directly, and bumps the last updated date (once a day only, though). It also doesn't seem to have a command line option to "check password first, and only set if it's different".

I'd say the intent of account is to be declarative and idempotent, while create_user is imperative. In that sense, we could to the following:

  • Differentiate between account and create_user calls a bit more in Rex::User::* implementations, based on the caller. Always update the password when called from create_user, and only update via account when the password is different.
  • For crypt_password, read the contents of /etc/shadow first, and only run the usermod call if the passed value doesn't match what is currently set.
  • For the account ..., password => $password case (=pass the plain password string), call perl's crypt function on the password string, and compare it to the one stored in /etc/shadow, and only set it if it's different.

@ferki ferki added bug Confirmed bugs and removed triage needed A potential bug that needs to be reproduced and understood labels Oct 24, 2021
@djzort
Copy link
Author

djzort commented Nov 13, 2021

In Rex/User/Linux.pm perhaps something like

  my $expire = q();
  if (exists $data->{expire}) {
     $expire = $data->{expire};
     $run_cmd = 1;
  }
  $cmd .= " --expiredate '" . $expire . "'";

The problem with the above is that if the expiry is removed from the config, then you would expect it to be removed from the system - but it wouldn't be.

Reviewing that Linux.pm module, that same sort of counter-intuitive expectation seems to exist with other values too, such as 'crypt_password' and 'comment'. In both cases i would assume that removing them would remove their respective value.

On the other hand I would expect the absence of 'uid' or 'home' to mean 'don't care'.

Not sure what the right answer is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Confirmed bugs
Projects
None yet
Development

No branches or pull requests

2 participants