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

Clean up and document JWT_AUTH settings #332

Open
robrap opened this issue Apr 14, 2023 · 0 comments
Open

Clean up and document JWT_AUTH settings #332

robrap opened this issue Apr 14, 2023 · 0 comments

Comments

@robrap
Copy link
Contributor

robrap commented Apr 14, 2023

The JWT_AUTH settings continue to be confusing. This ticket attempts to document current status and ideas for cleaning things up.

Note: Maybe this work could begin with writing a real document explaining the settings, and then we could improve it as we make improvements?

Here is a general search for JWT_AUTH in github.

The following was copied from the cookiecutter, and then comments were added (with some reordering):

JWT_AUTH.update({

    # The following 3 settings were presumably deprecated, in favor of the same settings
    #   moved under JWT_ISSUERS, and without the 'JWT_' prefix. See below.
    # This was marked deprecated in: https://github.com/openedx/edx-drf-extensions/blob/master/edx_rest_framework_extensions/settings.py#L61-L78.
    'JWT_AUDIENCE': None,
    'JWT_ISSUER': 'http://localhost:18000/oauth2',
    'JWT_SECRET_KEY': 'lms-secret',

    'JWT_VERIFY_AUDIENCE': False,

    'JWT_PUBLIC_SIGNING_JWK_SET': (
        '{"keys": [{"kid": "devstack_key", "e": "AQAB", "kty": "RSA", "n": "smKFSYowG6nNUAdeqH1jQQnH1PmIHphzBmwJ5vRf1vu'
        '48BUI5VcVtUWIPqzRK_LDSlZYh9D0YFL0ZTxIrlb6Tn3Xz7pYvpIAeYuQv3_H5p8tbz7Fb8r63c1828wXPITVTv8f7oxx5W3lFFgpFAyYMmROC'
        '4Ee9qG5T38LFe8_oAuFCEntimWxN9F3P-FJQy43TL7wG54WodgiM0EgzkeLr5K6cDnyckWjTuZbWI-4ffcTgTZsL_Kq1owa_J2ngEfxMCObnzG'
        'y5ZLcTUomo4rZLjghVpq6KZxfS6I1Vz79ZsMVUWEdXOYePCKKsrQG20ogQEkmTf9FT_SouC6jPcHLXw"}]}'
    ),

    'JWT_ISSUERS': [{
        'AUDIENCE': 'lms-key',
        'ISSUER': 'http://localhost:18000/oauth2',
        'SECRET_KEY': 'lms-secret',
    }],

})
  • The docstring for jwt_decode_handler is out of date:
    JWT_AUTH = {
    'JWT_DECODE_HANDLER': 'edx_rest_framework_extensions.auth.jwt.decoder.jwt_decode_handler',
    'JWT_ISSUER': 'https://the.jwt.issuer',
    'JWT_SECRET_KEY': 'the-jwt-secret-key', (defaults to settings.SECRET_KEY)
    'JWT_AUDIENCE': 'the-jwt-audience',
    'JWT_PUBLIC_SIGNING_JWK_SET': 'the-jwk-set-of-public-signing-keys',
    }

Setting notes:

  • JWT_VERIFY_AUDIENCE: There is a separate ticket: Clean up JWT_VERIFY_AUDIENCE and AUDIENCE setting #328

  • JWT_ISSUERS:

    • Although we should be down to a single issuer, we plan to continue to use this list. If we ever need to rotate or change issuers, this list can help.
    • Somewhat related is this ticket for simplifying issuer verification: Simplify JWT_ISSUER validation #327
    • When verifying claims (fields) in the JWT, we loop through each subkey as if they weren't actually related, but maybe we could treat them as a real set (see JWT_PUBLIC_SIGNING_JWK_SET)?
  • DEFAULT_JWT_ISSUER: Possibly retired, but the configs were never fully removed. Confirm and delete.

  • JWT_ISSUER: Although this is deprecated, it still may be used by the LMS when encoding JWTs. If we could get all encoding/decoding move to edx-drf-extensions, we could simplify.

  • JWT_SECRET_KEY: This is a tricky one since we want to retire symmetric keys, but we may need this for simpler testing. However, would it make sense to use this top-level key just for testing, or to use the JWT_ISSUERS list?

  • JWT_PUBLIC_SIGNING_JWK_SET: Why is this top-level and not also in JWT_ISSUERS? Should it move to complete the set?

  • JWT_PRIVATE_SIGNING_JWK: Do we want signing settings to be top-level, or also with JWT_ISSUERS[0]?

  • JWT_AUTH_COOKIE_HEADER_PAYLOAD and JWT_AUTH_COOKIE_SIGNATURE don't require clean-up, but should be documented along with the others.

  • JWT_AUTH_REFRESH_COOKIE: Should be removed, as documented in this DEPR: [DEPR]: Remove JWT_AUTH_REFRESH_COOKIE public-engineering#190

Note: any final doc should probably also point to the default settings from the docs for the library we are using. We may want to note where we've strayed not only from default values, but from default setting names. An example default setting might be JWT_LEEWAY. I'm not sure if this is used for consuming or producing JWTs, and where and how it should be set. Also, where and how should JWT_LEEWAY be set?

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