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

docs: jwe-decrypt secret length must be 32 chars #10883

Merged
merged 2 commits into from
Jan 31, 2024
Merged

docs: jwe-decrypt secret length must be 32 chars #10883

merged 2 commits into from
Jan 31, 2024

Conversation

Vacant2333
Copy link
Contributor

@Vacant2333 Vacant2333 commented Jan 30, 2024

Description

Fixes # (issue 10825)

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)

Signed-off-by: Vacant2333 <vacang2333@gmail.com>
@Vacant2333 Vacant2333 marked this pull request as ready for review January 30, 2024 03:19
@Vacant2333
Copy link
Contributor Author

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

| Name | Type | Required | Default | Valid values | Description |
|---------------|---------|-------------------------------------------------------|---------|-----------------------------|------------------------------------------------------------------------------------------------------------------------------------------|
| key | string | True | | | Unique key for a Consumer. |
| secret | string | True | | | The decryption key. The key could be saved in a secret manager using the [Secret](../terminology/secret.md) resource. (Must be 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
| secret | string | True | | | The decryption key. The key could be saved in a secret manager using the [Secret](../terminology/secret.md) resource. (Must be 32 chars) |
| 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. |

| 名称 | 类型 | 必选项 | 默认值 | 有效值 | 描述 |
|---------------|---------|-------|-------|-----|-------------------------------------------------------------------|
| key | string | True | | | Consumer 的唯一 key |
| secret | string | True | | | 解密密钥。秘钥可以使用 [Secret](../terminology/secret.md) 资源保存在密钥管理服务中(32 位) |
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
| secret | string | True | | | 解密密钥。秘钥可以使用 [Secret](../terminology/secret.md) 资源保存在密钥管理服务中(32 位) |
| secret | string | True | | | 解密密钥,必须为 32 位。秘钥可以使用 [Secret](../terminology/secret.md) 资源保存在密钥管理服务中 |

@Vacant2333
Copy link
Contributor Author

i need time to confirm is the solution right, close the pr now

@Vacant2333 Vacant2333 closed this Jan 30, 2024
@Vacant2333 Vacant2333 reopened this Jan 30, 2024
Signed-off-by: Vacant2333 <vacang2333@gmail.com>
@Vacant2333
Copy link
Contributor Author

i have remote the max-length of the scheme, cause if user set is_base64_encoded will get some problem, can u help me review and merge @shreemaan-abhishek @kayx23

@kayx23
Copy link
Member

kayx23 commented Jan 31, 2024

i have remote the max-length of the scheme

You mean remove? And where is it removed?

@Vacant2333
Copy link
Contributor Author

i have remote the max-length of the scheme

You mean remove? And where is it removed?

f2890ef#diff-6529ea6d6dfb2a162df8b4ef89a760c9acba198e139fba0faa58aed1e18dac08L50 here, In the first commit, I wanted to limit its length to the exact 32 bytes, but ci failed, because we have the requirement of base64, so I shouldn't have limited the length in scheme. I removed it in the second commit.

@Vacant2333 Vacant2333 changed the title fix: jwe-decrypt secret length must be 32 chars docs: jwe-decrypt secret length must be 32 chars Jan 31, 2024
@shreemaan-abhishek
Copy link
Contributor

@Vacant2333 you must also update the schema, shouldn't you? And is there any spec/rfc that supports your claim?

@Vacant2333
Copy link
Contributor Author

Vacant2333 commented Jan 31, 2024

@Vacant2333 you must also update the schema, shouldn't you? And is there any spec/rfc that supports your claim?

no, we cant limit the schemea, if use set is_base64_encoded to true, the secret will longger than 32 chars.
u can check this testcase, the secret after base64_encode will be longer than 32chars
https://github.com/apache/apisix/blob/master/t/plugin/jwe-decrypt.t#L348

Copy link
Contributor

@Revolyssup Revolyssup 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 6bb6069 into apache:master Jan 31, 2024
8 checks passed
@kayx23
Copy link
Member

kayx23 commented Feb 1, 2024

@Vacant2333 you must also update the schema, shouldn't you? And is there any spec/rfc that supports your claim?

no, we cant limit the schemea, if use set is_base64_encoded to true, the secret will longger than 32 chars. u can check this testcase, the secret after base64_encode will be longer than 32chars https://github.com/apache/apisix/blob/master/t/plugin/jwe-decrypt.t#L348

@Vacant2333 In that case, why is this PR still saying the secret must be 32 chars, instead of explaining more of what you explain above?

@Vacant2333
Copy link
Contributor Author

@Vacant2333 you must also update the schema, shouldn't you? And is there any spec/rfc that supports your claim?

no, we cant limit the schemea, if use set is_base64_encoded to true, the secret will longger than 32 chars. u can check this testcase, the secret after base64_encode will be longer than 32chars https://github.com/apache/apisix/blob/master/t/plugin/jwe-decrypt.t#L348

@Vacant2333 In that case, why is this PR still saying the secret must be 32 chars, instead of explaining more of what you explain above?

yes, u r right! i will raise a pr to mention these infomations, and it will check the secret length when APISIX running

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.

4 participants