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: allow to use environment variables for openid-connect plugin #11451

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

feat: allow to use environment variables for openid-connect plugin #11451

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
68d705c
feat: allow to use environment variables for openid-connect
Jul 31, 2024
2e7df95
feat: allow to use environment variables for openid-connect
Jul 31, 2024
5ab3f3e
feat: allow to use environment variables for openid-connect
Jul 31, 2024
dfd3005
feat: allow to use environment variables for openid-connect plugin
darkSheep404 Sep 25, 2024
cc39db4
feat: allow to use environment variables for openid-connect
Sep 26, 2024
980df99
feat: allow to use environment variables for openid-connect
Sep 26, 2024
a4b9e06
Merge branch 'master' into feat-openid-connect-support-env
Oct 8, 2024
3fe0e00
feat: allow to use environment variables for openid-connect
Oct 8, 2024
6c7a0b9
feat: allow to use environment variables for openid-connect
Nov 15, 2024
1f75535
Update apisix/plugins/openid-connect.lua
darkSheep404 Dec 24, 2024
0303851
Update openid-connect.lua
darkSheep404 Dec 24, 2024
db6bc80
Merge branch 'apache:master' into feat-openid-connect-support-env
darkSheep404 Jan 8, 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
6 changes: 4 additions & 2 deletions apisix/plugins/openid-connect.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ local core = require("apisix.core")
local ngx_re = require("ngx.re")
local openidc = require("resty.openidc")
local random = require("resty.random")
local fetch_secrets = require("apisix.secret").fetch_secrets
local string = string
local ngx = ngx
local ipairs = ipairs
Expand Down Expand Up @@ -290,7 +291,8 @@ local _M = {
}


function _M.check_schema(conf)
function _M.check_schema(plugin_conf)
local conf = fetch_secrets(plugin_conf)
Copy link
Contributor

Choose a reason for hiding this comment

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

this is not needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is not needed.↳

This is needed when someone puts a non-string value such as a Boolean into env var, otherwise the type inconsistency will fail the check

Copy link

Choose a reason for hiding this comment

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

This line is giving me some issues after apisix reload.
If line fetch_secrets is done in check_schema, the route fails showing me the following error:

2024/11/12 17:12:51 [error] 240#240: *13728 lua entry thread aborted: runtime error: ...isix/custom-plugins/apisix/plugins/openid-connect.lua:478: attempt to compare nil with number
stack traceback:
coroutine 0:
     ...isix/custom-plugins/apisix/plugins/openid-connect.lua: in function 'phase_func'
     /usr/local/apisix/apisix/plugin.lua:1166: in function 'run_plugin'
     /usr/local/apisix/apisix/init.lua:689: in function 'http_access_phase'
     access_by_lua(nginx.conf:310):2: in main chunk, client: 10.89.2.37, server: _, request: "GET /private/anything HTTP/2.0", host: "XXXX"

If I remove fetch_secrets from check_schema, the route work as expected but the following warning is shown at startup:
[warn] 187#187: *8391 [lua] utils.lua:418: find_and_log(): Using openid-connect discovery with no TLS is a security risk, context: init_worker_by_lua*
I assume that without the fetch_secrets the value of discovery is not resolved and openid checks https for it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So should we remove fetch_secrets from check_schema to avoid this?
By contrast, putting a Boolean value in a secret is not a particularly common case in this plugin. Typically, only string urls and secret keys will be placed in valut

Copy link
Contributor Author

Choose a reason for hiding this comment

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

pushed this change

if conf.ssl_verify == "no" then
-- we used to set 'ssl_verify' to "no"
conf.ssl_verify = false
Expand Down Expand Up @@ -471,7 +473,7 @@ local function required_scopes_present(required_scopes, http_scopes)
end

function _M.rewrite(plugin_conf, ctx)
local conf = core.table.clone(plugin_conf)
local conf = fetch_secrets(plugin_conf)
darkSheep404 marked this conversation as resolved.
Show resolved Hide resolved

-- Previously, we multiply conf.timeout before storing it in etcd.
-- If the timeout is too large, we should not multiply it again.
Expand Down
9 changes: 9 additions & 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 @@ -93,6 +93,15 @@ description: OpenID Connect allows the client to obtain user information from th

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

In addition, you can use Environment Variables or APISIX secret to store and reference plugin attributes. APISIX currently supports storing secrets in two ways - [Environment Variables and HashiCorp Vault](../terminology/secret.md).

For example, use below command to set environment variable
`export keycloak_secret=abc`

and use it in plugin conf like below

`"client_secret": "$ENV://keycloak_secret"`

## Scenarios

:::tip
Expand Down
49 changes: 49 additions & 0 deletions t/fips/openid-connect.t
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

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

please add new test cases in t/plugin/openid-connect

Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,10 @@ no_long_string();
no_root_location();
no_shuffle();

BEGIN {
$ENV{CLIENT_SECRET_ENV} = "60Op4HFM0I8ajz0WdiStAbziZ-VFQttXuxixHHs2R7r7-CW8GR79l-mmLqMhc-Sa";
}

add_block_preprocessor(sub {
my ($block) = @_;

Expand Down Expand Up @@ -109,3 +113,48 @@ passed
--- error_code: 401
--- error_log
jwt signature verification failed: invalid key length



=== TEST 3: configure oidc plugin with small public key using environment variable
--- config
location /t {
content_by_lua_block {
local t = require("lib.test_admin").test
local code, body = t('/apisix/admin/routes/1',
ngx.HTTP_PUT,
[[{ "plugins": {
"openid-connect": {
"client_id": "kbyuFDidLLm280LIwVFiazOqjO3ty8KH",
"client_secret": "$ENV://CLIENT_SECRET_ENV",
"discovery": "https://samples.auth0.com/.well-known/openid-configuration",
"redirect_uri": "https://iresty.com",
"ssl_verify": false,
"timeout": 10,
"bearer_only": true,
"scope": "apisix",
"public_key": "-----BEGIN PUBLIC KEY-----\n]] ..
[[MFwwDQYJKoZIhvcNAQEBBQADSwAwSAJBANW16kX5SMrMa2t7F2R1w6Bk/qpjS4QQ\n]] ..
[[hnrbED3Dpsl9JXAx90MYsIWp51hBxJSE/EPVK8WF/sjHK1xQbEuDfEECAwEAAQ==\n]] ..
[[-----END PUBLIC KEY-----",
"token_signing_alg_values_expected": "RS256"
}
},
"upstream": {
"nodes": {
"127.0.0.1:1980": 1
},
"type": "roundrobin"
},
"uri": "/hello"
}]]
)

if code >= 300 then
ngx.status = code
end
ngx.say(body)
}
}
--- response_body
passed
Loading