BMCWeb changes for expired password design

[Joseph Reynolds]

On 8/13/19 3:38 PM, Ed Tanous wrote:
> On 8/13/19 1:09 PM, Joseph Reynolds wrote:
>> Ed,
>>
>> Please review the "expired password" design:
>> https://gerrit.openbmc-project.xyz/c/openbmc/docs/+/23849
>>
> Richard would likely be a better person to poke, as he wrote the user
> management design doc originally.
+1

>
> My main concerns are around the requirements.  I would like to see
> mentioned that we:
> 1. Intend to implement something in compliance with California bill
> SB-327 Information privacy: connected devices, and document the relevant
> requirements from that bill.
> 2. We intend to implement the Redfish initial password mechanism as the
> spec has defined, to ensure interop with other Redfish devices.
>
> I think the details on the second one seem to be lacking at bit.  I see
> a lot of design elements and flows, but I would've expected most of it
> to simply be "we intend to implement the Redfish flow for default
> passwords" then only provide details where it's missing or optional in
> the spec.
>
> If we're building something that's redfish, but unique to OpenBMC , or
> counter to the spec, I'm going to have a tough time supporting it, as it
> will cause confusion, and somewhat defeat the purpose of having an
> industry standard.
>
>> Implementing this requires a few BMCWeb changes:
>> - For the `/login` URI: when a correct but expired password is given,
>> indicate the password was expired via HTTP response body:
>> "Unauthorized.  Password expired.  Use Redfish APIs to change the
>> password.", and do not create a session.
>> - For Basic Auth (https://user:password@host): when a correct but
>> expired password is given, give HTTP response code 403 or similar.
> With what content in the response?  This will need to differ for
> Redfish, the rest dbus apis, and HTML responses, as they all expect
> different error handling.  Simply mentioning that you intend to support
> the correct error codes for all 3 would be fine.
>
>> - For Redfish sessions: when a correct password is given, create the
>> session as usual, but set the PasswordChangeRequired field (based on
>> PAM_NEW_AUTHTOK_REQD).
>> - Limit access from sessions which have PasswordChangeRequired=True as
>> follows:
>>      + The session can only be used to GET its own account and session
>> information, PATCH its own account's password, and log out.
>>      + Successfully changing the password terminates the session. That
>> is, the session does not change from PasswordChangeRequired=True to
>> PasswordChangeRequired=False.
> I'm interested to see how this will be executed in practice, as we only
> have a fixed-at-compile-time privilege to url map.  This would need to
> be adjusted, and we would either need to define a new OEM privilege type
> (that we may or may not hide) or come up with a way to dynamically
> adjust privileges on the fly.
>
>>      + Other uses get HTTP response code 403 (or similar).
>> - The existing password changing mechanism would be used, with the
>> additional behavior that when ((PasswordChangeRequired=True) and (the
>> password was successfully changed)), the session will terminate.
>>
>> These changes are based on the design and the Redfish
>> PasswordChangeRequired handling specifications (referenced by the
>> design).  Would you take a BMCWeb patch to implement this?
> I typed the above having looked at an old version of your document.  You
> seem to have adjusted it a bit to cover the redfish stuff, which is
> excellen.
>
> Yes, I would have no problem with a patchset to do this.  Some important
> design points to consider.
>
> 1. Please do not recreate the existing privileges module to do the
> above.  Your design should be able to fit into what's there.
> 2. Please do not have a fixed "go/no-go" url list separate from the
> other 2 we already have.  Please keep your changes inside the existing
> url routing infrastructure by tagging your "ok without auth" routes
> appropriately.

Here are some low level design ideas:

Enhance bmcweb/include/pam_authenticate.hpp - pamAuthenticateUser() like:
   inline bool pamAuthenticateUser(const std::string_view username,
     const std::string_view password,
     bool &passwordChangeRequired)
so that if the password is correct but expired, the function will return 
true and set passwordChangeRequired=true.

This change will get the passwordChangeRequired status to where it is 
needed.  As you mentioned, each of those will respond differently:
- Basic Auth will fail when passwordChangeRequired=true
- POST /login will fail when passwordChangeRequired=true, with an 
additional message
- POST /redfish/v1/SessionManager/Sessions will succeed when 
passwordChangeRequired=true

Naturally, the crow::persistent_data::UserSession will store the new 
passwordChangeRequired field, with all the changes that requires.

Skipping ahead a bit, I think 
crow::token_authorization::Middleware.beforeHandle() should have the 
following logic after it successfully locates a session:
   if (req.session.passwordChangeRequired &&
       !isOnPasswordChangeRequiredWhitelist(req))
   then fail with HTTP status 403 and an explanation in the payload

Where new function isOnPasswordChangeRequiredWhitelist returns true in 
the following cases:
   isOnWhitelist(req) || GET or DELETE mySession || GET or PATCH myAccount

Doing it this way seems the most clear and only adds a few cycles in the 
usual case.  It seems like having a new whitelist for this situation is 
correct because Redfish specifies that these interfaces are needed for 
PasswordChangeRequired handling.  And this way avoids having to change 
the authority model.

What do you think?

- Joseph

>
> I suspect overall, this is going to be difficult, but worthwhile, and
> I'd be happy to help review your changes when they're ready.
>