feat(auth): Add an auth middleware.

[SimonTeixidor]

Related to nickel-org/nickel.rs#98.

Provides auth middleware with an example page. Depends on nickel-org/nickel.rs#234.

There's probably some stuff that needs cleaning up, but I figured we could get the discussion polish it up once we're happy with the design.

I have written some basic tests with bash and curl for the example page. I hope it's possible to get travis-ci to execute those.

As this is a matter of security, it'd be great to get as many eyes as possible on this. Ping @cburgdorf, @Ryman, @lambdaburrito, @rgs1, @ninjabear.

[Ryman]

I think this is a good initial sketch, I'd like to discuss this at a higher level as there are some issues we should get clear before reviewing the code too closely.

I think the roles of Authentication and Authorization could be a bit more explicit, where, authentication would manage some kind of current_user via login/logout and authorization would deal with 'does current_user have access to this page'. Authorization clearly depends on authentication, but the exact method of authentication should be irrelevant I think. I don't think we should need to pass in access_granted/access_denied handlers, as they should be part of the middleware flow? Errorhandlers should deal with a 403? I think all that's required, if authentication and authorization are split, would be the access_granted case.

For the implementation, this is dealing pretty low-level with raw cookie values, a lot of this logic is useful for other potential middleware too, so I think we should be implementing this on top of a Session Middleware (which may or may not be backed by Cookies). If a "remember me" type login were to be added in future though then that might explicitly use Cookies though. The session should be responsible for ensuring it's stored safely and cannot be tampered with so others can depend on it while also dealing with session timeouts. (I think the session could actually be strongly typed too, but it may be awkward with typeparams)

Security-wise, (afaik) you ⚠️ must ⚠️ authenticate what you encrypt, I'm pretty sure this could suffer a (reasonably trivial) bitflipping attack. Using the CookieJar from cookie.rs handles this already and I'd recommend skimming the source (it's based on rails by the looks of it and does some neat things with function pointers to mimic the api).

The shell script to test the implementation looks good, should be simple to get that running on travis via script. I think it's something we do seem to lack in the main repo, would be great to see if we can adopt something similar there to test example behaviour more thoroughly. Reducing regressions for sweeping changes would be great! 😅.

Lots to discuss! I am looking forward to see how this turns out, getting even a simple authentication/authorization setup going with a simple api would be wonderful!

[SimonTeixidor]

Cool! I'll work on splitting the code up in three middlewares then Authentication, Authorisation (well, do we have a consensus on wether we use American/International English? I can only think of utilize on the spot but there's an issue to replace that one anyways), and Session.

I'll read up on bitflipping attacks and do something about that.

Then I'll update to follow the new Cookie API, whenever you have something on that.

I'm excited as well, it will feel like a much more complete framework once we get authentication and authorisation working, like all the basic pieces are there :)

[SimonTeixidor]

I've updated this a bit. Still a bit messy, I'll look into cleaning up the uses and so on once we settle on a design.

There's now a ReadSession trait, which reads the current session from a cookie. It should not be too hard to make this trait get the session from other sources as well, such as a parameter in a POST call or something.

Then, there is the AuthenticatedSession trait, which exposes a method to retrieve the currently signed in user.

Lastly, there is the Authorizer, which is a middleware that passes authorized users through and sends 403 on unauthorized requests.

I used the encryption in the CookieJar so that we don't have to worry about getting that part wrong by doing it ourselves.

Session timeouts are dealt with in ReadSession (actually ParseSession, but the public interface is ReadSession), by simply setting the current session to None. If that session reaches the Authorizer, a 403 will be sent to the user. I figure that we don't care about the state of the session until the user actually tries to reach a page that requires it.

I'm not entirely sure what @Ryman means by ensuring the stored safety of sessions. As of now, they are retrieved from the CookieJar in Request, which is not mut, which means that the session is safely stored, no? Otherwise, feel free to explain if I should be doing something more.

[Ryman]

Haven't read over the changes yet (hopefully will later or tomorrow), but to clarify what I said about safely storing the session I just mean that whatever Session type there is, it needs to guarantee it's data persists across pages and that the client cannot read or tamper with the data stored within the session.

Most systems I'm aware of at least stores some kind of session_id inside a cookie on client, even if the rest of the session data is stored in something like memcache, the id on the client must be safe. So, I mean safeguard against a malicious client of the website, not safeguard against a nickel programmer doing odd things.

[SimonTeixidor]

I understand. I haven't implemented it like that, rather I store all necessary data in the cookie. That makes the session handling stateless on the server side so that we don't have to keep track of sessions at all.

I prefer this method, but there might be arguments for just sending a session id to the client and keeping track of the sessions on the server.

[SimonTeixidor]

Thinking a bit more about this, I wonder if we shouldn't make session handling generic, such that the user can store any serialisable type in the cookie, instead of just String. Sure, the most common case might be to store a user name, but there's no reason to limit ourselves here.

[Ryman]

I understand. I haven't implemented it like that, rather I store all necessary data in the cookie. That makes the session handling stateless on the server side so that we don't have to keep track of sessions at all.

Sorry, I meant that even if they use something other than cookies as their store, some minor interaction with cookies is usually still required :)

[Ryman]

After having a closer look at this, it makes me realise that nickel probably still needs changes to allow a Session API that I would enjoy using.

Specifically, I think the res.set_session_cookie(..) style api doesn't allow multiple Middleware to use and set their own session values (e.g. one is counting pageviews for the current session, one is tracking userid). The Middleware would have to read the cookie value, merge their own changes to it, then set it again, which may not even be possible if cookie-rs is eager in it's encryption.

I'd imagine something like req.session("foo") = bar, but the problem is that the session should only deal with a single cookie/record ("__token" in this case) and to update the cookie it needs to know its old value, whereas currently the api only allows reading from the request's cookies or writing new cookies to the response (which is unaware of the request value).

So, I think there's a bigger change required where we deal with plugins at an 'environment' level, that is, they have access to both the request and response, and writing to cookies would modify the response, reading would read from request (and changes made to the response). Perhaps there's a neater alternative?

Thinking a bit more about this, I wonder if we shouldn't make session handling generic, such that the user can store any serialisable type in the cookie, instead of just String.

Adding to your point, I don't think a session should care about authentication (other than verifying the session hasn't been tampered with). I feel the duplication present in a simple example highlights this where both SessionConfig and Authorizer have the same auth function. It should really be nothing more than a glorified hashmap imo (although potentially it could be strongly typed).

sidenote: the formatting for much of this code is strange where opening and closing braces/brackets can be difficult to match, is your editor setup for a different language?

[Ryman]

Just fyi: This needs an annotation to compile, e.g. Response::<'a, D>::create_..., or in more recent versions of rust you can do Self::create_....

Imo, something like create_session_token_str should be a regular function rather than a static trait method.

[SimonTeixidor]

After having a closer look at this, it makes me realise that nickel probably still needs changes to allow a Session API that I would enjoy using.

Perhaps you're right, yes.

Specifically, I think the res.set_session_cookie(..) style api doesn't allow multiple Middleware to use and set their own session values (e.g. one is counting pageviews for the current session, one is tracking userid). The Middleware would have to read the cookie value, merge their own changes to it, then set it again, which may not even be possible if cookie-rs is eager in it's encryption.

I'd imagine something like req.session("foo") = bar, but the problem is that the session should only deal with a single cookie/record ("__token" in this case) and to update the cookie it needs to know its old value, whereas currently the api only allows reading from the request's cookies or writing new cookies to the response (which is unaware of the request value).

But, with a plugin on Request that parses and caches the current session the first time it is read, this is pretty much doable without major changes to nickel, no? The only issue is that the last Middleware would have to copy and serialize whatever data we use for a session into a cookie in Response, it would of course be nicer if that was automatic.

Further, I think we should make it so that req.session() returns whatever underlying data it stores. I, for one, would not like a map interface as it would be a pain once you want to store anything else than strings, but for those who want it, it should be possible to use a hash map and write something like req.session().insert("foo", "bar").

So, I think there's a bigger change required where we deal with plugins at an 'environment' level, that is, they have access to both the request and response, and writing to cookies would modify the response, reading would read from request (and changes made to the response). Perhaps there's a neater alternative?

Is it possible to implement a plugin on (Request, Response)? I tried a bit but the compiler complains that neither the trait or the type is implemented in the crate. I guess that's just a language limitation at the moment, right? Otherwise one could have the api be something like (req, res).session(), which would not be too bad in my opinion.

Adding to your point, I don't think a session should care about authentication (other than verifying the session hasn't been tampered with). I feel the duplication present in a simple example highlights this where both SessionConfig and Authorizer have the same auth function. It should really be nothing more than a glorified hashmap imo (although potentially it could be strongly typed).

Yeah, let's throw that part out. It has a use case I suppose, where you want to serve different content in the same route depending on whether the session belongs to a valid user or not, but in those cases it's easy to just handle that in the Route. No need to make the most simple example look bad.

sidenote: the formatting for much of this code is strange where opening and closing braces/brackets can be difficult to match, is your editor setup for a different language?

Nah, I use vim with the rust settings. I'm just terrible at formatting :) I will go through it more carefully once we settle on some design and read some style guide as well to improve a bit.

[Ryman]

But, with a plugin on Request that parses and caches the current session the first time it is read, this is pretty much doable without major changes to nickel, no? The only issue is that the last Middleware would have to copy and serialize whatever data we use for a session into a cookie in Response, it would of course be nicer if that was automatic.

If it's a plugin then it won't cache if req.session() is not called before the res.session() is written to, if it's a middleware then there is the ordering requirement. And having to remember to serialize inside the last middleware (which may not always be the last middleware called) is a common bug waiting to happen.

Further, I think we should make it so that req.session() returns whatever underlying data it stores. I, for one, would not like a map interface as it would be a pain once you want to store anything else than strings, but for those who want it, it should be possible to use a hash map and write something like req.session().insert("foo", "bar")

I think you're right that a simple hashmap wouldn't be ideal, I was thinking a bit too dynamic. What do you think about using something like the TypeMap?

Otherwise one could have the api be something like (req, res).session(), which would not be too bad in my opinion.

I considered this, but I think it's really unintuitive for users.

[SimonTeixidor]

If it's a plugin then it won't cache if req.session() is not called before the res.session() is written to, if it's a middleware then there is the ordering requirement.

We could store the cached session in req though, could we not?

And having to remember to serialize inside the last middleware (which may not always be the last middleware called) is a common bug waiting to happen.

I agree!

I think you're right that a simple hashmap wouldn't be ideal, I was thinking a bit too dynamic. What do you think about using something like the TypeMap?

I think the TypeMap might be ok. Ideally I'd like to be able to use any data that is serialisable. Do you think we can get the TypeMap serialised somehow though?

I considered this, but I think it's really unintuitive for users.

Yes, it seems a bit wild and crazy at first, but the more you think about it, the more sense it starts to make. We do need to make an operation on both req and res, we might as well be explicit about it.

[Ryman]

If it's a plugin then it won't cache if req.session() is not called before the res.session() is written to, if it's a middleware then there is the ordering requirement.

We could store the cached session in req though, could we not?

But what if the request's session never gets called (to make it cache), or do you mean through (req, res).session()?

I think the TypeMap might be ok. Ideally I'd like to be able to use any data that is serialisable. Do you think we can get the TypeMap serialised somehow though?

Maybe not by default, but with the right bounds and probably introducing a newtype we can make it work 👍

[SimonTeixidor]

But what if the request's session never gets called (to make it cache), or do you mean through (req, res).session()?

But, that would mean no one read from the session, so why would we cache it? My idea is that a session is parsed and cached in the req upon it's first read, so that many middleware can operate on it without having to parse it each time. If no middleware reads the session, then it'd just be parsed and immediately moved to the res without any changes in the last middleware, I suppose. Perhaps I'm missing something!

Although, this would still not be good as we still have the issue of remembering to move the session in the last middleware.

Maybe not by default, but with the right bounds and probably introducing a newtype we can make it work 👍

Sounds good!

[Ryman]

If no middleware reads the session, then it'd just be parsed and immediately moved to the res without any changes in the last middleware, I suppose.

I thought we were talking about a plugin, but it seems like you're talking about a middleware for the session? This part can't work with a plugin as when a user accesses response.session() and the cache doesn't exist then the Response doesn't have access to anything with which to rebuild the session (it can't access the Request cookies or anything).

[SimonTeixidor]

No, I'm talking about a plugin, which middleware will use to access the session.

My point is that there does not need to be a response.session(). One would only use request.session(), until we actually have to set the response session, where there would be something like response.set_session(request.session()).

[Ryman]

That feels very similar to the 'bug waiting to happen' we agreed upon above? I think we're stuck in a bit of a deadlock here, maybe we should each code up something and compare?

[SimonTeixidor]

Yes, indeed it is! But perhaps we could attack that problem in Nickel somehow.

Sounds good, I'll see if I can come up with something.

[Ryman]

I've pushed up an experimental branch showing the basics of what I was talking about (Response == Environment for now): https://github.com/Ryman/nickel.rs/tree/environment

[SimonTeixidor]

Cool! I have updated this repo to use that branch. Nice job! 👍

I threw out session handling and authentication from this repo, only the Authorizer middleware is left.

What I missed was a provided ServerData equivalent with a new() function, it'd be nice if we didn't have to deal with the lower level detail like that in the Auth example. I was going to make one, but we need to have timeout() in SessionStore take self as a parameter so that one can store the Duration in ServerData.

We can discuss your work further once you feel ready to send a pull request :)

[Ryman]

What I missed was a provided ServerData equivalent with a new() function, it'd be nice if we didn't have to deal with the lower level detail like that in the Auth example. I was going to make one, but we need to have timeout() in SessionStore take self as a parameter so that one can store the Duration in ServerData.

I don't fully understand this, can you give more detail? Why do we need to store Duration in ServerData?

[Ryman]

Instead of having ValidateUserFunc, would an Authorize trait be useful, which we can bound T with?

e.g.

trait Authorize {
    type Permissions = bool;

    fn permission(&self) -> Self::Permissions;
}

Then the user could have more complicated permissions, e.g. if let Some(Write) = env.current_user().permissions() { .. }. I guess in my head there's still also a missing abstraction of current_user which is built upon session (as the session can store a lot of unrelated stuff, not user details).

Thinking further, then the Authorizer could be expressed something like server.get(Authorize::only(Permissions::Owner, middleware! { "Secrets" }))?

[SimonTeixidor]

That's a great idea! It looks really nice, if you ask me.

[SimonTeixidor]

Regarding current_user, I suppose that can be nice.

Could something like:

trait SessionUser{
    fn current_user(&self) -> Option<String>;
}

work? The user would implement this for whatever session data they store and we can provide some default implementation for String for example.

[Ryman]

I think the following may be along the lines we want:

trait CurrentUser : SessionStore {
   type User;

    fn current_user(&<Self as SessionStore>::Store) -> &Self::User;
}

And then base a trait on Environment/Response on that. I'm not sure where the value of adding a default implementation lies though, this behavior is very specific to the application. I think this trait is really just to extract the current user from a session so that Permissions can be written in terms of a user model rather than the entire Session.

[SimonTeixidor]

What I missed was a provided ServerData equivalent with a new() function, it'd be nice if we didn't have to deal with the lower level detail like that in the Auth example. I was going to make one, but we need to have timeout() in SessionStore take self as a parameter so that one can store the Duration in ServerData.

I don't fully understand this, can you give more detail? Why do we need to store Duration in ServerData?

Yes, I'd like to be able to do use nickel::ServerData, but with a better name of course, and then nickel::with_data(ServerData::new("SecretKey", Duration::minutes(60))); or something, so that the user don't have to create that struct. Just for convenience.

I wonder though, if we can get ServerData to be generic. I suppose we can put a PhantomData in it, but I don't know exactly how that works with initialization and so on.

[SimonTeixidor]

I've been through all your comments, @Ryman. Great advice, as always! 👍

[SimonTeixidor]

I've updated the Authorize struct, from what I understand it is correct now. I've also added an enum type for user classes in the example. Unfortunately, I could not get rid of type parameter P completely, I just get thread 'rustc' has overflowed its stack... Further, I had to introduce a PhantomData<D> field on Authorize to get it to compile, as Middleware<D> was not in the struct body any more. Perhaps there's a cleaner solution to that?

Finally, I'm still not sure how multiple permissions work with this trait. Right now we do an equality check in the middleware. If one route requires read permission, read and write permission is not going to match, for example. Do you have some other idea for how the check in the middleware should be implemented, @Ryman?

[Ryman]

Pushed a branch with 2 extra commits on top of this, one removes the boxing from Authorize and the second is a really quick sketch of how multiple permissions might be done, but I think PartialEq is a better bound (especially for the 'all' version). (added a third commit with PartialEq change for clarity)

[SimonTeixidor]

Nice. I still don't understand how Authorize::all is working though. The only way I can see anything get passed that is if you only specify one type to check against. How is the same return value from permissions() going to compare equal against a list of different permissions?

[Ryman]

@simonpersson Hmm, actually I think I went a bit mad with that quick sketchup. What I was proposing was kind of abusing equality to be true for multiple things, but that sounds like a hell of a footgun if people don't expect it. I guess I was more thinking of an ordering rather than equality.

Your suggestion of fn permissions(&self, &Permissions) -> bool might actually be better, I was thinking the role of the function would be different but it may be more understandable to be a simpler if we rename it a bit has_permission(&self, &Permission) -> bool.

[SimonTeixidor]

@Ryman - Yes, I had the feeling that things would get a bit tricky once you wanted to use multiple permissions.

I merged your changes in, rewrote history a bit to make the commit log reasonable again, and replaced the permissions with has_permission. It's starting to look nice if you ask me! :)

[Ryman]

It's starting to look nice if you ask me! :)

Definitely making progress 👍

I built on your rebase to add current_user which I feel is separate from session. The commits aren't entirely clean since I had some odd inference issues so I had to keep reworking it a bit (updated the nickel environment branch to allow hinting the macro what the datatype is for the middleware): https://github.com/Ryman/nickel-auth/tree/current_user

I fleshed out the example a bit to simulate more realistic requirements, it might be a bit overblown for a real example, but it should at least help us see how the design holds up. I think it's good that the user code has no nickel related code other than the trait name.

[SimonTeixidor]

Nice! Feel free to merge the current_user branch onto master once we get the changes in Nickel merged.

[SimonTeixidor]

I've updated this to latest nickel. If we're happy with how it looks, I'll start documenting so that we can eventually merge this. I feel that we might want to offer a more simple API on top of this that is not as generic, but perhaps that can come later.

Ping: @cburgdorf @Ryman.

[SimonTeixidor]

I screwed up with git so I this PR closed... :( I guess I can manually merge and force push once we're happy.

[Ryman]

Sorry, I realize now when trying to test this branch that I had an updated version of the current_user branch that was basically in line with the extracted session/cookies which I hadn't pushed up.

The diff for this pr seems to be broken so it's hard to see what the full code looks like, but if it's what's currently master on your fork then I still think it's worth pursuing the current_user style where permissions are not coupled so tightly to sessions.

I just added some more fixes to the current_user branch and pushed it to here for comparison.

related but offtopic: Doing the minor fixes did make me think naming the important trait in session as Store is kind of awkward, and that we should probably have inherent method on CookieSession, so a trait import for that isn't required.

[SimonTeixidor]

I see. Well, let's keep working from that branch then! :)

related but offtopic: Doing the minor fixes did make me think naming the important trait in session as Store is kind of awkward, and that we should probably have inherent method on CookieSession, so a trait import for that isn't required.

Good! It should make things a bit more readable.