Skip to content

Commit

Permalink
Configurable Nonce/Expiry -- PR Review Suggestions
Browse files Browse the repository at this point in the history 
* Client now throws an exception if an out of range expiry
  was given on initialization; the value is furthermore
  clamped to between 0 seconds and 5 minutes where it's used.
* Doc strings have been updated to reflect what the underlying
  OIDC API specifies around the state and nonce.
* Tests have been added to verify that the expiry is checked
  correctly.
  • Loading branch information
@rpcope1
rpcope1 committed Feb 1, 2024
1 parent ee00240 commit 865978f
Show file tree
Hide file tree
Showing 3 changed files with 51 additions and 18 deletions.
29 changes: 20 additions & 9 deletions duo_universal/client.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
CLIENT_ID_LENGTH = 20
CLIENT_SECRET_LENGTH = 40
JTI_LENGTH = 36
MINIMUM_STATE_LENGTH = 22
MINIMUM_STATE_LENGTH = 16
MAXIMUM_STATE_LENGTH = 1024
STATE_LENGTH = 36
SUCCESS_STATUS_CODE = 200
Expand All @@ -37,6 +37,8 @@
MIN=MINIMUM_STATE_LENGTH,
MAX=MAXIMUM_STATE_LENGTH
)
ERR_EXP_SECONDS_TOO_LONG = 'Client may not be configured for a JWT expiry longer than five minutes.'
ERR_EXP_SECONDS_TOO_SHORT = 'Invalid JWT expiry duration.'

API_HOST_URI_FORMAT = "https://{}"
OAUTH_V1_HEALTH_CHECK_ENDPOINT = "https://{}/oauth/v1/health_check"
Expand All @@ -52,6 +54,9 @@ class DuoException(Exception):


class Client:
@property
def _clamped_expiry_duration(self):
return max(min(FIVE_MINUTES_IN_SECONDS, self._exp_seconds), 1)

def _generate_rand_alphanumeric(self, length):
"""
Expand All @@ -75,7 +80,7 @@ def _generate_rand_alphanumeric(self, length):
return ''.join(generator.choice(characters) for i in range(length))

def _validate_init_config(self, client_id, client_secret,
api_host, redirect_uri):
api_host, redirect_uri, exp_seconds):
"""
Verifies __init__ parameters
Expand All @@ -85,6 +90,7 @@ def _validate_init_config(self, client_id, client_secret,
client_secret -- Client secret for the application in Duo
host -- Duo api host
redirect_uri -- Uri to redirect to after a successful auth
exp_seconds -- The JWT expiry window
Raises:
Expand All @@ -98,6 +104,10 @@ def _validate_init_config(self, client_id, client_secret,
raise DuoException(ERR_API_HOST)
if not redirect_uri:
raise DuoException(ERR_REDIRECT_URI)
if exp_seconds > FIVE_MINUTES_IN_SECONDS:
raise DuoException(ERR_EXP_SECONDS_TOO_LONG)
elif exp_seconds < 0:
raise DuoException(ERR_EXP_SECONDS_TOO_SHORT)

def _validate_create_auth_url_inputs(self, username, state, nonce=None):
if nonce and (MINIMUM_STATE_LENGTH >= len(nonce) or len(nonce) >= MAXIMUM_STATE_LENGTH):
Expand All @@ -112,7 +122,7 @@ def _create_jwt_args(self, endpoint):
'iss': self._client_id,
'sub': self._client_id,
'aud': endpoint,
'exp': time.time() + self._exp_seconds,
'exp': time.time() + self._clamped_expiry_duration,
'jti': self._generate_rand_alphanumeric(JTI_LENGTH)
}

Expand All @@ -133,13 +143,14 @@ def __init__(self, client_id, client_secret, host,
duo_certs -- (Optional) Provide custom CA certs
use_duo_code_attribute -- (Optional: default true) Flag to use `duo_code` instead of `code` for returned authorization parameter
http_proxy -- (Optional) HTTP proxy to tunnel requests through
exp_seconds -- (Optional) The number of seconds used for JWT expiry
exp_seconds -- (Optional) The number of seconds used for JWT expiry. Must be be at most 5 minutes.
"""

self._validate_init_config(client_id,
client_secret,
host,
redirect_uri)
redirect_uri,
exp_seconds)

self._client_id = client_id
self._client_secret = client_secret
Expand Down Expand Up @@ -213,10 +224,10 @@ def create_auth_url(self, username, state, nonce=None):
Arguments:
username -- username trying to authenticate with Duo
state -- Randomly generated character string of at least 22
chars returned to the integration by Duo after 2FA
state -- Randomly generated character string of at least 16
and at most 1024 characters returned to the integration by Duo after 2FA
nonce -- Randomly generated character string of at least 16
characters used as the nonce for the underlying OIDC flow
and at most 1024 characters used as the nonce for the underlying OIDC flow
Returns:
Expand All @@ -233,7 +244,7 @@ def create_auth_url(self, username, state, nonce=None):
'client_id': self._client_id,
'iss': self._client_id,
'aud': API_HOST_URI_FORMAT.format(self._api_host),
'exp': time.time() + self._exp_seconds,
'exp': time.time() + self._clamped_expiry_duration,
'state': state,
'response_type': 'code',
'duo_uname': username,
Expand Down
38 changes: 29 additions & 9 deletions tests/test_setup_client.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
REDIRECT_URI = "https://www.example.com"
CA_CERT_NEW = "/path/to/cert/ca_cert_new.pem"
PROXY_HOST = "http://proxy.example.com:8001"
EXP_SECONDS = client.FIVE_MINUTES_IN_SECONDS
NONE = None


Expand All @@ -26,7 +27,7 @@ def test_short_client_id(self):
"""
with self.assertRaises(client.DuoException) as e:
self.client._validate_init_config(SHORT_CLIENT_ID, CLIENT_SECRET,
HOST, REDIRECT_URI)
HOST, REDIRECT_URI, EXP_SECONDS)
self.assertEqual(e, client.ERR_CLIENT_ID)

def test_long_client_id(self):
Expand All @@ -35,7 +36,7 @@ def test_long_client_id(self):
"""
with self.assertRaises(client.DuoException) as e:
self.client._validate_init_config(LONG_CLIENT_ID, CLIENT_SECRET,
HOST, REDIRECT_URI)
HOST, REDIRECT_URI, EXP_SECONDS)
self.assertEqual(e, client.ERR_CLIENT_ID)

def test_no_client_id(self):
Expand All @@ -44,7 +45,7 @@ def test_no_client_id(self):
"""
with self.assertRaises(client.DuoException) as e:
self.client._validate_init_config(NONE, CLIENT_SECRET,
HOST, REDIRECT_URI)
HOST, REDIRECT_URI, EXP_SECONDS)
self.assertEqual(e, client.ERR_CLIENT_ID)

def test_short_client_secret(self):
Expand All @@ -53,7 +54,7 @@ def test_short_client_secret(self):
"""
with self.assertRaises(client.DuoException) as e:
self.client._validate_init_config(CLIENT_ID, SHORT_CLIENT_SECRET,
HOST, REDIRECT_URI)
HOST, REDIRECT_URI, EXP_SECONDS)
self.assertEqual(e, client.ERR_CLIENT_SECRET)

def test_long_client_secret(self):
Expand All @@ -62,7 +63,7 @@ def test_long_client_secret(self):
"""
with self.assertRaises(client.DuoException) as e:
self.client._validate_init_config(CLIENT_ID, LONG_CLIENT_SECRET,
HOST, REDIRECT_URI)
HOST, REDIRECT_URI, EXP_SECONDS)
self.assertEqual(e, client.ERR_CLIENT_SECRET)

def test_no_client_secret(self):
Expand All @@ -71,7 +72,7 @@ def test_no_client_secret(self):
"""
with self.assertRaises(client.DuoException) as e:
self.client._validate_init_config(CLIENT_ID, NONE,
HOST, REDIRECT_URI)
HOST, REDIRECT_URI, EXP_SECONDS)
self.assertEqual(e, client.ERR_CLIENT_SECRET)

def test_no_host(self):
Expand All @@ -80,7 +81,7 @@ def test_no_host(self):
"""
with self.assertRaises(client.DuoException) as e:
self.client._validate_init_config(CLIENT_ID, CLIENT_SECRET,
NONE, REDIRECT_URI)
NONE, REDIRECT_URI, EXP_SECONDS)
self.assertEqual(e, client.ERR_API_HOST)

def test_no_redirect_uri(self):
Expand All @@ -89,15 +90,34 @@ def test_no_redirect_uri(self):
"""
with self.assertRaises(client.DuoException) as e:
self.client._validate_init_config(CLIENT_ID, CLIENT_SECRET,
HOST, NONE)
HOST, NONE, EXP_SECONDS)
self.assertEqual(e, client.ERR_REDIRECT_URI)

def test_exp_seconds_too_long(self):
"""
Test to validate that a user can't set an expiry longer than 5 minutes.
"""
with self.assertRaises(client.DuoException) as e:
self.client._validate_init_config(CLIENT_ID, CLIENT_SECRET, HOST, REDIRECT_URI, 2*EXP_SECONDS)
self.assertEqual(e, client.ERR_EXP_SECONDS_TOO_LONG)
# Even if the end user forcefully sets the expiry, ensure the clamped value is in spec.
self.client._exp_seconds = 2*EXP_SECONDS
self.assertEqual(self.client._clamped_expiry_duration, client.FIVE_MINUTES_IN_SECONDS)

def test_exp_seconds_too_short(self):
"""
Test to validate that a user can't set an expiry shorter than 0.
"""
with self.assertRaises(client.DuoException) as e:
self.client._validate_init_config(CLIENT_ID, CLIENT_SECRET, HOST, REDIRECT_URI, -1)
self.assertEqual(e, client.ERR_EXP_SECONDS_TOO_SHORT)

def test_successful(self):
"""
Test successful _validate_init_config
"""
self.client._validate_init_config(CLIENT_ID, CLIENT_SECRET,
HOST, REDIRECT_URI)
HOST, REDIRECT_URI, EXP_SECONDS)

def test_no_duo_cert(self):
self.assertEqual(self.client._duo_certs, client.DEFAULT_CA_CERT_PATH)
Expand Down
2 changes: 2 additions & 0 deletions tests/test_validate_create_auth_url_inputs.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ def test_short_nonce(self):
"""
with self.assertRaises(client.DuoException) as e:
self.client._validate_create_auth_url_inputs(USERNAME, STATE, nonce=SHORT_STATE)
self.assertEqual(e, client.ERR_NONCE_LEN)

def test_long_nonce(self):
"""
Expand All @@ -59,6 +60,7 @@ def test_long_nonce(self):
"""
with self.assertRaises(client.DuoException) as e:
self.client._validate_create_auth_url_inputs(USERNAME, STATE, nonce=LONG_LENGTH)
self.assertEqual(e, client.ERR_NONCE_LEN)

def test_no_username(self):
"""
Expand Down

0 comments on commit 865978f

Please sign in to comment.