Variable 'aud' claims based on source of request and matching validation

[AlexanderNeilson]

Hi Team

I am coming up on a requirement at work for the ability we can set the audience dynamically based on which system our users login from and validate they are using the JWT on the same system. For us this involves our internal facing portal system and an external facing user portal allowing staff to log their timesheets / do time tracking and request leave etc which all needs to feed into the same core database and will share some endpoints.

To this library I am considering adding settings to allow signalling where the aud claim should be derived from and validated against and also either add a mechanism to use from permissions checking to ensure the audience further is allowed to access certain endpoints (or this would be handled inside my main code and leave this aside).

My review of the code here indicates support for static aud claims using the config but not currently a dynamic method so I am looking at adding such a feature (either locally vendored in my project or as a contribution).

As the name includes "simple" I wanted to check before I started coding if this was something the project would see a benefit from / want me to fork and propose a PR on this or if the solution above sits too far outside the goals / restrictions this project sees for itself so I can contribute it back if keen.

I would love to see any feedback / discussion on this and happy to answer questions about what I would be planning to implement or changes to bring it more inline with the goals of the project.

Thank you
Alexander

[Andrew-Chen-Wang]

Hi Alexander,

There is currently no support for authorization in this framework. There has been a request before about making SimpleJWT customizable to feel like OAuth (in summary) in that users can only be authorized to certain views by having a certain payload configuration. (I can't believe I can't find the issue... grr).

I don't think Dave imagined SimpleJWT to be restricted to authentication only, so it'd be great to see a contribution for a little bit of authorization!

If you have a plan for how you would implement this smoothly, I'd love to hear it. Class-based or function-based decorators on a DRF view? How would you go about implementing this in the serializers? Etc. I'm excited, so thanks!

Best,
Andrew

[AlexanderNeilson]

Hi @Andrew-Chen-Wang / others

This is a sort of spec I have written up to try get my core goals straight and how I am thinking of doing the implementation. Currently no decorator in the core config (sorry, I will try implement that but it isn't required for the core implementation at my work so I will try add it in my spare time)

Can you please run over this and see if I am stretching too far outside the design / standards for this project or any other feedback you may have.

Hopefully starting to code this in the next few days and have some drafts to review with.

The idea:

Dynamic AUD claim introduction into SimpleJWT

New settings config (names can be changed):
AUDIENCE - Optional, Default None (existing). A value of 'Dynamic' enables the dynamic AUD system. Only required field to enable dynamic aud in default mode (using origin header, fail any mismatch with default error message), None will continue to omit the AUD claim and any other value other than string 'Dynamic' will also retain the current behaviour of a static AUD claim of that value and comparison to same static value from provided token
DYNAMIC_AUDIENCE_HEADER_FIELD - Optional, default 'HTTP_ORIGIN', value to be passed to META.get() method on request object to retrieve the value to be used in the AUD claim on creation of JWT and to obtain value for comparison. Considering making this support a list of headers that can be selected from as my read of the specification for JWT indicates AUD can be a list of strings that any one matching from the client request to the AUD claim provides a match. My design doesn't require this additional feature support as it is simply single origin matching.
ALLOW_NO_AUDIENCE - Optional, default False. A value of true allows a JWT with no audience claim to be accepted when processing authentication of the supplied token. If set to False (default) (and AUDIENCE is not None) then any token received missing AUD as a claim will be immediately rejected as an invalid token (even if its signature would pass) and if none of the DYNAMIC_AUDIENCE_HEADER_FIELD entries above provided a header value then generating the token would fail (testing and failing on both to ensure that any change in policy is effected before tokens expire and to prevent a token generated with the same SIGNING_KEY being validated even if it was allowed to be generated on another system). It is not advised to set this to True except during a migration window where tokens generated beforehand should be accepted or where only a new set of API endpoints are being added that would require AUD claims and existing endpoints aren't being retrofitted. The signature will still be checked and provided the SIGNING_KEY hasn't leaked this should prevent people minting their own tokens even if the AUD is allowed to pass.

Authorization:
This part of the solution is a little more complex for my design vs a completely generalised structure. My solution using this already implements custom permissions and I have created custom viewsets including per app context and uses this and the permissions a user has per app to allow particular endpoints and particular methods on those endpoints being utilised by users (and also inside the selected methods on the selected endpoints also only allowing certain elements inside those endpoints to be modified even if the general modification access is available).

So my plan is to provide a method as part of my changes that would allow validating the AUD matches an approved list for that endpoint. I would be implementing this as permission calls allowing for single or multiple allowed AUD and the method would be passed the request object and a set of strings allowed as AUD's that can call that endpoint and it will return True if the AUD claim (or one of the AUD claims) exists in the set provided or false otherwise.

I do not yet have experience creating my own decorators so I would need to practice before I could implement that. I would like to learn and use that learning to include this as a feature however, as above, my work requirement doesn't include this feature so it would be a personal goal for me to add and not part of an initial pull of the feature (unless I magic up some more time or things go really fast).

Regards
Alexander

[Andrew-Chen-Wang]

Hey @AlexanderNeilson I wanted to comment awhile ago but kept running out of time to complete my draft response. Everything sounds good to me as a gist of my response, but the authorization part can be moved to a different PR. Please look forward to some comments on Saturday! Just wanted to make sure you still remembered this PR after so long.

[AlexanderNeilson]

@Andrew-Chen-Wang

I have worked up the first phase of this code in my fork

Current working branch

and if you would like to take a look I would be most appreciative.

Currently it is missing tests and a full separate documentation page talking about the pro's and con's of the system.

My work dev environment is running this code in full right now and working however we don't use sliding window tokens etc and there are still some debug prints through the code.

If you had a little bit of time to provide any feedback you had / guidance I would be happy to take it on board and maybe get the first phase in place to allow this to be launched (as the authorization part can be somewhat more cleanly added through individuals own local custom permissions - and I may write the reference for that better once I am doing per view / viewset / action decorated in my own project)

Thank you for the follow up.

Regards
Alexander

[Andrew-Chen-Wang]

Hey Alexander! Glad to hear from you. #279 has a little more authorization bits in place for SimpleJWT so I wouldn't worry too much about that portion. Again, I'll review your spec on Saturday to finalize any draft details.

As for speeding up code-wise, try to follow by the guidelines of "test-driven coding." It'll speed up the process a lot as you begin moving around parts and finding bugs and seeing if things work in general.

I honestly wouldn't worry about sliding tokens. I had the repo owner explain it to me, and even then it felt unwieldy. I honestly wouldn't bother with it; I think David wanted to remove it at some point as it's not really industry standard ;)

I believe Sphinx documentation allows you to link up code to the documentation itself and the block comments will be in the docs, for example: https://docs.donate-anything.org/en/latest/_source/users.html. So if you just add the comment blocks, they should just be inserted automatically. If you're running sphinx, you should be able to view them at localhost:7000.

[Andrew-Chen-Wang]

Long overdue review of this spec is here; it's not going to be much because... school. But I like the idea that's being brought forth:

DYNAMIC_AUDIENCE_HEADER_FIELD

Definitely great. Saw this in a different issue.

AUDIENCE and ALLOW_NO_AUDIENCE

The gotcha is right here.

[In the ALLOW_NO_AUDIENCE portion] A value of true allows a JWT with no audience claim to be accepted when processing authentication of the supplied token.

An issue is going to be confusion. Audience and no audience? Instead of setting something to static/dynamic and then configuring a secondary variable, we might be missing something if the tests aren't rigorous enough.

Additionally, the ALLOW_NO_AUDIENCE when set to False (as dynamic) should raise an exception on AUDIENCE being None. Are you relying on PyJWT to handle this?

One of the issues I'm seeing with this is the saying of "dynamic." Is it dynamic or is it a single environment variable being loaded in one time?

It is not advised to set this to True except during a migration window where tokens generated beforehand should be accepted or where only a new set of API endpoints are being added that would require AUD claims and existing endpoints aren't being retrofitted. The signature will still be checked and provided the SIGNING_KEY hasn't leaked this should prevent people minting their own tokens even if the AUD is allowed to pass.

This is a nice check, but I feel like the package should be checking this. By the docs statements, default setup doesn't require installing a package, but I believe this kind of setup would require app checking now.

Again with the dynamics of the API endpoints, I'm not entirely sure how you're passing this around between your nodes. Each node should have the same signing key otherwise cryptographic signing of something like the session cookies or user password generation are going to be different. Maybe I'm just not understanding.

Authorization:
This part of the solution is a little more complex for my design vs a completely generalised structure. My solution using this already implements custom permissions and I have created custom viewsets including per app context and uses this and the permissions a user has per app to allow particular endpoints and particular methods on those endpoints being utilised by users (and also inside the selected methods on the selected endpoints also only allowing certain elements inside those endpoints to be modified even if the general modification access is available).

We have an authorization callable now in place from a recent PR, but yours is a little more unique and a little more narrow that the one from there. I think this should be dropped and instead be implemented by a Django middleware or the new feature. And to be even simpler (not really), but take advantage of django-oauth-toolkit so database access to those permissions aren't necessary. However, the mentioning of selected endpoints and modifying stuff is... interesting, but I'd want to see that in a different PR.

My brain might be spaghetti right now, but large PRs are not good. It'll get confusing really quickly. We can introduce this in several chunks instead so that review is better processed (additionally, we would like to see tests included for each portion, and code coverage will likely show a lot of spots are just missing).