Support DPoP token_type

See infra/security#28 for context

Assumptions

• DPoP proof should always be sent to our provider in the new version
• for now we are only supporting the authorisation code grant type, which is used by sib-auth. Implicit grant type has been recommended against by the Solid-OIDC specification
• No work required for PKCE implementation (PKCE is already supported by Django-WebIDOIDC-Provider, via the code challenges included in the tokens)
• I think no changes are required to generate ID token/refresh token

Implementation

• Orientating to codebase, changes to unit tests (1.5 days)
• converts DPoP Public Key to JWT thumbprint (2h)
• binding user's Public Key to the Access Token ("DPoP-bound Access Token"), by including the JWT thumbprint in the Token (2h)

mentioned in issue infra/security#28

changed the description

changed the description

@balessan note the assumptions here

changed time estimate to 2d

assigned to @calummackervoy

added Doing label

mentioned in merge request !10 (merged)

@calummackervoy Sorry I had an incident to handle yesterday. I just had a look at your issue. I'm not sure where you me exactly so I just started with the functions that are missing something to start the discussion.

For example this one looks like it is missing something: https://git.startinblox.com/djangoldp-packages/django-webidoidc-provider/blob/1-dpop/oidc_provider/tests/cases/test_token_endpoint.py#L92

From what I read in the spec your DPOP Proof is a subset of a JWT (https://tools.ietf.org/html/draft-fett-oauth-dpop-04#section-4.1).

Before implementing the full stuff, I started to play with it a bit. I attached a file with some dummy code generating a HMAC-SHA256 token and a RSA-SHA256 token (closer to what we need to implement).

I just did that to, at least, make sure my understanding was compliant with the existing library. It seems ok as both generated tokens successfully passed the https://jwt.io/ verification tool.

Now I start implementing the extra stuff we need for DPOP Proof.

I think that the DPoP proof produces a valid DPoP header from that spec you posted ? I skipped on the htu for now and I suppose other things like the alg should be passed as parameters

Ah nice yeah the file you attached looks great :)

I'll message you about a call for how we can split the work

@calummackervoy Investigating a bit further on pyjwt I see that there is enough material there for what we need to do: https://github.com/jpadilla/pyjwt/blob/master/jwt/algorithms.py

If we can, don't you think it's better to implement our stuff in this project ?

Oh my bad. In the end there isn't anything we have to do on the cryptographic side. We are just packing headers differently. So we could you the pyjwt as it is. And we can check our DPOP token with jwt.io.

OP side we're using pyjwkest for info (it's not being maintained anymore though - #3 )

@calummackervoy I just figured I have a serialization issue with the RSA modules in the jwk object. I'll try to fix it as soon as I can.

It is fixed now ! But manipulation of raw data tends to be quite difficult to read. So you may have problems to follow my code. Plus, I think you may have difficulties to build the public key from the jwt. It contains only the modulus and the exponent. This is just what you need but it requires some computation to rebuild the whole public key.

Call me before hitting your head against the wall ;)

mentioned in issue #2

mentioned in issue #6 (closed)

changed the description

@plup please could you add DPoP to the default values of OIDC_ACCESS_CONTROL_ALLOW_HEADERS?

ping @plup could we also add Authorization to the same setting please ?

I think those are the only ones we need

In fact I think the default should look like this: djangoldp!212 (diffs)

@calummackervoy I think this default value should also go there no ?

https://git.startinblox.com/djangoldp-packages/djangoldp-account/blob/master/djangoldp_account/djangoldp_settings.py

Without the sentry-trace because it's specific to the way we deploy apps. So it would be authorization, Content-Type, if-match, accept, DPoP

And I added the new line to the deployed apps with ansible. It's there if you want to review: infra/platform!60 (diffs)

Ah yes, nice one

In future, do I also want to hardcode a default setting like I do there (djangoldp!212 (diffs)) just in case the djangoldp_settings.py is overridden at runtime ?

Or should I assume from now that if my_setting is defined in djangoldp_settings, then getattr(settings, 'my_setting') won't throw an error and I shouldn't hardcode a default ?

mentioned in merge request infra/platform!60 (merged)

removed Doing label

mentioned in merge request !18 (closed)

closed via merge request !10 (merged)