You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by "Vacant2333 (via GitHub)" <gi...@apache.org> on 2024/02/08 06:44:00 UTC
[PR] enhance: jwe-decrypt secret length restriction [apisix]
Vacant2333 opened a new pull request, #10928:
URL: https://github.com/apache/apisix/pull/10928
### Description
add restriction for `jwe-decrypt` plugin
the secret length should be 32 chars only, it will be checked on the `check_scheme`
<!-- Please include a summary of the change and which issue is fixed. -->
<!-- Please also include relevant motivation and context. -->
Fixes #
https://github.com/apache/apisix/pull/10883#issuecomment-1931380021
### Checklist
- [x] 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
- [x] I have added tests corresponding to this change
- [x] 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](https://github.com/apache/apisix/tree/master#community) first)
<!--
Note
1. Mark the PR as draft until it's ready to be reviewed.
2. Always add/update tests for any changes unless you have a good reason.
3. Always update the documentation to reflect the changes made in the PR.
4. Make a new commit to resolve conversations instead of `push -f`.
5. To resolve merge conflicts, merge master instead of rebasing.
6. Use "request review" to notify the reviewer after making changes.
7. Only a reviewer can mark a conversation as resolved.
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: jwe-decrypt secret length restriction [apisix]
Posted by "shreemaan-abhishek (via GitHub)" <gi...@apache.org>.
shreemaan-abhishek commented on code in PR #10928:
URL: https://github.com/apache/apisix/pull/10928#discussion_r1483269729
##########
t/plugin/jwe-decrypt.t:
##########
@@ -74,13 +74,13 @@ done
-=== TEST 3: wrong type of string
+=== TEST 3: wrong type of secret
Review Comment:
why change this test case?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: jwe-decrypt secret length restriction [apisix]
Posted by "shreemaan-abhishek (via GitHub)" <gi...@apache.org>.
shreemaan-abhishek merged PR #10928:
URL: https://github.com/apache/apisix/pull/10928
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: jwe-decrypt secret length restriction [apisix]
Posted by "Vacant2333 (via GitHub)" <gi...@apache.org>.
Vacant2333 commented on PR #10928:
URL: https://github.com/apache/apisix/pull/10928#issuecomment-1933499079
@kayx23 @shreemaan-abhishek can u help me take a look~~
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: jwe-decrypt secret length restriction [apisix]
Posted by "kayx23 (via GitHub)" <gi...@apache.org>.
kayx23 commented on code in PR #10928:
URL: https://github.com/apache/apisix/pull/10928#discussion_r1482519566
##########
docs/en/latest/plugins/jwe-decrypt.md:
##########
@@ -44,6 +44,12 @@ For Consumer:
| secret | string | True | | | The decryption key. Must be 32 characters. The key could be saved in a secret manager using the [Secret](../terminology/secret.md) resource. |
| is_base64_encoded | boolean | False | false | | Set to true if the secret is base64 encoded. |
+:::note
+
+After enabling `is_base64_encoded`, your `secret` length may exceed 32 chars. You only need to make sure that the length after Decode is still 32 chars.
Review Comment:
```suggestion
After enabling `is_base64_encoded`, your `secret` length may exceed 32 chars. You only need to make sure that the length after decoding is still 32 chars.
```
tbh i think this info should go into the description of `secret`, rather than a note at the bottom (if its not too difficult to read)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: jwe-decrypt secret length restriction [apisix]
Posted by "Vacant2333 (via GitHub)" <gi...@apache.org>.
Vacant2333 commented on code in PR #10928:
URL: https://github.com/apache/apisix/pull/10928#discussion_r1483783441
##########
t/plugin/jwe-decrypt.t:
##########
@@ -74,13 +74,13 @@ done
-=== TEST 3: wrong type of string
+=== TEST 3: wrong type of secret
Review Comment:
hi, i have explan this at here https://github.com/apache/apisix/pull/10928#issuecomment-1933498159, the test name was wrong, Test 3&4 was test for the wrong type of `secret` & `key`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: jwe-decrypt secret length restriction [apisix]
Posted by "Vacant2333 (via GitHub)" <gi...@apache.org>.
Vacant2333 commented on code in PR #10928:
URL: https://github.com/apache/apisix/pull/10928#discussion_r1482543940
##########
docs/en/latest/plugins/jwe-decrypt.md:
##########
@@ -44,6 +44,12 @@ For Consumer:
| secret | string | True | | | The decryption key. Must be 32 characters. The key could be saved in a secret manager using the [Secret](../terminology/secret.md) resource. |
| is_base64_encoded | boolean | False | false | | Set to true if the secret is base64 encoded. |
+:::note
+
+After enabling `is_base64_encoded`, your `secret` length may exceed 32 chars. You only need to make sure that the length after Decode is still 32 chars.
Review Comment:
if we add this description to `secret` it will be too long i think
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: jwe-decrypt secret length restriction [apisix]
Posted by "Vacant2333 (via GitHub)" <gi...@apache.org>.
Vacant2333 commented on PR #10928:
URL: https://github.com/apache/apisix/pull/10928#issuecomment-1933498159
the previos ci was wrong, i will fix it on here https://github.com/apache/apisix/pull/10928/commits/7052e25ee8e64bec1f2a0a3dbe38ac3904604877
the test 2 and 3 test for the value type of `key` and `secert`,
and i will remove the schema min length, because the length of `secret` will be checked on `check_schema`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org