-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Feature - Add JWT support #2648
Feature - Add JWT support #2648
Conversation
- Having an issue with `jwt:decode`, likely not pulling in the lib correctly?
- Have a funny feeling I'm not passing the data correctly
… into add-jwt-authentication-handler
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @atrauzzi ! Thanks so much for contributing to Apache CouchDB. We're really fortunate to have you here! I have a few comments.
I'm not the best Erlang developer in our group, so I've mostly reviewed for style, and compliance with our team rules & regulations. Hopefully someone else will step in and provide more in-depth comments than I can.
Please do not use any git submodules. We don't use them (or like them) here at CouchDB.
If you've written the code in those submodules yourself, and are contributing them to CouchDB, just add the entire tree of files under the appropriate src/module
directory. Remember this will place all of this code under the Apache License v2.
If those submodules (which I'm unable to review in the PR, sadly) are external, 3rd party dependencies, as with jwt
, add the URL as you've done to our rebar.config.script
file. It'll help us complete due diligence of the code if you can note the original developer + LICENSE of those modules in your pull request somewhere. As mentioned, we'll have to import those repositories into Apache infrastructure, as well as we must add the license information to the top level NOTICE
file in our repo. Information on dependency licensing that is compatible with the Apache License v2 is here: https://apache.org/legal/resolved.html .
For any 3rd party dependencies, you need to add those to our top level .gitignore
file so they are not imported into this repository accidentally. This goes for src/jwt
at the very least - I don't know about base64url and jsx.
FYI, we already have a base64 url encoder in mochiweb - have a look at mochiweb/src/mochiweb_base64url.erl
. Hopefully you can reuse that instead of introducing a redundant dependency?
On the topic of CORS and JWT - if CORS being enabled is required, we may not want to enable JWT by default. I'm concerned that opening CORS up to the world violates the principle of least surprise, especially for CouchDB users who expect us to ship a secure configuration by default.
Finally, I've had a long-standing knee-jerk reaction against JWT because of these two articles. Can you comment on this? Does your proposed approach address any of the (rather serious) accusations raised by Sven?
Thanks again for all your hard work on this PR.
finally, we need some tests and docs, but focus on the comments above first. |
Hey @wohali - thanks for the review! Lots of obvious cleanup in there, I wasn't sure whether to leave it as part of the review process, but yes, definitely will get on the specific code changes later today 🙂 I'll speak to the concern over JWTs for the rest of this comment as I think it's important conceptually to address separately. I apologize if this is too long, or if some of my explanations dip into the basics, it's necessary to keep oriented while giving these articles a considered look. On JWT
I'm going to give them their due, but overall, I'm not sure why these posts exist. I found them poorly reasoned and by their title alone, not applicable.
Honestly, I don't think the posts generate any actionables. If I could say one thing, it's that the purpose of this PR is not to introduce JWTs for sessions, only for identity. That might be the tldr;... 😆 I'll start out picking on a few notable points, but if there's anything specific that you feel applies, let me know. click to read...GeneralI don't see a legitimate complaint proven in the posts, the author is making it clear that they are only interested in single node monoliths and that they don't understand that JWTs are not intended to store server-side state! 😅 To discuss the topic implying that JWTs are equivalent to sessions is a false premise. And even despite the "A note upfront" section, the posts fail to demonstrate an understanding of JWTs. This is indicated by the list preceded by "I'll define a few terms first" which exposes the authors mistaken assumption that JWTs are used for session data. Another example of suspicious rationale: "While signed cookies are more secure than unsigned cookies, this is in no way unique to JWT, and good session implementations use signed cookies as well." ...Not exactly a well-reasoned indictment of the security or utility of JWTs. Let's clarify:
While cookies encode data, that data doesn't necessarily have to represent identity. Indeed many sites you visit will create a cookie without you even having taken an action! In fairness, it is very easy to conflate the concepts involved given the combination of abstractions that many web frameworks offer. I think we're seeing a bit of this in the posts, with the point being that before criticizing JWTs, the author should have spent some time getting an idea of their intended use. It wouldn't be too out-of-hand to stop at this point and dismiss these posts as fear mongering, but I will be fair and give it it a little more analysis. Easier to (horizontally) scaleThis is not a primary motivator for adopting JWT. The benefit offered by JWT is a single common and stateless format to provide and validate claims in a heterogenous system. This argument really doesn't fit with an analysis of why one shouldn't use JWTs. CSRFYes, JWTs in authorization headers do prevent CSRF. It's an oddly seldom known trick that you can cleverly mitigate against CSRF by explicitly requiring a header as part of a request. This header can be as simple as JWTs in headers - as I propose them here - are more secure than cookies. Though I'd also like to remind that this is not the primary reason why I'm looking to introduce them. Expiration, Easier to Use, More Flexible
This is the kind of assertion that really brings these posts down. Focusing on expiration as a flaw takes the architectural experience or preferences of the author and pushes them as absolutes. The whole point of JWT is that the token will not be visiting a single, centralized monolith. It will be shopped around to various cooperating services. Also worth noting that people often configure their cookies to live for far too long, vs say... Implementing a remember-me token, which is basically an ad-hoc refresh token built on top of cookies. Something already standardized by OpenID Connect. The fact that JWTs can be set to self-destruct means that rotations can be forced with zero overhead to servers that will be receiving the token. Case in point, my PR doesn't require any logic to direct the client on how to obtain new tokens. Single UseThis is a strange rule imposed by the author which I reject. I could use this same argument to say that a bearer token in a cookie should be single use as well. This point is easily disregarded as trying to kitchen-sink against JWTs. The flowchart offered in the follow-up post I feel doesn't really require much attention. Some of the statements within are cynical and are too far into prior misunderstandings to really warrant concern. Really, it just comes down to the fact that cookies and JWTs can be used for overlapping and sometimes different purposes in equally as nuanced situations. I use cookies for browser based authentication in the same application that also uses JWTs to validate API requests. There's nothing unusual about this, it's how many applications perform their oauth2 authentication and consent flows. Finally, these posts came out in June 2016, coming up to their 4 year anniversary. If the security concerns they detail were in any way legitimate, I'm very confident the greater communities we all participate in would have discarded JWT by now. Instead, I think we can see quite the opposite and that client & server libraries as well as many large scale services have come to embrace JWT. Again, I'd love to explore any specific concern, so do let me know if there's one in particular I didn't happen to choose to drag out here. 🙂 |
Most of the objection to JWT in those articles appears to be that the tokens can't be expired ahead of their built-in exp field, a criticism that can equally be levelled at our session cookies. On that point, this PR does not directly validate any of the time fields (iat, nbf and exp). If the jwt library is doing that, we should say so, as this validation is mandatory. The other objection I could determine is cryptographic, in that the tokens themselves dictate which encryption scheme to use to validate them, and there's at least one known weakness where a substitution can be an issue. We should get ahead of this and have server side controls over what we'll accept. I can think of a few settings off the top of my head but this should be thorough before merging;
Finally, if we're doing this for CouchDB I would prefer to use the JWT library I developed for Cloudant rather than this one. I will, of course, pursue the licensing steps, etc. |
For signature verification, HS256 signed, unencrypted seems to be the norm in the implementations and testers/tools I've seen. If we can get the JWT implementation you made, that should help with the transitive dependencies (base64 and jsx). I'm assuming it would also help with the edit: The |
yes, my library insists on valid tokens (current time must be between iat and exp) before it returns the payload, etc. I realise you can't see it to confirm that right now. |
Hi, IBM Cloudant has released its JWT library under the ASLv2 at https://github.com/cloudant/jwtf, please update to use this. This obsoletes the extra libraries this PR currently references. The jwtf library will be contributed to CouchDB as part of this effort. |
Hi there - thanks for the essay. I'm not a security expert and won't pretend to be one. I'm comfortable knowing that the issues have been reviewed. I wasn't planning on vetoing JWT support, in any case. Carry on. |
- Integrate `cloudant/jwtf`, drop `jwt` et. al. - Clean up and update `default.ini`
- Remove redundant sub claim config - Explicitly require binary to come back from re:split
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for working through all the feedback, this looks good to me.
Hey @atrauzzi , we need documentation for this change. Would you be willing to file a PR against https://github.com/apache/couchdb-documentation ? Thanks! Both myself and @flimzy can help if you need it. |
Add JWT Authentication Handler Co-authored-by: Robert Newson <[email protected]> Co-authored-by: Joan Touzet <[email protected]>
Add JWT Authentication Handler Co-authored-by: Robert Newson <[email protected]> Co-authored-by: Joan Touzet <[email protected]>
@atrauzzi I cannot for the life of me configure this and I need it for a project, can someone help, thanks! |
@jainishmehta Please don't spam closed PRs. For more immediate support please see https://couchdb.apache.org/#chat and join the Slack community, or you can post a new thread on the Discussions section of this repository, if the documentation isn't sufficient. |
Overview
👋 Hello! This PR is introduces support for JWT authentication in CouchDB.
Before any details, I would like to give a big thank-you to @rnewson for his guidance and help. He's been a great ambassador! 👏
Why?
I'm starting out with the familiar scenario of a JavaScript application using PouchDB that I wish to sync to CouchDB.
What's different is that my JS application performs authentication against an OpenID Connect/Oauth2 server that is not running on CouchDB. Think Auth0, Azure AD B2C, OpenIddict Core, etc...
At present, in order to authenticate with CouchDB from clients that are already performing authentication outside of the CouchDB server, I am forced to consider one-off workarounds, all while a de-facto standard already exists for the notion of a portable and stateless identity: JWT.
My hope is that this implementation weighed against the complexity and less deliberate nature of any one workaround makes a good case for itself.
Testing recommendations
After enabling the authorization handler and setting a matching JWT secret in your couchdb ini file, you should be able to authenticate to CouchDB using an HTTP
Authorization: Bearer [token]
header.Checklist
rel/overlay/etc/default.ini
Meta
I'm relatively new to Erlang but am interested enough to get this through review. I've tried to keep my changes as additive as possible however if any refactors or structural work ends up being required, I may need detailed guidance.
Finally, I feel this functionality would be valuable not just for my own personal use, but for future users considering CouchDB for their own mixed ecosystems. Many cloud vendors offer HTTP-centric API gateways and auth* endpoints which are a good match for CouchDB.
Not that I want to sound too product-y, but I think having JWT support built in would be a great way to broaden CouchDBs reach! 🙂