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

Feature - Add JWT support #2648

Merged
merged 46 commits into from
Mar 19, 2020

Conversation

atrauzzi
Copy link
Contributor

@atrauzzi atrauzzi commented Mar 10, 2020

Overview

👋 Hello! This PR is introduces support for JWT authentication in CouchDB.

image

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

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! 🙂

Copy link
Member

@wohali wohali left a 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.

rebar.config.script Outdated Show resolved Hide resolved
rel/overlay/etc/default.ini Outdated Show resolved Hide resolved
rel/overlay/etc/default.ini Outdated Show resolved Hide resolved
rel/overlay/etc/default.ini Outdated Show resolved Hide resolved
rel/overlay/etc/default.ini Outdated Show resolved Hide resolved
@rnewson
Copy link
Member

rnewson commented Mar 10, 2020

finally, we need some tests and docs, but focus on the comments above first.

@atrauzzi
Copy link
Contributor Author

atrauzzi commented Mar 10, 2020

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

Finally, I've had a long-standing knee-jerk reaction against JWT because of these two articles. Can you comment on this?

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.
If we ignore the title and take it as generalized advice on JWT (a-la knee-jerk reaction), I could foresee untangling it taking up a lot of space in this thread.

Does your proposed approach address any of the (rather serious) accusations raised by Sven?

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...

General

I 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! 😅
Nearly all the points in the posts are presenting a preference as advice on the basis of what's good for the goose.

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:

  • JWTs are a(n) (optionally expirable, self-signable & encryptable) bearer token capable of storing small bits of information, typically identifiers
  • Cookies are a set of conventions to round-trip state, but are most often used to transport a bearer token that corresponds to yet more state server-side
  • At the transport level, cookies and JWTs are both susceptible to the same security risks. TLS is the common ground that most popular bearer-based systems use
  • Fun fact: Some people even use JWTs as the content of their identity cookie!

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) scale

This 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.

CSRF

Yes, 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 content-type: application/json, and authorization: Bearer * has the same effect: <form>s can't set headers! 🏆

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 nonsense, and not a useful feature. Expiration can be implemented server-side just as well, and many implementations do.

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.
When taken as part of a complete OpenID Connect implementation, you can get some very cool SDKs like this JavaScript one which will auto-rotate your tokens for you 🌟 and more 🌟!

Single Use

This 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. 🙂

@rnewson
Copy link
Member

rnewson commented Mar 10, 2020

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;

  • a config setting listing which of the JWT schemes will be allowed.
  • a config setting listing which asymmetric signing keys are permitted.
  • configurable leniency on the various timestamp fields, defaulting to 0 (no leniency).

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.

@atrauzzi
Copy link
Contributor Author

atrauzzi commented Mar 10, 2020

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 nbf and iat claims as well.

edit: The jwt library here does validate exp, for what it's worth, but again, I suspect your implementation will be a better fit if/when it's available.

@rnewson
Copy link
Member

rnewson commented Mar 10, 2020

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.

@rnewson
Copy link
Member

rnewson commented Mar 10, 2020

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.

@wohali
Copy link
Member

wohali commented Mar 10, 2020

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`
@atrauzzi atrauzzi requested a review from wohali March 10, 2020 20:31
@rnewson rnewson mentioned this pull request Mar 13, 2020
4 tasks
Copy link
Member

@rnewson rnewson left a 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.

@rnewson rnewson merged commit 032934f into apache:master Mar 19, 2020
@wohali
Copy link
Member

wohali commented Mar 19, 2020

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.

@atrauzzi
Copy link
Contributor Author

@wohali - Sure thing!

@rnewson, would you like me to wait until you've made your changes before doing any doc work?

rnewson added a commit that referenced this pull request Apr 1, 2020
Add JWT Authentication Handler

Co-authored-by: Robert Newson <[email protected]>
Co-authored-by: Joan Touzet <[email protected]>
@rnewson rnewson mentioned this pull request Apr 1, 2020
4 tasks
rnewson added a commit that referenced this pull request Apr 2, 2020
Add JWT Authentication Handler

Co-authored-by: Robert Newson <[email protected]>
Co-authored-by: Joan Touzet <[email protected]>
@atrauzzi atrauzzi deleted the add-jwt-authentication-handler branch May 11, 2020 13:36
@jainishmehta
Copy link

jainishmehta commented Dec 4, 2020

@atrauzzi I cannot for the life of me configure this and I need it for a project, can someone help, thanks!

@wohali
Copy link
Member

wohali commented Dec 4, 2020

@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.

@apache apache locked as off-topic and limited conversation to collaborators Dec 4, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants