apisix icon indicating copy to clipboard operation
apisix copied to clipboard

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

Open liweitianux opened this issue 3 years ago • 15 comments

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

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

liweitianux avatar Aug 16 '22 04:08 liweitianux

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

liweitianux avatar Aug 16 '22 04:08 liweitianux

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 avatar Aug 16 '22 06:08 liweitianux

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).

liweitianux avatar Aug 17 '22 04:08 liweitianux

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 avatar Aug 17 '22 06:08 spacewander

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: https://github.com/keycloak/keycloak/issues/13823.

spacewander avatar Aug 17 '22 06:08 spacewander

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[bot] avatar Oct 16 '22 10:10 github-actions[bot]

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[bot] avatar Nov 19 '22 10:11 github-actions[bot]

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.

kayx23 avatar Nov 30 '23 01:11 kayx23

@shreemaan-abhishek please help to finish this pr

monkeyDluffy6017 avatar Dec 01 '23 09:12 monkeyDluffy6017

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[bot] avatar Jan 30 '24 10:01 github-actions[bot]

@liweitianux Can you no longer work on this PR?

Revolyssup avatar Mar 12 '24 17:03 Revolyssup

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

Revolyssup avatar Mar 13 '24 17:03 Revolyssup

@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.

liweitianux avatar Mar 14 '24 00:03 liweitianux

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[bot] avatar May 13 '24 10:05 github-actions[bot]

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[bot] avatar Jul 13 '24 10:07 github-actions[bot]

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[bot] avatar Aug 10 '24 10:08 github-actions[bot]