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

feat: add "valid_issuers" field in openidc plugin #12002

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
+471 −11
Open

feat: add "valid_issuers" field in openidc plugin #12002

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
0208ad1
feat: add "valid_issuers" field in openidc plugin
Revolyssup Feb 27, 2025
5edc3e0
test
Revolyssup Feb 27, 2025
7fdccb2
lint
Revolyssup Feb 27, 2025
e7c9e56
fix tests
Revolyssup Feb 27, 2025
624bf50
remove redundant test
Revolyssup Feb 27, 2025
4bf171a
only validate issuer with jwks
Revolyssup Feb 27, 2025
4b1a69d
add docs
Revolyssup Feb 27, 2025
b672a8b
add tests
Revolyssup Feb 28, 2025
0ea7647
fix lint
Revolyssup Feb 28, 2025
adf5af6
apply suggestion
Revolyssup Feb 28, 2025
580a515
verify in pub key
Revolyssup Feb 28, 2025
085cf49
fix tests
Revolyssup Feb 28, 2025
6f4ede5
fix test
Revolyssup Feb 28, 2025
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
33 changes: 28 additions & 5 deletions apisix/plugins/openid-connect.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -275,7 +275,16 @@ local schema = {
items = {
type = "string"
}
}
},
valid_issuers = {
description = [[Whitelist the vetted issuers of the jwt.
When not passed by the user, the issuer returned by discovery endpoint will be used.
In case both are missing, the issuer will not be validated.]],
type = "array",
items = {
type = "string"
}
},
Comment on lines +279 to +287
Copy link
Contributor

@bzp2010 bzp2010 Feb 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This has already had another implementation that was approved and we have to decide which one is better.

#11987

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Your PR adds audience validation and my PR adds issuer validation.
Audience validation matches "aud" in the JWT with "client_id"
Issuer validation matches "issuer" in the JWT with discovery.issuer/or passed by user.

If the problem that I am trying to solve is this "I want to reject a request because customer passed a JWT with unrecognised issuer."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bzp2010

Copy link
Contributor

Choose a reason for hiding this comment

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

@Revolyssup 🙌

I certainly know what I did, and know what you did, and I mentioned that we have to choose one of the better configuration schema designs, while accommodating design that are similar in use but so different is unacceptable.
Specifically, if they are tiled, then this and all subsequent validator configuration items need to be tiled, otherwise it's going to be structured like that PR #11987.

That's exactly why that PR hasn't merged yet, it's waiting for some feedback.

},
encrypt_fields = {"client_secret", "client_rsa_private_key"},
required = {"client_id", "client_secret", "discovery"}
Expand Down Expand Up @@ -375,17 +384,31 @@ local function introspect(ctx, conf)
end
end

-- If we get here, token was found in request.

if conf.public_key or conf.use_jwks then
local opts = {}
-- Validate token against public key or jwks document of the oidc provider.
-- TODO: In the called method, the openidc module will try to extract
-- the token by itself again -- from a request header or session cookie.
-- It is inefficient that we also need to extract it (just from headers)
-- so we can add it in the configured header. Find a way to use openidc
-- module's internal methods to extract the token.
local res, err = openidc.bearer_jwt_verify(conf)

local valid_issuers
if conf.valid_issuers then
valid_issuers = conf.valid_issuers
else
local discovery, discovery_err = openidc.get_discovery_doc(conf)
if discovery_err then
core.log.warn("OIDC access discovery url failed : ", discovery_err)
else
core.log.info("valid_issuers not provided, using issuer from discovery doc: ",
discovery.issuer)
valid_issuers = {discovery.issuer}
end
end
if valid_issuers then
opts.valid_issuers = valid_issuers
end
local res, err = openidc.bearer_jwt_verify(conf, opts)
if err then
-- Error while validating or token invalid.
ngx.header["WWW-Authenticate"] = 'Bearer realm="' .. conf.realm ..
Expand Down
1 change: 1 addition & 0 deletions docs/en/latest/plugins/openid-connect.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,7 @@ description: OpenID Connect allows the client to obtain user information from th
| introspection_interval | integer | False | 0 | | TTL of the cached and introspected access token in seconds. |
| introspection_expiry_claim | string | False | | | Name of the expiry claim, which controls the TTL of the cached and introspected access token. The default value is 0, which means this option is not used and the plugin defaults to use the TTL passed by expiry claim defined in `introspection_expiry_claim`. If `introspection_interval` is larger than 0 and less than the TTL passed by expiry claim defined in `introspection_expiry_claim`, use `introspection_interval`. |
| introspection_addon_headers | string[] | False | | | Array of strings. Used to append additional header values to the introspection HTTP request. If the specified header does not exist in origin request, value will not be appended. |
| valid_issuers | string[] | False | | | Whitelist the vetted issuers of the jwt. When not passed by the user, the issuer returned by discovery endpoint will be used. In case both are missing, the issuer will not be validated. |

NOTE: `encrypt_fields = {"client_secret"}` is also defined in the schema, which means that the field will be stored encrypted in etcd. See [encrypted storage fields](../plugin-develop.md#encrypted-storage-fields).

Expand Down
1 change: 1 addition & 0 deletions docs/zh/latest/plugins/openid-connect.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,7 @@ description: OpenID Connect（OIDC）是基于 OAuth 2.0 的身份认证协议
| introspection_interval | integer | 否 | 0 | | 以秒为单位的缓存和内省访问令牌的 TTL。 |
| introspection_expiry_claim | string | 否 | | | 过期声明的名称，用于控制缓存和内省访问令牌的 TTL。 |
| introspection_addon_headers | string[] | 否 | | | `introspection_addon_headers` 是字符串列表，用于配置额外添加到内省 HTTP 请求中的请求头，如果配置的请求头不存在于源请求中，它将被忽略。|
| valid_issuers | string[] | 否 | | | 将经过审查的 jwt 发行者列入白名单。当用户未传递时，将使用发现端点返回的颁发者。如果两者均缺失，发行人将无法得到验证|

注意：schema 中还定义了 `encrypt_fields = {"client_secret"}`，这意味着该字段将会被加密存储在 etcd 中。具体参考 [加密存储字段](../plugin-develop.md#加密存储字段)。

Expand Down
7 changes: 5 additions & 2 deletions t/plugin/jwt-auth2.t
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ add_block_preprocessor(sub {
if (!defined $block->request) {
$block->set_value("request", "GET /t");
}

});

run_tests;
Expand Down Expand Up @@ -174,7 +175,8 @@ hello world
[[G70MOLYvGCZxl1o8S3q4X67MxcPlfJaXnbog2AOOGRaFar88XiLFWTbXMCLuz7xD\n]] ..
[[zQIDAQAB\n]] ..
[[-----END PUBLIC KEY-----",
"token_signing_alg_values_expected": "RS256"
"token_signing_alg_values_expected": "RS256",
"valid_issuers": ["Mysoft corp"]
}
},
"upstream": {
Expand Down Expand Up @@ -327,7 +329,8 @@ qr/ailed to verify jwt: 'exp' claim expired at/
[[G70MOLYvGCZxl1o8S3q4X67MxcPlfJaXnbog2AOOGRaFar88XiLFWTbXMCLuz7xD\n]] ..
[[zQIDAQAB\n]] ..
[[-----END PUBLIC KEY-----",
"token_signing_alg_values_expected": "RS256"
"token_signing_alg_values_expected": "RS256",
"valid_issuers": ["Mysoft corp"]
}
},
"upstream": {
Expand Down
12 changes: 8 additions & 4 deletions t/plugin/openid-connect.t
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -479,7 +479,8 @@ OIDC introspection failed: Invalid Authorization header format.
[[G70MOLYvGCZxl1o8S3q4X67MxcPlfJaXnbog2AOOGRaFar88XiLFWTbXMCLuz7xD\n]] ..
[[zQIDAQAB\n]] ..
[[-----END PUBLIC KEY-----",
"token_signing_alg_values_expected": "RS256"
"token_signing_alg_values_expected": "RS256",
"valid_issuers": ["Mysoft corp"]
}
},
"upstream": {
Expand Down Expand Up @@ -553,7 +554,8 @@ true
[[G70MOLYvGCZxl1o8S3q4X67MxcPlfJaXnbog2AOOGRaFar88XiLFWTbXMCLuz7xD\n]] ..
[[zQIDAQAB\n]] ..
[[-----END PUBLIC KEY-----",
"token_signing_alg_values_expected": "RS256"
"token_signing_alg_values_expected": "RS256",
"valid_issuers": ["Mysoft corp"]
}
},
"upstream": {
Expand Down Expand Up @@ -623,7 +625,8 @@ x-userinfo: ey.*
"set_access_token_header": true,
"access_token_in_authorization_header": true,
"set_id_token_header": false,
"set_userinfo_header": false
"set_userinfo_header": false,
"valid_issuers": ["Mysoft corp"]
}
},
"upstream": {
Expand Down Expand Up @@ -687,7 +690,8 @@ x-real-ip: 127.0.0.1
[[G70MOLYvGCZxl1o8S3q4X67MxcPlfJaXnbog2AOOGRaFar88XiLFWTbXMCLuz7xD\n]] ..
[[zQIDAQAB\n]] ..
[[-----END PUBLIC KEY-----",
"token_signing_alg_values_expected": "RS256"
"token_signing_alg_values_expected": "RS256",
"valid_issuers": ["Mysoft corp"]
}
},
"upstream": {
Expand Down
Loading
Loading