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

darkSheep404 · 2024-07-31T08:40:29Z

Description

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

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)

darkSheep404 · 2024-09-24T01:17:13Z

hi good morning @shreemaan-abhishek clould you kindly review this PR for me ?

shreemaan-abhishek

please write a test case that uses vault as well.

apisix/plugins/openid-connect.lua

shreemaan-abhishek · 2024-09-25T08:21:02Z

apisix/plugins/openid-connect.lua

@@ -290,7 +291,8 @@ local _M = {
 }


-function _M.check_schema(conf)
+function _M.check_schema(plugin_conf)
+    local conf = fetch_secrets(plugin_conf)


this is not needed.

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

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.

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

pushed this change

shreemaan-abhishek

please write a test case that uses vault as well.

shreemaan-abhishek · 2024-09-25T08:22:50Z

t/fips/openid-connect.t

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

Co-authored-by: Shreemaan Abhishek <shreemaan.abhishek@gmail.com>

darkSheep404 · 2024-09-26T05:42:53Z

please write a test case that uses vault as well.↳
hi @shreemaan-abhishek
I tried to add a test for valut but I am not sure if it is correct, may need your help to have a look

shreemaan-abhishek

the test cases for secret resource seems correct to me, please resolve the conflicts with master so that the tests can run

alberto-corrales · 2024-10-07T16:14:30Z

Great contribution!! I'm looking forward to this fix, as we are using AWS Secrets Manager as a secrets provider and we need to configure the secret as an environment variable. I hope this fix is merged and released soon 🙏

# Conflicts: # t/fips/openid-connect.t

darkSheep404 · 2024-10-08T02:09:11Z

the test cases for secret resource seems correct to me, please resolve the conflicts with master so that the tests can run↳

hi @shreemaan-abhishek
The conflict has been resolved. Please help approve it when you are free so that can run the test

brmejia

This is exactly what I was looking for. I've tested this config on my local environment, and it works as expected.

client_id: "$env://APISIX_OIDC_CLIENT_ID"
client_secret: "$env://APISIX_OIDC_CLIENT_SECRET"
discovery: $env://APISIX_OIDC_DISCOVERY_URL

I hope this will be merged soon

alberto-corrales · 2024-12-17T13:38:30Z

Are there any updates on this? At the moment we have to keep a secret in our repo because of the bug that it is fixing this PR, and we need to move it to environment variables. So it would be awesome if we could have this fixed in a patch soon

darkSheep404 · 2024-12-17T13:42:50Z

Are there any updates on this? At the moment we have to keep a secret in our repo because of the bug that it is fixing this PR, and we need to move it to environment variables. So it would be awesome if we could have this fixed in a patch soon

We need another good Samaritan to approve this change, then this PR will have three approve and be merged

darkSheep404 · 2024-12-17T14:12:23Z

Are there any updates on this? At the moment we have to keep a secret in our repo because of the bug that it is fixing this PR, and we need to move it to environment variables. So it would be awesome if we could have this fixed in a patch soon

We need another good Samaritan to approve this change, then this PR will have three approve and be merged

@shreemaan-abhishek @nic-6443

Could any of you help approve this PR at your convenience? It has been pending here for a long time and only needs the last approver

darkSheep404 · 2024-12-24T14:35:52Z

Are there any updates on this? At the moment we have to keep a secret in our repo because of the bug that it is fixing this PR, and we need to move it to environment variables. So it would be awesome if we could have this fixed in a patch soon

We need another good Samaritan to approve this change, then this PR will have three approve and be merged

@shreemaan-abhishek @nic-6443

Could any of you help approve this PR at your convenience? It has been pending here for a long time and only needs the last approver

hi @kayx23 @membphis @Revolyssup Could any of you help?

shreemaan-abhishek · 2024-12-24T16:44:14Z

I have left a comment: https://github.com/apache/apisix/pull/11451/files#r1896868940

clone conf before fetch_secrets to adopt issue apache#1956 Co-authored-by: Shreemaan Abhishek <shreemaan.abhishek@gmail.com>

clone conf before fetch_secrets to adopt issue apache#1956

darkSheep404 · 2024-12-24T17:39:48Z

I have left a comment: https://github.com/apache/apisix/pull/11451/files#r1896868940

i think fetch_sercets should already return a clone conf instead of modify origianl plugin_conf
anyway, i just adopted your suggestion and committed the change which clone plugin_conf before fetch_sercets to avoid possible problems
pls have to review at your convinence, many thanks

membphis · 2024-12-31T07:40:13Z

apisix/plugins/openid-connect.lua

@@ -471,7 +472,8 @@ 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_clone = core.table.clone(plugin_conf)


performance tips:

we can use a lrucache, avoid repeated rendering of plugin_conf configuration

performance tips:

we can use a lrucache, avoid repeated rendering of plugin_conf configuration

Thank you, but I don't fully understand your suggestion

The original way, I just used fetch_secret without clone the config first , but with this suggestion, I add table.clone before fetch_secret

I think fetch_secret itself will return a clone of conf, and if you want to optimize it, maybe can just keep using fetch_secret directly, instead of add another extra lrucache

darkSheep404 · 2025-01-03T08:37:26Z

In addition, I noticed that two tests failed, but I don't know how to fix them. Is there anyone who is kind enough to help 😢

membphis · 2025-01-08T02:43:18Z

1th tip

=====bad style=====
reindex: t/plugin/openid-connect.t: done.

echo 'you need to run '''reindex''' to fix them. Read CONTRIBUTING.md for more details.'
you need to run 'reindex' to fix them. Read CONTRIBUTING.md for more details.
exit 1
make: *** [Makefile:173: lint] Error 1

2th tip

you can merge the latest master branch

darkSheep404 · 2025-01-08T07:39:49Z

1th tip

=====bad style===== reindex: t/plugin/openid-connect.t: done.↳

echo 'you need to run '''reindex''' to fix them. Read CONTRIBUTING.md for more details.'
you need to run 'reindex' to fix them. Read CONTRIBUTING.md for more details.

exit 1
make: *** [Makefile:173: lint] Error 1

2th tip

you can merge the latest master branch↳

merged latest master branch

for 1th tip, cloud you kindly help to run reindex for me ?
my local windows pc no apisix source code env, we do development related to apisix use apisix offical docker image

darksheep404 added 3 commits July 31, 2024 16:21

feat: allow to use environment variables for openid-connect

68d705c

feat: allow to use environment variables for openid-connect

2e7df95

feat: allow to use environment variables for openid-connect

5ab3f3e

pottekkat requested a review from shreemaan-abhishek August 7, 2024 12:31

darkSheep404 mentioned this pull request Aug 9, 2024

bug: openid-connect yml conf could not use $env to set plugin properties #11319

Open

juzhiyuan mentioned this pull request Aug 13, 2024

The client secret for oidc plugin cannot be configured as environment variable #11353

Open

shreemaan-abhishek reviewed Sep 25, 2024

View reviewed changes

t/fips/openid-connect.t Outdated

Copy link

Contributor

shreemaan-abhishek Sep 25, 2024

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

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

dfd3005

Co-authored-by: Shreemaan Abhishek <shreemaan.abhishek@gmail.com>

dosubot bot added the size:M This PR changes 30-99 lines, ignoring generated files. label Sep 25, 2024

darksheep404 added 2 commits September 26, 2024 13:39

feat: allow to use environment variables for openid-connect

cc39db4

feat: allow to use environment variables for openid-connect

980df99

dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. and removed size:M This PR changes 30-99 lines, ignoring generated files. labels Sep 26, 2024

shreemaan-abhishek reviewed Oct 3, 2024

View reviewed changes

darksheep404 added 2 commits October 8, 2024 10:03

Merge branch 'master' into feat-openid-connect-support-env

a4b9e06

# Conflicts: # t/fips/openid-connect.t

feat: allow to use environment variables for openid-connect

3fe0e00

brmejia approved these changes Nov 12, 2024

View reviewed changes

darkSheep404 requested a review from shreemaan-abhishek November 15, 2024 03:01

feat: allow to use environment variables for openid-connect

6c7a0b9

alberto-corrales approved these changes Nov 25, 2024

View reviewed changes

darkSheep404 and others added 2 commits December 25, 2024 01:32

Update apisix/plugins/openid-connect.lua

1f75535

clone conf before fetch_secrets to adopt issue apache#1956 Co-authored-by: Shreemaan Abhishek <shreemaan.abhishek@gmail.com>

Update openid-connect.lua

0303851

clone conf before fetch_secrets to adopt issue apache#1956

membphis reviewed Dec 31, 2024

View reviewed changes

Merge branch 'apache:master' into feat-openid-connect-support-env

db6bc80

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

darkSheep404 commented Jul 31, 2024

darkSheep404 commented Sep 24, 2024 •

edited

Loading

shreemaan-abhishek left a comment

shreemaan-abhishek Sep 25, 2024

darkSheep404 Sep 26, 2024

brmejia Nov 12, 2024

darkSheep404 Nov 15, 2024

darkSheep404 Nov 15, 2024

shreemaan-abhishek left a comment

shreemaan-abhishek Sep 25, 2024

darkSheep404 commented Sep 26, 2024 •

edited

Loading

shreemaan-abhishek left a comment

alberto-corrales commented Oct 7, 2024

darkSheep404 commented Oct 8, 2024

brmejia left a comment

alberto-corrales commented Dec 17, 2024

darkSheep404 commented Dec 17, 2024 •

edited

Loading

darkSheep404 commented Dec 17, 2024 •

edited

Loading

darkSheep404 commented Dec 24, 2024 •

edited

Loading

shreemaan-abhishek commented Dec 24, 2024

darkSheep404 commented Dec 24, 2024 •

edited

Loading

membphis Dec 31, 2024

darkSheep404 Jan 3, 2025

darkSheep404 commented Jan 3, 2025

membphis commented Jan 8, 2025 •

edited

Loading

darkSheep404 commented Jan 8, 2025

1th tip

2th tip

1th tip

2th tip

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

Are you sure you want to change the base?

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

Conversation

darkSheep404 commented Jul 31, 2024

Description

Checklist

darkSheep404 commented Sep 24, 2024 • edited Loading

shreemaan-abhishek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shreemaan-abhishek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darkSheep404 commented Sep 26, 2024 • edited Loading

shreemaan-abhishek left a comment

Choose a reason for hiding this comment

alberto-corrales commented Oct 7, 2024

darkSheep404 commented Oct 8, 2024

brmejia left a comment

Choose a reason for hiding this comment

alberto-corrales commented Dec 17, 2024

darkSheep404 commented Dec 17, 2024 • edited Loading

darkSheep404 commented Dec 17, 2024 • edited Loading

darkSheep404 commented Dec 24, 2024 • edited Loading

shreemaan-abhishek commented Dec 24, 2024

darkSheep404 commented Dec 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darkSheep404 commented Jan 3, 2025

membphis commented Jan 8, 2025 • edited Loading

1th tip

2th tip

darkSheep404 commented Jan 8, 2025

1th tip

2th tip

1th tip

2th tip

darkSheep404 commented Sep 24, 2024 •

edited

Loading

darkSheep404 commented Sep 26, 2024 •

edited

Loading

darkSheep404 commented Dec 17, 2024 •

edited

Loading

darkSheep404 commented Dec 17, 2024 •

edited

Loading

darkSheep404 commented Dec 24, 2024 •

edited

Loading

darkSheep404 commented Dec 24, 2024 •

edited

Loading

membphis commented Jan 8, 2025 •

edited

Loading