Skip to content
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

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Ansuel
Copy link
Member

@Ansuel Ansuel commented Apr 18, 2024

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)

config login 'root'
	option key 'password'
	option step '30'
	option counter '30'
	option type 'totp'

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.

image


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

image

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

  • Organize code
  • Password random generation
  • QR code generation button
  • Add all kind of information
  • Find a better place for OTP ucode script
  • Make OTP view optional in sysauth
  • Tweak other themes for new option
  • Improve uqr script
  • Add to ucode and luci ubus call to base32 encode (QR code require key value base32 hashed 2 times...)
  • other task...

Changelog

18/04/2024 Proposal of Project/POC

POC/WIP to split everything and reorganize, submitted as an example to
better discuss the implementation.

Signed-off-by: Christian Marangi <[email protected]>
@aparcar aparcar requested a review from jow- April 18, 2024 18:32
@systemcrash
Copy link
Contributor

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.

@Ansuel
Copy link
Member Author

Ansuel commented Apr 18, 2024

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
require uqr; and uqr.renderSVG(text)

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

@systemcrash
Copy link
Contributor

Is printf the correct keyword in Ucode? (it's basically js) In this case, to where do those messages get printed?

Maybe syslog is more appropriate?

@systemcrash
Copy link
Contributor

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

ES6 is OK. Retain those.

@Ansuel
Copy link
Member Author

Ansuel commented Apr 18, 2024

Is printf the correct keyword in Ucode? (it's basically js) In this case, to where do those messages get printed?

Maybe syslog is more appropriate?

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.

@systemcrash
Copy link
Contributor

systemcrash commented Apr 18, 2024

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 type field would be great.

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.

@Ansuel
Copy link
Member Author

Ansuel commented Apr 18, 2024

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.

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.

@systemcrash
Copy link
Contributor

We might consider disabling OTP in that case but IMHO it's a very corner case.

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.

@Ansuel
Copy link
Member Author

Ansuel commented Apr 18, 2024

We might consider disabling OTP in that case but IMHO it's a very corner case.

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)

@systemcrash
Copy link
Contributor

systemcrash commented Apr 18, 2024

the user should have access with ssh anyway

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.

@Ansuel
Copy link
Member Author

Ansuel commented Apr 18, 2024

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

@systemcrash
Copy link
Contributor

systemcrash commented Apr 18, 2024

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 uci.get('dropbear', ...).

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 :/

@Ansuel
Copy link
Member Author

Ansuel commented Apr 18, 2024

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 uci.get('dropbear', ...).

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.

@systemcrash
Copy link
Contributor

systemcrash commented Apr 18, 2024

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. sha256sum is available on openwrt base system, I think.

@Ansuel
Copy link
Member Author

Ansuel commented Apr 18, 2024

BTW is it much more difficult to use SHA256? Considering SHA1s deprecation. sha256 is available on openwrt base system, I think.

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.

@systemcrash
Copy link
Contributor

systemcrash commented Apr 18, 2024

BTW is it much more difficult to use SHA256? Considering SHA1s deprecation. sha256 is available on openwrt base system, I think.

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.

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 :)

@systemcrash
Copy link
Contributor

systemcrash commented Apr 18, 2024

Can we use these in the browser dircetly?
https://developer.mozilla.org/en-US/docs/Web/API/HmacKeyGenParams

Here's a usage example using the above API: https://github.com/pur3miish/Universal-HMAC-SHA256-js/blob/main/hmac-sha256-node.mjs

@Ansuel
Copy link
Member Author

Ansuel commented Apr 18, 2024

Can we use these in the browser dircetly? https://developer.mozilla.org/en-US/docs/Web/API/HmacKeyGenParams

mhhh we would leak the password to verify the OTP i think, The OTP generation must be done by the router internally

@systemcrash
Copy link
Contributor

@systemcrash
Copy link
Contributor

Hash choice in the GUI is also good. So having SHA1 for people who suffer M$ crapps, and SHA9000000 for those who don't :)

@stangri
Copy link
Member

stangri commented Apr 18, 2024

@Ansuel amazing work! May I suggest tho that instead of admin/system/system, the tab for 2FA setup is added to the admin/system/admin?

@systemcrash
Copy link
Contributor

Seems like a good and logical placement.

@Ansuel
Copy link
Member Author

Ansuel commented Apr 18, 2024

@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)

@Ansuel Ansuel marked this pull request as draft April 18, 2024 21:48
}

let otp_type = ctx.get('luci', username, 'type');
let counter = "";
Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Member Author

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?

Copy link
Contributor

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.

Copy link
Contributor

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.

@systemcrash
Copy link
Contributor

Hi @Ansuel - any refresh on this?

@Ansuel
Copy link
Member Author

Ansuel commented Apr 30, 2024

Pretty busy on other task :( so no progress

@systemcrash
Copy link
Contributor

See also #7102

@Neustradamus
Copy link

@Ansuel: Nice feature!

There is Free OTP which is very used:

@systemcrash
Copy link
Contributor

@Ansuel - any updates? :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants