openbmc / phosphor-user-manager

Apache License 2.0
8 stars 9 forks source link

Signal PropertiesChanged when password expires #9

Open joseph-reynolds opened 3 years ago

joseph-reynolds commented 3 years ago

This issue is to enhance phosphor-user-manager to signal PropertiesChanged when a local user account's password becomes expired (for example, because the password expires due to time passing) or becomes unexpired (for example, because the password is successfully changed). Note that both of these causes are outside the operation of phosphor-user-manager.

These transitions are not currently detected by phosphor-user-manager or any part of Linux. For background, see the shadow file, field 3 (days since epoch of last password change) and field 5 (days before change required). Note also that Linux-PAM also updates the shadow file, for example, as part of a BMCWeb POST /redfish/v1/AccountService/Accounts/USER --data {"Password': "NEWPASSWORD"}.

Here are two ideas (which need to both be implemented) to implement this:

  1. Watch (via inotify) the shadow file for changes and respond to changes: re-read the shadow file, update properties, and send appropriate PropertiesChanged signals.
  2. Predict when the next local user account password will expire (see user_mgr.cpp / userPasswordExpired()) and a set a timer to wake up and check. The handler would re-read the shadow file, update properties, and send appropriate PropertiesChanged signals, and reset the timer.

This would add complexity. Is there a more standard way to accomplish this?

This issue was created in response to a BMCWeb review which relies on the PropertiesChanged signal.

edtanous commented 3 years ago

Minor nit: I suspect if you implemented 1, you wouldn't need to implement 2, so you wouldn't necessarily need both for this to function as designed.

joseph-reynolds commented 3 years ago

I suspect if you implemented 1, you wouldn't need to implement 2

Yes, watching shadow file changes (1 above) without also doing (2 above) would get the function I need for BMCWeb. Specifically, a local user who logs in with an expired password and successfully changes it would result in Linux-PAM module pam_unix.so writing into the shadow file. Via (1 above) this property change would propagate to phosphor-user-manager to the D-Bus signal and then to BMCWeb who would then read the updated PasswordExpired property, as desired.

However, implementing (1 above) by itself does not cover the case of a password expiring due to password aging. (Note that OpenBMC does not configure password aging by default, but IMHO it is legitimate for a downstream user to configure their BMC so local account passwords do age out, for example they could expire every 120 days.) In this scenario, the key point is that when the password ages out, the shadow file does not change at that point in time. Nor does the shadow file (or any other file) change if someone's login request is rejected due to an expired password. For example, if a user attempts to log in with an expired password, the pam_unix.so module will reject the request based on the content of /etc/shadow, but will not modify that file.

The proposal for (2 above) addresses this case.

joseph-reynolds commented 3 years ago

Would it be better to require all password changes to go through phosphor-user-manager D-Bus APIs? Doing so removes the need to watch the shadow file. Here is a design sketch:

  1. Create a new write-only Password property in User.Attributes. This would invoke pam_chauthtok() as the mechanism to to change the shadow file. Send the PropertiesChanged D-Bus signal for success.
  2. Then migrate all existing uses of pam_chauthtok() to use the new D-Bus API. These include: A. IPMI user set password. B. BMCWeb PamUpdatePassword(). C. SSH server change password dialog (not present in the dropbear server, but functional in OpenSSH) D. Use of the passwd command

I believe handling cases A & B would get a large subset of the function we need. And the effort seems cleaner and easier than enhancing phosphor-user-manager to use inotify. Here is a design sketch:

rthomaiy commented 3 years ago

@joseph-reynolds Few are discussed in early phosphor-user-manager design discussion but never went to implementation stage.

Related to password update and to log out the existing session was planned. The logic planned was

  1. To keep password update in pam itself, so that updating password will be secured (No D-Bus monitoring agent will be able to see the password and memory cleanup will not be a problem too).
  2. generate a PasswordUpdated signal with property to indicate the user name (can rely on pam_exec or write our own pam_module to indicate to a daemon, which can send out the signal (In this way, bmcweb, IPMI etc, can watch for PasswordUpdated signal and accordingly close the session).

Related to password expiry For password expiry, watching the password file alone itself will not solve the problem, as even a time change can make the password expiry along with regular aging. Hence better to run a timer say every 24 hours (or X time), and accordingly query & send out PropertyChanged signal.

ratagupt commented 2 years ago

@joseph-reynolds : I am suspecting why do we need to send the signal out of the BMC, for the password expired ?

I have not seen this in any application which sends the update that your password has been expired, However when you try to login through the password which is expired, then you get a message that your password has been expired which I am hoping that is working fine.

joseph-reynolds commented 2 years ago

Thanks for asking. As I mentioned in the comment, this is in response to https://gerrit.openbmc-project.xyz/c/openbmc/bmcweb/+/39756 In that patchset, the thinking was for an application (such as BMCWeb) to use D-Bus to read all local user accounts and subscribe to signals to know when there are changes. In this way, BMCWeb would not have to make a D-Bus call for every operation and could respond to account changes as they happen. For example: if user X is a Administrator account, and they login, and then while they are logged in their account changes to ReadOnly.... then all subsequent operations performed by that session should be authorized as a ReadOnly account, not as an Administrator.

However, to make this a complete solution, the "password expired" property should also participate. As you can see in the example above, there is no second login attempt.

I'm not sure I explained this well enough. Note also the gerrit review has stalled. So if you can re-read the comments in the gerrit which led to this request, maybe you can get more insight or propose an alternate solution?