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

20201118 token expiration #478

Merged
merged 5 commits into from
Dec 7, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion api/api/settings.py
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@

# CORS
# No need to add Authorization to CORS_ALLOW_HEADERS (included by default)
CORS_ORIGIN_ALLOW_ALL = True
CORS_ALLOW_ALL_ORIGINS = True

TEMPLATES = [
{
Expand Down
17 changes: 11 additions & 6 deletions api/desecapi/authentication.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,20 @@
class TokenAuthentication(RestFrameworkTokenAuthentication):
model = Token

# Note: This method's runtime depends on in what way a credential is invalid (expired, wrong client IP).
# It thus exposes the failure reason when under timing attack.
def authenticate(self, request):
try:
user, token = super().authenticate(request)
except TypeError: # TypeError: cannot unpack non-iterable NoneType object
user, token = super().authenticate(request) # may raise exceptions.AuthenticationFailed if token is invalid
except TypeError: # if no token was given
return None # unauthenticated

if not token.is_valid:
raise exceptions.AuthenticationFailed('Invalid token.')

token.last_used = timezone.now()
token.save()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this result in concurrency exceptions for parallel requests using the same token?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not behavior introduced by this PR. Still, question is still worth thinking about it

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so. First, I believe that this does not happen inside a transaction; second, even if it were, one requests would simply delay the other, but not raise an exception (if I got it right).


# REMOTE_ADDR is populated by the environment of the wsgi-request [1], which in turn is set up by nginx as per
# uwsgi_params [2]. The value of $remote_addr finally is given by the network connection [3].
# [1]: https://github.com/django/django/blob/stable/3.1.x/django/core/handlers/wsgi.py#L77
Expand All @@ -44,10 +52,7 @@ def authenticate(self, request):

def authenticate_credentials(self, key):
key = Token.make_hash(key)
user, token = super().authenticate_credentials(key)
token.last_used = timezone.now()
token.save()
return user, token
return super().authenticate_credentials(key)


class BasicTokenAuthentication(BaseAuthentication):
Expand Down
25 changes: 25 additions & 0 deletions api/desecapi/migrations/0010_token_expiration.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
# Generated by Django 3.1.3 on 2020-11-19 09:55

import datetime
import django.core.validators
from django.db import migrations, models


class Migration(migrations.Migration):

dependencies = [
('desecapi', '0009_token_allowed_subnets'),
]

operations = [
migrations.AddField(
model_name='token',
name='max_age',
field=models.DurationField(default=None, null=True, validators=[django.core.validators.MinValueValidator(datetime.timedelta(0))]),
),
migrations.AddField(
model_name='token',
name='max_unused_period',
field=models.DurationField(default=None, null=True, validators=[django.core.validators.MinValueValidator(datetime.timedelta(0))]),
),
]
24 changes: 23 additions & 1 deletion api/desecapi/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
from django.contrib.postgres.fields import ArrayField, CIEmailField, RangeOperators
from django.core.exceptions import ValidationError
from django.core.mail import EmailMessage, get_connection
from django.core.validators import RegexValidator
from django.core.validators import MinValueValidator, RegexValidator
from django.db import models
from django.db.models import Manager, Q
from django.db.models.expressions import RawSQL
Expand Down Expand Up @@ -397,10 +397,32 @@ def _allowed_subnets_default():
last_used = models.DateTimeField(null=True, blank=True)
perm_manage_tokens = models.BooleanField(default=False)
allowed_subnets = ArrayField(CidrAddressField(), default=_allowed_subnets_default.__func__)
max_age = models.DurationField(null=True, default=None, validators=[MinValueValidator(timedelta(0))])
max_unused_period = models.DurationField(null=True, default=None, validators=[MinValueValidator(timedelta(0))])

plain = None
objects = NetManager()

@property
def is_valid(self):
now = timezone.now()

# Check max age
try:
if self.created + self.max_age < now:
return False
except TypeError:
pass

# Check regular usage requirement
try:
if (self.last_used or self.created) + self.max_unused_period < now:
return False
except TypeError:
pass

return True

def generate_key(self):
self.plain = secrets.token_urlsafe(21)
self.key = Token.make_hash(self.plain)
Expand Down
4 changes: 3 additions & 1 deletion api/desecapi/serializers.py
Original file line number Diff line number Diff line change
Expand Up @@ -51,10 +51,12 @@ def validate(self, attrs):
class TokenSerializer(serializers.ModelSerializer):
allowed_subnets = serializers.ListField(child=netfields_rf.CidrAddressField(), required=False)
token = serializers.ReadOnlyField(source='plain')
is_valid = serializers.ReadOnlyField()

class Meta:
model = models.Token
fields = ('id', 'created', 'last_used', 'name', 'perm_manage_tokens', 'allowed_subnets', 'token',)
fields = ('id', 'created', 'last_used', 'max_age', 'max_unused_period', 'name', 'perm_manage_tokens',
'allowed_subnets', 'is_valid', 'token',)
read_only_fields = ('id', 'created', 'last_used', 'token')

def __init__(self, *args, include_plain=False, **kwargs):
Expand Down
11 changes: 5 additions & 6 deletions api/desecapi/tests/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,6 @@ def __init__(self, test_case, expected_requests, single_expectation_single_reque

def __enter__(self):
hr_core.POTENTIAL_HTTP_PORTS.add(8081) # FIXME should depend on self.expected_requests
self.expected_requests = self.expected_requests
# noinspection PyProtectedMember
self.old_httpretty_entries = httpretty._entries.copy() # FIXME accessing private properties of httpretty
for request in self.expected_requests:
Expand Down Expand Up @@ -297,7 +296,7 @@ def request_pdns_zone_create(cls, ns, **kwargs):
'method': 'POST',
'uri': cls.get_full_pdns_url(cls.PDNS_ZONES, ns=ns),
'status': 201,
'body': None,
'body': '',
'match_querystring': True,
**kwargs
}
Expand Down Expand Up @@ -332,7 +331,7 @@ def request_pdns_zone_delete(cls, name=None, ns='LORD'):
'method': 'DELETE',
'uri': cls.get_full_pdns_url(cls.PDNS_ZONE, ns=ns, id=cls._pdns_zone_id_heuristic(name)),
'status': 200,
'body': None,
'body': '',
}

@classmethod
Expand All @@ -341,7 +340,7 @@ def request_pdns_zone_update(cls, name=None):
'method': 'PATCH',
'uri': cls.get_full_pdns_url(cls.PDNS_ZONE, id=cls._pdns_zone_id_heuristic(name)),
'status': 200,
'body': None,
'body': '',
}

def request_pdns_zone_update_assert_body(self, name: str = None, updated_rr_sets: Union[List[RRset], Dict] = None):
Expand Down Expand Up @@ -455,7 +454,7 @@ def request_pdns_zone_axfr(cls, name=None):
'method': 'PUT',
'uri': cls.get_full_pdns_url(cls.PDNS_ZONE_AXFR, ns='MASTER', id=cls._pdns_zone_id_heuristic(name)),
'status': 200,
'body': None,
'body': '',
}

@classmethod
Expand All @@ -464,7 +463,7 @@ def request_pdns_update_catalog(cls):
'method': 'PATCH',
'uri': cls.get_full_pdns_url(cls.PDNS_ZONE, ns='MASTER', id=cls._pdns_zone_id_heuristic('catalog.internal')),
'status': 204,
'body': None,
'body': '',
'priority': 1, # avoid collision with DELETE zones/(?P<id>[^/]+)$ (httpretty does not match the method)
}

Expand Down
120 changes: 118 additions & 2 deletions api/desecapi/tests/test_authentication.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,11 @@
from datetime import timedelta
import json
from unittest import mock

from django.utils import timezone
from rest_framework.status import HTTP_200_OK, HTTP_401_UNAUTHORIZED

from desecapi.models import Token
from desecapi.tests.base import DynDomainOwnerTestCase


Expand Down Expand Up @@ -45,8 +49,14 @@ def test_malformed_basic_auth(self):

class TokenAuthenticationTestCase(DynDomainOwnerTestCase):

def assertAuthenticationStatus(self, code, token=None, **kwargs):
self.client.set_credentials_token_auth(token or self.token.plain)
def setUp(self):
super().setUp()
# Refresh token from database, but keep plain value
self.token, self.token.plain = Token.objects.get(pk=self.token.pk), self.token.plain

def assertAuthenticationStatus(self, code, plain=None, expired=False ,**kwargs):
plain = plain or self.token.plain
self.client.set_credentials_token_auth(plain)

# only forward REMOTE_ADDR if not None
if kwargs.get('REMOTE_ADDR') is None:
Expand All @@ -56,6 +66,10 @@ def assertAuthenticationStatus(self, code, token=None, **kwargs):
body = json.dumps({'detail': 'Invalid token.'}) if code == HTTP_401_UNAUTHORIZED else None
self.assertResponse(response, code, body)

if expired:
key = Token.make_hash(plain)
self.assertFalse(Token.objects.get(key=key).is_valid)

def test_token_case_sensitive(self):
self.assertAuthenticationStatus(HTTP_200_OK)
self.assertAuthenticationStatus(HTTP_401_UNAUTHORIZED, self.token.plain.upper())
Expand All @@ -78,3 +92,105 @@ def test_token_subnets(self):
self.token.save()
for client_ip in client_ips:
self.assertAuthenticationStatus(status, REMOTE_ADDR=client_ip)

def test_token_max_age(self):
# No maximum age: can use now and in ten years
self.token.max_age = None
self.token.save()

self.assertAuthenticationStatus(HTTP_200_OK)
with mock.patch('desecapi.models.timezone.now', return_value=timezone.now() + timedelta(days=3650)):
self.assertAuthenticationStatus(HTTP_200_OK)

# Maximum age zero: token cannot be used
self.token.max_age = timedelta(0)
self.token.save()
self.assertAuthenticationStatus(HTTP_401_UNAUTHORIZED, expired=True)

# Maximum age 10 10:10:10: can use one second before, but not once second after
period = timedelta(days=10, hours=10, minutes=10, seconds=10)
self.token.max_age = period
self.token.save()

second = timedelta(seconds=1)
with mock.patch('desecapi.models.timezone.now', return_value=self.token.created + period - second):
self.assertAuthenticationStatus(HTTP_200_OK)
with mock.patch('desecapi.models.timezone.now', return_value=self.token.created + period + second):
self.assertAuthenticationStatus(HTTP_401_UNAUTHORIZED, expired=True)

def test_token_max_unused_period(self):
plain = self.token.plain
second = timedelta(seconds=1)

# Maximum unused period zero: token cannot be used
self.token.max_unused_period = timedelta(0)
self.token.save()
self.assertAuthenticationStatus(HTTP_401_UNAUTHORIZED, expired=True)

# Maximum unused period
period = timedelta(days=10, hours=10, minutes=10, seconds=10)
self.token.max_unused_period = period
self.token.save()

# Can't use after period if token was never used (last_used is None)
self.assertIsNone(self.token.last_used)
with mock.patch('desecapi.models.timezone.now', return_value=self.token.created + period + second):
self.assertAuthenticationStatus(HTTP_401_UNAUTHORIZED, plain=plain, expired=True)
self.assertIsNone(Token.objects.get(pk=self.token.pk).last_used) # unchanged

# Can use after half the period
with mock.patch('desecapi.models.timezone.now', return_value=self.token.created + period/2):
self.assertAuthenticationStatus(HTTP_200_OK, plain=plain)
self.token = Token.objects.get(pk=self.token.pk) # update last_used field

# Can't use once another period is over
with mock.patch('desecapi.models.timezone.now', return_value=self.token.last_used + period + second):
self.assertAuthenticationStatus(HTTP_401_UNAUTHORIZED, plain=plain, expired=True)
self.assertEqual(self.token.last_used, Token.objects.get(pk=self.token.pk).last_used) # unchanged

# ... but one second before, and also for one more period
with mock.patch('desecapi.models.timezone.now', return_value=self.token.last_used + period - second):
self.assertAuthenticationStatus(HTTP_200_OK, plain=plain)
with mock.patch('desecapi.models.timezone.now', return_value=self.token.last_used + 2*period - 2*second):
self.assertAuthenticationStatus(HTTP_200_OK, plain=plain)

# No maximum age: can use now and in ten years
self.token.max_unused_period = None
self.token.save()

self.assertAuthenticationStatus(HTTP_200_OK, plain=plain)
with mock.patch('desecapi.models.timezone.now', return_value=timezone.now() + timedelta(days=3650)):
self.assertAuthenticationStatus(HTTP_200_OK, plain=plain)

def test_token_max_age_max_unused_period(self):
hour = timedelta(hours=1)
self.token.max_age = 3 * hour
self.token.max_unused_period = hour
self.token.save()

# max_unused_period wins if tighter than max_age
with mock.patch('desecapi.models.timezone.now', return_value=self.token.created + 1.25*hour):
self.assertAuthenticationStatus(HTTP_401_UNAUTHORIZED, expired=True)

# Can use immediately
self.assertAuthenticationStatus(HTTP_200_OK)

# Can use continuously within max_unused_period
with mock.patch('desecapi.models.timezone.now', return_value=self.token.created + 0.75*hour):
self.assertAuthenticationStatus(HTTP_200_OK)
with mock.patch('desecapi.models.timezone.now', return_value=self.token.created + 1.5*hour):
self.assertAuthenticationStatus(HTTP_200_OK)

# max_unused_period wins again if tighter than max_age
with mock.patch('desecapi.models.timezone.now', return_value=self.token.created + 2.75*hour):
self.assertAuthenticationStatus(HTTP_401_UNAUTHORIZED, expired=True)

# Can use continuously within max_unused_period
with mock.patch('desecapi.models.timezone.now', return_value=self.token.created + 2.25*hour):
self.assertAuthenticationStatus(HTTP_200_OK)
with mock.patch('desecapi.models.timezone.now', return_value=self.token.created + 2.75*hour):
self.assertAuthenticationStatus(HTTP_200_OK)

# max_age wins again if tighter than max_unused_period
with mock.patch('desecapi.models.timezone.now', return_value=self.token.created + 3.25*hour):
self.assertAuthenticationStatus(HTTP_401_UNAUTHORIZED, expired=True)
28 changes: 20 additions & 8 deletions api/desecapi/tests/test_tokens.py
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ def test_list_tokens(self):
self.assertEqual(len(response.data), 2)
self.assertIn('id', response.data[0])
self.assertFalse(any(field in response.data[0] for field in ['token', 'key', 'value']))
self.assertFalse(any(token.encode() in response.content for token in [self.token.plain, self.token2.plain,]))
self.assertFalse(any(token.encode() in response.content for token in [self.token.plain, self.token2.plain]))
self.assertNotContains(response, self.token.plain)

def test_delete_my_token(self):
Expand All @@ -48,8 +48,10 @@ def test_retrieve_my_token(self):
self.assertStatus(response, status.HTTP_200_OK)
self.assertEqual(
set(response.data.keys()),
{'id', 'created', 'last_used', 'name', 'perm_manage_tokens', 'allowed_subnets'}
{'id', 'created', 'last_used', 'max_age', 'max_unused_period', 'name', 'perm_manage_tokens',
'allowed_subnets', 'is_valid'}
)
self.assertFalse(any(token.encode() in response.content for token in [self.token.plain, self.token2.plain]))

def test_retrieve_other_token(self):
token_id = Token.objects.get(user=self.user).id
Expand All @@ -62,11 +64,20 @@ def test_update_my_token(self):
url = self.reverse('v1:token-detail', pk=self.token.id)

for method in [self.client.patch, self.client.put]:
data = {'name': method.__name__, 'allowed_subnets': ['127.0.0.0/8']}
response = method(url, data=data)
self.assertStatus(response, status.HTTP_200_OK)
self.assertEqual(Token.objects.get(pk=self.token.id).name, method.__name__)
self.assertEqual(Token.objects.get(pk=self.token.id).allowed_subnets, [IPv4Network('127.0.0.0/8')])
datas = [
{'name': method.__name__},
{'allowed_subnets': ['127.0.0.0/8']},
{'allowed_subnets': ['127.0.0.0/8', '::/0']},
{'max_age': '365 00:10:33.123456'},
{'max_age': None},
{'max_unused_period': '365 00:10:33.123456'},
{'max_unused_period': None},
]
for data in datas:
response = method(url, data=data)
self.assertStatus(response, status.HTTP_200_OK)
for k, v in data.items():
self.assertEqual(response.data[k], v)

# Revoke token management permission
response = self.client.patch(url, data={'perm_manage_tokens': False})
Expand All @@ -90,7 +101,8 @@ def test_create_token(self):
self.assertStatus(response, status.HTTP_201_CREATED)
self.assertEqual(
set(response.data.keys()),
{'id', 'created', 'last_used', 'name', 'perm_manage_tokens', 'allowed_subnets', 'token'}
{'id', 'created', 'last_used', 'max_age', 'max_unused_period', 'name', 'perm_manage_tokens',
'allowed_subnets', 'is_valid', 'token'}
)
self.assertEqual(response.data['name'], data.get('name', ''))
self.assertEqual(response.data['allowed_subnets'], data.get('allowed_subnets', ['0.0.0.0/0', '::/0']))
Expand Down
Loading