When is client_secret validated?

[shawn-monadical]

For confidential clients, when and where is the client_secret validated? Is this something that's supposed to be done in the get_client implementation of the storage class? In the case where the client_secret is wrong, is it correct to return "None" in the implementation of get_client? It seems like this isn't implemented in the example provided in https://github.com/aliev/aioauth-fastapi/blob/master/aioauth_fastapi_demo/oauth2/storage.py#L206, is that a bug or am I misunderstanding the intention?

Thanks, Shawn

[aliev]

In some cases, client_secret is not required, and its value will be None in get_client (for example, for the authorization_code grant type).

In cases where it is required (for example, for the client_credentials grant type), this value will be presented. There is already a check for this in aioauth: https://github.com/aliev/aioauth/blob/master/aioauth/grant_type.py#L261.

You are correct that the second case is not considered in Storage.get_client. Since this is a demo project, such issues can exist.

[shawn-monadical]

In the case of an authorization_code grant, it's optional in the general case, but if the client has a secret assigned to them, then it becomes required, right? And in that case, it should be passed to get_client and I should return None if it's not correct, is that right? I.e. in those cases where the secret is required, the correct thing to do for a wrong secret is to return None from get_client?

[aliev]

Hi, @shawn-monadical.

According to the standard of authorization_code grant, the client_secret is neither required nor needs to be provided. Therefore, I would just ignore it and check whether client_id exists in the database.

In other cases, where client_secret is required, but it was not provided, indeed, you can return None to get_client, what raises an exception here:

aioauth/aioauth/grant_type.py

Lines 66 to 69 in 56bb61c

	if not client:
	raise InvalidClientError[TRequest](
	request=request, description="Invalid client_id parameter value."
	)

[shawn-monadical]

For an authorization_code grant, a client_secret is required sometimes: It's not ever required on the authorization request, but it IS required on the token request IF the client has a secret assigned to it (making it a "confidential client"). However it's not necessary to assign client secrets to your clients for them to be able to use authorization_code grants, and if they don't have a secret assigned then they don't need to provide one. But if they do have one assigned, then they MUST use it in e.g. the token request.

See here: https://www.rfc-editor.org/rfc/rfc6749#section-4.1.3

If the client type is confidential or the client was issued client
credentials (or assigned other authentication requirements), the
client MUST authenticate with the authorization server as described
in Section 3.2.1.

After doing some more looking though the code, it looks like @tdg5 already implemented support for this in #80. That PR makes sure that the client_secret parameter will never be None in cases where authentication is required. Since that PR got merged, it seems like the contract of the get_client function is now intended to be like this:

If the client_secret parameter is None, that means it's a request which doesn't need authentication and so therefore the client_secret shouldn't be validated. But if the client_secret parameter is not None (including empty string), then the request does need authentication IF the client has a secret assigned to it, otherwise if it doesn't have a client secret assigned to it then it doesn't need authentication and shouldn't be validated.

So, authentication is required if the client_secret parameter is not None AND the client_secret in the database is not None.

In the case where the request does need authentication, and the client_secret is wrong, then the correct thing to do is to return None. If the request doesn't need authentication, then just return the client object as normal without checking the client_secret.

So basically, the correct implementation of get_client should be something like this for example:

db_client = select(DbClient).where(DbClient.client_id == client_id)
if db_client is None:
    return None
if client_secret is not None and db_client.client_secret is not None:
    # validate the client_secret
    if client_secret != db_client.client_secret:
        return None
return db_client_to_aioauth_client(db_client)

Does that sound right?

[shawn-monadical]

Note there's one extra detail I didn't include above which is that the client credentials grant ALWAYS needs authentication on the token request whether or not a secret is assigned in the database. But the developer can account for that by just making sure any client which supports the client credentials grant always has a client_secret set in the database. I just thought I should note that it's a situation which is not explicitly enforced by this library.

Nevermind, my mistake: this situation is checked in the client credentials validate_request function.

[aliev]

Yes, your code looks like the right solution. Sorry if my answers were not accurate enough. OAuth 2.0 has many conditions and rules that are hard to remember, especially when you haven’t worked with it for a while.

[shawn-monadical]

Thanks for confirming, I just wanted to make explicitly sure that I wasn't using it wrong and leaving open some kind of security vulnerability in my app!

OAuth 2.0 has many conditions and rules that are hard to remember, especially when you haven’t worked with it for a while.

No kidding!

[shawn-monadical]

Check here for a PR to add client secret validation to the demo app: aliev/aioauth-fastapi#15