Token endpoint expects client_id in request

[vladimir-mencl-eresearch]

Hi,

I'm running a SATOSA deployment with OpenIDConnect (pyop) frontend and SAML2 backend.

Based on discussion in IdentityPython/pyop#39, I've tried using oidcop instead of pyop.

When testing this with an RP/client running mod_auth_openidc with authorization_code grant, I was getting

OpenID Connect Provider error: Error in handling response type from the client.

And from the logs, I could see SATOSA logging

[ERROR] [oidcop.oidc.token.post_parse_request] Access Code invalid

After some digging, I found the load_session_from_db method in satosa_oidcop/core/storage/mongo.py was not finding anything in MongoDB, because it was querying with _q={'authorization_code': 'Z0FBQU...5a', 'client_id': None} - with client_id populated from parse_req.get("client_id") .

The code expects to client_id param to be passed to the token endpoint, even though the OpenID Connect specification of Token Endpoint makes no mention of it.

I was able to work around this by configuring the OP to send client_id as an extra parameter:

IDCProviderTokenEndpointParams client_id=my_sample_client_id

... but I believe this should be fixed in oidcop, removing the requirement to send client_id as an extra parameter.

Thanks a lot in advance for addressing this.

Cheers,
Vlad

[peppelinux]

Hi Vlad, thank you for this!

OIDC Core 1.0 states that token endpoint should be protected with a client authentication if the requester is a confidential (not public) client. And, well, the client authentication can be private_key_jwt or all those listed in the section below (we may also support mTLS and so on, for the future ...)

https://openid.net/specs/openid-connect-core-1_0.html#ClientAuthentication

as you can read in all the non normative examples in OIDC Core we have the client_id in the request.
All but not the one you mentioned, because it uses Basic Authentication (so the client_id is encoded in the base64).

Well, we can say that if the authz header is Basic and client_id is there, well, we can take it from there, it would be

parse_req.get("client_id", None) or take_client_id_from_basic(request_object) 

But I don't see any parse_req.get in the token endpoint method below
https://github.com/UniversitaDellaCalabria/SATOSA-oidcop/blob/main/satosa_oidcop/idpy_oidcop.py#L305

The place where you can obtain the client_id (whatever it is and verified with a well known client authentication) is here, in ._load_cdb (cdb, acronym of client db)

SATOSA-oidcop/satosa_oidcop/idpy_oidcop.py

Line 54 in a1984f8

def _load_cdb(self, context: ExtendedContext, client_id: str = None) -> dict:

the door is open, we can do whatever we want with the software, now we just have to put here the right assumptions before doing the right thing

I'd ask to @melanger to give his feedback on this proposal.

[vladimir-mencl-eresearch]

Hi @peppelinux ,

Thanks for the detailed reply - and sorry, I see had not followed into the OAuth 2.0 specification. I now see that other client authentication methods would pass client_id ... and my client happened to use Basic Auth.

And I see I should have included a direct link to the code using the parse_req.get - it's hiding in a long chain of delegation at https://github.com/UniversitaDellaCalabria/SATOSA-oidcop/blob/main/satosa_oidcop/core/storage/mongo.py#L131

The proposal (parse_req.get("client_id", None) or take_client_id_from_basic(request_object)) looks reasonable to me - let's see what @melanger thinks about it.

Cheers,
Vlad

[peppelinux]

you can easily achieve that adding the support of the basic auth in ._load_cdb

it will query mongo and load valid clients inmemory

so you can intercept the client authz in that method and then the query on mongo will work

every choice we'll take, we'll always prefer the simplest one

[melanger]

Hi, it seems reasonable to me. I also experience some strange problems when varying the client authentication method, so this might be the fix.

[peppelinux]

@vladimir-mencl-eresearch considering that you're working with basic auth, can you propose a PR to cover this feature?

[vladimir-mencl-eresearch]

Hi @peppelinux ,

PR #26 sent - please let me know what you think about it.

Cheers,
Vlad

[github-actions]

🎉 This issue has been resolved in version 1.0.1 🎉

The release is available on:

• v1.0.1
• GitHub release

Your semantic-release bot 📦🚀