Skip to content

TLS: Hide password in config #13998

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

TLS: Hide password in config #13998

wants to merge 1 commit into from
+2 −1

Conversation

dcorbacho
Copy link
Contributor

See #13958

@dcorbacho dcorbacho requested a review from michaelklishin June 2, 2025 09:39
@michaelklishin
Copy link
Collaborator

@dcorbacho I have considered this approach and concluded that it is likely to break when encrypted values are used in rabbitmq.conf.

I am talking a different approach in rabbit_networking:

start_ssl_listener(Listener, SslOpts0, NumAcceptors, ConcurrentConnsSupsCount) ->
    SslOpts = case proplists:get_value(password, SslOpts0) of
                undefined -> SslOpts0;
                Password  ->
                  %% A password can be a value or a function returning that value.
                  %% See the key_pem_password/0 type in https://github.com/erlang/otp/pull/5843/files.
                  NewOpts = proplists:delete(password, SslOpts0),
                  Fun = fun() -> Password end,
                  [{password, Fun} | NewOpts]
              end,
    start_listener(Listener, NumAcceptors, ConcurrentConnsSupsCount, 'amqp/ssl',
                   "TLS (SSL) listener", tcp_opts() ++ SslOpts).

@michaelklishin
Copy link
Collaborator

We have gone over the reason why I haven't touched the rabbit.schema on my branch with @dcorbacho. I will document it here for other contributors:

  1. We support encrypted values in rabbitmq.conf in 4.x
  2. After Cuttlefish generates a final erlang.config from rabbitmq.conf and advanced.config, the prelaunch app will decrypt all {encrypted, …} values and it cannot decrypt an {encrypted, fun(() -> binary())

Therefore we need to update the sites where these values are used for ssl_options.password and definitions.tls.password.

I will create a PR with that approach since I've had a branch ready.

@michaelklishin michaelklishin mentioned this pull request Jun 2, 2025
Merged
michaelklishin added a commit that referenced this pull request Jun 2, 2025
@michaelklishin
Use rabbit_ssl:wrap_password_opt in relevant places
30c32ee 
We do it at the latest possible moment to not break
encrypted value support in 'rabbitmq.conf' files.

See #13998 for context.

Closes #13958.
mergify bot pushed a commit that referenced this pull request Jun 2, 2025
@michaelklishin @mergify
Use rabbit_ssl:wrap_password_opt in relevant places
8e4c072 
We do it at the latest possible moment to not break
encrypted value support in 'rabbitmq.conf' files.

See #13998 for context.

Closes #13958.

(cherry picked from commit 30c32ee)
@mergify mergify bot mentioned this pull request Jun 2, 2025
Merged
LoisSotoLopez pushed a commit to cloudamqp/rabbitmq-server that referenced this pull request Jul 16, 2025
@michaelklishin @LoisSotoLopez
Use rabbit_ssl:wrap_password_opt in relevant places
b311d47 
We do it at the latest possible moment to not break
encrypted value support in 'rabbitmq.conf' files.

See rabbitmq#13998 for context.

Closes rabbitmq#13958.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michaelklishin michaelklishin Awaiting requested review from michaelklishin

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@dcorbacho @michaelklishin