rabbitmq / rabbitmq-server

Open source RabbitMQ: core server and tier 1 (built-in) plugins
https://www.rabbitmq.com/
Other
12.3k stars 3.91k forks source link

4.x: investigate if management plugin's TLS options key can be renamed to ssl_options for consistency #10691

Closed lukebakken closed 8 months ago

lukebakken commented 8 months ago

Most SSL/TLS options are specified using proplist key ssl_options, except those for the management plugin, which uses ssl_opts. We should consider standardizing.

I noticed this here - https://groups.google.com/g/rabbitmq-users/c/2EZif7pcG60

Also see ancient PT chore here created by @lhoguin: https://www.pivotaltracker.com/n/projects/1560991/stories/135606833

image

michaelklishin commented 8 months ago

I was involved in the previous step of unification that has gotten us from management.listener.* to management.ssl.*.

I'm looking at what would be involved, we can workaround the Cuttlefish limitation where two rabbitmq.conf keys cannot map to a single Erlang app environment key. We'd also have to support two sets of legacy keys in different places in the management plugin because management.listener. is likely still used in a fair number of places :(

michaelklishin commented 8 months ago

The most recent change in this area was in https://github.com/rabbitmq/rabbitmq-management/pull/618, which has gotten us to

%% HTTPS listener consistent with Web STOMP and Web MQTT.
%%
%% {ssl_config, [{port,       15671},
%%               {ip,         "127.0.0.1"},
%%               {cacertfile, "/path/to/cacert.pem"},
%%               {certfile,   "/path/to/cert.pem"},
%%               {keyfile,    "/path/to/key.pem"}]}

{mapping, "management.ssl.port", "rabbitmq_management.ssl_config.port",
    [{datatype, integer}]}.
{mapping, "management.ssl.backlog", "rabbitmq_management.ssl_config.backlog",
    [{datatype, integer}]}.
{mapping, "management.ssl.ip", "rabbitmq_management.ssl_config.ip",
    [{datatype, string}, {validators, ["is_ip"]}]}.
{mapping, "management.ssl.certfile", "rabbitmq_management.ssl_config.certfile",
    [{datatype, string}, {validators, ["file_accessible"]}]}.
{mapping, "management.ssl.keyfile", "rabbitmq_management.ssl_config.keyfile",
    [{datatype, string}, {validators, ["file_accessible"]}]}.
{mapping, "management.ssl.cacertfile", "rabbitmq_management.ssl_config.cacertfile",
    [{datatype, string}, {validators, ["file_accessible"]}]}.
{mapping, "management.ssl.password", "rabbitmq_management.ssl_config.password",
    [{datatype, string}]}.

{mapping, "management.ssl.verify", "rabbitmq_management.ssl_config.verify", [
    {datatype, {enum, [verify_peer, verify_none]}}]}.

{mapping, "management.ssl.fail_if_no_peer_cert", "rabbitmq_management.ssl_config.fail_if_no_peer_cert", [
    {datatype, {enum, [true, false]}}]}.

is what I see in rabbitmq.schema.

So the ssl_opts key is how things were configured before 2018. The docs were not updated at first and then it's been forgotten.

However, there is no reason to configure management plugin settings via advanced.config. The OP is making their life unnecessarily complicated by trying to do everything via advanced.config instead of using rabbitmq.conf where it can be used — which is the majority of places, including TLS listener configuration in the management plugin.

While we can rename the key to ssl_options I am quite certain that there was a reason why it is the way it is. Possibly to be consistent with rabbitmq_management.tcp_config.

michaelklishin commented 8 months ago

@lukebakken I now understand why it is the way it is. Management plugin's TLS settings are not exactly like those of messaging protocols:

  1. It is not passed directly to the TLS library, they are passed to Cowboy
  2. We include cowboy_opts into these options, something that does not apply to other protocols
  3. We still support a legacy listener format (in rabbitmq.conf) and the earliest we can drop support for it is 4.0. Supporting two legacy formats, even though they come from different files/config formats, can make the plugin a real pain to reason about
  4. In the management plugin, rabbit_mgmt_app more specifically, we post-process the options which makes the problem in item 3 even worse

@lukebakken my suggestion would be to leave things the way they are for now, or in general. Management plugin is a bit different from others in naming but it is also fairly consistent with its TCP listener settings, and with rabbitmq.conf the settings make sense. This is not a common complaint anyway.

For 4.x, we can drop the legacy option and perhaps then consider renaming the app env key produced by Cuttlefish.

michaelklishin commented 8 months ago

The following config works fine with 3.13.0:

[
  {rabbitmq_management,
   [
    {ssl_config, [{port,     15671},
                  {ssl,      true},
                  {cacertfile, "/path/to/ca_certificate.pem"},
                  {certfile,   "/path/to/server_certificate.pem"},
                  {keyfile,    "/path/to/server_key.pem"},

                  %% don't do peer verification to HTTPS clients
                  {verify,               verify_none},
                  {fail_if_no_peer_cert, false},

                  {client_renegotiation, false},
                  {secure_renegotiate,   true},
                  {honor_ecc_order,      true},
                  {honor_cipher_order,   true},

                  {versions,['tlsv1.2']},
                  {ciphers, ["ECDHE-ECDSA-AES256-GCM-SHA384",
                             "ECDHE-RSA-AES256-GCM-SHA384",
                             "ECDHE-ECDSA-AES256-SHA384",
                             "ECDHE-RSA-AES256-SHA384",
                             "ECDH-ECDSA-AES256-GCM-SHA384",
                             "ECDH-RSA-AES256-GCM-SHA384",
                             "ECDH-ECDSA-AES256-SHA384",
                             "ECDH-RSA-AES256-SHA384",
                             "DHE-RSA-AES256-GCM-SHA384"
                            ]}
                ]}
   ]}
 ].
lhoguin commented 8 months ago

The ancient PT chore is more about internal listener information and code to start and manage listeners. A good time to do a thorough cleanup of those would be after the QUIC work is complete, as that adds further differences between the various listeners.

michaelklishin commented 8 months ago

After discussing this with @lukebakken we have decided to not change anything. The issue turned out to be with an outdated doc example. That's been corrected and as of https://github.com/rabbitmq/rabbitmq-website/commit/3cb16809cbaf283b3b25359429ee084d061ac669#diff-44cd5654f1b3fc3a3aa9b2a38c9621d7ebe347ddba8a17ec4e8577edd0661986R605, there is a warning that explains that rabbitmq.conf should be sufficient for 99% of management plugin configuration needs.

In the near future we will delete the classic config examples from the guides where the use of advanced.config is not an actual requirement (namely the LDAP guide for configuring queries).