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.

Sign up for GitHub

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

Jump to bottom

fix: jwe-decrypt secret length restriction #10928

Merged
merged 5 commits into from
Feb 12, 2024
Merged

fix: jwe-decrypt secret length restriction #10928

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
62337f2
enhance-jwe-decrypt-secret-length-restiection
Feb 8, 2024
7052e25
fix wrong ci
Feb 8, 2024
46df8d2
remove the schema min length, it will check at `check_schema`
Feb 8, 2024
1977f37
fix docs
Feb 8, 2024
e17f65e
fix: check the schema first, then check the length of secret
Feb 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 13 additions & 1 deletion apisix/plugins/jwe-decrypt.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ local consumer_schema = {
type = "object",
properties = {
key = { type = "string" },
secret = { type = "string", minLength = 32 },
secret = { type = "string" },
is_base64_encoded = { type = "boolean" },
},
required = { "key", "secret" },
Expand All @@ -66,6 +66,18 @@ local _M = {

function _M.check_schema(conf, schema_type)
if schema_type == core.schema.TYPE_CONSUMER then
-- restrict the length of secret, we use A256GCM for encryption,
-- so the length should be 32 chars only
if conf.is_base64_encoded then
if #base64.decode_base64url(conf.secret) ~= 32 then
return false, "the secret length after base64 decode should be 32 chars"
end
else
if #conf.secret ~= 32 then
return false, "the secret length should be 32 chars"
end
end

return core.schema.check(consumer_schema, conf)
end
return core.schema.check(schema, conf)
Expand Down
6 changes: 6 additions & 0 deletions docs/en/latest/plugins/jwe-decrypt.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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


:::

For Route:

| Name | Type | Required | Default | Description |
Expand Down
6 changes: 6 additions & 0 deletions docs/zh/latest/plugins/jwe-decrypt.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,12 @@ Consumer 配置：
| secret | string | True | | | 解密密钥，必须为 32 位。秘钥可以使用 [Secret](../terminology/secret.md) 资源保存在密钥管理服务中 |
| is_base64_encoded | boolean | False | false | | 如果密钥是 Base64 编码，则需要配置为 `true` |

:::note

注意，在启用 `is_base64_encoded` 后，你的 `secret` 长度可能会超过 32 位，你只需要保证在 Decode 后的长度仍然是 32 位即可。

:::

Route 配置：

| 名称 | 类型 | 必选项 | 默认值 | 描述 |
Expand Down
88 changes: 64 additions & 24 deletions t/plugin/jwe-decrypt.t
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ qr/{"key":"123","secret":"[a-zA-Z0-9+\\\/]+={0,2}"}/



=== TEST 2: wrong type of string
=== TEST 2: wrong type of key
--- config
location /t {
content_by_lua_block {
Expand All @@ -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

--- config
location /t {
content_by_lua_block {
local core = require("apisix.core")
local plugin = require("apisix.plugins.jwe-decrypt")
local ok, err = plugin.check_schema({key = "123", secret = "123456"}, core.schema.TYPE_CONSUMER)
local ok, err = plugin.check_schema({key = "123", secret = 12345678901234567890123456789012}, core.schema.TYPE_CONSUMER)
if not ok then
ngx.say(err)
end
Expand All @@ -89,12 +89,52 @@ done
}
}
--- response_body
property "secret" validation failed: string too short, expected at least 32, got 6
property "secret" validation failed: wrong type: expected string, got number
done



=== TEST 4: add consumer with username and plugins
=== TEST 4: secret length too long
--- config
location /t {
content_by_lua_block {
local core = require("apisix.core")
local plugin = require("apisix.plugins.jwe-decrypt")
local ok, err = plugin.check_schema({key = "123", secret = "123456789012345678901234567890123"}, core.schema.TYPE_CONSUMER)
if not ok then
ngx.say(err)
end

ngx.say("done")
}
}
--- response_body
the secret length should be 32 chars
done



=== TEST 5: secret length too long(base64 encode)
--- config
location /t {
content_by_lua_block {
local core = require("apisix.core")
local plugin = require("apisix.plugins.jwe-decrypt")
local ok, err = plugin.check_schema({key = "123", secret = "YWJjZGVmZ2hpamtsbW5vcHFyc3R1dnd4eXphYmNkZWZn", is_base64_encoded = true}, core.schema.TYPE_CONSUMER)
if not ok then
ngx.say(err)
end

ngx.say("done")
}
}
--- response_body
the secret length after base64 decode should be 32 chars
done



=== TEST 6: add consumer with username and plugins
--- config
location /t {
content_by_lua_block {
Expand Down Expand Up @@ -123,7 +163,7 @@ passed



=== TEST 5: enable jwe-decrypt plugin using admin api
=== TEST 7: enable jwe-decrypt plugin using admin api
--- config
location /t {
content_by_lua_block {
Expand Down Expand Up @@ -158,7 +198,7 @@ passed



=== TEST 6: create public API route (jwe-decrypt sign)
=== TEST 8: create public API route (jwe-decrypt sign)
--- config
location /t {
content_by_lua_block {
Expand All @@ -184,7 +224,7 @@ passed



=== TEST 7: sign / verify in argument
=== TEST 9: sign / verify in argument
--- config
location /t {
content_by_lua_block {
Expand Down Expand Up @@ -214,14 +254,14 @@ hello world



=== TEST 8: test for unsupported method
=== TEST 10: test for unsupported method
--- request
PATCH /apisix/plugin/jwe/encrypt?key=user-key
--- error_code: 404



=== TEST 9: verify, missing token
=== TEST 11: verify, missing token
--- request
GET /hello
--- error_code: 403
Expand All @@ -230,7 +270,7 @@ GET /hello



=== TEST 10: verify: invalid JWE token
=== TEST 12: verify: invalid JWE token
--- request
GET /hello
--- more_headers
Expand All @@ -241,7 +281,7 @@ Authorization: invalid-eyJhbGciOiJkaXIiLCJraWQiOiJ1c2VyLWtleSIsImVuYyI6IkEyNTZHQ



=== TEST 11: verify (in header)
=== TEST 13: verify (in header)
--- request
GET /hello
--- more_headers
Expand All @@ -251,7 +291,7 @@ hello world



=== TEST 12: verify (in header without Bearer)
=== TEST 14: verify (in header without Bearer)
--- request
GET /hello
--- more_headers
Expand All @@ -261,7 +301,7 @@ hello world



=== TEST 13: verify (header with bearer)
=== TEST 15: verify (header with bearer)
--- request
GET /hello
--- more_headers
Expand All @@ -271,7 +311,7 @@ hello world



=== TEST 14: verify (invalid bearer token)
=== TEST 16: verify (invalid bearer token)
--- request
GET /hello
--- more_headers
Expand All @@ -282,7 +322,7 @@ Authorization: bearer invalid-eyJhbGciOiJkaXIiLCJraWQiOiJ1c2VyLWtleSIsImVuYyI6Ik



=== TEST 15: delete a exist consumer
=== TEST 17: delete a exist consumer
--- config
location /t {
content_by_lua_block {
Expand Down Expand Up @@ -332,7 +372,7 @@ code: true body: passed



=== TEST 16: add consumer with username and plugins with base64 secret
=== TEST 18: add consumer with username and plugins with base64 secret
--- config
location /t {
content_by_lua_block {
Expand Down Expand Up @@ -362,7 +402,7 @@ passed



=== TEST 17: enable jwt decrypt plugin with base64 secret
=== TEST 19: enable jwt decrypt plugin with base64 secret
--- config
location /t {
content_by_lua_block {
Expand Down Expand Up @@ -396,7 +436,7 @@ passed



=== TEST 18: create public API route (jwe-decrypt sign)
=== TEST 20: create public API route (jwe-decrypt sign)
--- config
location /t {
content_by_lua_block {
Expand All @@ -422,7 +462,7 @@ passed



=== TEST 19: sign / verify in argument
=== TEST 21: sign / verify in argument
--- config
location /t {
content_by_lua_block {
Expand Down Expand Up @@ -454,7 +494,7 @@ hello world



=== TEST 20: verify (in header)
=== TEST 22: verify (in header)
--- request
GET /hello
--- more_headers
Expand All @@ -464,7 +504,7 @@ hello world



=== TEST 21: verify (in header without Bearer)
=== TEST 23: verify (in header without Bearer)
--- request
GET /hello
--- more_headers
Expand All @@ -474,7 +514,7 @@ hello world



=== TEST 22: enable jwt decrypt plugin with test upstream route
=== TEST 24: enable jwt decrypt plugin with test upstream route
--- config
location /t {
content_by_lua_block {
Expand Down Expand Up @@ -508,7 +548,7 @@ passed



=== TEST 23: verify in upstream header
=== TEST 25: verify in upstream header
--- request
GET /headers
--- more_headers
Expand Down
Loading