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

Enable asymmetric JWTs everywhere #333

Open
robrap opened this issue Apr 17, 2023 · 3 comments
Open

Enable asymmetric JWTs everywhere #333

robrap opened this issue Apr 17, 2023 · 3 comments

Comments

@robrap
Copy link
Contributor

robrap commented Apr 17, 2023

As part of openedx/public-engineering#83, we attempted at edx.org to force asymmetric JWTs everywhere. This resulted in some failures. This ticket is about cleaning up these issues, but also doing other consolidation and monitoring work for the following reasons:

  1. It may help us fix other issues that we had, but didn't know about when we last tried to force asymmetric JWTs.
  2. It may help reduce the effort around any JWT related future maintenance, because we will be doing different things in fewer places and it will be simpler to make fixes in one place.

See private 2U link to RCA for details of the rollback. The known issues that required the rollback were:

  1. The edx-notes-api is using custom JWT decoding code, rather than the jwt_decode_handler from this library, and it couldn't handle the asymmetric keys, even though it looks like the settings are in place. See related https://openedx.atlassian.net/browse/ARCHBOM-1156 (unfinished).
  2. Edx.org's third-party proctoring vendor proctortrack had a max size constraint on this jwt query parameter, and the size of the JWT grew when signed asymmetrically and busted the size limitation. The fix for this issue is being tracked in this private JIRA ticket: MST-1878.

Ideas for clean-up:

  • Note that the following ideas are not all the same level of priority, and someone should prioritize this list and potentially task things separately and/or toss out ideas that are not deemed important enough.
  • Add an ADR detailing what will be done and why, noting improved maintenance.
  • Replace all jwt encoding/decoding/signing in all libraries and services with calls from edx-drf-extensions. This would shrink the surface area of any underlying library changes, and ensure that we are handling everything the same way unless we actually require a difference. The following code searches should find what needs to change.
  • Included in the above searches is edx-platform's _create_jwt method. Although this code should only be used from edx-platform, it might make sense to be moved to edx-drf-extensions so all JWT creation and decoding is done from the library. It could be added to an identity_provider.py module that is clearly documented as only being allowed to be called by the identity provider, which at this time is the LMS. Additionally, the method name could be something like create_auth_jwt to differentiate from other JWTs that are being added to the platform.
  • If create_auth_jwt is moved here, possible the generate_jwt test utils could use the real code, rather than re-implementing and missing features. For example, the test payload doesn't include some additional claims around staff and superuser.
  • If create_auth_jwt is moved here we could potentially more cleanly see what settings are used, and consolidate on the same JWT_AUTH['ISSUERS'][0] settings for creation and decoding, so they don't need to be duplicated in the LMS. See this related setting clean-up issue: Clean up and document JWT_AUTH settings #332.
  • Any decoding of our auth JWT should be using the auth.jwt_decode_handler. This includes edx-notes-api. One implementation possibility is to insert a try/except for using jwt_decode_handler before whatever the current decoding implementation is, and add custom attributes for observability to see if the standard decoder worked fine, or if we needed to use the custom decoding for some reason. See this code for a similar pattern of trying different things and adding monitoring.
  • Note that there are possibly some cases where we are creating custom JWTs for other purposes other than authentication, like possibly in exams. Maybe we could add a create_custom_jwt method (and decoding method) outside the auth directory, and clarify that it is for use outside of auth and will not rely on JWT_AUTH settings. Maybe this is less urgent? However, it would be nice if these other bits of code could be clearly documented as being different than the authentication JWT. We could also add monitoring here, or everywhere, that includes the JWT's issuer, or anything else that would be useful. Note that these too should be signed asymmetrically, and shouldn't allow for a secret key.
  • It is unclear if the apple id JWT code (as found in the above searches) needs to continue to exist and/or whether it is the auth JWT or if it is its own custom JWT.
  • Potentially add linting for import jwt or jws.sign_compact to ensure that these aren't used outside of edx-drf-extensions. This will help ensure we don't regress, or at least that this issue doesn't spread.
  • Ultimately, it would be great to remove all of the symmetric key code. This means that we may need to introduce a generate_asymmetric_jwt call to the testing utils so libraries/services can switch to just testing using asymmetric key settings.
  • If we are using this library code in services, they probably don't need to retest certain cases, like ensuring that a user gets created during authentication, like in this ecommerce test.
  • Update New Relic rollout dashboard with any new custom attributes to ensure that no JWTs are broken when re-enabling the forced asymmetric JWTs.
  • Relates to Improve observability of JWT decoding errors edx/edx-arch-experiments#274
@robrap
Copy link
Contributor Author

robrap commented Oct 25, 2023

See private link: https://2u-internal.atlassian.net/browse/MST-1878. @zacharis278 has helped resolve the proctoring issue, but that would need to be tested on stage before we ever roll this out to prod again.

@robrap
Copy link
Contributor Author

robrap commented Jun 12, 2024

2U had a New Relic dashboard for rollout that was not yet migrated to Datadog. The details are being captured in this comment.

Screenshot 2024-06-12 at 12 53 05 PM Screenshot 2024-06-12 at 12 53 19 PM Screenshot 2024-06-12 at 12 53 51 PM Screenshot 2024-06-12 at 12 54 14 PM

@robrap
Copy link
Contributor Author

robrap commented Jun 12, 2024

Another 2U specific comment: It may also be useful to have data from the JWT Change rollout dashboard, which also wasn't yet migrated from New Relic to Datadog. See edx/edx-arch-experiments#684.

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

1 participant