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

adds jwk alg matcher #1329

Merged
merged 4 commits into from
Apr 5, 2022
Merged

adds jwk alg matcher #1329

merged 4 commits into from
Apr 5, 2022

Conversation

kevprice83
Copy link
Member

This PR adds a function to match the algorithm of the public key retrieved from the jwks_uri against the jwt.header.alg field. Currently we only match the JWT's alg against a whitelist which is populated based on the response from the discovery endpoint but this further check is required to ensure we do the verification correctly.

Fixes THREESCALE-8249

@kevprice83 kevprice83 requested a review from a team as a code owner February 25, 2022 17:06
@kevprice83 kevprice83 changed the title adds jwk alg matcher [WIP] adds jwk alg matcher Feb 25, 2022
Copy link
Contributor

@eloycoto eloycoto left a comment

Choose a reason for hiding this comment

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

woah, good one.

@gsaslis
Copy link
Member

gsaslis commented Mar 8, 2022

@eloycoto I am wondering if we can have this PR merged, since it has been approved, but I see a couple of broken CI checks.

On the one hand, I see the prove job has been failing ... almost consistently in - at least - a few other PRs I checked. As such, I am inclined to ignore it.

On the other hand, I am not sure about the busted job failure? Should that failure block the merging of this PR, do you think?

Thanks in advance!

@eloycoto
Copy link
Contributor

eloycoto commented Mar 8, 2022

Both should work, someone should have a look at that, both are important.

@kevprice83
Copy link
Member Author

@gsaslis before merging I need to write the integration & unit tests. At the moment it's not urgent as we are planning to included this in the 2.12 release but I have it as a top priority for next week!

@kevprice83 kevprice83 changed the title [WIP] adds jwk alg matcher adds jwk alg matcher Mar 29, 2022
@kevprice83 kevprice83 requested review from eguzki, samugi and a team and removed request for eguzki March 29, 2022 19:01
Copy link
Contributor

@samugi samugi left a comment

Choose a reason for hiding this comment

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

assuming the circleci failures are all due to external factors: lgtm

if [[ -n "${GIT_BRANCH:-}" && "${GIT_BRANCH:-}" != master && -z "${GIT_TAG:-}" ]]; then
PATCH_BRANCH=THREESCALE

if [[ -n "${GIT_BRANCH:-}" && "${GIT_BRANCH:-}" != master && "${GIT_BRANCH:-}" != *"${PATCH_BRANCH}"* ]]; then
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this was maybe included by mistake in this PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

well it's not a big deal but yeah cleaner to remove it I guess

Copy link
Member Author

Choose a reason for hiding this comment

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

Done!

@eguzki
Copy link
Member

eguzki commented Apr 1, 2022

LGTM

I share the comment for the change in gateway/.s2i/bin/assemble

Other than that, is should be rebased on top of master.

Great work

Copy link
Contributor

@samugi samugi left a comment

Choose a reason for hiding this comment

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

lgtm

@kevprice83 kevprice83 force-pushed the THREESCALE-8249-alg-mismatch-patch branch from adfaec9 to 9d959bf Compare April 5, 2022 10:24
@eguzki eguzki merged commit 56e4967 into master Apr 5, 2022
@eguzki eguzki deleted the THREESCALE-8249-alg-mismatch-patch branch April 5, 2022 12:17
@kevprice83 kevprice83 restored the THREESCALE-8249-alg-mismatch-patch branch April 5, 2022 17:19
@kevprice83 kevprice83 deleted the THREESCALE-8249-alg-mismatch-patch branch April 5, 2022 17:26
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.

None yet

5 participants