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

feat: add symmetric key monitoring #324

Merged
merged 7 commits into from
Apr 12, 2023
Merged
Show file tree
Hide file tree
Changes from 2 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
13 changes: 13 additions & 0 deletions CHANGELOG.rst
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,19 @@ Change Log
Unreleased
----------

[8.6.0] - 2023-04-11
--------------------

Added
~~~~~

* Added ``jwt_auth_decode_symmetric_token``, ``jwt_auth_asymmetric_verified``, ``jwt_auth_symmetric_verified``, and ``jwt_auth_verification_failed`` custom attributes to aid in deprecation and removal of symmetric keys.

Changed
~~~~~~~

* Changed ``jwt_auth_verify_keys_count`` custom attribute to aid in key rotations, to instead be ``jwt_auth_verify_asymmetric_keys_count`` and ``jwt_auth_verify_all_keys_count``. The latter count is only used in the case that the token can't be verified with the asymmetric keys alone.

[8.5.3] - 2023-04-11
--------------------

Expand Down
2 changes: 1 addition & 1 deletion edx_rest_framework_extensions/__init__.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
""" edx Django REST Framework extensions. """

__version__ = '8.5.3' # pragma: no cover
__version__ = '8.6.0' # pragma: no cover
59 changes: 57 additions & 2 deletions edx_rest_framework_extensions/auth/jwt/decoder.py
Original file line number Diff line number Diff line change
Expand Up @@ -173,18 +173,73 @@ def _set_filters(token):


def _verify_jwt_signature(token, jwt_issuer, decode_symmetric_token):
"""
Verifies the JWT signature. Raises InvalidTokenError in the event of an error.

Arguments:
token (str): JWT to be decoded.
jwt_issuer (dict): A dict of JWT issuer related settings, containing the symmetric key.
decode_symmetric_token (bool): Whether to decode symmetric tokens or not. Pass False for asymmetric tokens only
"""
# .. custom_attribute_name: jwt_auth_decode_symmetric_token
# .. custom_attribute_description: True if symmetric keys will also be used for checking
# the JWT signature, and False if only asymmetric keys will be used.
set_custom_attribute('jwt_auth_decode_symmetric_token', decode_symmetric_token)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: This reads a little misleadingly to me; I would expect it to mean that the token was decoded using a symmetric key, not that the symmetric key is allowed to be used. (Non-blocking.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to jwt_auth_check_symmetric_key.


# For observability purposes, we will first try asymmetric keys only to verify
# that we no longer need the symmetric key. However, if this fails, we will
# continue on to the original code path and try all keys (including symmetric)
# and add monitoring to let us know. This is meant to be temporary, until we
# can fully retire code paths for symmetric keys, as part of
# DEPR: Symmetric JWTs: https://github.com/openedx/public-engineering/issues/83

# Use add_symmetric_keys=False to only include asymmetric keys at first
key_set = _get_signing_jwk_key_set(jwt_issuer, add_symmetric_keys=False)
# .. custom_attribute_name: jwt_auth_verify_asymmetric_keys_count
# .. custom_attribute_description: Number of JWT verification keys in use for this
# verification. Should be same as number of asymmetric public keys. This is
# intended to aid in key rotations; once the average count stabilizes at a
# higher number after adding a public key, it should be safe to change the secret key.
set_custom_attribute('jwt_auth_verify_asymmetric_keys_count', len(key_set))

try:
_ = JWS().verify_compact(token, key_set)
# .. custom_attribute_name: jwt_auth_asymmetric_verified
# .. custom_attribute_description: The count of JWTs successfully verified
# using an asymmetric key.
robrap marked this conversation as resolved.
Show resolved Hide resolved
set_custom_attribute('jwt_auth_asymmetric_verified', True)
return
except Exception: # pylint: disable=broad-except
# Continue to the old code path of trying all keys
pass

# The following is the original code that includes both the symmetric and asymmetric keys
# as requested with the decode_symmetric_token argument. Note that the check against
# the asymmetric keys here is redundant and unnecessary, but this code is temporary and
# will be simplified once symmetric keys have been fully retired.

key_set = _get_signing_jwk_key_set(jwt_issuer, add_symmetric_keys=decode_symmetric_token)
# .. custom_attribute_name: jwt_auth_verify_keys_count
# .. custom_attribute_name: jwt_auth_verify_all_keys_count
# .. custom_attribute_description: Number of JWT verification keys in use for this
# verification. Should be same as number of asymmetric public keys, plus one if
# a symmetric key secret is set. This is intended to aid in key rotations; once
# the average count stabilizes at a higher number after adding a public key, it
# should be safe to change the secret key.
set_custom_attribute('jwt_auth_verify_keys_count', len(key_set))
set_custom_attribute('jwt_auth_verify_all_keys_count', len(key_set))

try:
_ = JWS().verify_compact(token, key_set)
# .. custom_attribute_name: jwt_auth_symmetric_verified
# .. custom_attribute_description: The count of JWTs successfully verified
# using a symmetric key.
robrap marked this conversation as resolved.
Show resolved Hide resolved
# Note: A separate custom attribute is used in case there are different JWTs decoded
# in the same request.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm having trouble understanding this comment. What other attribute is this being contrasted to?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried a longer version. You can let me know if it is more clear.

set_custom_attribute('jwt_auth_symmetric_verified', True)
return
except Exception as token_error:
# .. custom_attribute_name: jwt_auth_verification_failed
# .. custom_attribute_description: True if the JWT token verification failed.
set_custom_attribute('jwt_auth_verification_failed', True)
logger.exception('Token verification failed.')
exc_info = sys.exc_info()
raise jwt.InvalidTokenError(exc_info[2]) from token_error
Expand Down
26 changes: 19 additions & 7 deletions edx_rest_framework_extensions/auth/jwt/tests/test_decoder.py
Original file line number Diff line number Diff line change
Expand Up @@ -200,20 +200,32 @@ def test_success_asymmetric_jwt_decode(self):
self.assertEqual(get_asymmetric_only_jwt_decode_handler(token), self.payload)

@mock.patch('edx_rest_framework_extensions.auth.jwt.decoder.set_custom_attribute')
def test_keyset_size_monitoring(self, mock_set_custom_attribute):
def test_keyset_size_and_other_monitoring(self, mock_set_custom_attribute):
"""
Validates that a custom attribute is recorded for the keyset size.
Validates a variety of custom attributes are recorded, including the keyset size.
"""
token = generate_asymmetric_jwt_token(self.payload)
asymmetric_token = generate_asymmetric_jwt_token(self.payload)
symmetric_token = generate_jwt_token(self.payload)

# The secret key is included by default making a list of length 2, but for
# asymmetric-only there is only 1 key in the keyset.
self.assertEqual(jwt_decode_handler(token), self.payload)
self.assertEqual(get_asymmetric_only_jwt_decode_handler(token), self.payload)
self.assertEqual(jwt_decode_handler(asymmetric_token), self.payload)
self.assertEqual(get_asymmetric_only_jwt_decode_handler(asymmetric_token), self.payload)
self.assertEqual(jwt_decode_handler(symmetric_token), self.payload)

assert mock_set_custom_attribute.call_args_list == [
mock.call('jwt_auth_verify_keys_count', 2),
mock.call('jwt_auth_verify_keys_count', 1),
mock.call('jwt_auth_decode_symmetric_token', True),
mock.call('jwt_auth_verify_asymmetric_keys_count', 1),
mock.call('jwt_auth_asymmetric_verified', True),

mock.call('jwt_auth_decode_symmetric_token', False),
mock.call('jwt_auth_verify_asymmetric_keys_count', 1),
mock.call('jwt_auth_asymmetric_verified', True),

mock.call('jwt_auth_decode_symmetric_token', True),
mock.call('jwt_auth_verify_asymmetric_keys_count', 1),
mock.call('jwt_auth_verify_all_keys_count', 2),
mock.call('jwt_auth_symmetric_verified', True),
]


Expand Down