fix(s3): disallow partially defined credentials in schema #12574
Conversation
keynslug
force-pushed
the
fix/EMQX-11888/no-partial-creds
branch
from
fe3619e
to
3684637
Compare
apps/emqx_s3/src/emqx_s3_schema.erl
Outdated
| AccessKeyId = hocon_maps:get("s3.access_key_id", Config), | ||
| SecretAccessKey = hocon_maps:get("s3.secret_access_key", Config), | ||
| case {AccessKeyId, SecretAccessKey} of | ||
| {_Defined, undefined} -> |
There was a problem hiding this comment.
Shouldn't we guard that Defined =/= undefined here?
There was a problem hiding this comment.
Oh right! 🫠 Thanks.
keynslug
force-pushed
the
fix/EMQX-11888/no-partial-creds
branch
from
3684637
to
89f4095
Compare
| @@ -169,6 +170,27 @@ desc(s3_upload) -> | |||
| desc(transport_options) -> | |||
| "Options for the HTTP transport layer used by the S3 client". | |||
|
|
|||
| validations() -> | |||
There was a problem hiding this comment.
Since the values are not cross roots, so there is no maybe need to add a root level validator which is evaluated by all config checks even if it's not checking this root.
IIRC, we can implement roots/0 like this.
roots() ->
[{s3, hoconsc:mk(hoconsc:ref(s3), #{validator => fun(Conf) -> ... end})].
There was a problem hiding this comment.
What do you mean by "cross roots"?
There was a problem hiding this comment.
I mean if one needs to verify root1.key1 = bar is valid given root0.key0 = foo, then it's a cross-root validation,
one would have to add a global validator.
e.g. the recently added cluster.discoverty_strategy = dns vs node.name = emqx@FQDN check.
Fixes EMQX-11888.
Release version: e5.6
Summary
Running S3 client with only partially defined credentials does not make sense and usually causes crashes somewhere down the stack. Better to disallow this on the schema level.
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:
Added property-based tests for code which performs user input validationChange log has been added tochanges/(ce|ee)/(feat|perf|fix|breaking)-<PR-id>.en.mdfilesCreated PR to emqx-docs if documentation update is required, or link to a follow-up jira ticket