feat: add support for TOTP multi-factor authentication #220

AlexCuse · 2023-10-13T15:07:13Z

No description provided.

An attempt to see keratin#133 through.

The mysql:5.7 image does not yet have an Apple Silicon compatible build.

AlexCuse

@cainlevy pretty determined to get this through this time. I've been looking at the code for a while and I think it seems sound - just noting one change to the API that I think would be beneficial. I am working on adapting the demo app to work as a test harness and hoping once that is available to prove things out we should be able to get this merged.

server/handlers/get_totp.go

AlexCuse · 2023-10-17T14:48:44Z

app/services/credentials_verifier.go

+			return nil, errors.Wrap(err, "TOTPDecrypt")
+		}
+		if !totp.Validate(totpCode, secret) {
+			return nil, FieldErrors{{"totp", ErrInvalidOrExpired}}


Should we vary the error here if MFA code is missing? Thinking mostly in terms of how applications might try submitting without MFA then prompt user for a code based on the response. This also comes into play if we want to add HOTP support that requires out-of-band code delivery.

Note the timing above we should keep going at least through password validation when making any changes.

I think probably not - maybe a back end api could support a status check on a user though.

While considering this, I came to believe that it would be more secure to test and verify a TOTP prior to the account password. Given a choice between someone testing and confirming the account password or the TOTP, I'd choose the TOTP since it's such a temporary victory.

In that event, I don't see the harm in responding with a new error code for a missing TOTP. It seems like a simpler API than a status check. What are your thoughts?

I like it - I presume we should go ahead with the password check in all scenarios still and NOT inform whether failure is bad password or bad OTP. Can do this + adding secret removal to the expire password handler after knocking out the easy stuff here.

Hmm yes a constant time comparison that doesn't reveal whether the password or OTP was correct might be ideal. However if there's any chance something is revealed, I'd prefer to reveal the correctness of the OTP.

It is already returning invalid or expired for the OTP. Added missing in 153d385

AlexCuse · 2023-10-17T15:20:47Z

Final note I think that I can't really find a place for in the changeset is that there should be a private API that can be used by back office tooling to remove the MFA secret. Its common to see MFA enrollment used in account takeovers as a way to buy the attacker time.

I would be happy to just include this as part of the existing ExpirePassword functionality but open to arguments for a dedicated endpoint.

cainlevy

it's looking good! while reviewing, i thought about a few related upgrades that could come later:

remember this device for 30 days - extra cookie
account recovery codes - generate and handle nonces for bypassing 2fa
session security level - indicate if session was secured by 2fa so main app can implement related policies

app/data/encrypted_blob_store.go

cainlevy · 2023-11-04T16:46:43Z

app/services/credentials_verifier.go

+			return nil, errors.Wrap(err, "TOTPDecrypt")
+		}
+		if !totp.Validate(totpCode, secret) {
+			return nil, FieldErrors{{"totp", ErrInvalidOrExpired}}


While considering this, I came to believe that it would be more secure to test and verify a TOTP prior to the account password. Given a choice between someone testing and confirming the account password or the TOTP, I'd choose the TOTP since it's such a temporary victory.

In that event, I don't see the harm in responding with a new error code for a missing TOTP. It seems like a simpler API than a status check. What are your thoughts?

app/services/passwordless_token_verifier.go

app/services/totp_creator.go

app/services/totp_deleter.go

app/services/totp_setter.go

app/data/totp_cache.go

app/services/totp_creator.go

app/services/totp_setter.go

server/handlers/post_session_token.go

AlexCuse · 2023-11-07T01:55:41Z

I think I addressed the main issues @cainlevy take a look.

I am about tired of looking at this PR think it is good to go at this point. I'm happy to do a follow up to clear the TOTP secret on ExpirePassword admin action. We can write up issues for that and the 3 improvements you identified and tackle in bite size chunks from here I think. Happy to let this sit in main awhile before releasing to give time for those + finish building up the new demo I'm working on.

cainlevy

I think it's ready! Nicely done.

thenhawke and others added 8 commits October 13, 2023 11:04

Add TOTP multi-factor authentication

4cb278a

An attempt to see keratin#133 through.

docker: add platform to mysql service

27413be

The mysql:5.7 image does not yet have an Apple Silicon compatible build.

fix sqlite3 migration idempotency

f24e3c8

PasswordChanger is used by authenticated sessions

241ffeb

prefer /totp/new to begin onboarding

711cfd7

return key and png for new totp

4b0834d

remove unused arg

79a8824

prefer empty value with error code

a55604c

AlexCuse commented Oct 13, 2023

View reviewed changes

server/handlers/get_totp.go Outdated Show resolved Hide resolved

rebase/lint fixes

2e4eebe

AlexCuse force-pushed the jlomfa branch from d5a5b83 to 2e4eebe Compare October 16, 2023 18:00

AlexCuse added 2 commits October 16, 2023 14:14

lint fixes for newer go

0feaded

include raw secret in get TOTP response

376808c

AlexCuse commented Oct 17, 2023

View reviewed changes

AlexCuse added 4 commits October 18, 2023 13:29

remove PNG from /totp/new response

ab4a823

always pass code as otp

86bcd88

use POST for /totp/new

890be18

docs first pass and some status code / input name normalization

8d6ac5e

AlexCuse force-pushed the jlomfa branch from be81fa9 to 8d6ac5e Compare October 20, 2023 13:40

cainlevy approved these changes Nov 4, 2023

View reviewed changes

AlexCuse added 3 commits November 6, 2023 18:54

service tests for TOTP + some remaining renames in handlers

324b886

delete key from cache on confirmation

6a7bd79

missing OTP error + field error renames

153d385

cainlevy approved these changes Nov 7, 2023

View reviewed changes

add OTP field errors to login endpoint docs

9a33c25

AlexCuse linked an issue Nov 9, 2023 that may be closed by this pull request

Add support for Multifactor Authentication (MFA / TFA) #92

Closed

add param to docs for /totp/confirm

52daa93

AlexCuse merged commit bbc6311 into keratin:main Nov 28, 2023
2 checks passed

AlexCuse deleted the jlomfa branch November 28, 2023 01:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add support for TOTP multi-factor authentication #220

feat: add support for TOTP multi-factor authentication #220

AlexCuse commented Oct 13, 2023

AlexCuse left a comment

AlexCuse Oct 17, 2023

AlexCuse Oct 19, 2023

cainlevy Nov 4, 2023

AlexCuse Nov 6, 2023

cainlevy Nov 6, 2023

AlexCuse Nov 7, 2023

AlexCuse commented Oct 17, 2023

cainlevy left a comment

cainlevy Nov 4, 2023

AlexCuse commented Nov 7, 2023

cainlevy left a comment

feat: add support for TOTP multi-factor authentication #220

feat: add support for TOTP multi-factor authentication #220

Conversation

AlexCuse commented Oct 13, 2023

AlexCuse left a comment

Choose a reason for hiding this comment

AlexCuse Oct 17, 2023

Choose a reason for hiding this comment

AlexCuse Oct 19, 2023

Choose a reason for hiding this comment

cainlevy Nov 4, 2023

Choose a reason for hiding this comment

AlexCuse Nov 6, 2023

Choose a reason for hiding this comment

cainlevy Nov 6, 2023

Choose a reason for hiding this comment

AlexCuse Nov 7, 2023

Choose a reason for hiding this comment

AlexCuse commented Oct 17, 2023

cainlevy left a comment

Choose a reason for hiding this comment

cainlevy Nov 4, 2023

Choose a reason for hiding this comment

AlexCuse commented Nov 7, 2023

cainlevy left a comment

Choose a reason for hiding this comment