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

Conversation

robrap
Copy link
Contributor

@robrap robrap commented Apr 11, 2023

Description:

Adds monitoring around JWT decoding and symmetric keys, to help with the eventual deprecation and removal of the symmetric keys.

Issue:

See DEPR: Symmetric JWTs:
openedx/public-engineering#83

Merge checklist:

  • All reviewers approved
  • CI build is green
  • Version bump if needed
  • Changelog record added
  • Documentation updated (not only docstrings)
  • Commits are squashed

Post merge:

  • Create a tag
  • Check new version is pushed to PyPi after tag-triggered build is
    finished.
  • Delete working branch (if not needed anymore)

Adds monitoring around JWT decoding and symmetric keys, to
help with the eventual deprecation and removal of the
symmetric keys.

See DEPR: Symmetric JWTs:
openedx/public-engineering#83
Comment on lines 184 to 187
# .. 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.

Comment on lines 235 to 236
# 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.

Copy link
Contributor

@timmc-edx timmc-edx left a comment

Choose a reason for hiding this comment

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

A couple nits, but everything else looks fine, so approving in advance to unblock.

Copy link
Contributor

@timmc-edx timmc-edx left a comment

Choose a reason for hiding this comment

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

Looks great! (Remember to update changelog date.)

@robrap robrap merged commit 813cdc7 into master Apr 12, 2023
@robrap robrap deleted the robrap/monitor-symmetric-keys branch April 12, 2023 18:56
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

Successfully merging this pull request may close these issues.

2 participants