BMCWeb changes for expired password design

Fri Aug 23 09:20:55 AEST 2019

On 8/15/19 6:13 PM, Ed Tanous wrote:
> On 8/15/19 3:46 PM, Joseph Reynolds wrote:
>> 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
> Sounds good.
>
>> Naturally, the crow::persistent_data::UserSession will store the new
>> passwordChangeRequired field, with all the changes that requires.
> This is what I meant about using the existing privilege system and not
> hardcoding it.  I suspect when we hit this, we need to give the user a
> "OnlyCanChangePassword" privilege, then tag some routes appropriately
> with that flag.  In this way, we will still properly handle all the
> routes that don't require auth (like ServiceRoot) but can reject any
> requests that require any privileges if the password hasn't been set.

Agreed.  The OnlyCanChangePassword privilege is exactly the Redfish 
ConfigureSelf privilege.  (I learned about that after I wrote my first 
email.)

>
> You would amend this function to handle the case where the users
> password needs changed, and to apply the correct Privilege.
> https://github.com/openbmc/bmcweb/blob/a2730f017069aeb39ea5d3bf4c1403965b2ba2f9/redfish-core/include/privileges.hpp#L180
>
>
>> 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
> See above statement, use the privileges module to do this.
> This would mechanically look like going here:
> https://github.com/openbmc/bmcweb/blob/a2730f017069aeb39ea5d3bf4c1403965b2ba2f9/crow/include/crow/routing.h#L1245
>
> and adding something like
>          if (!rules[ruleIndex]->checkPrivileges(userPrivileges))
>          {
>              // if the user was only granted the ability to change the
> password, print the correct message.
>              if (userPrivileges.isSupersetOf({"OnlyCanChangePassword"}){
> 		// res.result.jsonValue = ...... appropriate error message for the type.
>              } else {
>                  //While we're here, and we're implementing error codes
> properly, we might as well fix the normal 403 handler.
>              }
>              res.result(boost::beast::http::status::forbidden);
>              res.end();
>              return;
>          }
>
>
>> Where new function isOnPasswordChangeRequiredWhitelist returns true in
>> the following cases:
>>    isOnWhitelist(req) || GET or DELETE mySession || GET or PATCH myAccount
> This is exactly what I meant when I said "do not have a fixed "go/no-go"
> url list".  We already have a privilege system that can tag handlers,
> and has more information about the route map than can be provided in a
> single if statement.  Lets use it as it was intended.
>
> Also, I'm not sure what you meant by mySession and myAccount.   I'm
> assuming you meant AccountService and SessionService?
Close.  I meant the actual account and session resource within the 
collection.  I've clarified this in my discussion below.
>
> Another thing to realize:  With the last round of per-verb router
> registrations that went in several months ago, isOnWhitelist is likely
> going away, as it's redundant to the existing privilege mechanisms (and
> super inefficient to boot).  It's only still there because I have a
> healthy paranoia of removing security features whitelists like that
> without testing the hell out of the changeset ahead of time.

Thanks for your response and the links into BMCWeb code.

I think BMCWeb needs tweaks to its authority model to match the Redfish 
spec for the ConfigureSelf privilege.  (And I apologize in advance for 
the C++ish code in this email.)

The key parts of the Redfish spec are below. An understanding of these 
sections is required to understand the BMCWeb authority changes I am 
proposing.

The Redfish spec 1.7.0
http://redfish.dmtf.org/schemas/DSP0266_1.7.0.html
   Section 13.2.6.1 ("Password change required handling")
   Section 13.2.10.5 ("Property override example")

DSP2046 The Redfish Resource and Schema Guide, version 2019.1
http://redfish.dmtf.org/schemas/DSP2046_2019.1.html
   search for "ConfigureSelf"

Change 1: When a user establishes a session where 
PasswordChangeRequired=True, that session should only have the 
ConfigureSelf privilege; the user's normal role should be disregarded 
for that session.

One way to handle this is to create a new user role 
"priv-configure-self" (used only internally within BMCWeb) which grants 
only the Redfish ConfigureSelf privilege.  Then when creating the 
UserSession:
   if (pamAuthenticateUser() indicated passwordChangeRequired==true)
     session.userRole = "priv-configure-self";  // override

This sets up the idea (mentioned in your email) that you can use the 
following code in routing.h:Router.handle() to detect if the current 
session is for password change only:
     userPrivileges.isSupersetOf({"ConfigureSelf"})

[routing.h]: 
https://github.com/openbmc/bmcweb/blob/master/crow/include/crow/routing.h

Change 2: In routing.h:Router.handle(), when 
(userPrivileges.isSupersetOf({"ConfigureSelf"}) == true) add the:
    "@Message.ExtendedInfo object containing the PasswordChangeRequired 
message from the Base Message Registry"
to the HTTP 403 response.  (The quoted language is from the Redfish 
spec, section 13.2.6.1.)

Change 3: Ignore the user's ConfigureSelf privilege when accessing an 
account or session which is not theirs.  Details:

I think we need to change routing.h:Router.handle() to implement the 
Redfish ConfigureSelf privilege.
The Redfish ConfigureSelf privilege 
(http://redfish.dmtf.org/schemas/DSP2046_2019.1.html) is defined as, 
"Able to change the password for the current user Account."  I 
understand this privilege should also allow the user to terminate their 
session.  The relevant routes and verbs which should be allowed are:
   /redfish/v1/AccountService/Accounts/<account>/  GET
   /redfish/v1/AccountService/Accounts/<account>/  PATCH (only Password, 
see change 4 below) and GET
   /redfish/v1/SessionService/Sessions/<session>/   DELETE

Specifically, in routing.h:Router.handle():
if
   (((rules[ruleIndex].rule matches
       "/redfish/v1/AccountService/Accounts/<account>/")
   and
     ("<account>" does not match session.username))
or
     (((rules[ruleIndex].rule matches
     "/redfish/v1/SessionService/Sessions/<session>/")
   and
     ("<session>" does not match session.uniqueId)))
then
     // if we got here, the user is accessing an account
     // or session not their own, so the ConfigureSelf
     // privilege does not apply.
     // remove the ConfigureSelf privilege:
     userPrivileges = userPrivileges.remove({ConfigureSelf});
endif
...perform the authority check against userPrivileges as usual ...

Change 4: ConfigureSelf should only apply to PATCH 
/redfish/v1/AccountService/Accounts/<account> Password, not to the 
entire ManagerAccount resource.
This special case is seen in the example in section 13.2.10.5 referenced 
above.  It is needed because ConfigureSelf applies specifically to the 
ManagerAccount Password property, and no other properties in that resources.

To handle this make two changes to: 
https://github.com/openbmc/bmcweb/blob/master/redfish-core/lib/account_service.hpp 
class ManagerAccount:
A. For this resource, boost::beast::http::verb::patch should allow 
{ConfigureSelf}.
B. Add an additional check to doPatch() to ensure ConfigureSelf 
privilege only allows the the Password property to be patched.
If we are trying to PATCH something other than the Password property, 
re-check authority like this:
if ((newUserName or enabled or roleId or locked) and
   (!check user privileges with ({ConfigureSelf}) removed))
then
   fail the request with HTTP 403

Whew.  I think that covers the authority changes needed for BMCWeb to 
implement the PasswordChangeRequired design.  It seems like the design 
is getting closer.  What do you think?

- Joseph

>
>> 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 I suggested above does not require any changes to the authority
> model, aside from adding a privilege type which is supported already,
> and shouldn't require any code changes to the privileges module itself.
>