search

openbmc / bmcweb

A do everything Redfish, KVM, GUI, and DBus webserver for OpenBMC
Apache License 2.0
148 stars 130 forks source link

Web sessions are not terminated for user change events #255

Closed thangqn-ampere closed 1 year ago

thangqn-ampere commented 1 year ago

Describe the bug

It is expected for all user sessions terminated for the following changes:

https://github.com/openbmc/bmcweb/commit/9fa06f190a025693af3b7cc76c8b19163afe4d08 supported the case of user deletion and username changed. Need to support for other cases.

Environment

Tested on Ampere Mt.Jade and Mt.Mitchell at https://github.com/openbmc/openbmc/commit/099306430f5219b5f5805368f6052f920af3c260.

To Reproduce

Steps to reproduce the behavior:

  1. Log in WebUI with root account and create a new test user.
  2. Using another web browser to login with the new user.
  3. On the root's web browser, update the test user like change username, set password, ...
  4. It is expected all sessions for the test user are terminated, but not happen.
  5. Change test user information via IPMI with root account. No session is terminated.
  6. Change test user information via BMC console with root account. No session is terminated also.

Is this a regression No

edtanous commented 1 year ago

It is expected

What are you basing this on? Is this a Redfish requirement? To my reading, this is sounding like a feature you're looking for. If it's not a bug, please feel free to discuss with the community on discord or the mailing list. Opening bugs is not how we manage features, as the templates suggest.

thangqn-ampere commented 1 year ago

It is not new feature but security issue from my point of view. I don't find any requirement for Redfish on this topic but let consider situation when users still work why administrator makes change on his account, even deleting it. I opened this ticket to discuss more about above cases and how we can fix them. If you think email is better, I can send email for more discussion about what should be fixed and how.

williamspatrick commented 1 year ago

There were already some discussions on this topic in Discord in March. Linking the start of the conversation here as a reference:

https://discord.com/channels/775381525260664832/1005215504417435668/1085438478893981776

I think this is only a bug if you can point to a readily available / widely accepted standard that suggests the current behavior is wrong from a security posture. Ed has previously asked for that and nobody has pointed to one.

williamspatrick commented 1 year ago

I think this is only a bug if...

That's not to say it wouldn't be good for someone to make this better. Just a matter of someone deciding to contribute it.

edtanous commented 1 year ago

I think this is only a bug if...

That's not to say it wouldn't be good for someone to make this better. Just a matter of someone deciding to contribute it.

+1. If this is something you're interested in tackling, it seems fairly straightforward in code. Please just send patches to that effect, including the widely accepted standard @williamspatrick mentions.

thangqn-ampere commented 1 year ago

There were already some discussions on this topic in Discord in March. Linking the start of the conversation here as a reference:

https://discord.com/channels/775381525260664832/1005215504417435668/1085438478893981776

I think this is only a bug if you can point to a readily available / widely accepted standard that suggests the current behavior is wrong from a security posture. Ed has previously asked for that and nobody has pointed to one.

Can't still find any specification for it also. Just feel like it should be so I would like to open to discuss

thangqn-ampere commented 1 year ago

I think this is only a bug if...

That's not to say it wouldn't be good for someone to make this better. Just a matter of someone deciding to contribute it.

+1. If this is something you're interested in tackling, it seems fairly straightforward in code. Please just send patches to that effect, including the widely accepted standard @williamspatrick mentions.

Thanks for your comments. We will submit a patch to handle privilege change. Still no idea for account locked and password change (and wonder if it is needed for these cases).

edtanous commented 1 year ago

Closing, as discussion seems complete, and any further discussion should happen on the appropriate discussion channels.