Skip to content
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.

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

Fixed reading SSL settings from rabbitmqadmin config file #4408

Merged
merged 1 commit into from Apr 1, 2022
Merged

Fixed reading SSL settings from rabbitmqadmin config file #4408

merged 1 commit into from Apr 1, 2022

Conversation

fwolfsjaeger
Copy link
Contributor

@fwolfsjaeger fwolfsjaeger commented Apr 1, 2022

I am using a self-signed certificate for RabbitMQ. In order to connect to the server using rabbitmqadmin I have to set the flag --ssl-insecure. When trying to set this flag in the rabbitmqadmin config file I noticed that it doesn't work.

Proposed Changes

I've extended the python script so that the boolean SSL options can be defined in the config file. It is still possible to override the config values with cli options.

Types of Changes

What types of changes does your code introduce to this project?
Put an x in the boxes that apply

  • Bug fix (non-breaking change which fixes issue #NNNN)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause an observable behavior change in existing systems)
  • Documentation improvements (corrections, new content, etc)
  • Cosmetic change (whitespace, formatting, etc)
  • Build system and/or CI

Checklist

Put an x in the boxes that apply.
You can also fill these out after creating the PR.
If you're unsure about any of them, don't hesitate to ask on the mailing list.
We're here to help!
This is simply a reminder of what we are going to look for before merging your code.

  • I have read the CONTRIBUTING.md document
  • I have signed the CA (see https://cla.pivotal.io/sign/rabbitmq)
  • I have added tests that prove my fix is effective or that my feature works
  • All tests pass locally with my changes
  • If relevant, I have added necessary documentation to https://github.com/rabbitmq/rabbitmq-website
  • If relevant, I have added this change to the first version(s) in release-notes that I expect to introduce it

Fixed reading boolean values from rabbitmqadmin config file
@pivotal-cla
Copy link

@fwolfsjaeger Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

@pivotal-cla
Copy link

@fwolfsjaeger Thank you for signing the Contributor License Agreement!

@michaelklishin
Copy link
Member

Thank you for taking the time to contribute. The changes look OK. I'll try to reproduce to verify the effectiveness now.

@michaelklishin
Copy link
Member

michaelklishin commented Apr 1, 2022

I QA'ed this PR with the following:

  • Two tls-gen clones that each have generated certificates using the basic profile (in other words, two CA certificates, and client and server key-certificate pairs)
  • A RabbitMQ node with a .conf file that enables TLS for the HTTP API
  • A rabbitmqadmin.conf
# rabbitmq.conf
management.ssl.port       = 15671

management.ssl.cacertfile = /tmp/tmp-tls-gen/basic/result/ca_certificate.pem
management.ssl.certfile   = /tmp/tmp-tls-gen/basic/result/server_certificate.pem
management.ssl.keyfile    = /tmp/tmp-tls-gen/basic/result/server_key.pem

# management.ssl.versions.1 = tlsv1.3
management.ssl.versions.1 = tlsv1.2

management.ssl.verify               = verify_none
management.ssl.fail_if_no_peer_cert = false

## These are TLS 1.3 cipher suites
# management.ssl.ciphers.1  = TLS_AES_256_GCM_SHA384
# management.ssl.ciphers.2  = TLS_AES_128_GCM_SHA256
# management.ssl.ciphers.3  = TLS_CHACHA20_POLY1305_SHA256
# management.ssl.ciphers.4  = TLS_AES_128_CCM_SHA256
# management.ssl.ciphers.5  = TLS_AES_128_CCM_8_SHA256
# rabbitmqadmin.conf
[localhost_https]
hostname = my-hostname
port = 15671
username = guest
password = guest

ssl = True
ssl_key_file = /tmp/tmp-tls-gen2/basic/result/client_key.pem
ssl_cert_file = /tmp/tmp-tls-gen2/basic/result/client_certificate.pem

ssl_insecure = True

and while this PR works as expected, so does rabbitmqadmin from 3.9.14: the ssl_insecure setting is effective
when it is present. When it is commented out, peer verification fails with

ssl.SSLCertVerificationError: [SSL: CERTIFICATE_VERIFY_FAILED] certificate verify failed: self signed certificate in certificate chain (_ssl.c:1129)

since the server certificate is not in the trusted certificate chain.

rabbitmqadmin was run like this:

# RabbitMQ 3.9.14
rabbitmqadmin --config=/tmp/rabbitmqadmin.conf --node=localhost_https show overview
# rabbitmq-server#4408
./bin/rabbitmqadmin --config=/tmp/rabbitmqadmin.conf --node=localhost_https show overview

@michaelklishin
Copy link
Member

michaelklishin commented Apr 1, 2022

I'm OK with accepting this contribution since the behavior is what we expect but both trying to reproduce the scenario where the key isn't taken from rabbitmqadmin.conf and looking at the code changes make me wonder what I may be missing
about your setup @fwolfsjaeger.

@michaelklishin michaelklishin merged commit 0314035 into rabbitmq:master Apr 1, 2022
michaelklishin added a commit that referenced this pull request Apr 1, 2022
Fixed reading SSL settings from rabbitmqadmin config file (backport #4408)
michaelklishin added a commit that referenced this pull request Apr 1, 2022
Fixed reading SSL settings from rabbitmqadmin config file (backport #4408) (backport #4409)
@michaelklishin
Copy link
Member

@fwolfsjaeger since rabbitmqadmin is a standalone zero-dependency script you can use it today but if you need it in a release, it will ship in 3.9.15 and 3.10.0. Thanks again for contributing.

@fwolfsjaeger
Copy link
Contributor Author

Sorry for the late reply. Thank you for accepting the changes.

I have not set the following 2 settings in rabbitmq.conf:

management.ssl.verify               = verify_none
management.ssl.fail_if_no_peer_cert = false

However, the issue for me was the the rabbitmqadmin.conf setting ssl_insecure was not used at all. The condition if getattr(cli_options, key) is None was always false as it had the default value False that was set in line 398 of version 3.9.14.

@fwolfsjaeger fwolfsjaeger deleted the rabbitmqadmin-fix-reading-config branch April 3, 2022 10:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants