Bug 28787 - Send a notice with the TOTP token
Summary: Send a notice with the TOTP token
Status: Needs Signoff
Alias: None
Product: Koha
Classification: Unclassified
Component: Authentication (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on: 28786
Blocks: 20476
  Show dependency treegraph
 
Reported: 2021-07-30 12:05 UTC by Jonathan Druart
Modified: 2022-06-27 16:10 UTC (History)
5 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 28787: Send a notice with the TOTP token (18.72 KB, patch)
2022-05-23 21:04 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28787: Send a notice with the TOTP token (18.74 KB, patch)
2022-06-27 11:52 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28787: (follow-up) Changes in API auth, moved otp out of Letters (3.42 KB, patch)
2022-06-27 11:52 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28787: Typo authenticaction (924 bytes, patch)
2022-06-27 11:52 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2021-07-30 12:05:21 UTC
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.
Comment 1 Jonathan Druart 2022-04-08 07:16:17 UTC
Will start this once bug 28786 is pushed.
Comment 2 Jonathan Druart 2022-05-23 21:04:20 UTC
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?
Comment 3 Jonathan Druart 2022-05-23 21:07:48 UTC
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' ) {
Comment 4 Martin Renvoize 2022-05-24 14:50:05 UTC
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 5 David Cook 2022-05-25 00:01:47 UTC
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?
Comment 6 David Cook 2022-05-25 02:23:34 UTC
(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.)
Comment 7 Jonathan Druart 2022-05-25 08:00:58 UTC
(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
Comment 8 Jonathan Druart 2022-05-25 08:07:42 UTC
> (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.
Comment 9 Marcel de Rooy 2022-05-25 11:20:27 UTC
(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
Comment 10 David Cook 2022-05-26 00:08:01 UTC
(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.
Comment 11 David Cook 2022-05-26 00:23:38 UTC
(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...
Comment 12 David Cook 2022-06-08 03:04:57 UTC
(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.)
Comment 13 Marcel de Rooy 2022-06-27 11:52:17 UTC
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?
Comment 14 Marcel de Rooy 2022-06-27 11:52:21 UTC
Created attachment 136572 [details] [review]
Bug 28787: (follow-up) Changes in API auth, moved otp out of Letters
Comment 15 Marcel de Rooy 2022-06-27 11:52:25 UTC
Created attachment 136573 [details] [review]
Bug 28787: Typo authenticaction

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 16 Marcel de Rooy 2022-06-27 11:59:47 UTC
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.
Comment 17 Marcel de Rooy 2022-06-27 12:00:32 UTC
Tomas, what do you think? See prev comment.
Comment 18 Jonathan Druart 2022-06-27 16:04:54 UTC
(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.
Comment 19 Jonathan Druart 2022-06-27 16:10:17 UTC
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