fix: always check authn_http's header and ssl_option #10449
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
zhongwencool
requested review from
a team,
JimMoen and
savonarola
as code owners
zhongwencool
force-pushed
the
authn-http-validation
branch
from
36c5bac
to
27d8eec
Compare
Pull Request Test Coverage Report for Build 4741588951
馃挍 - Coveralls |
savonarola
reviewed
Apr 20, 2023
savonarola
reviewed
Apr 20, 2023
savonarola
reviewed
Apr 20, 2023
| @@ -207,3 +209,27 @@ array(Name) -> | |||
|
|
|||
| array(Name, DescId) -> | |||
| {Name, ?HOCON(?R_REF(Name), #{desc => ?DESC(DescId)})}. | |||
|
|
|||
| validations() -> | |||
There was a problem hiding this comment.
Will these validations be called for listener-specific authentications?
There was a problem hiding this comment.
No.
Right now, hocon can't do that.
There was a problem hiding this comment.
But we will deprecated listener-specific authentications in the future.
zhongwencool
force-pushed
the
authn-http-validation
branch
from
0100295
to
1c25a1a
Compare
HJianBo
reviewed
Apr 21, 2023
| @@ -46,6 +46,103 @@ array_nodes_test() -> | |||
| ), | |||
| ok. | |||
|
|
|||
| authn_validations_test() -> | |||
There was a problem hiding this comment.
Why do we need to put the unit tests of authn in emqx_conf instead of the authn application?
| "", | ||
| Conf = <<BaseConf/binary, (list_to_binary(DisableSSLWithHttps))/binary>>, | ||
| {ok, ConfMap} = hocon:binary(Conf, #{format => richmap}), | ||
| ?assertThrow( |
There was a problem hiding this comment.
We also need a test to cover all cases where the checks have passed.
Co-authored-by: JianBo He <heeejianbo@163.com>
zhongwencool
force-pushed
the
authn-http-validation
branch
2 times, most recently
from
e5314ad
to
d288327
Compare
zhongwencool
force-pushed
the
authn-http-validation
branch
from
d288327
to
fdf9b2a
Compare
HJianBo
approved these changes
Apr 21, 2023
8 tasks
zmstone
reviewed
Apr 21, 2023
| <<"get">> -> | ||
| case maps:is_key(<<"content-type">>, Headers) of | ||
| false -> ok; | ||
| true -> <<"HTTP GET requests cannot include content-type header.">> |
There was a problem hiding this comment.
if we add this restriction, i guess Kjell fixed an issue for nothing in ehttpc ?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes EMQX-9631
If the ssl_options and header configurations are set incorrectly when creating
authn_http, it could result in the entire authn being unusable.Before
we can create an
authn_httpwith{ssl_options.enable=false, url = "https://xxx"}, but https must enable ssl_options.enable.After
Summary
馃 Generated by Copilot at dfb7bbf
Added and improved validation functions and tests for the http authentication backend in
emqx_authn. Fixed some minor issues and updated the version number.PR Checklist
Please convert it to a draft if any of the following conditions are not met. Reviewers may skip over until all the items are checked:
changes/{ce,ee}/(feat|perf|fix)-<PR-id>.en.mdfilesChecklist for CI (.github/workflows) changes
changes/dir for user-facing artifacts update