opnsense / core

OPNsense GUI, API and systems backend
https://opnsense.org/
BSD 2-Clause "Simplified" License
3.38k stars 757 forks source link

Sensible to hide OTP seed in System > Access > Users > User Edit > OTP seed #7911

Open andrew-ma opened 1 month ago

andrew-ma commented 1 month ago

Important notices

Before you add a new report, we ask you kindly to acknowledge the following:

Is your feature request related to a problem? Please describe.

For the purpose of managing users, I think it is better to hide the OTP seed by default because it is sensitive information and is easily captured by screen recording/screen sharing.

I have to regenerate the OTP seed and re-add it to my OTP Authenticator App every time I

Describe the solution you like

I think it would be sensible to hide the OTP seed either with: 1) similar "Click to unhide" button that is used with the OTP QR code, or 2) make it hidden with asterisks "****" and displayed when a "Show Seed" checkbox next to the input field is toggled on. And make it blank "" when a OTP seed is not generated yet.

Describe alternatives you considered

There are no alternatives.

Additional context

In System > Access > Users > User Edit > OTP seed

cpalv commented 1 month ago

this seems pretty reasonable to me. I can put something together an open a PR if there are no objections

fichtner commented 1 month ago

I don't mind too much either way, but which security issue are we fixing? An admin allowed to edit all users on the system not being allowed to see the OTP seed he can click on to see anyway?

Cheers, Franco

cpalv commented 1 month ago

is easily captured by screen recording/screen sharing.

shoulder surfers?

fichtner commented 1 month ago

Is that the technical term? What about network internals on an admin screen? Passwords on Slack? ;)

Well be my guest to raise a PR but take note that #7904 will rewrite the page for 25.1 anyway and additions may disappear for lack of core team time while converting the pages. The self-service Lobby: Password use case probably also needs fixing.

That being said in MVC we would have the "advanced" toggle which may be all you need here so it's hidden by default. It may just be better to wait this out.

Cheers, Franco

cpalv commented 1 month ago

Not sure I understand the "Lobby: Password" use case. The sensitive information is already hidden by the "password" input type.

Im not exactly sold on the advanced toggle. If its usage is the same as in Services: Unbound DNS: General or Services: Intrusion Detection: Administration then wouldn't that just hide the whole OTP field? As in it wouldn't even appear on the page in the first place? Its nice that the feature is just there and you don't have to find it. It does hide the OTP seed though.

fichtner commented 1 month ago

Its nice that the feature is just there and you don't have to find it.

Well, you're arguing from your preference, not a requirement perspective.

fichtner commented 1 month ago

And just check src/www/system_usermanager_passwordmg.php to see what I mean...

cpalv commented 1 month ago

On another note, I noticed extensive use of jquery's $().click() method. Which has been deprecated. Are there any plans to update? Or is it not a concern?

fichtner commented 1 month ago

Ok I confused the seed and the QR code. Password page seems fine.

Unless we decide to update jquery there is no need for noise.