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

feat(openid-connect): Add symmetric_key option to support verifying tokens with symmetric algorithms #7691

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

liweitianux
Copy link
Contributor

Description

The underlying lua-resty-openidc module already supports the
symmetric_key option to specify the HMAC key for verifying HS???
tokens. However, note that lua-resty-openidc just passes the
symmetric_key value as-is to HMAC. So we choose to accept a
base64url-encoded secret, which is easier to obtain from OP and
configure, and then decode it before passing it to lua-resty-openidc.

Checklist

  • I have explained the need for this PR and the problem it solves
  • I have explained the changes or the new features added to this PR
  • I have added tests corresponding to this change
  • I have updated the documentation to reflect this change
  • I have verified that this change is backward compatible (If not, please discuss on the APISIX mailing list first)

The underlying `lua-resty-openidc` module already supports the
`symmetric_key` option to specify the HMAC key for verifying HS???
tokens.  However, note that `lua-resty-openidc` just passes the
`symmetric_key` value as-is to HMAC.  So we choose to accept a
base64url-encoded secret, which is easier to obtain from OP and
configure, and then decode it before passing it to `lua-resty-openidc`.
@liweitianux
Copy link
Contributor Author

NOTE: the lua-resty-openidc is having a bug in verifying HS??? tokens, and I've submitted a PR to fix it: zmartzone/lua-resty-openidc#447

Copy link
Member

@spacewander spacewander left a comment

Choose a reason for hiding this comment

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

Could you add some tests for this feature? How can we test it?

apisix/plugins/openid-connect.lua Outdated Show resolved Hide resolved
@liweitianux
Copy link
Contributor Author

Could you add some tests for this feature? How can we test it?

Hmm, I'll have to first check out the existing tests (e.g., for RS256) and figure out one for the HS256/HS512 tokens.

@liweitianux
Copy link
Contributor Author

NOTE: the lua-resty-openidc is having a bug in verifying HS??? tokens, and I've submitted a PR to fix it: zmartzone/lua-resty-openidc#447

Well. lua-resty-openidc is actually good. It's the Keycloak (I was testing with) having the issue with HS256 (and other HS algorithms) tokens.

By the way, I'll have to consider how to best pass the symmetric_key option (e.g., whether to encode or not).

@spacewander
Copy link
Member

It's the Keycloak (I was testing with) having the issue with HS256 (and other HS algorithms) tokens.

Is there any link for this issue in Keycloak? Would you explain it in depth?

@spacewander
Copy link
Member

It's the Keycloak (I was testing with) having the issue with HS256 (and other HS algorithms) tokens.

Is there any link for this issue in Keycloak? Would you explain it in depth?

Solved it: keycloak/keycloak#13823.

@github-actions
Copy link

This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 4 weeks if no further activity occurs. If you think that's incorrect or this pull request should instead be reviewed, please simply write any comment. Even if closed, you can still revive the PR at any time or discuss it on the [email protected] list. Thank you for your contributions.

@github-actions github-actions bot added the stale label Oct 16, 2022
@github-actions
Copy link

This pull request/issue has been closed due to lack of activity. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time.

@github-actions github-actions bot closed this Nov 19, 2022
@kayx23 kayx23 reopened this Nov 30, 2023
@kayx23 kayx23 changed the title feat(openid-connect): Add 'symmetric_key' option to support HS??? tokens feat(openid-connect): Add symmetric_key option to support verifying tokens with symmetric algorithms Nov 30, 2023
@kayx23 kayx23 removed the stale label Nov 30, 2023
@kayx23
Copy link
Member

kayx23 commented Nov 30, 2023

Re-opened as this is a meaningful contribution.

Helped resolve conflicts and update the explanation for the symmetric_key to be a bit more readable, because the PR author mentioned in a different PR they've changed jobs and might not be able to continue the effort here, especially with adding test cases.

@monkeyDluffy6017
Copy link
Contributor

@shreemaan-abhishek please help to finish this pr

Copy link

This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 4 weeks if no further activity occurs. If you think that's incorrect or this pull request should instead be reviewed, please simply write any comment. Even if closed, you can still revive the PR at any time or discuss it on the [email protected] list. Thank you for your contributions.

@github-actions github-actions bot added the stale label Jan 30, 2024
@kayx23 kayx23 removed the stale label Feb 23, 2024
@Revolyssup
Copy link
Contributor

@liweitianux Can you no longer work on this PR?

@Revolyssup
Copy link
Contributor

@liweitianux Looks like you're busy. We will assign this issue to someone else.

@liweitianux
Copy link
Contributor Author

@liweitianux Looks like you're busy. We will assign this issue to someone else.

Hi @Revolyssup, sorry that I haven't been using APISIX for some time, and I currently don't have the time for this PR. Please help reassign it. Thanks.

Copy link

This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 4 weeks if no further activity occurs. If you think that's incorrect or this pull request should instead be reviewed, please simply write any comment. Even if closed, you can still revive the PR at any time or discuss it on the [email protected] list. Thank you for your contributions.

@github-actions github-actions bot added the stale label May 13, 2024
@kayx23 kayx23 removed the stale label May 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants