opnsense / core

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

Separate 2FA OTP field on login page #6310

Closed mimizone closed 11 months ago

mimizone commented 1 year 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.

User Experience improvement, not a functional problem

Describe the solution you like

When OTP 2FA is enabled for the Web UI access, there should be a separate/dedicated input field for the OTP in order for password managers to autofill.

Describe alternatives you considered

The alternative now is to have a 2 step process in a password manager to retrieve the password and then the OTP and copy paste them one after another, which is cumbersome to do, and not automatic.

Additional context

.

OPNsense-bot commented 1 year ago

Thank you for creating an issue. Since the ticket doesn't seem to be using one of our templates, we're marking this issue as low priority until further notice.

For more information about the policies for this repository, please read https://github.com/opnsense/core/blob/master/CONTRIBUTING.md for further details.

The easiest option to gain traction is to close this ticket and open a new one using one of our templates.

meyergru commented 1 year ago

This has got to be the bazillionth time this has been requested. See https://github.com/opnsense/core/issues/4966 for an example.

AdSchellevis commented 1 year ago

https://github.com/opnsense/core/issues/6228#issuecomment-1423363905

mimizone commented 1 year ago

I noticed now yes. That's good. Many people seem to need this for the good reason of using 2FA and be more secured.

All issues being closed, it doesn't show up if you do a default search for issues, thinking the feature is not implemented yet. Is that something considered to be added to upcoming features already and/or it is stuck in looking for a contributor?

mimugmail commented 1 year ago

A third field field instead of 2 where OTP is behind or infront is no security gain:)

meyergru commented 1 year ago

In and of itself, it is not. However, if you also want to eat your cake (= more security) and still have it (= no less comfortable), then you probably use a password manager that can handle TOTP automatically. I know, you could argue that using a passwort manager is insecure by any stretch of imagination.

Most of those tools can handle 3 fields, but not 2 with one being split up, 2FAS being an example.

So the reason would be that more people might actually use TOTP if OpnSense's implementation was more "normal" in that respect, fitting those management tools. By "normal" I mean 3 separate fields, as for example openssh does when configured correctly via PAM. In their case, you can even see if TOTP is active for an account after having entered the correct password. With a web GUI, you would probably use 3 fields and use the TOTP one only when needed, as having a popup or second page would also lock out some passwort managers - or so I guess.

meyergru commented 1 year ago

Actually, I also do not quite understand @AdSchellevis to close this issue, pointing to another issue, having critized me for hijacking that very other issue with this topic. The other one is about a third input for VPN, not for logging into the Web UI, which this and many other closed issues were about...

Now we again do have a separate issue and then it is closed?

AdSchellevis commented 1 year ago

probably misread the topic, reopened.

mimizone commented 1 year ago

mimugmail Of course the point is not that a third field brings additional security technically speaking. It is that without it, it's cumbersome to use when users are used to autofill from their password managers. So the user/admin will revert to not using 2FA, which is less secure. Password managers could help by automatically concatenating the 2 values (for instance there is a feature request for 1password). But I haven't seen any doing it yet. (I use both 1password for work and Bitwarden for personal).

meyergru commented 1 year ago

I found that KeePass2 together with Kee browser plugin cannot do this either, but KeePassXC with its browser plugin actually can via a self-defined variable consisting of the concatenated password and TOTP, For the time being, that is what I use.

shaul75 commented 1 year ago

mimugmail Of course the point is not that a third field brings additional security technically speaking. It is that without it, it's cumbersome to use when users are used to autofill from their password managers. So the user/admin will revert to not using 2FA, which is less secure. Password managers could help by automatically concatenating the 2 values (for instance there is a feature request for 1password). But I haven't seen any doing it yet. (I use both 1password for work and Bitwarden for personal).

I doubt password managers would implement that anytime soon, OPNsense is the only one I've seen that doesn't do the so-called "standard" 2FA login.

Is there a reason not to split it to a 3rd field? (the reason for it have obviously been stated already)

mimugmail commented 1 year ago

mimugmail Of course the point is not that a third field brings additional security technically speaking. It is that without it, it's cumbersome to use when users are used to autofill from their password managers. So the user/admin will revert to not using 2FA, which is less secure. Password managers could help by automatically concatenating the 2 values (for instance there is a feature request for 1password). But I haven't seen any doing it yet. (I use both 1password for work and Bitwarden for personal).

I doubt password managers would implement that anytime soon, OPNsense is the only one I've seen that doesn't do the so-called "standard" 2FA login.

Is there a reason not to split it to a 3rd field? (the reason for it have obviously been stated already)

So, FortiGate seems to do it like OPN: https://www.token2.com/site/page/hardware-tokens-for-two-factor-authentication-with-fortigate?totpradius

With Cisco ASA I also never saw a field in ASDM Manager, and Sophos UTM also doesn't support it. With Sophos XG there is a captcha field, but I never used OTP there.

Which Firewalls you have used and to support a separate field for login?

fichtner commented 1 year ago

One reason I still doubt separate OTP field, especially on a separate page after login is that user enumeration is easily carried out. Sure if you look at GitHub for example where they do this you have to keep in mind user names are already public. ;)

meyergru commented 1 year ago

It should not be on a separate page, as it may be dependent on whether 2FA is needed in case you specify multiple authentication mechanisms. You would leave the 3rd field empty when you use a password-only credential.

fichtner commented 1 year ago

Should we tell GitHub then they do it wrong? I'm really unsure here where the baseline is at.

meyergru commented 1 year ago

They definitely do it wrong, you are totally correct with the enumeration argument. Although in practice, the "independent factor" criteria is problematic for some use-cases: For PSD2 e.g., in theory, you should never give away which of the two factors were wrong when denying access, This gets a bit complicated (and intransparent) when the second factor can only be selected after the first has been verified, for example for SMS OTP. You would need to present fake selections in case the first factor was wrong.

For TOTP, this is no problem and the error message should not give away what exactly went wrong.

shaul75 commented 1 year ago

mimugmail Of course the point is not that a third field brings additional security technically speaking. It is that without it, it's cumbersome to use when users are used to autofill from their password managers. So the user/admin will revert to not using 2FA, which is less secure. Password managers could help by automatically concatenating the 2 values (for instance there is a feature request for 1password). But I haven't seen any doing it yet. (I use both 1password for work and Bitwarden for personal).

I doubt password managers would implement that anytime soon, OPNsense is the only one I've seen that doesn't do the so-called "standard" 2FA login. Is there a reason not to split it to a 3rd field? (the reason for it have obviously been stated already)

So, FortiGate seems to do it like OPN: https://www.token2.com/site/page/hardware-tokens-for-two-factor-authentication-with-fortigate?totpradius

With Cisco ASA I also never saw a field in ASDM Manager, and Sophos UTM also doesn't support it. With Sophos XG there is a captcha field, but I never used OTP there.

Which Firewalls you have used and to support a separate field for login?

I wasn't referring to any specific firewall, but to 2FA authentications on web login pages, in general. Why should it be different?

If the field is not separate from the password, and the error message is telling you "Wrong username or password.", I will think my password is wrong, not that I need to prepend/append a TOTP to the actual password. This is especially relevant when you're not frequently signing-in, people are likely to forget about setting a TOTP which needs to be prepended and just get frustrated about the failed login. This "security through obscurity" has some benefit against malicious actors, but it's so tiny when comparing it to the bad UX you get.

AnAnalogGuy commented 1 year ago

Also a vote from my side to have the OTP field seperated (on the same page!) as it improves UX with no negative impact on security. In terms of implementation: Do i miss something or wouldn't this be a quick change? A new box on the FE and some var1+var2 on the backend? We could even implement a switch so users can decide which implementation they want, right?

I'm not familiar with the dev process in the opnsense project: Besides the code, what else do we need? Documentation, test cases, unit tests, ...?

-- From a very very quick look into the code it seems a quick and dirty hack could be to introcude a OTP field in the login form, transmit it via POST and concatenate the string here, to change as little as possible (yes, i'm aware it will need so additional checks i.e. reverse order):

grafik

If i get a feedback from the devs regarding if they would accept such an approach, i would have a closer look at it.

OPNsense-bot commented 11 months ago

This issue has been automatically timed-out (after 180 days of inactivity).

For more information about the policies for this repository, please read https://github.com/opnsense/core/blob/master/CONTRIBUTING.md for further details.

If someone wants to step up and work on this issue, just let us know, so we can reopen the issue and assign an owner to it.

AnAnalogGuy commented 11 months ago

Help wanted? Well, I offered help like half a year ago with one very simple little question to be answered by the lead of this project. Since i got no reply, i just changed the implementation on my side and did not contribute back. Somehow a bit of a waste of time this board - lots of discussion why and why not and ..... but when someone is willing to talk less and get the things done, no reaction.

mimizone commented 8 months ago

i just changed the implementation on my side and did not contribute back.

@AnAnalogGuy Do you mind sharing the code?

JSuenram commented 1 month ago

+1 from here. We have about 24 opnsense Boxes most of them reachable from the Internet to be remote mangaable. The missing extra field for the TOTP-Code makes our customers man usability problems. They simply forgot that they have to add it to the passoword field. A simple box for the code to be entered, can't be that hard?

AnAnalogGuy commented 1 month ago

It's still up to the project owners to tell me, if they would accept a change as i proposed and to provide a bit guideance, since the last thing I'm going to do is to waste my time providing a pull request finding out they don't care about it. Now it's more than a year and not one single reaction from the project and the ticket closed without any reply. Very very professional and repectful.

AdSchellevis commented 1 month ago

@AnAnalogGuy

We responded on this and earlier tickets that it's not a priority, but we will review a clean (non hacky) solution in a PR. In the long run this page likely needs an overhaul to move it out of the legacy scheme.

Some of the challenges of implementing this cleanly are:

To some degree I can understand people don't want to waste their time, but that's not really how this works (remember you are asking time from other people here). If there is a proposal to discuss in terms of code (or a step by step plan), it makes it more valuable from our end to spend (waste?) our time as well.

So, if the question is, "is there room to implement a solution for the ui for this", the answer is yes. When the question is, "are you going to spend a lot of your time to make it happen", the current answer is no. A proper implementation this costs quite some time to build and test given the constraints that exists.

A hacky solution is likely already implementable using a custom theme package by the way when adding a theme.js file to overlay the input box.

When reimplementing the legacy page into a new mvc one from our end, it makes sense to take this into account as well, but in the grant scheme of things, this, at the moment, is just not a priority.