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

refactor(ndi): use jose #595

Merged
merged 1 commit into from
Oct 6, 2023
Merged

refactor(ndi): use jose #595

merged 1 commit into from
Oct 6, 2023

Conversation

justin-tay
Copy link
Contributor

This refactors the ndi implementation to use jose, add additional logging and fix some bugs.

This should also close #594

Copy link
Contributor

@LoneRifle LoneRifle left a comment

Choose a reason for hiding this comment

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

lgtm. thanks once again for working on this!

@LoneRifle LoneRifle merged commit fc5bfef into opengovsg:master Oct 6, 2023
2 of 3 checks passed
@cflee
Copy link
Contributor

cflee commented Oct 6, 2023

For anyone else coming along and wondering what are the bugs fixed in this refactor PR, aka why their client suddenly broke against mockpass after upgrading from v4.0.8 to v4.0.9, here are what I think are the more material bugs fixed and changes made:

  • On the v2 discovery endpoint, the token_endpoint_auth_signing_alg_values_supported and id_token_encryption_enc_values_supported now match the current values on NDI's endpoints and have multiple values; previously, they only had a single value.
  • On the v2 token endpoint, the value of client assertion JWS header alg is now checked against the list of accepted algorithms as declared in token_endpoint_auth_signing_alg_values_supported; previously, it was not checked.
  • On the v2 token endpoint, the presence of the client assertion JWS header typ key is now checked; previously, the presence was not checked.
  • On the v2 token endpoint, the ID token JWE now uses the encryption method A256CBC-HS512 as declared in id_token_encryption_enc_values_supported; previously, it was using A128CBC-HS256, which was mismatching what was expected based on the discovery endpoint.
  • On the v2 token endpoint, the ID token JWE is now encrypted to the appropriate RP key selected using the NDI-specified logic; previously, it was encrypting to the first encryption key found.
  • On the v2 token endpoint, for some error responses, response status 401 is now used instead of 400.

If there are any other bugs that are known to be fixed, or if any of these are wrong, please correct me!

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.

Corppass claims are of the incorrect format
3 participants