canonical
fix: cc_user_groups incorrectly assumes "useradd" never locks password field
Proposed Commit Message
fix: cc_user_groups incorrectly assumes "useradd" never locks password
Currently cc_user_groups assumes that "useradd" never locks the password
field of newly created users. This is an incorrect assumption.
From the useradd manpage:
'-p, --password PASSWORD
The encrypted password, as returned by crypt(3). The default is to
disable the password.'
That is, if cloud-init runs 'useradd' but does not pass it the "-p"
option (with an encrypted password) then the new user's password field
will be locked by "useradd".
cloud-init only passes the "-p" option when calling "useradd" when
user-data specifies the "passwd" option for a new user. For user-data
that specifies either the "hashed_passwd" or "plain_text_passwd"
options instead then cloud-init calls "useradd" without the "-p" option
and so the password field of such a user will be locked by "useradd".
For user-data that specifies "hash_passwd" for a new user then "useradd"
is called with no "-p" option, so causing "useradd" to lock the
password field, however then cloud-init calls "chpasswd -e" to set the
encrypted password which also results in the password field being
unlocked.
For user-data that specifies either "plain_text_passwd" for a new user
then "useradd" is called with no "-p" option, so causing "useradd" to
lock the password. cloud-init then calls "chpasswd" to set the password
which also results in the password field being unlocked.
For user-data that specifies no password at all for a new user then
"useradd" is called with no "-p" option, so causing "useradd" to lock
the password. The password field is left locked.
In all the above scenarios "passwd -l" may be called later by
cloud-init to enforce "lock_passwd: true").
Conversely where "lock_passwd: false" applies the above "usermod"
situation (for "hash_passwd", "plain_text_passwd" or no
password) means that newly created users may have password
fields locked when they should be unlocked.
For Alpine, "adduser" does not support any form of password being
passed and it always locks the password field (the same point
applies about password field being unlocked when/if "chpasswd" is
called). Therefore in some situations (i.e. no password specified
in user-data) the password needs to be unlocked if
"lock_passwd: false".
This PR changes add_user (in both __init__.py and alpine.py) to
explicitly call either lock_passwd or unlock_passwd at all times to
achieve the desired final result.
Additional Context
Test Steps
User-data:
users:
- name: hash
hash_passwd: $6$nonsense
locked_passd: false
- name: password
passwd: $6$nonsense
locked_passd: false
- name: plain
plain_text_passwd: test
locked_passd: false
- name: none
locked_passd: false
Then check /etc/shadow to see if all the above users have locked or unlocked password fields.
Checklist
- [x] My code follows the process laid out in the documentation
- [x] I have updated or added any unit tests accordingly
- [x] I have updated or added any documentation accordingly
Merge type
- [x] Squash merge using "Proposed Commit Message"
- [ ] Rebase and merge unique commits. Requires commit messages per-commit each referencing the pull request number (#<PR_NUM>)
@holmanb I'm guessing your "exception" changes (#5202) suggestion would also apply here to init.py
Hmm, I just noticed that freebsd.py does not have an unlock_passwd function, whereas netbsd.py and opensdb.py do.
The BSDs are a little strange in that there is no distinction between password locking and account locking/disabling, unlike Linux.
@igalic any thoughts? Would "pw usermod
Given that we want security review on this behavior let's push this to 24.3 milestone to make sure we can come up with an acceptable/explicit solution that allows password-less account and test this broadly
I feel like this represents a behavior change broadly that now allows for setup of password-less user accounts which I didn't think cloud-init allowed previously because of security concerns.
@blackboxsw
Well cloud-init has always allowed the setup of password-less user accounts when neither "passwd", "hashed_passwd", nor "plain_text_passwd" are specified for a user.
I assume you meant 'password-less and not-locked user accounts'. cloud-init has always only enforced the locking of passwords whenever "lock_passwd: true" (whether as defined by default in /etc/cloud/cloud.cfg or explicitly specified in user-data).
If "lock_passwd: true" is not specified, indeed if "lock_passwd" is not specified at all, then cloud-init makes no attempt to lock passwords.
I think we may want to avoid 24.2 for this particular behavior change because I think it warrants an extra set of eyes from our security team to think through the implications of a configured vm w/ user accounts that can be allowed to have empty passwords.
Ok.
I believe if we can come to a strict requirement in cloud-config configuration that establishes a pattern where the admin can be explicit that they do want a password-less user account, I'd feel better about this approach instead of just keying on
lock_passwd: false.
There are 2 distinct issues here: the issue of empty/blank passwords, and the issue of password-accessible accounts (i.e. via SSH) whether they have passwords or not. Yes the 2 issues are inter-related but they are also distinct issues.
One suggestion would be to deprecate "lock_passwd" and as a replacement to define "unlock_passwd" instead (which is "false" by default) and perhaps to also define "unlock_when_passwd_blank" (again "false" by default) as an additional level of safety.
@blackboxsw
I looked at your comments out of order. I'll look into the combination of lock_passwd: false and passwd: "" as you suggested.
@blackboxsw
How would you suggest treating "lock_passwd" for existing users? i.e. this config:
users:
- name: existinguser
lock_passwd: false
Should the code grep /etc/shadow to check whether the existing user's password is blank and only unlock when it is not blank?
@blackboxsw
How would you suggest treating "lock_passwd" for existing users? i.e. this config:
users: - name: existinguser lock_passwd: falseShould the code grep /etc/shadow to check whether the existing user's password is blank and only unlock when it is not blank?
@dermotbradley That adds a bit of complexity certainly. But, I'm understanding that the use-case here would be to support custom images w/ precreated user account which allows image creators to define an opaque password that we don't want to represent in sensitive user-data. I think we actually may have to treat that case as I believe Azure has cases where they may set lock_passwd = False during preprovisioning and not provide a hashed_password when the password was redacted due to lack of complexity. In those cases where user-exists from an earlier provisioning stage, but we don't represent that in current boot's user-data, we'd probably want to allow that account to login, but only if it has some representation of complexity (like non-empty).
CC: @cjp256 FYI here as this PR in progress touches represents a potential change in behavior as it will explicitly perform an unlock of a account's password via passwd -u <default_username> when lock_passwd: false is provided for the user in configuration. This differs from previous behavior on lock_passwd: false which only would only avoid calling passwd -l <username> in distros.lock_passwd
@dermotbradley That adds a bit of complexity certainly. But, I'm understanding that the use-case here would be to support custom images w/ precreated user account which allows image creators to define an opaque password that we don't want to represent in sensitive user-data.
@blackboxsw I already went ahead and implemented /etc/shadow password value checking for pre-existing user accounts.
I have reworked my changes based on your feedback and manually tested it locally. I haven't yet pushed my changes as I'm trying to determine which tests to modify or add for this.
I could go ahead now and push my code changes (without testcase changes) anyway if you want to have a look at it...
@dermotbradley please feel free to push and I'll understand more may come or you may squash merge testing as added. I could assist if needed on test generation front for this.
@dermotbradley please feel free to push and I'll understand more may come or you may squash merge testing as added. I could assist if needed on test generation front for this.
Ok, I've pushed my current version.
Addressed:
- FreeBSD/OpenBSD changes
Still to be addressed:
- testcases