feat(mqttbridge): support file-sourced secrets as passwords #11809

keynslug · 2023-10-24T07:57:04Z

How it works

Assuming we have the password for it stored in /var/lib/secrets/mqtt1/password, to source the password from this file, one need to either set an environment variable:

env EMQX__BRIDGES__MQTT__MQTT1__PASSWORD=file:///var/lib/secrets/mqtt1/password emqx ...

Or a HOCON config entry:

bridges.mqtt.mqtt1 { password = "file:///var/lib/secrets/mqtt1/password" }

In general, any string starting with file:// prefix is always considered a file-based secret, where the rest of the string interpreted as a file name.

Important implementation details

File-based secret is read in whole and then whitespaces are trimmed at the end of the file. This means that secrets can contain \r\ns but not at the very end.
There is currently no limitations on which files can be read and which can not.
File-based secrets (again, in general) are lazy: failure to specify existing file will not translate to a configuration error, rather it will implode at the time of first use. In case of data bridges it’s usually transition from the disconnected to the connected state.
As a consequence, secret file is read each time it’s used. If some bridge employs a pool of clients, the secret will be read from the file system during each client startup. This also means that secrets can be freely rotated, the operator does not currently need to do anything special with EMQX itself.
As a consequence, there’s now a possibility that EMQX will do frequent file system reads, which may be costly in some rare setups.

Summary

`🤖 Generated by Copilot at 7b109e7`

This pull request introduces a new feature to handle secret values, such as passwords, in the configuration of EMQ X. It adds a new secret() type to the HOCON schema and a new emqx_secret module to wrap and unwrap secret values using closures. It also updates the MQTT bridge connector and its test suite to use the new feature, and adds Swagger API documentation for it.

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 tests for the changes
Added property-based tests for code which performs user input validation
Changed lines covered in coverage report
Change log has been added to changes/(ce|ee)/(feat|perf|fix)-<PR-id>.en.md files
For internal contributor: there is a jira ticket to track this change
Created PR to emqx-docs if documentation update is required, or link to a follow-up jira ticket
Schema changes are backward compatible

These secrets follow the same `emqx_secret` convention of 0-arity functions. Also provide a simple HOCON schema module for use in application schemas.

zmstone · 2023-10-24T15:19:02Z

apps/emqx/src/emqx_schema_secret.erl

+        {ok, Secret} ->
+            string:trim(Secret, trailing, [$\n]);
+        {error, Reason} ->
+            error({inaccessible_secret_file, Reason}, [Filename])


maybe no need for stacktrace here

thorw(#{msg => failed_to_read_secret_file, path => Filename, reason => emqx_utils:explain_posix(Reason)})

apps/emqx/src/emqx_secret.erl

zmstone · 2023-10-24T18:14:27Z

apps/emqx/src/emqx_schema_secret.erl

+-spec wrap(source()) -> emqx_secret:t(t()).
+wrap(Source) ->
+    try
+        _Secret = load(Source),


we can probably leave the exception to runtime.
i.e. allow the file to be missing when emqx boots up or when new config is added.

integrations usually has a retry loop, so sysadmin can crate the file after seeing error messages.

Good point. On the other hand, should this be configurable? I.e. are there potential situations when we want to crash during configuration loading?

They are intended to be used mostly in the context of resources, which have their own feedback mechanism: statuses, retries, etc. Also turn the error into a throw exception, so that it can be interpreted as a regular error condition, for example by the resource manager.

savonarola · 2023-10-30T12:43:49Z

apps/emqx/src/emqx_secret.erl

+
+%% @doc Inspect the term wrapped in a secret closure.
+-spec term(t(_)) -> _Term.
+term(Wrap) when is_function(Wrap, 0) ->


It's a bit unclear why we need quite a tricky way to extract an "intermediate value for unwrap"...

Maybe I am missing something, but could we be more explicit:

wrap(Fun, Arg) -> fun(arg) -> Arg; (value) -> Fun(Arg) end.

This somewhat hacky approach is motivated by the fact that we want to keep emqx_secret following the convention (followed in turn by quite a few libraries) that says that wrapped secrets are 0-arity functions.

zmstone · 2023-10-31T12:15:02Z

apps/emqx/src/emqx_secret.erl

+-spec wrap(atom(), _Term) -> t(_).
+wrap(Function, Term) ->
+    fun() ->
+        apply(?LOADER, Function, [Term])
+    end.


maybe:

Suggested change

-spec wrap(atom(), _Term) -> t(_).

wrap(Function, Term) ->

fun() ->

apply(?LOADER, Function, [Term])

end.

-spec wrap(atom(), _Term) -> t(_).

wrap_dynamic(Term) ->

fun() ->

apply(?LOADER, load, [Term])

end.

I mean if Term is already a contract between the caller and emqx_secret_loader:Function, maybe just keep it to one place.

Fixed in d278486.

To make code employing `emqx_secret` easier to follow.

id · 2023-10-31T13:26:21Z

May be test it in higher level tests as well?

keynslug · 2023-10-31T14:10:47Z

May be test it in higher level tests as well?

Basically no objections, but this PR currently affects only handling of secrets in the MQTT bridge app, so it's kinda unclear how to test this feature there. Should be more apparent how to do that in some of the followup PRs?

HJianBo · 2023-11-06T08:37:14Z

apps/emqx_bridge_mqtt/src/emqx_bridge_mqtt_connector.erl

+    }).
+
+mk_client_opt_password(Options = #{password := Secret}) ->
+    %% TODO: Teach `emqtt` to accept 0-arity closures as passwords.


https://github.com/emqx/emqtt/blob/a990a1f8332e8ea830e39a361300d3d538526ab8/src/emqtt.erl#L996

This is also needed I think.
emqx/emqtt#223

keynslug force-pushed the ft/EMQX-10808/file-secrets branch 2 times, most recently from 8596163 to ce0f64b Compare October 24, 2023 09:21

keynslug added 2 commits October 24, 2023 16:24

feat(emqx): add file-sourced generic secrets

1c2f932

These secrets follow the same `emqx_secret` convention of 0-arity functions. Also provide a simple HOCON schema module for use in application schemas.

feat(mqttbridge): support file-sourced secrets as passwords

52f4519

keynslug force-pushed the ft/EMQX-10808/file-secrets branch from ce0f64b to 52f4519 Compare October 24, 2023 09:24

zmstone reviewed Oct 24, 2023

View reviewed changes

savonarola reviewed Oct 30, 2023

View reviewed changes

keynslug marked this pull request as ready for review October 30, 2023 14:54

keynslug requested review from JimMoen, lafirest and a team as code owners October 30, 2023 14:54

keynslug requested a review from zmstone October 30, 2023 16:04

zmstone reviewed Oct 31, 2023

View reviewed changes

fix(secret): dedicate a specific loader module for file secrets

d278486

To make code employing `emqx_secret` easier to follow.

keynslug force-pushed the ft/EMQX-10808/file-secrets branch from 28c00b5 to d278486 Compare October 31, 2023 12:26

zmstone approved these changes Oct 31, 2023

View reviewed changes

keynslug merged commit 7092c75 into emqx:release-54 Oct 31, 2023
142 checks passed

keynslug deleted the ft/EMQX-10808/file-secrets branch November 2, 2023 18:14

HJianBo reviewed Nov 6, 2023

View reviewed changes

keynslug mentioned this pull request Nov 8, 2023

feat(bridge): accept wrapped secrets as passwords #11896

Merged

28 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(mqttbridge): support file-sourced secrets as passwords #11809

feat(mqttbridge): support file-sourced secrets as passwords #11809

keynslug commented Oct 24, 2023 •

edited

zmstone Oct 24, 2023

zmstone Oct 24, 2023

keynslug Oct 26, 2023

savonarola Oct 30, 2023 •

edited

keynslug Oct 30, 2023

zmstone Oct 31, 2023

zmstone Oct 31, 2023

keynslug Oct 31, 2023

id commented Oct 31, 2023

keynslug commented Oct 31, 2023 •

edited

HJianBo Nov 6, 2023

keynslug Nov 6, 2023

feat(mqttbridge): support file-sourced secrets as passwords #11809

feat(mqttbridge): support file-sourced secrets as passwords #11809

Conversation

keynslug commented Oct 24, 2023 • edited

How it works

Important implementation details

Summary

🤖 Generated by Copilot at 7b109e7

PR Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

savonarola Oct 30, 2023 • edited

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

id commented Oct 31, 2023

keynslug commented Oct 31, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keynslug commented Oct 24, 2023 •

edited

`🤖 Generated by Copilot at 7b109e7`

savonarola Oct 30, 2023 •

edited

keynslug commented Oct 31, 2023 •

edited