Decorate the user.auth service

Created on 30 July 2023, over 1 year ago
Updated 20 November 2023, about 1 year ago

Problem/Motivation

SA-CONTRIB-2023-030 revealed some faults in how we protect accounts as external code may call UserAuthInterface::authenticate() to validate a user and log the account in

If we decorate the user.auth service we can evaluate authentication based on user/passwords provided anywhere. As long as a plugin can determine what portion of the password should be a token (TOTP and HOTP know their code lengths for example will always be X digits) it can remove that portion from the password provided and compare that portion against its validation before we pass through the remainder for password verification from the rest of the user.auth stack.

In reference to ✨ Allow TFA authentication through REST routes Active this would protect the known core routes (session and http_basic) authentication without any authentication provider specific changes (though http_basic would be impractical to use each request would require a new token, it would still technically work)

In reference to ✨ Increase security with single failure for login, password, and tfa Postponed: needs info this could help move us forward as it would provide a a way for all login methods to only make one call and receive back only a single answer on pass/fail not knowing if it was token failure or password failure.

Steps to reproduce

N/A.

Proposed resolution

Evaluate feasabiity of converting our code over to decorate the user.auth service and to decorate the user.auth service to be the location that tokens are validated.

Remaining tasks

Patch

User interface changes

TBD

API changes

TBD. I anticipate at least one new method will be needed for validation plugins.

Data model changes

TBD

πŸ“Œ Task
Status

Fixed

Version

2.0

Component

Code

Created by

πŸ‡ΊπŸ‡ΈUnited States cmlara

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @cmlara
  • πŸ‡ΊπŸ‡ΈUnited States cmlara

    Preliminary testing with a decorator appears to indicate this would be very doable and very maintainable long term.

    We do however appear to need to implement ✨ Increase security with single failure for login, password, and tfa Postponed: needs info first as we will need both the password and the token to be passed through to UserAuthInterface::authenticate() as part of the user_login_form.

    This points to the most significant negative aspet of implementing this feature, that any contrib calls to UserAuthInterface::authenticate() now need to provide a TFA token (or be accepted with a LoginPlugin). The Browser plugin may help sites mitigate this concern to some degree, however that isn't a full solution.

    We might need to provide a method to allow overwriting existing forms to prompt for tokens, we could possibly do that via a hook_form_alter() call where we inject our own additional field and validator A similar request is open in #3323866: Offer TFA flow for other use cases than login β†’ that perhaps is a viable solution as well. This however could be done in a followup issue and site owners will just need not note to users (or provide their own form_alter) that login fields must also include an OTP.

  • πŸ‡ΊπŸ‡ΈUnited States cmlara

    ✨ Increase security with single failure for login, password, and tfa Postponed: needs info may cause bigger security concerns so we need to implement this without that change.

    Using the per-request memory cache should be sufficient, though it does make it easier for 3rd party code to disable TFA during authentication. However once code has that level of control it could just login a user directly making this minimal risk from our side.

  • Open in Jenkins β†’ Open on Drupal.org β†’
    Core: 10.0.7 + Environment: PHP 8.1 & MySQL 5.7
    last update over 1 year ago
    40 pass
  • Status changed to Needs review over 1 year ago
  • πŸ‡ΊπŸ‡ΈUnited States cmlara

    Here is the first run a this. We likely should add validateRequest onto the ValidationInterface API, however I have left that out on this changeset so it can be discussed separately. A plugin must implement validateRequest() to approve a submission through the decorator.

    Sequence of operations:
    TfaUserAuth is installed as a decorator to the user.auth service.

    When using the TfaLoginForm:
    A flag is set during the validateAuthentication() stage to allow the parent call to user.auth service to bypass the TFA service. The TfaLoginForm/EntryForm will continue to validate TFA tokens using the existing workflow.

    When not using the TfaLoginForm:
    The TfaUserAuth decorator will follow a similar workflow to the TfaLoginForm/Entryform, it will check if TFA is enabled globally and for the user, that the default plugin reports ready(), attempt to to use a login plugin, than poll the default validation plugin and finally poll all other configured validation plugins. If at any step in the flow a plugin approves the request the token portion (if present) will be stripped from the end of the password, and the new string will be passed through the rest of the user.auth decoration stack and the result from that call passed up the chain.

    This means that TOTP and HOTP for example could be used through the user.login.http route by appending the token to the end of the password, so that

    {
        "username": "user",
        "password": "password"
    }
    

    becomes:

    {
        "username": "user",
        "password": "password123456"
    }
    

    The new tokenLength() method on the API allows the submitted password string to be analyzed in case it contains variable length data that can not be known until it is submitted. Plugins are left to their own design on how they encode the token and determine its length when submitted as part of a password string.

  • πŸ‡ΊπŸ‡ΈUnited States cmlara

    This will also partially solve πŸ“Œ SA-CONTRIB-2023-030 and 2.x Active

  • Open in Jenkins β†’ Open on Drupal.org β†’
    Core: 10.0.7 + Environment: PHP 8.1 & MySQL 5.7
    last update over 1 year ago
    40 pass
  • Status changed to Fixed about 1 year ago
  • πŸ‡ΊπŸ‡ΈUnited States cmlara
  • πŸ‡ΊπŸ‡ΈUnited States cmlara

    Issue credits.

  • Automatically closed - issue fixed for 2 weeks with no activity.

Production build 0.71.5 2024