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

API token rotation and granularity #434

Closed
mbakke opened this issue Apr 21, 2021 · 3 comments
Closed

API token rotation and granularity #434

mbakke opened this issue Apr 21, 2021 · 3 comments

Comments

@mbakke
Copy link
Contributor

mbakke commented Apr 21, 2021

Currently the Django API token never changes, but if its created time is too old, a full login is required.

There are concerns that if a third party can access a token (e.g. from a stolen backup), they can make authenticated requests impersonating the user as long as the user has recently logged in.

Currently the only way to rotate the API token is by explicitly calling the logout endpoint, e.g. by typing logout in mreg-cli. That will delete the token from the Django database and create a new one the next login.

(That has the unfortunate side effect that it will also delete the current user for non-LDAP setups, but that's another issue..!)

Here is a patch that generates a new token every login:

diff --git a/mreg/api/views.py b/mreg/api/views.py
--- a/mreg/api/views.py
+++ b/mreg/api/views.py
@@ -23,11 +23,13 @@ class ObtainExpiringAuthToken(ObtainAuthToken):
             else:
                 raise err
 
-        token, created = Token.objects.get_or_create(user=serializer.validated_data['user'])
-        if not created:
-            # update the created time of the token to keep it valid
-            token.created = timezone.now()
-            token.save()
+        user = serializer.validated_data['user']
+
+        # Invalidate any previous token.
+        Token.objects.filter(user=user).delete()
+
+        # Generate a new one.
+        token, created = Token.objects.get_or_create(user=user)
 
         return Response({'token': token.key})
 

However there is also a demand for longer-lived tokens, e.g. for scheduled jobs; and read-only tokens as in #342.

Perhaps it could be useful to introduce an external library for more granular token management? I.e. django-rest-knox, django-rest-durin, or "standardized" authentication frameworks such as Simple JWT.

The latter has the benefit of being widely supported in various programming ecosystems, but all comes at the expense of added complexity and relinquishing control of an important code path.

What do mreg stakeholders think? I believe the patch above will solve the immediate concerns, but I found it as a good excuse to consider the alternatives.

@oyvindhagberg
Copy link
Contributor

I don't like Simple JWT because it would require changes to every client that's using mreg today, to handle refresh/access tokens. Knox and Durin seem to use the Authentication header like DRF does, so that's good.

Knox has a general setting for how long the tokens should live, but I don't see a way we could use different values for different tokens. Durin seems to support this by using different types of "API Client", not sure if that fits our needs. Perhaps we should keep looking for a better framework.

Implementing your patch would also be fine by me, at least for now.

@ponas @paalbra @oyvindkolbu any thoughts?

@mbakke
Copy link
Contributor Author

mbakke commented Apr 23, 2021

To complement my crude patch above, I think it would be nicer to only rotate the token if it has expired, and perhaps also keep bumping the expiry time at usage or login to enable long-running jobs.

@mbakke
Copy link
Contributor Author

mbakke commented Jun 22, 2021

Fixed with #442.

@mbakke mbakke closed this as completed Jun 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants