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

Signal PropertiesChanged when password expires #9

Open
joseph-reynolds opened this issue Feb 19, 2021 · 6 comments
Open

Signal PropertiesChanged when password expires #9

joseph-reynolds opened this issue Feb 19, 2021 · 6 comments

Comments

@joseph-reynolds
Copy link

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
Copy link

edtanous commented Feb 19, 2021

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
Copy link
Author

joseph-reynolds commented Feb 22, 2021

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
Copy link
Author

joseph-reynolds commented Feb 23, 2021

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:

  • set up inotify to get shadow file changes, and send the PropertiesChanged signal as appropriate.
  • event loop to listen for both D-Bus events and inotify events (can we use select(2) with D-Bus?)
  • any issues with mutual exclusion (handle in the event loop?) or sending excessive signals?

@rthomaiy
Copy link

@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
Copy link
Contributor

ratagupt commented Feb 3, 2022

@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
Copy link
Author

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?

dkodihal pushed a commit to NVIDIA/phosphor-user-manager that referenced this issue May 7, 2024
```
Changes Added : Added event loop support in test code

problem : After adding support for sending events from phosphor-user-manager it is obsderved that it is calling sendEvent which internally calls async_send_handler and allocates memeory for context
since the event loop is not present in test code, callback is never called and
the CI was throwing memory leak error

Direct leak of 1 byte(s) in 1 object(s) allocated from:
    #0 0x7ffa787b91e7 in operator new(unsigned long) ../../../../src/libsanitizer/asan/asan_new_delete.cpp:99
    openbmc#1 0x7ffa77ad6383 in operator() /usr/local/include/sdbusplus/asio/detail/async_send_handler.hpp:40
    openbmc#2 0x7ffa77ad6383 in async_send<sdbusplus::asio::connection::async_method_call_timed<phosphor::logging::sendEvent(phosphor::logging::MESSAGE_TYPE, sdbusplus::xyz::openbmc_project::Logging::server::Entry::Level, const std::vector<std::__cxx11::basic_string<char> >&, const string&)::<lambda(boost::system::error_code)>, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::map<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::less<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::allocator<std::pair<const std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > > >
    (phosphor::logging::sendEvent(phosphor::logging::MESSAGE_TYPE, sdbusplus::xyz::openbmc_project::Logging::server::Entry::Level, const std::vector<std::__cxx11::basic_string<char> >&, const string&)::<lambda(boost::system::error_code)>&&, const string&, const string&, const string&, const string&, uint64_t, const std::__cxx11::basic_string<char>&, const std::__cxx11::basic_string<char>&, const std::map<std::__cxx11::basic_string<char>, std::__cxx11::basic_string<char> >&)::<lambda(boost::system::error_code, sdbusplus::message_t&)> > /usr/local/include/sdbusplus/asio/connection.hpp:98
    openbmc#3 0x7ffa77ad6383 in async_method_call_timed<phosphor::logging::sendEvent(phosphor::logging::MESSAGE_TYPE, sdbusplus::xyz::openbmc_project::Logging::server::Entry::Level, const std::vector<std::__cxx11::basic_string<char> >&, const string&)::<lambda(boost::system::error_code)>, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::map<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::less<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::allocator<std::pair<const std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > > > /usr/local/include/sdbusplus/asio/connection.hpp:192
    openbmc#4 0x7ffa77ad6383 in async_method_call<phosphor::logging::sendEvent(phosphor::logging::MESSAGE_TYPE, sdbusplus::xyz::openbmc_project::Logging::server::Entry::Level, const std::vector<std::__cxx11::basic_string<char> >&, const string&)::<lambda(boost::system::error_code)>, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::map<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::less<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::allocator<std::pair<const std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > > > /usr/local/include/sdbusplus/asio/connection.hpp:221
    openbmc#5 0x7ffa77ad6383 in phosphor::logging::sendEvent(phosphor::logging::MESSAGE_TYPE, sdbusplus::xyz::openbmc_project::Logging::server::Entry::Level, std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) ../lib/redfish_event_log.cpp:112
    openbmc#6 0x55dfba9084a3 in phosphor::certs::Manager::replaceCertificate(phosphor::certs::Certificate*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) ../certs_manager.cpp:493
    openbmc#7 0x55dfba8ab09d in phosphor::certs::Certificate::replace(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >) ../certificate.cpp:315
    openbmc#8 0x55dfba7981e0 in TestBody ../test/certs_manager_test.cpp:677
    openbmc#9 0x7ffa786e3f2e in void testing::internal::HandleSehExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) /googletest-662fe38e44900c007eccb65a5d2ea19df7bd520e/googletest/src/gtest.cc:2607
    openbmc#10 0x7ffa786e3f2e in void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) /googletest-662fe38e44900c007eccb65a5d2ea19df7bd520e/googletest/src/gtest.cc:2643

```

Solution : The memory leak error was thrown because the
memory allocated by "async_send_handler" in sdbusplus was not getting de-allocated
because the callback is never getting called called since there was no event loop
present in test code.

Added event loop support in test code

Fixes jira https://jirasw.nvidia.com/browse/DGXOPENBMC-8881
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants