Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

API support for multiple secrets? #91

Closed
abathur opened this issue Oct 19, 2017 · 19 comments
Closed

API support for multiple secrets? #91

abathur opened this issue Oct 19, 2017 · 19 comments

Comments

@abathur
Copy link
Contributor

abathur commented Oct 19, 2017

We've considered using a JWT as a relatively long-lived server-to-server API key (for a handful of first and second-party servers) that specifies permitted endpoints/restrictions/etc, on which we'd use blacklisting. We've also considered various short-lived client authorization uses where we don't really need to worry about blacklisting.

I'm not sure how much it would matter in reality, but I would be a little more comfortable using JWTs for both purposes if it was trivial to specify separate secrets for each use case. This would:

  • reduce the number (probably by at least a few orders of magnitude) of tokens floating around that could be used to attempt brute-force recovery of the secret used to sign the API-key tokens
  • enable us to rotate more heavily-used auth secrets without having to temporarily break integrations and redistribute API-keys

One approach might be using the existing config vars when the decorators are used normally, and adding support for explicitly passing one or more secrets to the decorator. I think this could be reasonably intuitive:

API_KEY_SECRET = os.getenv("API_KEY_SECRET")
AUTH_SECRET = os.getenv("AUTH_SECRET")

@jwt_required(API_KEY_SECRET)
@route("/partner_api")
def partners(self):
   ...

@jwt_required(AUTH_SECRET)
@route("/account")
def account(self):
   ...

A more robust version might be for the decorators to accept your config objects, along with a factory that'll generate a config object as you do currently (with defaults from app.config), but override any user-supplied values.

@vimalloc
Copy link
Owner

I'm sure we could add support for this into the application. My initial reaction would be to prefer something like @jwt_required(secret=AUTH_SECRET) just because it would be easier to implement with the current code base, but I would like to think on that more. If you wanna take a stab at this I would be happy to help get a pull request merged, or I can look into in the next week or so when I have some time on my hands.

Cheers. 👍

@vimalloc
Copy link
Owner

Sorry for the delay in this, the past month has been crazy hectic.

I'm hacking on something right now that would allow you to pass in optional kwargs to the protected endpoints, then setup a configuration context to be used during the request. It will look something like this:

@jwt_required(secret_key='foobarbaz')
def foo:
    return jsonify(hello='world')

Any relevant options that would be used during a request will show up as kwargs in here. I'm hoping to have this finished up in the next couple days.

Cheers.

vimalloc added a commit that referenced this issue Nov 30, 2017
This should be good to go. I want to add a few more tests in here for
completeness sake, and sit on this for a few days to make sure I'm happy
with the changes and maintaining this feature in the future, but I don't
expect any issues here.
@vimalloc
Copy link
Owner

I did some more work on the ability to have totally different option contexts at any stage in the extension, and after coming up with a solution, I decide I really didn't like the added complexity and edge cases that feature got us, compared to the general usefulness it provided.

I since changed back to my original plan of adding a single decode_key kwarg into the view decorators. I have that pretty much done, I just need to add some more unit tests and update create_access_token and create_refresh_token to take a new encode key as well, but that shouldn't be hard to do.

Because I have gone back and forth on how I want to implement this a few times, I'm gonna finish adding the last features, then sit on it for a few days before pushing out a new release, just to make sure I'm happy with how it all turns out.

Cheers.

@abathur
Copy link
Contributor Author

abathur commented Nov 30, 2017

No worries re: delay--our interest in using multiple secrets is a future to-do item that is probably a few months down our list. Picking at the feature has been on my to-do list as well, but it's been a busy few weeks. I agree with both sitting on the changes until you're confident and not significantly complicating the extension if the full set of options does so.

I'm in a different mental code context at the moment and don't want to break it by reviewing relevant code, but AFAIR the secret key is the most obvious setting. When I originally mentioned a full config option, I think I was imagining some consumers wanting key-specific default expiration deltas and such, but it's not that much more complex for them to specify app-specific config values and use those to generate different expiration times on token creation.

@zmellal
Copy link

zmellal commented Jun 5, 2018

is this issue still relevant ?
I have the same interest

@vimalloc
Copy link
Owner

vimalloc commented Jun 5, 2018

It is. I have the work largely done for this, but never got around to finishing it up and adding unit tests. I’ll revisit this when I have some free time and see if I can finish it up

@nicostuhlfauth
Copy link

Would be really awesome 😄

@Cryp10M1n3r
Copy link

Similar question: What about supporting multiple JWT_PUBLIC_KEYs with some kind of identifier? Is there maybe some way to hack something like this together, at the moment? I've looked into expanding the current decorators but as far as I understand there is no way to hook into the signing process before the token is decoded (which will inevitably fail if I use the wrong key).

@vimalloc
Copy link
Owner

I was thinking about this more last week, and I think it would be really cool to have something like a @jwt.encode_key and @jwt.decode_key callback functions that would pass in either the JWT data (obviously unverified in the case of the decode key), and lets you return what encode or decode key you want to use based on that data. I think that would be a lot more versatile then the @jwt_required(secret_key='foobarbaz') approach that I was previously working on.

Thoughts?

@Cryp10M1n3r
Copy link

Sounds great. With something like this you could both support multiple private-public-keypairs and keys for symmetric encryption.

@abathur
Copy link
Contributor Author

abathur commented Jul 5, 2018

I agree that there's value in a more versatile implementation. I think the JWT data would be enough in our case.

I do wonder if someone would want to return distinct keys for different applications of a JWT format that are programmatically indistinguishable without more context (endpoint? function name/object? kwargs passed through the decorator?) than just the JWT data? I'm stretching to invent a plausible example, but perhaps to support distinct login endpoints/keys for users/partners/admins.

Thinking through that example, I guess it's easier to just document that anyone who needs this should either imbed a distinct value in otherwise indistinguishable JWTs per use-case, or fish what they need out of the request object?

@vimalloc
Copy link
Owner

vimalloc commented Jul 5, 2018

Yeah, that's a good point. Although all the information could be retrieved from flask.Request as you mentioned, that sounds really brittle and hard to refactor/maintain.

That said, I'm leaning towards the identifiers in the JWT being good enough, it seems like it would be a pretty natural fit for the use cases I'm coming up with. Very similar to how role base authentication is normally handled.

Thanks for the feedback! 👍

@steinitzu
Copy link
Contributor

Are there any news on this?

A @decode_key callback with access to the unverified claims would be perfect for my use case as I'll be loading the secret based on a client ID in the token.

Cheers

@vimalloc
Copy link
Owner

Sorry, I know I’ve been saying I would finish this forever now. I’ve been stupid busy at a newish job and haven’t gotten around to it yet. I’ll put this at the top of my list of things to do. In the mean time, if anyone else needs it now and wants to submit a PR I would be more then happy to work with them to make sure it looks good and get everything merged.

@steinitzu
Copy link
Contributor

No worries, just wanted to make sure you weren't already working on it in case I attempt a PR. I can take a stab at it, hopefully in the next few days.

@hanrok
Copy link

hanrok commented Sep 3, 2018

+1 waiting for this feature too. Do you have some branch for this feature already?

@steinitzu
Copy link
Contributor

Implemented decode key callback in #191

Not sure if encode key callback can be done without breakage, I think signatures of encode methods here would have to change at least https://github.com/vimalloc/flask-jwt-extended/blob/master/flask_jwt_extended/tokens.py#L35

@vimalloc
Copy link
Owner

vimalloc commented Sep 11, 2018

Thanks for your work on the decode token! I've gotten that merged, it looks great!

Per the encode tokens, https://github.com/vimalloc/flask-jwt-extended/blob/master/flask_jwt_extended/tokens.py#L35 isn't exported in the API, so I'm totally cool if this changes. We will make a note of it in the release notes in case someone is using it, but I specifically make no guarentees to these helper functions so that making changes to the extension is as easy as possible.

As long as https://github.com/vimalloc/flask-jwt-extended/blob/master/flask_jwt_extended/utils.py#L98 doesn't change (which calls https://github.com/vimalloc/flask-jwt-extended/blob/master/flask_jwt_extended/jwt_manager.py#L420 behind the scenes), then I'm totally happy.

Thanks for contributing 👍

@vimalloc
Copy link
Owner

vimalloc commented Sep 16, 2018

This has now been released in v3.13.0. Check out the documentation for the custom decode key and the custom encode key

Cheers! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants