-
Notifications
You must be signed in to change notification settings - Fork 2.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[POC,WIP] Implement 2-Factor Authentication with TOTP or HOTP #7069
base: master
Are you sure you want to change the base?
Conversation
POC/WIP to split everything and reorganize, submitted as an example to better discuss the implementation. Signed-off-by: Christian Marangi <[email protected]>
Pretty good. I was already poking around the sources and wondering how to implement PassKeys for the webGUI. I think PassKeys is a separate effort, yet would make our lives easier/safer. uqr outputs to SVG which seems an improvement over qrencode and it is much smaller. How do we handle includes (do we minify and add to the commit)? It has an MIT license. |
no includes uqr have 0 dependency. I removed the unused part, adapted to work with our current js system and it can be used with just What I need to better check is convert the library to the format we use since it does use let, const shorthand like =>... but those should be easy to convert and can come later... |
Is Maybe syslog is more appropriate? |
ES6 is OK. Retain those. |
no those are output that are then parsed by the luci ubus ucode. It will probably change and that script will be changed to a script that will be imported and the function be used directly... For the sake of the POC I currently call popen and manually call the script. |
So a couple of functionality questions: what happens if there is a config stub for TOTP, but nothing useful is set, or something invalid is set. Does this interfere with login (inhibit it)? I assume it should stop login but I'm thinking about people who test and edge-cases here. Also, for TOTP case, choosing TOTP might mandate that the user have an NTP server set. So a brief description of those two choices under the Something like: o = s.taboption('2factauth', form.ListValue, 'type', _('OTP Type'),
_('TOTP are time-based chronologically sensitive, and require accurate time: NTP must be set.') + '<br />' +
_('HOTP are time insensitive and can be used in any situation.'));
o.uciconfig = 'luci';
o.ucisection = 'root';
o.default = 'TOTP';
o.value('totp', 'TOTP');
/* check NTP is set */
o.value('hotp', 'HOTP'); PS use tabs for indentation. |
Well the verify OTP require all those option so yes those info ""corrupted"" will produce a wrong OTP or actually report error... We might consider disabling OTP in that case but IMHO it's a very corner case. Logic should relay in the dispatcher.uc where the login is handled. Yes all the info still needs to be filled and yes the indentation and spacing is all over the place. |
Also, design: how to handle lock-out? Any user could reset the router via a reset process (that's an involved process) but when turning on 2FA, I think we should also ensure e.g. ssh login is enabled and optionally also some ssh user key is added, in case something with (T)OTP fucks up (time source lost, user lost phone etc etc). Then there is another viable access vector. |
I need to investigate how recovery codes work if it's really worth but for these case the user should have access with ssh anyway. In that case he can just manually disable 2 factor but yes an additional warning to add. Reset the router will disable 2FA (it won't be enabled by default) |
This is my point. Should. But before turning something like this on, (should it have an on/off?), it should verify and say "yo gringo, ssh must be enabled" or something to that effect if ssh access is not enabled. |
I assume a big red box should be enough ? We can assume userbase of openwrt is advanced enough to now lock outside and always have a way to login At worst you have failsafe |
2FA is about additional factors: if one factor is lost, another (ssh) should be available (on) to the legitimate user/owner/admin of the router. And that's low-cost to do with Warning boxes are not guarantees (and they usually mean something popping up which must be swatted away) and a lost phone could mean a router reset. Just thinking that's problematic when maybe the "find my phone" function requires Internet :/ |
sooo check if there is at least one entry enabled, those check can be done directly on enabling the thing. I assume we will have a bool to toggle it... Also I'm thinking of enabling 2 OTP at once. One time based and one HOTP to handle case when the internet is offline. Might be an interesting solution to handle case when time is not OK. |
Good approach. 👍 Some of these scrappy routers don't keep time very well, so some drift might be expected. BTW is it much more difficult to use SHA256? Considering SHA1s deprecation. |
main problem is the app support is shit for advanced option from qr code.... for example ms authenticator app doesn't correctly parse HOTP and assume TOTP is always used. (google authenticator and other app correctly works tho) For sha256 we can use busybox utility directly i assume if we ever wants that. |
Ah, maybe not. That's a sum, not an encryption algo....? Edit: should be fine - it's just a hash used. The MSauth app is shit. I use Authenticator by Matt Rubin, based on OSS stuff. https://mattrubin.me/authenticator/ So there's no excuse not to up our security game :) |
Can we use these in the browser dircetly? Here's a usage example using the above API: https://github.com/pur3miish/Universal-HMAC-SHA256-js/blob/main/hmac-sha256-node.mjs |
mhhh we would leak the password to verify the OTP i think, The OTP generation must be done by the router internally |
Then something like this https://gist.github.com/stevendesu/2d52f7b5e1f1184af3b667c0b5e054b8 |
Hash choice in the GUI is also good. So having SHA1 for people who suffer M$ crapps, and SHA9000000 for those who don't :) |
@Ansuel amazing work! May I suggest tho that instead of |
Seems like a good and logical placement. |
@stangri totally if you have other suggestion also on how to split the ucode script and the qrcode library feel free to put there here. The current setup is the least shit I found so nothing is permanent (again it's a POC and everything is to clean) |
} | ||
|
||
let otp_type = ctx.get('luci', username, 'type'); | ||
let counter = ""; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know how ucode responds to 64bit uints
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/BigInt
64bit may be necessary to avoid Y2038 problems.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It consistently uses 64bit integers internally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jow- even on 32bit targets? Soo %d is actually long int?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, even on 32bit targets.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think %d is just int
- irrespective of length. a 32bit platform probably does twice as many operations and then glues the lower and upper halves together again.
Hi @Ansuel - any refresh on this? |
Pretty busy on other task :( so no progress |
See also #7102 |
@Ansuel: Nice feature! There is Free OTP which is very used: |
@Ansuel - any updates? :) |
Hi, this is a WIP of a project I have been working on in the last few days, but it reached a good state where I can propose as a POC to expand for real implementation.
Why?
The idea comes from the fact that someone might restrict SSH access to a public key and still wants to permit access to the webui with normal password combo. Now simple password login always has been insecure and modern login system almost always propose or enforce 2 factor authentication (2FA) via certain methods.
One of the most used methods is with an external app that generates OTP code and the user scans a QR code to generate them on the external device. This protects the scenario where the password is leaked (the good old sticky notes on the router) as the external app is required for the second OTP code to be entered.
What these OTP use
What is used by these methods is formally TOTP, that is, HOTP, but with time from epoch divided by some steps used as counter.
And HOTP is just some additional logic of a base32 hash of the password and counter signed with HMAC SHA1.
What makes new OTP generated is the counter value.
In TOTP this changes every defined step (the default value and what THE WORLD use is 30 seconds) aka a password change every 30 second
In HOTP the logic is to increase the counter every time an OTP is generated.
Problem and solution
You would quickly say: "It's a router and the correct time is not always present/set on the system."
Yes this is a problem, but HOTP can just be used for the task since it can totally work offline and is not time based.
For everyone that wants to have fun with time, TOTP can be used.
BOTH are supported since the logic is the same with the only difference of using time and setting the step.
Current state
The heavy work has been done. My intention was to have something self-contained and very small that didn't depend on any external library. And indeed this all works in a single ucode file that manually implements HMAC and SHA1.
What is left to do is integrate this in the ui.
The current provided POC is a very rudimentary implementation of this that works.
Implementation
Ideally each 2-factor implementation would have a uci entry somewhere (in luci currently) for each user (luci currently supports root so we can make assumption)
Key will be in plaintext and it will be different than the one used for user login... (the idea is to generate a big enough string). Then 'type' as 'totp' or 'hotp', 'step' for time based or counter for 'hotp' (the ucode script takes care of incrementing this value with 'hotp' OTP).
On login, if enabled, a box will be provided where to insert the OTP. Internally the luci dispatcher will call the
verifyOTP()
and the provided OTP code is compared with the expected one.If they match, login is permitted. If not, login is denied.
To enable 2-factor auth (2FA), a new tab is provided where these options can be configured.
The idea for the key value is to be read-only and enforce generation internally. The problem of too short a password doesn't arise but this is to deny a user to set the same password for root and for OTP generation (knowing the password makes the OTP generation useless since you can derive it).
A user will be able to decide the kind of OTP (HOTP or TOTP) and set the time step or the counter. (still also to decide whether or not these values will be read-only).
Finally the idea is to provide a button to generate the QR code to use the external app and register the OTP. This provided screen is a very WIP and rudimentary implementation of this (the generate button is missing and other parts need to be addressed).
QR Code situation
We have 2 luci-apps that make use of qrencode to generate a QR code. There was also an idea to generate a QR code of the WiFi password but this was never shipped. The main problem is that qrencode IS BIG. And most of the qrcode library has all kinds of bloat for the """simple""" (it's not) task of generating the code.
A solution to this problem that I found is a good and slim modern library "uqr". MIT licensed, a javascript library that generates the QR code in the screenshot.
I built the library and adapted it for usage with luci with the require system. It's a little library of 11kb... much better than the massive 200kb qrencode. I also modified it to better follow the pixel size option.
Having this library built-in permits us to implement generating WiFi QR code and maybe other useful things.
End words
Hope this little project finds you well. To me it seems a nice addition to the ui and I assume the required changes won't be even that bad.
This is a POC and everything needs to be split and better organized.
I would love some hints on that and how this can be improved. Also I assume the OTP ucode script can be greatly optimized but that is the first time I use ucode sooo don't blame me (also it needs to be tested on big endian systems).
Todo
Changelog
18/04/2024 Proposal of Project/POC