BMCWeb changes for expired password design

[Ed Tanous]

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.

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?

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.

> 
> 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.