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

scitokens_internal: catch matching exception type after jwt-cpp update #125

Merged
merged 5 commits into from
Jun 5, 2023

Conversation

olifre
Copy link
Contributor

@olifre olifre commented Jun 2, 2023

After updating the vendored jwt-cpp version in:
a8c5977
the exception type if a claim is not found has changed, breaking the "no kid claim" use case.

Adapt the code to catch the new exception type.

This essentially broke:
#55

@djw8605 Can you think of a good test we could add to ensure this issue does not reappear?

@djw8605
Copy link
Contributor

djw8605 commented Jun 5, 2023

I suppose you can just add a kid test to start with? For example, create a token without a kid, then test getting the kid.

@olifre olifre force-pushed the fix-jwt-exception-compatibility branch from 5b5276a to a3280c9 Compare June 5, 2023 19:58
test/main.cpp Outdated Show resolved Hide resolved
@olifre olifre force-pushed the fix-jwt-exception-compatibility branch from a3280c9 to d513fcf Compare June 5, 2023 20:00
In case the constructor default "none" is passed, the "kid"
header claim is left out.
This also drops the unused "std::error_code ec" in the serialize function.
This creates a token without kid header claim and runs an example
at+jwt token verification test to ensure tokens without kid are accepted.
After updating the vendored jwt-cpp version in:
 a8c5977
the exception type if a claim is not found has changed,
breaking the "no kid claim" use case.

Adapt the code to catch the new exception type.
The vendored jwt-cpp version offers a convenience function
to check whether the header claim is present, use that instead of
accessing it and catching the exception.
@olifre olifre force-pushed the fix-jwt-exception-compatibility branch from d513fcf to 66f8d67 Compare June 5, 2023 20:02
@djw8605
Copy link
Contributor

djw8605 commented Jun 5, 2023

did you accidently merge something else into this pull request? It's adding more than just the exception fix and test.

@olifre
Copy link
Contributor Author

olifre commented Jun 5, 2023

did you accidently merge something else into this pull request? It's adding more than just the exception fix and test.

Indeed, sorry, it should be fixed now — I have also taken the chance to rebase onto current master.

The changes now are:

  • Fixup which exception was caught (to adapt to the new jwt-cpp version).
  • Allow to serialize a token without kid header claim.
  • Add a test doing that and verifying the token.
  • Finally, drop the exception stuff altogether and use the new convenience function shipped by jwt-cpp. That should also be more stable 👍 .

@djw8605
Copy link
Contributor

djw8605 commented Jun 5, 2023

Quick question, are we using the exception stuff for any other parts?

@olifre
Copy link
Contributor Author

olifre commented Jun 5, 2023

I think all other places either catch std::exception directly, i.e. are generic enough, and then just pass on the error message, or don't cross the boundary between jwt-cpp and scitokens-cpp. So while there are other uses of exceptions, these appear to be safe / more safe.

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@djw8605 djw8605 merged commit e0251c9 into scitokens:master Jun 5, 2023
@olifre
Copy link
Contributor Author

olifre commented Jun 5, 2023

@djw8605 Many thanks for merging!
In case it's not too much of a hassle, it would be nice to get a release with this fix — we've realized this hits us with tokens issued by the Unity IAM (e.g. used by B2Access, Helmholtz AAI etc.).
Thanks in advance!

@olifre olifre deleted the fix-jwt-exception-compatibility branch June 5, 2023 20:29
@olifre
Copy link
Contributor Author

olifre commented Jun 15, 2023

@djw8605 Sorry for bumping this, but since this issue is biting us (and is a regression), would it be possible to get a new release within the next weeks=
This would help us to use scitokens-cpp with the Unity IAM.

Many thanks in advance!

@djw8605
Copy link
Contributor

djw8605 commented Jun 16, 2023

Hi, yes I expect to see a release in epel in the next week. I’ll comment here when it’s in epel to let you know.

@olifre
Copy link
Contributor Author

olifre commented Jun 16, 2023

This is great to hear, many thanks in advance, and have a nice weekend! 😄

@djw8605
Copy link
Contributor

djw8605 commented Jun 16, 2023

Hi, I had some time tonight, so updates pushed out the door. They are in the bodhi update system, I expect them to be in epel-testing in ~24 hours, and in epel-release repos in ~7 days.

Please test the 1.0.2 version! You can easily grab it from epel-testing when that hits.
EL7: https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2023-52740761bf
EL8: https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2023-5586be8a7b
EL9: https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2023-9948cdabde

@olifre
Copy link
Contributor Author

olifre commented Jun 17, 2023

@djw8605 Many thanks, I tested the new packages on el7 and el8 (RockyLinux) together with XRootD and everything works as expected, so I provided some karma on the packages 👍 .
Thanks again!

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