-
Notifications
You must be signed in to change notification settings - Fork 170
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
adds jwk alg matcher #1329
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
woah, good one.
@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 On the other hand, I am not sure about the Thanks in advance! |
Both should work, someone should have a look at that, both are important. |
@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! |
There was a problem hiding this 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
gateway/.s2i/bin/assemble
Outdated
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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
LGTM I share the comment for the change in Other than that, is should be rebased on top of master. Great work |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
adfaec9
to
9d959bf
Compare
This PR adds a function to match the algorithm of the public key retrieved from the
jwks_uri
against thejwt.header.alg
field. Currently we only match the JWT'salg
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