Bug 28786 will let librarians enable a Two-factor authentication but will force them to use an application to generate the TOTP token. It would be nice to have the ability to send a notice with the code, via email or SMS.
Will start this once bug 28786 is pushed.
Created attachment 135287 [details] [review] Bug 28787: Send a notice with the TOTP token Bug 28786 let librarians enable a Two-factor authentication but force them to use an application to generate the TOTP token. This new enhancement add the ability to send an email containing the token to the patron once it's authenticaed The new notice template has the code '2FA_OTP_TOKEN' Test plan: - Setup the two-factor authentication (you need the config entry and the syspref ON) - Enable it for your logged in patron - Logout - Login and notice the new link "Send the code by email" - Click on it and confirm that you received an email with the code - Use the code to be fully logged in QA question: Is 400 the correct error code to tell the email has not been sent?
TODO - I am not sure about the following line, so I commented it but let it in the patch + #|| $c->req->url->to_abs->path eq '/api/v1/auth/send_otp_token' ) {
Hmm, I'm not so sure about this.. whilst I understand TOTP over SMS delivery makes sense as SMS is in theory immediate delivery.. Email has lots of caveats around delivery speed and so it's more common to send an HOTP or even a simple random string OTP in the email case due to the timeout factor?
Comment on attachment 135287 [details] [review] Bug 28787: Send a notice with the TOTP token Review of attachment 135287 [details] [review]: ----------------------------------------------------------------- ::: C4/Letters.pm @@ +1605,5 @@ > > + if ( $content =~ m|\[% otp_token %\]| ) { > + my $patron = Koha::Patrons->find(C4::Context->userenv->{number}); > + $tt_params->{otp_token} = Koha::Auth::TwoFactorAuth->new({patron => $patron})->code; > + } This looks like a hack. We should pass the code in via a public method/function. That said, it looks like this OTP will wind up in the message_queue table?
(In reply to Martin Renvoize from comment #4) > Hmm, I'm not so sure about this.. whilst I understand TOTP over SMS delivery > makes sense as SMS is in theory immediate delivery.. Email has lots of > caveats around delivery speed and so it's more common to send an HOTP or > even a simple random string OTP in the email case due to the timeout factor? I agree that a 30 second time window is probably too short for email. I suppose alternatively you could set a longer interval when using email TOTPs. (I did a little bit of a deep dive into Auth::GoogleAuth and it's actually kind of interesting how simple the mathematical mechanism is for establishing time windows for TOTPs.) Another thing we could do is add the range parameter to the verify() function I believe. At the moment, it looks like we're not following the recommendations of rfc6238 to allow additional backwards steps. (Typically, with a TOTP, you can usually use up to 2-3 old codes and still work to allow for clock drift and slow users.)
(In reply to Martin Renvoize from comment #4) > Hmm, I'm not so sure about this.. whilst I understand TOTP over SMS delivery > makes sense as SMS is in theory immediate delivery.. Email has lots of > caveats around delivery speed and so it's more common to send an HOTP or > even a simple random string OTP in the email case due to the timeout factor? Well, the description was clear enough and the bug has been flagged as sponsored. I don't think it's fair to ask for a rewrite once the code has been written. (In reply to David Cook from comment #5) > Comment on attachment 135287 [details] [review] [review] > > + if ( $content =~ m|\[% otp_token %\]| ) { > > + my $patron = Koha::Patrons->find(C4::Context->userenv->{number}); > > + $tt_params->{otp_token} = Koha::Auth::TwoFactorAuth->new({patron => $patron})->code; > > + } > > This looks like a hack. We should pass the code in via a public > method/function. That said, it looks like this OTP will wind up in the > message_queue table? Hum yes, maybe. It seemed weird to add a pattern/variable that would be available for a single notice template. (In reply to David Cook from comment #6) > Another thing we could do is add the range parameter to the verify() > function I believe. At the moment, it looks like we're not following the > recommendations of rfc6238 to allow additional backwards steps. (Typically, > with a TOTP, you can usually use up to 2-3 old codes and still work to allow > for clock drift and slow users.) Yes, that's a bug. I was pretty sure it was allowing at least 1 old code. It's in the POD of ->verify, and members/two_factor_auth.pl, but C4/Auth.pm
> (In reply to David Cook from comment #6) > > Another thing we could do is add the range parameter to the verify() > > function I believe. At the moment, it looks like we're not following the > > recommendations of rfc6238 to allow additional backwards steps. (Typically, > > with a TOTP, you can usually use up to 2-3 old codes and still work to allow > > for clock drift and slow users.) > > Yes, that's a bug. I was pretty sure it was allowing at least 1 old code. > It's in the POD of ->verify, and members/two_factor_auth.pl, but C4/Auth.pm Fixed on bug 30842.
(In reply to David Cook from comment #5) > This looks like a hack. We should pass the code in via a public > method/function. That said, it looks like this OTP will wind up in the > message_queue table? How vulnerable is that? Surely, the token will be expired very quickly but can we get back to the originating secret? And that said, would an attack on the email not have a higher chance of success ? https://security.stackexchange.com/questions/42671/is-oath-totp-and-or-google-authenticator-vulnerable-if-an-attacker-has-n-pre
(In reply to Jonathan Druart from comment #7) > Well, the description was clear enough and the bug has been flagged as > sponsored. I don't think it's fair to ask for a rewrite once the code has > been written. In that case, I should mark more bugs as sponsored heh. (In reply to Jonathan Druart from comment #8) > > Yes, that's a bug. I was pretty sure it was allowing at least 1 old code. > > It's in the POD of ->verify, and members/two_factor_auth.pl, but C4/Auth.pm > > Fixed on bug 30842. Between bug 30842 and bug 30843, that might be enough to cover off the delivery delay of email.
(In reply to Marcel de Rooy from comment #9) > (In reply to David Cook from comment #5) > > > This looks like a hack. We should pass the code in via a public > > method/function. That said, it looks like this OTP will wind up in the > > message_queue table? > > How vulnerable is that? Surely, the token will be expired very quickly but > can we get back to the originating secret? And that said, would an attack on > the email not have a higher chance of success ? > > https://security.stackexchange.com/questions/42671/is-oath-totp-and-or- > google-authenticator-vulnerable-if-an-attacker-has-n-pre I'm not an expert on the topic, but in theory you could try an offline brute force attack that could potentially reveal the secret eventually, although I imagine we're using complex enough secrets that it would probably be computationally improbable at this time. Technically, I suppose we could encrypt the email contents at rest (like https://docs.aws.amazon.com/sns/latest/dg/sns-server-side-encryption.html), but I think the risk is small enough that can be a future enhancement...
(In reply to Martin Renvoize from comment #4) > Hmm, I'm not so sure about this.. whilst I understand TOTP over SMS delivery > makes sense as SMS is in theory immediate delivery.. Email has lots of > caveats around delivery speed and so it's more common to send an HOTP or > even a simple random string OTP in the email case due to the timeout factor? I just signed up for a new system that uses OTP over email and it looks like they've set their code expiry to 5 minutes. (They tell the user in the email of that expiration time.)
Created attachment 136571 [details] [review] Bug 28787: Send a notice with the TOTP token Bug 28786 let librarians enable a Two-factor authentication but force them to use an application to generate the TOTP token. This new enhancement add the ability to send an email containing the token to the patron once it's authenticaed The new notice template has the code '2FA_OTP_TOKEN' Test plan: - Setup the two-factor authentication (you need the config entry and the syspref ON) - Enable it for your logged in patron - Logout - Login and notice the new link "Send the code by email" - Click on it and confirm that you received an email with the code - Use the code to be fully logged in QA question: Is 400 the correct error code to tell the email has not been sent?
Created attachment 136572 [details] [review] Bug 28787: (follow-up) Changes in API auth, moved otp out of Letters
Created attachment 136573 [details] [review] Bug 28787: Typo authenticaction Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Generally, I have some doubts about the API path api/v1/auth/send_otp_token. Sending a token is not a normally expected API action; it sounds like a 'misused verb'. You could think of creating a OTP code as an API action, although we do not really add it as entity. Apart from that it works. See some details hereunder. [1] Your TODO - I am not sure about the following line, so I commented it but let it in the patch + #|| $c->req->url->to_abs->path eq '/api/v1/auth/send_otp_token' ) { The otp path should go thru the chain. So this line should not be here although commented. Removed it. [2] Code segment from Koha/REST/V1/Auth.pm if ( !$authorization and ( $params->{is_public} and ( C4::Context->preference('RESTPublicAnonymousRequests') or $user) or $params->{is_plugin} ) or $pending_auth This does not look good to me. Do we need pending_auth here ? If so, at least we need parentheses etc. My follow-up removes the line now. [3] This segment is incomplete: elsif ($status eq "additional-auth-needed") { if ( $c->req->url->to_abs->path eq '/api/v1/auth/send_otp_token' ) { $user = Koha::Patrons->find( $session->param('number') ); $cookie_auth = 1; $pending_auth = 1; } I think we should raise an exception if we have this status and the api path does not match (so add an else). Removed pending_auth. Added a simple exception in my follow-up. [4] When I tested this API path via API keys, I got no authorization. I added a permission catalogue (staff access) to get around that. If you dont have that permission, we should not even send a code. [5] Letters: + if ( $content =~ m|\[% otp_token %\]| ) { + my $patron = Koha::Patrons->find(C4::Context->userenv->{number}); + $tt_params->{otp_token} = Koha::Auth::TwoFactorAuth->new({patron => $patron})->code; + } This seems quite hacky. Why not pass it to Letters from the api module? Moved it now. This still needs updating the notice stuff. [6] QA question: Is 400 the correct error code to tell the email has not been sent? I guess it is not. The client did nothing wrong. Maybe just plain 500? But having some doubts about that too. Or always 200/201 and refer for details to JSON body? [7] TODO Hardcoded phrase: It is valid one minute. [8] Functional question: When you want to enable 2FA without a mobile phone, what should you do? There is no link to send the code on that form. [9] Current code: C4::Context->config('encryption_key') <encryption_key>__ENCRYPTION_KEY__</encryption_key> Do we still need to replace it in koha-create by the actual key ? Enable 2FA: Form text: Can't scan the code? To add the entry manually, provide the following details to the application on your phone. Account: BRANCH Key: BRANCH_EMAIL Time based: Yes But the form does not show the Secret. So telling the user to enter the details on their phone is useless? Let me know if you agree with the follow-up.
Tomas, what do you think? See prev comment.
(In reply to Marcel de Rooy from comment #16) > [2] Code segment from Koha/REST/V1/Auth.pm > if ( !$authorization and > ( $params->{is_public} and > ( C4::Context->preference('RESTPublicAnonymousRequests') or > $user) or $params->{is_plugin} ) > or $pending_auth > This does not look good to me. Do we need pending_auth here ? If so, at > least we need parentheses etc. My follow-up removes the line now. Why? Can you explain? If the user is not fully authenticated they shouldn't be allowed to access REST API route. With your follow-up patch the tests are failing now.
Note that the tests need an SMTP server configured to pass (which is wrong - TODO). They can be proved anyway with: apt install python && python -m smtpd -n -c DebuggingServer localhost:25