View Issue Details
| ID | Project | Category | View Status | Date Submitted | Last Update |
|---|---|---|---|---|---|
| 0007996 | Taler | libeufin-bank | public | 2023-12-03 12:49 | 2024-03-07 20:49 |
| Reporter | Christian Grothoff | Assigned To | Antoine A | ||
| Priority | immediate | Severity | block | Reproducibility | N/A |
| Status | closed | Resolution | fixed | ||
| Platform | i7 | OS | Debian GNU/Linux | OS Version | squeeze |
| Product Version | git (master) | ||||
| Target Version | 0.9.4 | Fixed in Version | 0.9.4 | ||
| Summary | 0007996: fixing bank 2-FA | ||||
| Description | We 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). | ||||
| Tags | No tags attached. | ||||
|
|
Please bump this bug between Antoine and Sebastian until resolved ;-). |
|
|
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) |
|
|
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. |
|
|
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 |
|
|
Yes, let's first only allow the admin to change 2-FA. |
|
|
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. |
|
|
Sounds nice. |
| 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 |
| 2024-03-07 20:49 | Christian Grothoff | Status | resolved => closed |
