Conversation
|
Whoops, I wasn't aware that a Draft PR wouldn't notify anybody to actually take a look. So, marked as Ready for Review, but again, still just WIP and looking for high level feedback before finalizing. Thanks! |
|
Hi. Thank you for your contribution. We had the notification even when the PR was in draft. |
* Add DPoP extension for AuthorizationCodeGrant and RefreshTokenGrant * Add DPoPTokenValidator for ResourceProtector * Add DPoPProofValidator to validate proofs * Add DPoPNonceGenerator protocol for server-side nonce creation and management * Add DPoPNonceCache protocol for client-side nonce cache * Add DPoPTokenGenerator for public-key bound access tokens of DPoP type * Remove update_nonces, as nonce-loss is self-correcting
| @property | ||
| def dpop_jkt(self): | ||
| if self._dpop_jkt: | ||
| return self._dpop_jkt | ||
| return self.data.get("dpop_jkt") | ||
|
|
||
| @dpop_jkt.setter | ||
| def dpop_jkt(self, value): | ||
| self._dpop_jkt = value |
| def validate_token_type(self, token, request): | ||
| from ..rfc6750.errors import InvalidTokenError | ||
| from ..rfc9449 import DPoPTokenValidator | ||
| if token.get_dpop_jkt(): | ||
| if self.TOKEN_TYPE != DPoPTokenValidator.TOKEN_TYPE: | ||
| raise InvalidTokenError(description=f"Access token is bound to a DPoP proof, but token type is {self.TOKEN_TYPE}", | ||
| token_type="DPoP", | ||
| realm=self.realm, | ||
| extra_attributes=self.extra_attributes) | ||
| else: | ||
| if "DPoP" in request.headers and self.TOKEN_TYPE != DPoPTokenValidator.TOKEN_TYPE: | ||
| raise InvalidTokenError( | ||
| token_type=self.TOKEN_TYPE, | ||
| description=f"DPoP proof not expected for {self.TOKEN_TYPE} token type", | ||
| realm=self.realm, | ||
| extra_attributes=self.extra_attributes | ||
| ) | ||
|
|
||
| saved_token_type = token.get_token_type() | ||
| if saved_token_type.lower() != self.TOKEN_TYPE: | ||
| raise InvalidTokenError(description=f"Access token is of type {saved_token_type}, but token type is {self.TOKEN_TYPE}", | ||
| token_type=saved_token_type, | ||
| realm=self.realm, | ||
| extra_attributes=self.extra_attributes) |
There was a problem hiding this comment.
Mixin, if possible. Though this was a bit more difficult to do, but I'll give it another look.
|
Based on the discussions on #814 I'll likely refactor the server-side component a bit, so take the existing structure with a grain of salt. |
|
Refactored and addressed most of the comments I left above, and from ideas from the PAR #814 PR. The one remaining bit is in I haven't fully finished the httpx/requests integrations yet, but I refactored their Auth classes to remove code duplication and make it easier to add new extension Auths in the future via a CompositeAuth, and handle retrying for auth reasons. Again, please give feedback on the overall approach before I address cleanup, tests, docs, etc. I added an overall server extension which automatically hooks into AuthorizationCodeGrant and the RefreshTokenGrant, so developers don't have to add the same extension multiple times (like in the original top-level comment). |
| self.dpop_proof = dpop_proof | ||
| if self.dpop_proof: | ||
| self.dpop_proof.generate_jwk(token, metadata.get("dpop_signing_alg_values_supported", None)) | ||
| self.dpop_auth = DPoPAuth(self.dpop_proof) |
|
|
||
| return True | ||
|
|
||
| def validate_token_type(self, token, request): |
There was a problem hiding this comment.
Instead of adding this validate_token_type in TokenValidator, you should subclass TokenValidator and create a new DPoPTokenValidator.
Subclass this validator to register into ResourceProtector instance.
| raise ValueError(f'"{key}" MUST be JSON array') | ||
|
|
||
|
|
||
| def _validate_boolean_value(metadata, key): |
There was a problem hiding this comment.
An underscore method is a private method, if you need to import this method elsewhere, better to make it a public method by removing the first underscore.
This is a WIP to request feedback early on in the approach before I clean things up, write tests, and submit an official PR.
This implements RFC-9449: OAuth 2.0 Demonstrating Proof of Possession (DPoP), #315 .
I'm supporting ATProto (Bluesky) in my application, and their implementation requires DPoP and PAR (which I also have a change I'm testing that I'll put up as a WIP PR later). You can read more about their specific requirements here for context.
Please ignore the requests/django integration at the moment, as I've only focused on HTTPX in FastAPI at the moment. I'll update the other integrations once the approach is more finalized.
To use in Client:
I explored refactoring TokenAuth/ClientAuth to support multiple Auths (separating Authorization header/client_secret_* modifications, and the DPoP proof header modifications), but that turned out to be more complicated than I originally expected. This would've introduced a new CompositeAuth to handle the pairs: TokenAuth/DPoPAuth, ClientAuth/DPoPAuth. I'm not against further exploring this idea, if it's desired by the maintainers. For now, I've implemented DPoP Proof directly in TokenAuth/ClientAuth implementations.
To use in Server:
Feel free to give any comments or ask questions as necessary about the implementation. Once I've got go ahead for the approach I'll clean up, implement the further integrations, add tests, and put up a final PR.
Thanks!
TODO:
dpop_signing_alg_values_supportedto metadata, and check alg against it