View Issue Details

IDProjectCategoryView StatusLast Update
0007996Talerlibeufin-bankpublic2024-02-10 23:31
ReporterChristian Grothoff Assigned ToAntoine A  
PriorityimmediateSeverityblockReproducibilityN/A
Status resolvedResolutionfixed 
Platformi7OSDebian GNU/LinuxOS Versionsqueeze
Product Versiongit (master) 
Target Version0.9.4Fixed in Version0.9.4 
Summary0007996: fixing bank 2-FA
DescriptionWe urgently need to address the 2-FA design and
implementation, which today remains a mess. I would
like to completely eliminate the captcha, and allow
a per-account configurable 2-FA. To make this work
nicely, we need to proceed in several steps to always
maintain compatibility. Please read this also as
a learning experience for future changes.

1) /config should return the set of available 2nd
factors, for example: second_factors: ["sms", "email"].
The array could be empty. The backend should look at
which shell scripts are configured in the configuration
and simply add the respective strings to the array.
This will allow the SPA to only show the valid options.

2) The database schema should be extended to allow
a 2-FA configuration for each account. Here, we should
store either NULL (no 2nd factor), "sms" or "email"
(IMO as strings is fine). Default is NULL (for migration
of existing accounts). Note: this must be a new schema
version with an ALTER TABLE.

3) Extend GET on account to return new field "second_factor_auth_method" with current 2-FA authentication value (NULL == missing). Extend POST (account creation) and PATCH (account modification) with *optional* field to set 2-FA method. If missing, 2-FA is to be set to NULL. Requests where 2-FA is set to "sms" or "email" without providing a phone number or email address respectively are 400 bad requests.

Requests to disable or change 2-FA (change from sms to email or change of the e-mail address or phone number if it is used for 2-FA) by non-admin PATCH requests require 2-FA using the previously configured 2-FA method. I propose we use the exact same endpoint, just on first PATCH the 2nd factor is sent and a 403 (?) is returned, and then the SPA asks for the TAN code and submits *again* this time with the TAN code in some HTTP header. (Backend needs to protect against brute forcing, counting failed requests, yada yada.)

*IF* this is too complex to do for 0.9.4, we could 403 any PATCH request by non-admin that would change 2-FA and simply tell the user that only the admin can change these values.

Here, we should bump major protocol version and age
(both +1).


4) Add a 2-FA drop down box to both account creation and account change forms in the SPA. Drop-down should only allow selection of allowed 2-FA methods based on /config. If 2-FA is selected, entering the e-mail/phone number should become mandatory (with *) and 'submit' disabled to avoid any 400 bad request experiences.

5) Signal the required 2-FA method of the account to the SPA during withdraw/cashout. If none is required, we do NOT do the captcha, but simply show a button asking to "confirm" the transaction. If 2-FA is required, the SPA has to ask
for the TAN.

I'm not sure yet, but this may require a coordinated
change if this breaks the existing protocol work-flow.
In that case, maybe we need to bump the major protocol
again and lower the protocol age to 0. This will require
some discussion as I don't know what is currently done
here. In any case, after this the SPA will no longer
give the user any choice (notebook) between captcha
and sms/e-mail 2-FA, but simply request the specific
2-FA code sent via the configured 2-FA method (if any).

TagsNo tags attached.

Activities

Christian Grothoff

2023-12-03 12:50

manager   ~0020713

Please bump this bug between Antoine and Sebastian until resolved ;-).

sebasjm

2023-12-04 19:04

developer   ~0020726

we should support multiple 2fa in one account, every site that i know that has 2fa allows you to setup multiple devices and you can ask to validate with different one than the default if you lost one. (offline key codes, sms, email)

I would leave the PATCH account as is, simple modifying attributes but add a new endpoint just for 2fa.

type TanChannel = "email" | "sms" ...

GET /accounts/$ACCOUNT/2fa
returns a list of enabled 2fa
interface EnabledAuths {
  usable: TanChannel[],
  pending: PendingAuth[],
}
interface PendingAuth {
  channel: TanChannel,
  operation_id: string;
}

POST /accounts/$ACCOUNT/2fa/$CHANNEL
adds a new 2fa method in "pending" state
fails if there is already one
fails if tries to create a 2fa and value (email,phone) is missing in account details.
returns OPERATION_ID to complete the operation

DELETE /accounts/$ACCOUNT/2fa/$CHANNEL
remove the 2fa from the lists
if status === pending, complete 200 Ok (successfully)
if status === active, return 202 Accepted with operation_id in the response

POST /accounts/$ACCOUNT/2fa/$OPERATION_ID/confirm
complete the operation
in case of "adding new 2fa" it change the state from "pending" to "active"

POST /accounts/$ACCOUNT/2fa/$OPERATION_ID/abort
should be the same as DELETE

This way has 3 major benefits:
 * we can reuse the logic for other things/service without rewriting.
If server requires 2fa auth for an operation, return $OP to the UI and ask the user to confirm (changing debt limit, conversion rate, etc...) using the
 * it is extensible if we add new another type of 2fa (offline keys, OTP)
 * we leave the PATCH /account as is, 2fa logic will take info from account

NOTE:
after activating "email" as 2fa then patching account will become "restricted-to-2fa".
For every endpoint that required 2fa instead of returning 200 Ok, it will return 202 (accepted) with the operation_id.
Meaning that the change is in the server and it should complete 2fa to complete.
Calling /accounts/$ACCOUNT/2fa/$OPERATION_ID/confirm should complete the operation (patching account, confirming withdrawal, ... or anythin else)

Antoine A

2023-12-05 01:07

developer   ~0020727

I like the idea of having separate 2FA endpoints, it will make error handling and status monitoring easier.

To have a centralised /confirm endpoint for all 2FA challenges we could, each time we create a new operation, return the id of the operation and the id of the 2FA challenge if it exists. From the challenge id we should be able to find the operation id, but we need to see what this means in terms of performance. We can keep two confirmation endpoints, first we confirm the 2FA challenge and then the operation. This may be necessary if some operations require other confirmation information, such as authentication for example.

Antoine A

2023-12-05 12:33

developer   ~0020728

Last edited: 2023-12-05 12:34

If we want to do things properly, it will take time. I think for 0.9.4 we should aim for a minimal implementation where only the administrator can change 2-FA so that we don't have to solve any new challenges and change as few APIs as possible. Then, for a later release, we'll implement 2-FA the right way and use it for cashout, whithdraw, account patching and account deletion.

I propose to have these endpoints for version 0.9.4:

type TanChannel = "email" | "sms" ...

GET /accounts/$ACCOUNT/2fa
returns a list of enabled 2fa
interface EnabledAuths {
  usable : TanChannel[],
}

POST /accounts/$ACCOUNT/2fa/$CHANNEL
adds a new 2fa method, any user without 2fa or admin only if 2fa is already configured

DELETE /accounts/$ACCOUNT/2fa/$CHANNEL
deletes a 2fa method, administrator only

And the cashout API remains the same

Christian Grothoff

2023-12-05 12:58

manager   ~0020729

Yes, let's first only allow the admin to change 2-FA.

Antoine A

2023-12-08 02:31

developer   ~0020735

Last edited: 2023-12-08 02:33

Whilst supporting 2FA, I don't want to change current APIs too much or polute all the protected endpoints with 2FA specific errors. I also don't want to add too much complexity or pressure to the backend/database, which means I don't want to add a table to store pending operations or add a status column to existing tables.

I also don't want to check the TAN challenge code with the same request as the data from a protected operation, as we would have to deal with additional 2FA-specific errors in each of them (duplicate 2FA error code everywhere).

The solution is to solve the TAN challenge before allowing the user to perform the operation. Solving the challenge gives you a token that is consumed by a successful protected operation, allowing you to retry it until it succeeds with the same token.

GET /accounts/$USERNAME/tan
List of user-activated tan channels

POST /accounts/$USERNAME/tan/$CHANNEL
Add a new tan channel (requires a 2FA token)

DELETE /accounts/$USERNAME/tan/$CHANNEL
Delete a tan channel (requires a 2FA token)

POST /accounts/$USERNAME/tan/challenge/$CHANNEL
Request a new challenge via the chosen channel

POST /accounts/USERNAME/tan/challenge/$CHALLENGE_ID/confirm
Resolve a challenge and receive a unique token

As soon as 2-FA is enabled for an account, each protected endpoint will wait for a 2FA token in an HTTP header and return 403 if it is missing. This way we can keep the existing body and errors.

Christian Grothoff

2023-12-09 04:57

manager   ~0020736

Sounds nice.

Issue History

Date Modified Username Field Change
2023-12-03 12:49 Christian Grothoff New Issue
2023-12-03 12:49 Christian Grothoff Status new => assigned
2023-12-03 12:49 Christian Grothoff Assigned To => Antoine A
2023-12-03 12:50 Christian Grothoff Note Added: 0020713
2023-12-04 19:04 sebasjm Note Added: 0020726
2023-12-05 01:07 Antoine A Note Added: 0020727
2023-12-05 12:33 Antoine A Note Added: 0020728
2023-12-05 12:34 Antoine A Note Edited: 0020728
2023-12-05 12:58 Christian Grothoff Note Added: 0020729
2023-12-08 02:31 Antoine A Note Added: 0020735
2023-12-08 02:33 Antoine A Note Edited: 0020735
2023-12-08 02:33 Antoine A Note Edited: 0020735
2023-12-09 04:57 Christian Grothoff Note Added: 0020736
2024-01-12 18:34 Antoine A Severity feature => block
2024-01-12 18:34 Antoine A Priority normal => immediate
2024-01-15 15:03 Antoine A Status assigned => resolved
2024-01-15 15:03 Antoine A Resolution open => fixed
2024-02-10 23:31 Christian Grothoff Fixed in Version => 0.9.4