BMCWeb changes for expired password design

BMCWeb changes for expired password design

[Joseph Reynolds]

Ed,

Please review the "expired password" design: 
https://gerrit.openbmc-project.xyz/c/openbmc/docs/+/23849

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

- Joseph

Re: BMCWeb changes for expired password design

[Ed Tanous]

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.

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.

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.

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

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

Re: BMCWeb changes for expired password design

[Joseph Reynolds]

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