chore: warn log when sending requests to external services insecurely#11403
Conversation
| } | ||
|
|
||
| function _M.check_schema(conf) | ||
| local check = {"idp_uri"} |
There was a problem hiding this comment.
Should cas_callback_uri and logout_uri be listed as checks? Similar fields in authz-casdoor and authz-keycloak are listed as checks.
There was a problem hiding this comment.
cas_callback_uri and logout_uri are in the form of "/foo/bar" so we don't need to check them.
|
|
||
|
|
||
| function _M.check_schema(conf) | ||
| local check = {"uri"} |
There was a problem hiding this comment.
The ssl_verify should also be checked, TLS without server certificate validation is meaningless.
| if schema_type == core.schema.TYPE_METADATA then | ||
| return core.schema.check(metadata_schema, conf) | ||
| end | ||
| local check = {"endpoint_addrs"} |
| if schema_type == core.schema.TYPE_METADATA then | ||
| return core.schema.check(metadata_schema, conf) | ||
| end | ||
| local check = {"endpoint_addrs"} |
|
|
||
|
|
||
| function _M.check_schema(conf) | ||
| local check = {"host"} |
| end | ||
|
|
||
| local check = {"discovery", "introspection_endpoint", "redirect_uri", | ||
| "post_logout_redirect_uri", "proxy_opts.http_proxy"} |
There was a problem hiding this comment.
Also check ssl_verify and proxy_opts.https_proxy.
|
|
||
|
|
||
| function _M.check_schema(conf) | ||
| local check = {"api_host"} |
| } | ||
| --- response_body | ||
| done | ||
| --- error_log |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
it would be nice to have but there are 20+ files that would need update, very tedious job and not worth the effort IMHO.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
on long term maintainability
I agree with you on this. 🙏🏼
e0b5bfe
bzp2010
left a comment
bzp2010
left a comment
There was a problem hiding this comment.
LGTM, only the API style and testing files are still in question.
| core.utils.check_https(check, conf, plugin_name) | ||
| core.utils.check_tls_bool({conf.ssl_verify}, {"ssl_verify"}, plugin_name) |
There was a problem hiding this comment.
Just simply curious, why is the style of the two check functions so inconsistent? 🤔
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
great catch 🙏🏼 , thanks for your detailed attention.
| } | ||
| --- response_body | ||
| done | ||
| --- error_log |
There was a problem hiding this comment.
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.
4 participants
Description
Enforcing TLS is important when using third party services.
Checklist