Import JWT Authentication middleware

[secretfader]

This PR imports the JWT Authentication middleware I was chatting about earlier in #289. The current version is set up to work with Gotham 0.3.

[secretfader]

Here you go, @colinbankier. I hope everyone else approves. 👍

[colinbankier]

Great, thanks @secretfader. At a quick first scan this looks great.
There does seem to be some issue with the Windows build in one of the deps:

"crypto/fipsmodule/aes/aes.c"]: The system cannot find the file specified.

I haven't looked in detail what can be done about that.

@nyarly @whitfin - any issues with including this once all checks pass?

[secretfader]

I'm not sure what's going on with the Windows -gnu build, but similar build errors have shown up in ring before. Unless someone else with existing knowledge swings in with a tidy solution, I'll do some research and see how we can get it building on both toolchains.

[secretfader]

As I read reports from around the Rust community, it seems the most common recommendation is to rely on the Windows MSVC toolchain primarily because its ABI produces better interop support for standard Windows applications. Rustup's documentation addresses this, even.

My question then is: whether to allow failures on what is considered a lower-tier support level for most of the Rust community?

[colinbankier]

I'm totally fine with switching to the MSVC toolchain if that fixes the issue easily, unless there's a good reason not to. I'd be nice to understand a little more what the issue is before switching though (if it doesn't become a rabbit hole digging into it).

[whitfin]

@colinbankier as a general, I don't mind liberally merging things like middleware which are self contained and don't affect the general codebase.

However... I'm not sure if we want to take ownership of things like JWT rather than having it separately owned. I don't know enough about JWT to know if this implementation covers the entire surface area of JWT auth - I'm thinking about the case where something is missing and @secretfader doesn't have time to update it and we're left with an incomplete implementation. I guess I'm saying that I'm not really the right person to give a sign off on this :p

It might be fine just on the premise that this will be a separate crate rather than shipping with core. I know @nyarly had some thoughts on middleware distribution a while ago so I'm interested in hearing from them.

Aside from the above, I'll give the code itself a quick pass anyway!

Edit: forgot to mention; I'd be against merging with a failing Windows build without doing everything we can to fix it.

[whitfin]

I left a couple of comments, but generally things look good!

[colinbankier]

Thanks for the feedback @whitfin.
I must admit, I initially glossed over the fact that it would be published as a separate crate and not ship with core. Is that the normal way we would deal with middleware that lives in this repo? I also don't want it to add unnecessary friction around release - either to gotham or the middleware crate.

One motivator for including it was simply for discoverability. There are other ways to help this though, and might not be a good reason.

[secretfader]

Maybe the solution isn't, as I proposed, moving middleware into core. This thread has me wondering if I should've suggested a document of known middlewares or a link to the gotham-middleware keyword search on crates.io. Maybe middleware doesn't belong in the core repo at all.

If there's serious interest in moving this PR forward, then I'll fix the issues raised by @whitfin and we can proceed to merge at will. (In fact, most of the changes are already made over at the old repo, and just need to be synced to this PR.)

But I have the distinct hunch that going through this process has raised better ideas than the original. I'd hate to waste your time if there are better approaches on the table.

[whitfin]

I think that we are yet to determine exactly what we're doing with middleware.

The current state of things is that "general" middleware (i.e. loggers and such) as in core itself, and ship in the gotham crate. They're used by enough developers that they make sense to be included.

There is a Diesel based middleware which lives in this repo, but ships as a separate middleware. This predates me on this project, but I believe it was done this way to enable iteration separately. That makes sense to me (because you can fix bugs in that without shipping the entire codebase).

However! A lot of people seem to enjoy when things "just work", so I've been thinking about a model where we basically carry on as we're currently doing (and as such, this would move into the core repository in the same way the Diesel one exists). We could them add dependencies to them and re-expose them from the gotham crate, perhaps via feature flags to avoid compiling them unnecessarily. This would make adoption of these middlewares much easier.

The only downside is what I said before; there's a notion that because it lives in this repository, it is owned by this team. I don't mind this (especially enough to block merging this PR), but it was mainly just a note for the future.

So really, what I'm saying is pretty much summed up as: merge away! If we decide something later we can always change it up (there are middlewares in three places already, so we'd have to change something anyway).

@secretfader I'm very unfamiliar with JWT, but could this implementation be considered "complete"? Is there some validation document we can compare against? JWT is so general that this might actually be worth moving into the core crate itself, but I'm unsure exactly what is in this PR vs. what could be in this PR. If this is complete, I'd vote for moving it into the gotham crate. If it's not, then I'd have it where you currently have it targeted - which would meant it shipped under its own name.

[secretfader]

This middleware is only a portion of your authentication system where JWTs are concerned, but to answer your primary question, yes, I believe it represents the most secure approach. By default, it adopts the default JWT validation from the jsonwebtoken crate, but allows it to be overridden with custom validation, making the validation of incoming requests even more secure.

While it will only allow tokens that validate to pass (checking that both the crypto hasn't been tampered with before checking validity of the issuer and time of any expiration keys), you'll still need to implement logic for generating and storing new JWTs, purging expired tokens, and linking them to accounts inside your system. But this module provides assurances that when you look up a user from the parsed values inside the token, those values haven't been tampered with during transport.

I have tinkered with the idea of implementing a turn-key auth platform on top of Gotham, and this crate would be a vital piece whether in core or not. For what it's worth, I think the module should probably always be distributed as a separate crate even if it lives in this repo. While authentication is a general need among web apps, not all require it (and even fewer require a specific implementation like JWT).

That's the main story, @whitfin and @colinbankier. I'd be happy to finish this up and get it ready to go if that's what you want.

[secretfader]

I updated the PR in case it's decided to place the code in this repo. Alternatively, we could also have most of the middlewares (at least first party ones) hosted on the Gotham organization, too.

[colinbankier]

Thanks for updating things @secretfader. I think, based all the discussion above, we're pretty much ready to merge it 👍
The last pesky thing is to get the build to pass.
@secretfader, I'm not super familiar with appveyor and the windows builds, but from a glance it seems like it should be possible in .appveyor.yml to add an if check in the test_script based on TARGET - so that we can do cargo test --all on x86_64-pc-windows-msvc toolchain, but add an --exclude for middleware/jwt on x86_64-pc-windows-gnu.

It'd be nice to allow the rest of Gotham to still build on x86_64-pc-windows-gnu if something like that is possible. If that doesn't work out, I'm OK with removing x86_64-pc-windows-gnu as a target. I'm not sure how important that is to Windows users, but noted the recommendation you linked above for ring that "People should just use the -msvc toolchain".

[secretfader]

@colinbankier I've read over the AppVeyor documentation, and it seems relatively easy to allow failures on certain targets, but that feels overly broad. The original plan of mine was to take a different approach and set different test_script values on a per-target basis, but I'm not sure it's possible.

[codecov]

Codecov Report

Merging #296 into master will increase coverage by 0.2%.
The diff coverage is 89.36%.

@@            Coverage Diff            @@
##           master     #296     +/-   ##
=========================================
+ Coverage   79.23%   79.44%   +0.2%     
=========================================
  Files          92       93      +1     
  Lines        4508     4602     +94     
=========================================
+ Hits         3572     3656     +84     
- Misses        936      946     +10

Impacted Files	Coverage Δ
middleware/jwt/src/middleware.rs	89.36% <89.36%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 36cb21a...16acf89. Read the comment docs.

[secretfader]

Ok, after several minutes wrestling with AppVeyor configuration, I didn't find a way to successfully exclude crates to build/test only on a single TARGET.

I'd like to find a way to re-enable x86_64-pc-windows-gnu, but I'm not sure how to proceed even after spending more time than I should've in the docs. (AppVeyor config looks simple on first glance, but quickly becomes a confusing mess as you add more build options.)

[colinbankier]

Thanks @secretfader - I'll have poke at it too when I can, and if not successful either, will go with what you've got here.

[colinbankier]

Great work, thanks @secretfader !

Let's go with removing the gnu toolchain for windows for now.
If this is important to any windows users down the line, this can easily be re-assessed.

[secretfader]

Thanks for handling this, @colinbankier. I also appreciated the helpful review, @whitfin.

Here's to making Gotham even more awesome. 👍