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

fix: jwe-decrypt secret length restriction #10928

Merged
merged 5 commits into from
Feb 12, 2024
Merged

fix: jwe-decrypt secret length restriction #10928

merged 5 commits into from
Feb 12, 2024

Conversation

Vacant2333
Copy link
Contributor

Description

add restriction for jwe-decrypt plugin
the secret length should be 32 chars only, it will be checked on the check_scheme

Fixes #
#10883 (comment)

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)

@Vacant2333 Vacant2333 changed the title enhance: jwe-decrypt secret length restriction fix: jwe-decrypt secret length restriction Feb 8, 2024
Vacant2333 added 2 commits February 8, 2024 15:23
@Vacant2333
Copy link
Contributor Author

the previos ci was wrong, i will fix it on here 7052e25
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

@Vacant2333
Copy link
Contributor Author

@kayx23 @shreemaan-abhishek can u help me take a look~~

@@ -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.
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if we add this description to secret it will be too long i think

Vacant2333 added 2 commits February 8, 2024 15:50
@@ -74,13 +74,13 @@ done



=== TEST 3: wrong type of string
=== TEST 3: wrong type of secret
Copy link
Contributor

Choose a reason for hiding this comment

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

why change this test case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hi, i have explan this at here #10928 (comment), the test name was wrong, Test 3&4 was test for the wrong type of secret & key

Copy link
Contributor

@shreemaan-abhishek shreemaan-abhishek left a comment

Choose a reason for hiding this comment

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

LGTM

@shreemaan-abhishek shreemaan-abhishek merged commit ec38094 into apache:master Feb 12, 2024
50 checks passed
illidan33 pushed a commit to illidan33/apisix that referenced this pull request Feb 18, 2024
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.

3 participants