Contrib.social

Cannot authorize clients with empty string set as secret

Open on Drupal.org →
Created on 18 November 2022, about 2 years ago
Updated 31 October 2023, about 1 year ago

Problem/Motivation

We're running into an unfortunate bug where clients with the secret set to an empty string cannot be authenticated for any grant type.

This happens because a hash of the empty string is stored in the database instead of being set to NULL and since #3230707: 5.x broken on php 8 due to incompatibility with lcobucci/jwt v4 via league/oauth2-server ^8.2 → (also see this comment → ) there is logic to enforce that clients with a secret must always be validated. The validation logic does not attempt validation of a provided client_secret with an empty string nor does it check if the stored secret is a hash of the empty string.

The logic to enforce that clients with a secret must always be validated makes sense. But it also introduced a breaking change from 5.0.x -> 5.2.x (not exactly sure when this change finally landed) in that the client secret with an empty string is impossible to authenticate.

Two things changed from 5.0.x to 5.2.x in this commit: https://git.drupalcode.org/project/simple_oauth/-/commit/ca09275e16969a1...

First, the logic to enforce client with a secret must always be validated:
Relevant code: https://git.drupalcode.org/project/simple_oauth/-/blob/5.2.x/src/Reposit...

    // Determine whether the client is public. Note that if a client secret is
    // provided it should be validated, even if the client is non-confidential.
    // The client_credentials grant is specifically special-cased.
    // @see https://datatracker.ietf.org/doc/html/rfc6749#section-4.4
    if (!$client_drupal_entity->get('confidential')->value &&
      $secret_field->isEmpty() &&
      empty($client_secret) &&
      $grant_type !== 'client_credentials') {
      return TRUE;
    }

By itself this change means that clients configured with an empty string would need the client secret to be provided. This is not great (as it basically breaks existing clients) but it is possible:

curl -i -X POST -d "grant_type=password&username=admin&password=admin&client_id=farm&client_secret=" http://farmos.ddev.site/oauth/token

However, simple_oauth will no longer attempt to validate an "empty" client secret provided with the authorization request. This is because of the boolean logic equating the empty string '' == FALSE. So even if you provide an empty client_secret, it will not be validated.

Relevant code: https://git.drupalcode.org/project/simple_oauth/-/blob/5.2.x/src/Reposit...
(if I remove the $client_secret && from line 75 then this works, given an empty client secret is provided)

    // Check if a secret has been provided for this client and validate it.
    // @see https://datatracker.ietf.org/doc/html/rfc6749#section-3.2.1
    return (!$secret_field->isEmpty())
      // The client secret may be NULL; it fails validation without checking.
      ? $client_secret && $this->passwordChecker->check($client_secret, $client_drupal_entity->get('secret')->value)
      : TRUE;

This is a breaking change, but according to the spec it seems that an empty string should be allowed and be optional: https://datatracker.ietf.org/doc/html/rfc6749#section-2.3.1

client_secret
REQUIRED. The client secret. The client MAY omit the
parameter if the client secret is an empty string.

Steps to reproduce

1. Configure a client with a empty string for the client secret (here is where we do this on hook_install in farmOS: https://github.com/farmOS/farmOS/blob/03279969ab3dffc291b17033b801e2bff7...)

$farm_consumer = Consumer::create([
    'label' => 'Farm default',
    'client_id' => 'farm',
    'redirect' => $base_url,
    'is_default' => TRUE,
    'owner_id' => '',
    'secret' => '',
    'confidential' => FALSE,
    'third_party' => FALSE,
  ]);
  $farm_consumer->save();

2. Observe in the database consumer_field_data a secret is populated.

3. Attempt the password grant for this client with an empty client secret

4. Observe 401 client authentication error

{"error":"invalid_client","error_description":"Client authentication failed","message":"Client authentication failed"}

Proposed resolution

In the short term 5.2.x should support clients configured with an empty string for client secret. Both previous & the current version of simple_oauth allow saving an empty string that gets hashed and stored in the DB which is incompatible with the latest authentication verification logic.

In the future 6.x+ perhaps this could be changed to enforce that NULL is stored in the DB. But it's interesting that the empty string is a part of the oauth spec.

Remaining tasks

User interface changes

N/A

API changes

N/A

Data model changes

🐛 Bug report
Status

Needs review

Version

6.0

Component

Code

Created by

🇺🇸United States paul121 Spokane, WA

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

Merge Requests

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • Comment about 1 year ago →
    🇺🇸United States paul121 Spokane, WA

    This issue still exists in v6. Attaching a patch with same solution as above. Also have a MR ready once the Drupal gitlab instance is back up.

  • Merge request !110Validate clients with an empty string secret → (Open) created by paul121
  • Comment about 1 year ago →
    🇺🇸United States paul121 Spokane, WA

    Patch for 6.0.0-beta5

  • Pipeline finished with Success
    about 1 year ago
    Total: 193s
    #42402
  • Comment about 1 year ago →
    🇿🇦South Africa rudolfbyker South Africa

    I'm also running into this issue. Thanks for the patches.

    I think two more things are needed here:

    1. Unit tests, so that this does not regress again.
    2. A way to clear the secret in the UI when editing the consumer. Currently, if you leave the field empty, it just means "don't update".
  • Status changed to Needs work about 1 year ago
  • Comment about 1 year ago →
    🇳🇱Netherlands kingdutch

    Marking as needs work per #9

    I would also want test coverage to ensure that in case the client secret is set to empty the client can not authenticate with a non-empty secret (something I think the current code allows if I look at it). In the case a client provides a secret but is configured not to use one, they're still using an incorrect secret and should be denied.

  • Pipeline finished with Success
    7 months ago
    Total: 213s
    #166999
  • Pipeline finished with Success
    7 months ago
    Total: 293s
    #167003
  • Comment 7 months ago →
    🇺🇸United States paul121 Spokane, WA

    In reviewing this issue again I believe there is another issue regarding client_credentials grant with public clients.

    A "confidential" consumer with a field value consumer.confidential === TRUEthat is configured *without* a secret (or an empty secret string) is not validated and could be authenticated. This is a problem. The spec says "the client credentials grant type MUST only be used by confidential clients" - the spec does not care nor know about the concept of a consumer.confidential field, solely that if a client secret is not configured (or has an empty string, per the spec) then client_credentials grant cannot be used.

    I've refactored the logic in this validate function to explicitly check for this case with a "public" client + client_credentials grant. I think this makes things simpler and easier to follow. I've added a kernel test to cover this case. Using the Gitlab CI "test only changes" you can see this test fails on current code: https://git.drupalcode.org/issue/simple_oauth-3322325/-/jobs/1539305#L110

    I would also want test coverage to ensure that in case the client secret is set to empty the client can not authenticate with a non-empty secret (something I think the current code allows if I look at it). In the case a client provides a secret but is configured not to use one, they're still using an incorrect secret and should be denied.

    Good catch @kingdutch. These changes should cover this case as well. With my changes we only return TRUE in one place for a fully correct public client w/o any client secret provided. The final check does a password check only when the stored and provided client secret exist, else returns FALSE.

    Unit tests, so that this does not regress again.

    Agreed @rudolfbyker, this ClientRepository::validateClient logic would best be covered in a unit test. Unfortunately I don't have much experience writing unit tests and don't have time at the moment.

    A way to clear the secret in the UI when editing the consumer. Currently, if you leave the field empty, it just means "don't update".

    Agreed this is needed as well. I'm not sure the best approach to do this from the UI. Perhaps we could discuss and agree on an approach before starting work? Personally I'd like to see the UI changes happen in a separate issue. We've been dealing with this issue and patching in FarmOS for a while and would love to just get the validateClient logic in place.

  • Status changed to Needs review 7 months ago
  • Comment 7 months ago →
    🇺🇸United States paul121 Spokane, WA

    Marking Needs Review since I have refactored some of the logic here, would love a review of this. Acknowledge that we could still use tests

  • Comment 7 months ago →
    🇺🇸United States paul121 Spokane, WA

    Including patch with latest changes for 6.0.0-beta6

  • Status changed to Needs work 7 months ago
  • Comment 7 months ago →
    🇳🇱Netherlands bojan_dev
  • Pipeline finished with Failed
    1 day ago
    Total: 161s
    #370538
  • Pipeline finished with Canceled
    1 day ago
    Total: 122s
    #370547
  • Pipeline finished with Failed
    1 day ago
    Total: 229s
    #370550
  • Merge request !156Update validateClient logic for public client credentials grant → (Open) created by paul121
  • Pipeline finished with Failed
    1 day ago
    Total: 410s
    #370555
  • Pipeline finished with Failed
    1 day ago
    Total: 346s
    #370557
  • Pipeline finished with Success
    1 day ago
    Total: 240s
    #370567
  • Comment 1 day ago →
    🇺🇸United States paul121 Spokane, WA

    Sorry for the above noise. I made a new MR 156 and pushed a few times to get tests passing. This is a slightly different approach at the validation logic so that it checks both the confidential boolean and the secret value. Previously this only checked the secret value.

contrib.social Blog FAQ Discussions
Production build 0.71.5 2024