panva / openid-client

OAuth 2 / OpenID Connect Client API for JavaScript Runtimes
MIT License
1.83k stars 392 forks source link

feat: support extra claims in JWT client_assertion #149

Closed apparentvisuals closed 5 years ago

apparentvisuals commented 5 years ago

OpenID allows extra claims to be added to client_assertion as part of private_key_jwt and client_secret_jwt. Added an optional and backward compatible way to insert extra claims into the client_assertion during a grant request with unit tests.

codecov[bot] commented 5 years ago

Codecov Report

Merging #149 into master will decrease coverage by 0.12%. The diff coverage is 90%.

@@            Coverage Diff             @@
##           master     #149      +/-   ##
==========================================
- Coverage     100%   99.87%   -0.13%     
==========================================
  Files          18       18              
  Lines         798      803       +5     
==========================================
+ Hits          798      802       +4     
- Misses          0        1       +1
panva commented 5 years ago

Please swap the merge order of extra vs well defined. Whatever is provided extra should not overwrite the conform implementation.

Also fix the lint by assigning the argument a defaul value rather then assigning to it ad-acta

Thank you for this. I’ll have a deeper look the coming days.

panva commented 5 years ago

@apparentvisuals Just checking in, i haven't forgotten about this, just now planning to revive the work on 3.x (due in April with node 12 release) and i'll include this there.

panva commented 5 years ago

@apparentvisuals can you explain what you need to add to the JWT client assertions?

apparentvisuals commented 5 years ago

We add some extra service specific metadata as part of the client assertion claims for generating slightly different token claims. Since in OpenID it is allowed to add extra claims to the JWT there wasn't any need to implement every single one as a custom grant type.

panva commented 5 years ago

I can understand extra request body properties but please clarify what’s the point of extending the client assertion

apparentvisuals commented 5 years ago

When used during client_credentials grant or custom grant types I see it similar to how Request Object works for authorization_code, the values could be in the request body, but inside the assertion the entire value set becomes signed by the private_ket_jwt.

panva commented 5 years ago

I still fail to see the point given that the assertion itself is a one-time use and the request itself is covered by TLS.

Authorization Request parameters are transmitted in plain text over the user-agent so thats why we have Request Objects. Here, i don’t see the need.

apparentvisuals commented 5 years ago

I agree, there may not be much difference between request body and inside the assertion JWT, I needed it because I used some grant types against existing idp that used it that way, and openid connect core specifically allows custom claims to be part of the assertion I didn't see any reason to not support this in a generic openid client for flexibility of the API.

panva commented 5 years ago

The point behind the statements about other claims is mostly extensibility by future profiles and specifications. Just sayin’

It will be part of 3.x nontheless but i would stay away from an IdP that’s not interoperable and seems to make up its own “profiles”.

pr4xx commented 5 years ago

Is this already implemented? We really need a way to add extra body params to the code exchange.

panva commented 5 years ago

yes, see the extras param.

pr4xx commented 5 years ago

Thanks! Is there any way to add these extra params to the passport strategy?