chore: warn log when sending requests to external services insecurely #11403

shreemaan-abhishek · 2024-07-13T07:19:21Z

Description

Enforcing TLS is important when using third party services.

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)

bzp2010 · 2024-07-15T17:09:48Z

apisix/plugins/cas-auth.lua

@@ -49,6 +49,8 @@ local _M = {
 }

 function _M.check_schema(conf)
+    local check = {"idp_uri"}


Should cas_callback_uri and logout_uri be listed as checks? Similar fields in authz-casdoor and authz-keycloak are listed as checks.

cas_callback_uri and logout_uri are in the form of "/foo/bar" so we don't need to check them.

bzp2010 · 2024-07-15T17:13:11Z

apisix/plugins/forward-auth.lua

@@ -78,6 +78,8 @@ local _M = {


 function _M.check_schema(conf)
+    local check = {"uri"}


The ssl_verify should also be checked, TLS without server certificate validation is meaningless.

bzp2010 · 2024-07-15T17:14:31Z

apisix/plugins/elasticsearch-logger.lua

@@ -124,6 +124,8 @@ function _M.check_schema(conf, schema_type)
    if schema_type == core.schema.TYPE_METADATA then
        return core.schema.check(metadata_schema, conf)
    end
+    local check = {"endpoint_addrs"}


Also check ssl_verify.

bzp2010 · 2024-07-15T17:14:45Z

apisix/plugins/clickhouse-logger.lua

@@ -89,6 +89,8 @@ function _M.check_schema(conf, schema_type)
    if schema_type == core.schema.TYPE_METADATA then
        return core.schema.check(metadata_schema, conf)
    end
+    local check = {"endpoint_addrs"}


Also check ssl_verify.

bzp2010 · 2024-07-15T17:14:57Z

apisix/plugins/authz-keycloak.lua

@@ -114,6 +114,9 @@ local _M = {


 function _M.check_schema(conf)
+    local check = {"discovery", "token_endpoint", "resource_registration_endpoint",


Also check ssl_verify.

bzp2010 · 2024-07-15T17:18:23Z

apisix/plugins/loki-logger.lua

@@ -125,6 +125,8 @@ function _M.check_schema(conf, schema_type)
        return core.schema.check(metadata_schema, conf)
    end

+    local check = {"endpoint_addrs"}


Also check ssl_verify.

bzp2010 · 2024-07-15T17:18:41Z

apisix/plugins/opa.lua

@@ -65,6 +65,8 @@ local _M = {


 function _M.check_schema(conf)
+    local check = {"host"}


Also check ssl_verify.

bzp2010 · 2024-07-15T17:20:07Z

apisix/plugins/openid-connect.lua

@@ -305,6 +305,10 @@ function _M.check_schema(conf)
        }
    end

+    local check = {"discovery", "introspection_endpoint", "redirect_uri",
+                    "post_logout_redirect_uri", "proxy_opts.http_proxy"}


Also check ssl_verify and proxy_opts.https_proxy.

bzp2010 · 2024-07-15T17:22:19Z

apisix/plugins/openwhisk.lua

@@ -63,6 +63,9 @@ local _M = {


 function _M.check_schema(conf)
+    local check = {"api_host"}


Also check ssl_verify.

bzp2010 · 2024-07-15T17:34:26Z

t/plugin/authz-casdoor.t

@@ -105,10 +105,43 @@ __DATA__
    }
 --- response_body
 done
+--- error_log


Should tests about these warnings use a sperate test file instead of putting them in each plugin's own tests.
i.e. create a new test file that specifically tests for security risk warnings.

it would be nice to have but there are 20+ files that would need update, very tedious job and not worth the effort IMHO.

I mean, if such is better (for reasons of complexity of modification now and on long term maintainability) and there is no such file now, we can create such a file and place the tests in one place.

Do you think this has advantages for long term maintenance? The problem I can imagine is that plugin contributors need to know that their tests need to be written in multiple test files to test different parts. This requires either contributor knowledge or a careful reviewer.

If so, let's move those tests to a new file.

and cc @moonming @membphis for advice

on long term maintainability

I agree with you on this. 🙏🏼

bzp2010

LGTM, only the API style and testing files are still in question.

bzp2010 · 2024-07-18T06:57:49Z

apisix/plugins/authz-keycloak.lua

+    core.utils.check_https(check, conf, plugin_name)
+    core.utils.check_tls_bool({conf.ssl_verify}, {"ssl_verify"}, plugin_name)


Just simply curious, why is the style of the two check functions so inconsistent? 🤔

I mean check and conf seem to be reversed. And it seems that check_tls_bool can continue to use the string pattern in check_https to get data in conf instead of supplying it upfront in the code.

great catch 🙏🏼 , thanks for your detailed attention.

bzp2010 · 2024-07-18T07:05:59Z

t/plugin/authz-casdoor.t

@@ -105,10 +105,43 @@ __DATA__
    }
 --- response_body
 done
+--- error_log


I mean, if such is better (for reasons of complexity of modification now and on long term maintainability) and there is no such file now, we can create such a file and place the tests in one place.

Do you think this has advantages for long term maintenance? The problem I can imagine is that plugin contributors need to know that their tests need to be written in multiple test files to test different parts. This requires either contributor knowledge or a careful reviewer.

If so, let's move those tests to a new file.

and cc @moonming @membphis for advice

shreemaan-abhishek added 6 commits July 13, 2024 12:58

chore: warn log when sending requests to external services insecurely

fab6750

fix tests

0ef61a6

fix

c53c202

fix

d38990a

fix lint

18688cd

fix lint and doc

d090827

shreemaan-abhishek requested review from Revolyssup, membphis and moonming July 15, 2024 02:33

moonming previously approved these changes Jul 15, 2024

View reviewed changes

Revolyssup previously approved these changes Jul 15, 2024

View reviewed changes

shreemaan-abhishek requested a review from bzp2010 July 15, 2024 11:21

bzp2010 reviewed Jul 15, 2024

View reviewed changes

verify ssl_verify

e0b5bfe

shreemaan-abhishek dismissed stale reviews from Revolyssup and moonming via e0b5bfe July 17, 2024 08:19

shreemaan-abhishek added 4 commits July 18, 2024 10:31

make error logger similar to master

994985f

do field bool check

bd19581

fix long line

095b071

fix tests

78fc8db

bzp2010 self-requested a review July 18, 2024 06:45

bzp2010 reviewed Jul 18, 2024

View reviewed changes

shreemaan-abhishek added 4 commits July 22, 2024 16:39

shift tests to separate file

24881bc

remove tests from individual files

d23c4b2

delete remaining tests

d04f15b

make api similar

a71ec99

bzp2010 approved these changes Jul 24, 2024

View reviewed changes

shreemaan-abhishek requested a review from moonming July 24, 2024 02:14

Revolyssup self-requested a review July 24, 2024 04:59

Revolyssup approved these changes Jul 24, 2024

View reviewed changes

moonming approved these changes Jul 24, 2024

View reviewed changes

moonming merged commit 36b2b83 into apache:master Jul 24, 2024
35 checks passed

shreemaan-abhishek deleted the security-warning branch July 24, 2024 05:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: warn log when sending requests to external services insecurely #11403

chore: warn log when sending requests to external services insecurely #11403

shreemaan-abhishek commented Jul 13, 2024 •

edited

Loading

bzp2010 Jul 15, 2024

shreemaan-abhishek Jul 16, 2024

bzp2010 Jul 15, 2024

bzp2010 Jul 15, 2024

bzp2010 Jul 15, 2024

bzp2010 Jul 15, 2024

bzp2010 Jul 15, 2024

bzp2010 Jul 15, 2024

bzp2010 Jul 15, 2024

bzp2010 Jul 15, 2024

bzp2010 Jul 15, 2024

shreemaan-abhishek Jul 16, 2024

bzp2010 Jul 18, 2024

shreemaan-abhishek Jul 23, 2024

bzp2010 left a comment

bzp2010 Jul 18, 2024

bzp2010 Jul 18, 2024

shreemaan-abhishek Jul 22, 2024

bzp2010 Jul 18, 2024

		@@ -78,6 +78,8 @@ local _M = {


		function _M.check_schema(conf)
		local check = {"uri"}

		@@ -114,6 +114,9 @@ local _M = {


		function _M.check_schema(conf)
		local check = {"discovery", "token_endpoint", "resource_registration_endpoint",

		@@ -65,6 +65,8 @@ local _M = {


		function _M.check_schema(conf)
		local check = {"host"}

		@@ -63,6 +63,9 @@ local _M = {


		function _M.check_schema(conf)
		local check = {"api_host"}

		core.utils.check_https(check, conf, plugin_name)
		core.utils.check_tls_bool({conf.ssl_verify}, {"ssl_verify"}, plugin_name)

chore: warn log when sending requests to external services insecurely #11403

chore: warn log when sending requests to external services insecurely #11403

Conversation

shreemaan-abhishek commented Jul 13, 2024 • edited Loading

Description

Checklist

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bzp2010 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

shreemaan-abhishek commented Jul 13, 2024 •

edited

Loading