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

Simplify JWT_ISSUER validation #327

Open
robrap opened this issue Apr 13, 2023 · 2 comments
Open

Simplify JWT_ISSUER validation #327

robrap opened this issue Apr 13, 2023 · 2 comments

Comments

@robrap
Copy link
Contributor

robrap commented Apr 13, 2023

Ideally we would set `verify_iss' to True on this line:

'verify_iss': False, # TODO (ARCH-204): manually verify until issuer is configured correctly.

This would be in place of the manual verification done across all the JWT_ISSUERS, using this code (which could be removed):

# TODO (ARCH-204): verify issuer manually until it is properly configured.
token_issuer = decoded_token.get('iss')
# .. custom_attribute_name: jwt_auth_issuer
# .. custom_attribute_description: Value set to the JWT auth issuer.
set_custom_attribute('jwt_auth_issuer', token_issuer)
issuer_matched = any(issuer['ISSUER'] == token_issuer for issuer in get_jwt_issuers())
if token_issuer == jwt_issuer['ISSUER']:
# .. custom_attribute_name: jwt_auth_issuer_verification
# .. custom_attribute_description: Depending on issuer verification, the value will
# be one of: matches-first-issuer, matches-later-issuer, or no-match.
set_custom_attribute('jwt_auth_issuer_verification', 'matches-first-issuer')
elif issuer_matched:
set_custom_attribute('jwt_auth_issuer_verification', 'matches-later-issuer')
else:
set_custom_attribute('jwt_auth_issuer_verification', 'no-match')
logger.info('Token decode failed due to mismatched issuer [%s]', token_issuer)
raise jwt.InvalidTokenError('%s is not a valid issuer.' % token_issuer)

Note: this code could be removed once monitoring proves out that jwt_auth_issuer_verification always has a value of matches-first-issuer across services, especially including ecommerce and discovery, which still have add settings.

Note: Last discussed, it seemed to make sense the JWT_ISSUERS would remain a list, just in case we want to expand again in the future for rotating or moving.

@feanil
Copy link
Contributor

feanil commented May 9, 2023

@robrap the last note here seems to suggest that we would still want to manually verify the issuers because then we could have a list of valid issuers? Is that right or is there a way to do this with the underlying JWT library?

@feanil feanil closed this as completed May 9, 2023
@feanil feanil reopened this May 9, 2023
@robrap
Copy link
Contributor Author

robrap commented May 9, 2023

@feanil: One possibility is to run through the list and pull out the issuer and audience and keys for each item and use the libary's validation. If it works, success. If not, move on to the next one. This is basically what the old ecommerce decoder did.

In the past, we were trying to get down to a single issuer to make it clear that LMS should be the only identity provider. However, while we may want a single identity provider, we might need a list to migrate to a new provider in the future.

Also note that JWT_PUBLIC_SIGNING_JWK_SET contains a set of keys, but that is meant for rotating keys. We currently have one list, and it isn't part of the ISSUERS list. We could make a separate decision as to whether or not each issuer could have its own jwk set, which could override the main one, if you didn't want to combine keys from different issuers. Sorry if this is confusing, and it certainly is not urgent. For now, we'd just add all keys to the same set, and we'd try all keys for each issuer, even if they weren't relevant.

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

2 participants