Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix token introspection and revocation authentication checks #93

Merged
merged 3 commits into from
Aug 21, 2024

Conversation

shawnz
Copy link
Contributor

@shawnz shawnz commented Aug 11, 2024

This PR fixes two issues I noticed with the token introspection and revocation endpoints:

  • Neither endpoint actually checks to make sure that the client secret provided is actually correct, only that one is provided.

  • The revoke token endpoint fails if a client secret is not provided, even if the client is a public client that doesn't have a client secret. The RFC 7009 specification seems to imply that public clients should be allowed to revoke tokens in section 2.1 "Revocation Request":

    The authorization server first validates the client credentials (in
    case of a confidential client) and then verifies whether the token
    was issued to the client making the revocation request. If this
    validation fails, the request is refused and the client is informed
    of the error by the authorization server as described below.

    https://www.rfc-editor.org/rfc/rfc7009.html#section-2.1

    Note how only in the case of a confidential client is it necessary to validate client credentials. So public clients should not have their credentials validated (since they don't have any).

    One thing that's not exactly clear, is how should public clients provide their client ID to the endpoint if they don't use the authorization header? In RFC 7009 the section 2.1 "Revocation Request" doesn't indicate the presence of any client_id field in the request body for this purpose. However, later on in section 5 "Security Considerations" they imply that a client_id field in the request body actually is the right way to do it after all:

    A malicious client may attempt to guess valid tokens on this endpoint
    by making revocation requests against potential token strings.
    According to this specification, a client's request must contain a
    valid client_id, in the case of a public client, or valid client
    credentials, in the case of a confidential client.

    https://www.rfc-editor.org/rfc/rfc7009.html#section-5

    The ability to read from the client_id field in the request body is already supported by get_client_credentials, so no change was required to implement that.

This PR fixes the first issue by adding calls to storage.get_client, which validates the secret.

The second issue is fixed by adding a new parameter to the get_client_credentials function, secret_required, which is set to False for the revoke token endpoint. This new parameter is also used to simplify some of the logic in the create_token_response function, which previously used a complicated try/except block to achieve the same thing.

Some tests are also added to ensure the new functionality works.

Before making the change:

FAILED tests/test_endpoint.py::test_introspect_token_with_wrong_client_secret - assert <HTTPStatus.OK: 200> == <HTTPStatus.UNAUTHORIZED: 401>
FAILED tests/test_endpoint.py::test_revoke_access_token_without_client_secret - assert <HTTPStatus.UNAUTHORIZED: 401> == <HTTPStatus.NO_CONTENT: 204>
FAILED tests/test_endpoint.py::test_revoke_access_token_with_wrong_client_secret - assert <HTTPStatus.NO_CONTENT: 204> == <HTTPStatus.UNAUTHORIZED: 401>

Results (0.58s):
      44 passed
       3 failed
         - tests/test_endpoint.py:163 test_introspect_token_with_wrong_client_secret
         - tests/test_endpoint.py:272 test_revoke_access_token_without_client_secret
         - tests/test_endpoint.py:297 test_revoke_access_token_with_wrong_client_secret

After making the change:

Results (0.81s):
      47 passed

@shawnz shawnz requested a review from aliev as a code owner August 11, 2024 00:52
@shawnz
Copy link
Contributor Author

shawnz commented Aug 11, 2024

An additional note: the specification for token introspection, RFC 7662, doesn't actually say client authentication is necessary for token introspection. But, it does say that some kind of authentication is necessary, and suggests client authentication as one possible option. However, in its current state, this library only supports client authentication and not any other possible authentication system for token introspection.

I didn't make any effort to fix that here, I simply added validation to make sure that the client secret is correct (since it was already required to be present with the existing logic anyway).

@aliev aliev merged commit ef63132 into aliev:master Aug 21, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants