search

jazzband / django-rest-knox

Authentication Module for django rest auth
MIT License
1.15k stars 211 forks source link

[discussion] Provide endpoint for refreshing token expiry #156

Open sphrak opened 5 years ago

sphrak commented 5 years ago

As discussed in #131 @johnraz suggested that we also provide an endpoint for refreshing or increasing the expiration date by simply hitting the new endpoint and the tokens expiration date is increased.

I am thinking that we provide a new url /refresh/ that just requires an authenticated PATCH request to the endpoint which returns the new expiry for the token: {"expiry": "2019-01-11T07:54:34.504745"}

There should also be some control over how often this can happen. Not sure what to do here though.

I am willing to take this on me, but I would like to hear what you think before I start :)

belugame commented 5 years ago

for disallowing too fast/many refreshes we could plugin throttling: https://www.django-rest-framework.org/api-guide/throttling/#scopedratethrottle

and make it configurable via knox settings so the user does not need to override the view for it

johnraz commented 5 years ago

The semantic of PATCH seems to be appropriate as we are only updating a part of the resource.

However we are not really updating a resource via its specific resource URI /token/<id>/ and a payload providing the data but we are more implicitly updating it by triggering an action on /refresh. So I'm wondering if a POST is not more appropriate in this "action like" context - really unsure about this.

I like the fact that we don't have to deal with the token id because it makes things simpler but it also limits the feature to the currently logged-in user. eg: You have a dashboard listing all your active tokens and you want to specifically refresh one of them which is used by another device / service.

Maybe an intermediate solution to cover the above use case and avoid the complexity of exposing all the token resources would be to provide a refresh-all feature, the same way we currently have a logoutall endpoint.

This is all I have to say about this, I'm just dropping what comes to my mind, you can definitely move ahead with the simple version 🤓

johnraz commented 5 years ago

Oh and about the throttling part, I agree with @belugame that this should be optin and configurable. It's great to offer a solution that is easy to setup and doesn't require other tooling but I generally prefer to keep this logic at the loadbalancer / proxy level.

sphrak commented 5 years ago

@belugame thats a good suggestion.

Would it be a good idea to refactor the MIN_REFRESH_INTERVAL implementation into something that relies on drf's scoped throttling instead then? The drf throttling seems a bit more fancy than the current implementation of the MIN_REFRESH_INTERVAL -- and API-wise the MIN_REFRESH_INTERVAL is only triggered on AUTO_REFRESH which is False by default, so it can remain optional.

@johnraz I think I agree with you said about being able to refresh all tokens at the same time, there is definitely a usecase for that, I will think some more about that.

The cool thing about refreshing all tokens at once say are that we dont need to do the expensive hashing on each one, we just need 1 for the initial authentication then its just a matter of a single update statement on the queryset to extend the expiry field.

belugame commented 5 years ago

@sphrak yes, i like that idea. To make use of the parent-library as much as possible. KISS. Maybe we should delay the 4.0 release and give us more time to think about more things that we want to break & rebuild.

sphrak commented 5 years ago

@belugame I agree with you on that, lets delay the 4.0 release. I am going to hack on this during the weekend so chances are that we have a prototype of this on monday already. But yeah, we might want to break and refactor some more thing before 4.0.

johnraz commented 5 years ago

I'm a bit confused about the MIN_REFRESH_INTERVAL refactoring, I had the idea that the throttling feature of DJRF was related to views only. How would you see it fit in the authentication flow?

sphrak commented 5 years ago

@johnraz Maybe I misunderstood something but, afaik the MIN_REFRESH_INTERVAL seems like a simple throttling mechanism (I havent read the entire code base yet) but we could repurpose the MIN_REFRESH_INTERVAL setting to something that can be used by DRF's scoped throttling mechanism that yes would stop the request at the view already if its determined to be violating the throttle limit.

Now that would touch other parts aswell since this setting is used each time a protected endpoint is interacted with to increase the expiry datetime.. hmm I would have to think a bit more about this.

But I do think that if we use drf scoped throttling the use of MIN_REFRESH_INTERVAL in auth.py at least, would no longer be needed because its handled before hand and acts as a protection against excessive refreshing of tokens.

Or am I thinking totally wrong?

sphrak commented 5 years ago

@belugame You dont happen to know if I can reload the urls.py via reload_module() in testing? I cannot get it to work.

I am trying to implement a conditional urlpattern setting so that we only expose the refresh endpoint if the user has AUTO_REFRESH and TOKEN_TTL set to not None otherwise it wont make sense to have it exposed.

johnraz commented 5 years ago

@sphrak : MIN_REFRESH_INTERVAL was initially introduced in the context of the auto-refresh feature to avoid refreshing the token on every requests and hit the DB like crazy when it's not necessary. The check for MIN_REFRESH_INTERVAL is triggered for every authenticated request on any endpoint, not on a specific endpoint.

So if you'd want to use DJRF's throttling system to achieve this, this would mean that the MIN_REFRESH_INTERVAL would be the value at which you can hit ANY authenticated endpoint before triggering the throttle.

eg: You only want to auto-refresh tokens every 30 seconds. You have an authenticated endpoint that is usually hit by a user 3 times per second, and that's the expected usage of that endpoint, nothing's wrong with hitting it 3 times per second.

If you'd want to make use of DJRF's throttling system to limit the auto-refresh, you'd have to throttle the endpoint itself because auto-refresh happens everytime you authenticate which in turn would block your user from accessing it 3 times per second which would break the endpoint's usage.

I hope I'm clear, this is not easy to explain 😄

belugame commented 5 years ago

@belugame You dont happen to know if I can reload the urls.py via reload_module() in testing? I cannot get it to work.

I am trying to implement a conditional urlpattern setting so that we only expose the refresh endpoint if the user has AUTO_REFRESH and TOKEN_TTL set to not None otherwise it wont make sense to have it exposed.

Does this help you? It does not reload it, but you can set custom urls only for the one test method. https://stackoverflow.com/a/31838024/640916

sphrak commented 5 years ago

@belugame Yes sorry for wasting your time, I ended up doing that during the weekend but forgot to redact my request for help.

James1345 commented 5 years ago

Can I ask: should we be extending tokens?

I've not thought very long and hard about it, but I'm instinctively against the idea (I don't know why yet, I might just be being paranoid)

Would it not serve us better for the "Refresh" endpoint to cancel the token that is used for auth, and issue a new token as its response?

johnraz commented 5 years ago

@James1345 this is a very very valid point!

Refreshing is indeed better for security than extending.

A few points I think of on the top of my head if we go with proper refresh:

@sphrak, @belugame: thoughts ?

belugame commented 5 years ago

I also find returning a new token better than extending the old one. That avoids clients having an token infinitely. The longer a client uses the same password, the higher the risk it gets leaked/stolen and the longer an intruder is able to use it.

sphrak commented 5 years ago

Its a good point, but I see a few problems with it and frankly I dont see what problem it solves. But I am in no way a security expert but here are my thoughts on this.

Firstly, it should not be a problem extending the lifetime of a token as long as the communication channel is secured over https. So for this to be a problem you first have to defeat https -- in which case it will be game over anyway and it wont matter if you regenerate the token.

Second, generating a new token, saving it and deleting the old one is a more costly operation than just simply updating the datetime of the already available token object upon authentication, at any bigger scale at least.

Thirdly, an auth token is something I consider mostly the same as a password and normally its not enforced on a user to do a password regeneration on set interval. My point being if extending the token lifetime is a security concern, then so must also the "never expiring" password be. At least in my mind.

If we'd opt for a solution where we instead exchange a token for a new token it feels like we have recreated JWT authentication but with extra steps, clunkier and less scalable.

Having said that, I am not entirely against this. There might still be some value in this sort of support, where security is a top priority.

But again it comes down to https. If thats compromised it doesnt matter what we do -- and to get the token you have to do that or compromise the device on which the token is stored on. So I am having a hard time coming up with a reason to actually justify this. But please, enlighten me if I am missing something.

I think its far more likely that a token is leaked by poor "token management" on the client side, at which point I would argue that it is beyond our "responsibility" and even capability -- there is nothing we can do about that, more than doing our part on server-side which is letting unused tokens at least die and also store the token encrypted in the database.

For now I am gonna not agree with the statement that regenerating a token is better for security than the reasons I have listed. Again I am not an expert but this does not seem to solve or improve anything. Feel free to tell me im wrong :-)

Kindly, sphrak

belugame commented 5 years ago

@johnraz The question for me now is when you think about re-using the login logic... why do we even bother :) the client already has a login logic... and he will get a new token.. so basically he is simply login in again. It is no longer a refresh

johnraz commented 5 years ago

@belugame : You don't want to keep the user/pass in your storage - the all purpose of the token is that it should be shortlived in order to be stored somewhere. So this would mean asking the user his username/pass everytime you refresh, which is impractical and that's why we decided to have auto-refresh IIRC.

@sphrak: I'll try to explain clearly my point of view because those topics are always a bit shady 🤓 I think it takes a lot of focus to be a security expert and I don't pretend to be one either, so take what I say with a bit of salt.

Firstly, it should not be a problem extending the lifetime of a token as long as the communication channel is secured over https. So for this to be a problem you first have to defeat https -- in which case it will be game over anyway and it wont matter if you regenerate the token.

There are ways a token could be stored or distributed that don't involve TLS where you don't want a token to be long lived. Eg: you send an email with a link containing a token to trigger a login in a no-password flow. Here you would refresh the token as soon as it is first used, on the client side.

Second, generating a new token, saving it and deleting the old one is a more costly operation than just simply updating the datetime of the already available token object upon authentication, at any bigger scale at least.

I definitely agree here.

Thirdly, an auth token is something I consider mostly the same as a password and normally its not enforced on a user to do a password regeneration on set interval. My point being if extending the token lifetime is a security concern, then so must also the "never expiring" password be. At least in my mind.

See my answer to @belugame above and my answer to your first argument. I think this idea of a token being equal to a user password is a miss-conception, token being shortlived by nature allows a more flexible handling. They can be stored and invalidated more easily than user passwords which makes them more flexible. And we don't do that with user password for the exact same reason you state: you wouldn't enforce a user to reset his password every day but you would enforce him to keep it safe at all cost.

If we'd opt for a solution where we instead exchange a token for a new token it feels like we have recreated JWT authentication but with extra steps, clunkier and less scalable.

JWT is a different beast and I think it indeed solves the scalability problem at the cost of complexity. I don't agree our solution would add extra steps (what steps would that be?), would be clunkier (why?) but it would definitely be less scalable and that's ok because the purpose of knox is to have something simple and reliable.

I think its far more likely that a token is leaked by poor "token management" on the client side, at which point I would argue that it is beyond our "responsibility" and even capability -- there is nothing we can do about that, more than doing our part on server-side which is letting unused tokens at least die and also store the token encrypted in the database.

I again agree here, we can't solve how the client side handles the security but we can do our best to enforce good practice.

I think the difference between "extending" and "refreshing" is quite thin...

I think it all boils down to this question: "Do we favor performances or good security practices?" I would answer that someone looking for a simple solution like knox for token authentication would probably care more about security than he would about performances. If he cares about performance he could still go with JWT and we could make that clear in the doc.

This also bring another point I was thinking about regarding performances, we should brainstorm a bit around how to use caching to make things more efficient 😄

I also understand you put a lot of efforts in the current implementation and that changing ways now would mean ditching a lot of what you did but I think it's a learning process and nothing of all that would be lost in the end.

Let's keep sharing and find a solution that suits the project well 🎉

sphrak commented 5 years ago

continuing the follow up discussion in #168

You raise some valid points regarding deprecation warning and security backports. I would still argue that it's the responsibility of the user to not upgrade to a major version.

It is indeed the users resposibility to eventually decide on if he or she should upgrade. But my point was not that. Unless we do security backports (and decide on EOL) on major versions its not up to the user, its up to the good will of the developers choosing to allow them to do that. Which wont be the case if we break default behaivor.

I would also argue that you are not making your library's user a favor by influencing them in using bad security practice and the change discussed in #156 is all about that, not opening the door to bad practice.

One of the reason I like Django so much is that it pushes good practices on the user, by being opinionated, in a good way.

I agree here but they still manage to keep breakage at the minimum while still enforcing good practices.

Being opinionated is not a bad thing as long as its clear what is "good". Which is why this is a problem because its not immediatly clear which is side is the 'good practice' one.

To decide its 'good' merely due to it being technically more secure does not make it good practice.

Let me illustrate what I mean with 'technically more secure', I open a https connection, and inside that I open another https connection -- by that logic this is now good practice, because it does indeed make it more secure, but is it solving any problems?

Please show me a usecase where extending the lifetime of a token is a 'bad practice' (over TLS) or does 'opening the door to bad practice'.

The only usecase yet provided is the email one, where TLS mostly is not involved. I dont see why its a valid argument to adopt functionality for cases that involve insecure communication. I think changes should be based on what most users will benefit from.

I think it all boils down to this question: "Do we favor performances or good security practices?"

You make it sound as if we cannot have a balance between good performance and good security practices.

johnraz commented 5 years ago

Hey :-)

About EOL, backports etc: This is a community driven library and a relatively small one.

AFAIK we never had to issue a security fix, so until the situation is "real" I don't believe we should consider this. Nevertheless, I will answer what I think should be a reasonable reaction from the community in such a case:

I strongly believe we won't get any better than that with a free and without sponsor project like this one.

About whether or not the refresh makes sense: I agree that the email case is communicating through an insecure channel and makes it a specific one so here is another use case.

Let's say you have a routine running on a server that talks to your API. At some point, you will need someone to put the token in the config or in a secret vault to initiate the process. This means a human being has access to the token. This person could (voluntarily or not) leak the token. Also, this means that the token needs to be stored somewhere in a secret vault.

With a long-lived token or even worse a never expiring token that extends itself, if the one person who got access to the token goes rogue and wants to hurt you or if your secret vault gets compromised the token will be usable and valid

With a refresh procedure and a short-lived token, as soon as the token expires, the initial "visible" token will be invalidated and the security breach ends there. You can basically leave it up to machines to deal with the tokens and nobody will ever get access to it unless they get access to the machine and scan the memory but at that stage, the problem lies somewhere else.

I think it all boils down to this question: "Do we favor performances or good security practices?"

You make it sound as if we cannot have a balance between good performance and good security practices.

I was only saying that in regard to what you said about performance as it's the only valid point I see against refreshing the token. So definitely yes, I think we can achieve a balance between performance and security by actually refreshing the token. I actually addressed that above when I talked about caching.

So that's it for the technical bikeshedding.

Potential solutions and options:

1) We consider that extending a token is secure enough and acceptable and we keep it there. 2) We consider that extending is not secure enough and not acceptable and we replace it with a refresh procedure. 3) We consider that both usages solve different use cases and that it's up to the user of the library to decide which one to use and we extract this in a setting.

My opinion on those: 1) I would personally use this only on not critical workloads were leaking data would be not damageable to the business. This would, in turn, make this library only usable in such use cases. 2) This defeats 1. regarding security. It can have a significant impact on performance and we need to be careful about that. It can potentially cause trouble for people having to upgrade (but implementing a refresh procedure client side should not be that of a big deal) 3) This is probably the best of both world but we need to make sure that the code complexity and maintainability is not impacted too much.

About my general feeling I'm a bit puzzled as to why you seem so much against this... Maybe you have another use case I don't see (a lot of different client libraries to update? an upgrade path to the refresh policy that would hurt a lot of your customers?).

Either way, I feel that the discussion is getting tenser and I don't feel good about it. So I would like to emphasize that I don't pretend to know better, I'm just trying to explain why I see this the way I see it. I think I do have good arguments but it doesn't make your opinion less valuable than mine in any way.

Let's try to keep it fun and dandy :bowtie:

sphrak commented 5 years ago

@johnraz you are right, it does feel a bit tense and that is not fun. We are sort of drifting away from the actual objective here. Props to you for being the bigger person and bringing it up, thank you for that :smiley_cat:

.. and I apologize if I in anyway offended you or made you feel uneasey about this. That was not my intension at all, and again my apologies.

I'm a bit puzzled as to why you seem so much against this...

I will stress this again -- my main and absolute main issue with this proposal is the performance hit vs the security "gained" given the current usecases given does not justify the implementation to me in its current proposal (ie being the default). I dont have any special interest in "opposing" this change other than the aforementioned reason.

However, given those 3 options -- I agree option 3 is probably the best one, but also the hardest. If I can elaborate a bit on what I have in mind for that its this:

In other words I'd say we drop #158 but we keep the auto_refresh functionality as it currently stands. Then we let the actual new "refresh endpoint" have the token exchange functionality and the two functions are mutually exclusive so that only one or none can be enabled at any given time.

edit: just to clarify, have the refresh endpoint have two modes -- once that just extends and one that returns a new token?

Would that be a good compromise?

johnraz commented 5 years ago

@sphrak thanks for the openness and understanding ☺️

The solution I have in mind if we want to keep auto expiry extension:

We clearly dissociate the 2 concepts in the documentation:

We also make it clear in the doc what's the drawback of each:

We rename AUTO_REFRESH to AUTO_EXTEND to avoid confusion. We could deprecate AUTO_REFRESH instead of dropping it now if needed but I think just renaming a setting is simple enough to not go through the all deprecation flow.

The AUTO_EXTEND feature doesn't change and stay exactly the same as today.

The refresh endpoint should only allow doing a "refresh", not an "extend". So calling it would return a new token and its expiry, a bit of the same way the login endpoint behaves. I would refrain from mixing extend and refresh on this endpoint as it will make the code complicated and the concept hard to explain.

We could also have another endpoint to fetch information about the token, namely its expiry. Something like: /token/details/ That way with AUTO_EXTEND enabled, this endpoint would return an extended expiry for the token. and act as the "extend" endpoint.

Another option would be to ditch AUTO_EXTEND and replace it with a dedicated /token/extend/ endpoint but I don't think this is something you'd like @sphrak ?

sphrak commented 5 years ago

@johnraz I have been swamped with other work lately and will be for a couple more months. All your points are valid and I agree with you. I am unsure about ditching AUTO_EXTEND just as you suspected, but I guess its okay since we in that case would just move it to a dedicated endpoint which is of greater value than having a misleading API.

If you dont have anything more in particular to add design-wise I'd say work could be started on this. But again, I will not be able to author it currently, but I could squeeze in a few reviews here and there if needed or just someone to rubberduck against :smile_cat: