-
Notifications
You must be signed in to change notification settings - Fork 839
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
Update the client and server configuration examples for TLS communication #1030
Conversation
- ``ssl.PROTOCOL_TLSv1`` is now deprecated (https://docs.python.org/3/library/ssl.html#ssl.PROTOCOL_TLSv1), ``ssl.PROTOCOL_TLS`` should be preferred, which is used by ``ssl.create_default_context`` among other other parameters which provide a higher security level (https://docs.python.org/3/library/ssl.html#ssl.create_default_context); - checking server hostnames prevents man in the middle attacks (pika#464); - RabbitMQ 3.7.0 introduced a new systemctl syntax for rabbitmq.conf (https://www.rabbitmq.com/configure.html#config-file).
cafile="/Users/me/tls-gen/basic/testca/cacert.pem") | ||
context.load_cert_chain("/Users/me/tls-gen/basic/client/cert.pem", | ||
"/Users/me/tls-gen/basic/client/key.pem") | ||
server_hostname = "example.com" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A single-use variable containing a simple value is unnecessary. Use 'example.com'
directly in the pika.SSLOptions()
instantiation below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, thanks for the review. Done.
|
||
rabbitmq.config:: | ||
|
||
%% Both the client and rabbitmq server were running on the same machine, a MacBookPro laptop. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lukebakken or @michaelklishin : need you to review the rabbitmq sample config change here.
cafile="/Users/me/tls-gen/basic/testca/cacert.pem") | ||
context.load_cert_chain("/Users/me/tls-gen/basic/client/cert.pem", | ||
"/Users/me/tls-gen/basic/client/key.pem") | ||
server_hostname = "example.com" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please apply the suggested changes in this function as well:
pika/tests/acceptance/async_test_base.py
Line 120 in 9fb234c
def _new_tls_connection_params(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
] | ||
} | ||
]. | ||
# Enable A.M.Q.P.S. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please make sure these are consistent with https://github.com/pika/pika/blob/master/testdata/rabbitmq.conf.in
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean the paths of the certificates and private keys (I had kept the ones used in the old version of the example)? Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@maggyero, sorry, I should have been more specific - I meant the properties being set, not the values (e.g., file paths) themselves. Thank you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To me the other properties in the suggested rabbitmq.conf.in (listeners.tcp.default = 5672
, num_acceptors.tcp = 10
, num_acceptors.ssl = 10
, reverse_dns_lookups = false
, loopback_users.guest = true
, log.console = false
, log.console.level = debug
) are distracting for the reader who would expect only the necessary parameters for setting up TLS in an example like this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @maggyero.
See also issue #1035. |
ssl_options.verify = verify_peer | ||
ssl_options.fail_if_no_peer_cert = true | ||
|
||
# Enable H.T.T.P.S. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we change this to just "HTTPS" (this is not a music album cover ;)) and add a comment that says "See http://www.rabbitmq.com/ssl.html to learn more"? The rest looks reasonable to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From Wikipedia:
In English, abbreviations have traditionally been written with a full stop/period/point in place of the deleted part to show the ellipsis of letters—although the colon and apostrophe have also had this role—and with a space after full stops (e.g. "A. D."). In the case of most acronyms, each letter is an abbreviation of a separate word and, in theory, should get its own termination mark. Such punctuation is diminishing with the belief that the presence of all-capital letters is sufficient to indicate that the word is an abbreviation.
Some influential style guides, such as that of the BBC, no longer require punctuation to show ellipsis; some even proscribe it. Larry Trask, American author of The Penguin Guide to Punctuation, states categorically that, in British English, "this tiresome and unnecessary practice is now obsolete".
Nevertheless, some influential style guides, many of them American, still require periods in certain instances. For example, The New York Times' guide recommends following each segment with a period when the letters are pronounced individually, as in K.G.B., but not when pronounced as a word, as in NATO. The logic of this style is that the pronunciation is reflected graphically by the punctuation scheme.
But since periods are disappearing, I’ve just removed them (just note that they’re not wrong). Thanks for the review. Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add a comment that says "See http://www.rabbitmq.com/ssl.html to learn more"
It’s actually already there at the beginning of the example:
See https://www.rabbitmq.com/ssl.html for certificate generation and RabbitMQ TLS configuration.
Thank you! |
ssl.PROTOCOL_TLSv1
is now deprecated (https://docs.python.org/3/library/ssl.html#ssl.PROTOCOL_TLSv1),ssl.PROTOCOL_TLS
should be preferred, which is used byssl.create_default_context
among other parameters which provide a higher security level (https://docs.python.org/3/library/ssl.html#ssl.create_default_context).rabbitmq-management
plugin might as well be demonstrated.