SSLTransport error: unexpected keyword argument 'ca_certs'

[Cameron-Calpin]

Using amqp version 5.0.1, I discovered an SSLTransport error. When I use amqp version 5.0.0b1, SSL works just fine.

Here's the error I get when using amqp==5.0.1:

`/usr/local/lib/python3.8/dist-packages/kombu/connection.py:283: in channel
chan = self.transport.create_channel(self.connection)
/usr/local/lib/python3.8/dist-packages/kombu/connection.py:858: in connection
return self._ensure_connection(
/usr/local/lib/python3.8/dist-packages/kombu/connection.py:435: in _ensure_connection
return retry_over_time(
/usr/local/lib/python3.8/dist-packages/kombu/utils/functional.py:325: in retry_over_time
return fun(*args, **kwargs)
/usr/local/lib/python3.8/dist-packages/kombu/connection.py:866: in _connection_factory
self._connection = self._establish_connection()
/usr/local/lib/python3.8/dist-packages/kombu/connection.py:801: in _establish_connection
conn = self.transport.establish_connection()
/usr/local/lib/python3.8/dist-packages/kombu/transport/pyamqp.py:128: in establish_connection
conn.connect()
/usr/local/lib/python3.8/dist-packages/amqp/connection.py:314: in connect
self.transport.connect()
/usr/local/lib/python3.8/dist-packages/amqp/transport.py:77: in connect
self._init_socket(
/usr/local/lib/python3.8/dist-packages/amqp/transport.py:188: in _init_socket
self._setup_transport()
/usr/local/lib/python3.8/dist-packages/amqp/transport.py:323: in _setup_transport
self.sock = self._wrap_socket(self.sock, **self.sslopts)

---

self = <amqp.transport.SSLTransport object at 0x7f8a0083a370>
sock = <socket.socket fd=11, family=AddressFamily.AF_INET, type=SocketKind.SOCK_STREAM, proto=6, laddr=('127.0.0.1', 41008), raddr=('127.0.0.1', 5671)>
context = None
sslopts = {'ca_certs': '/home/cameron/Defense/tls-gen/basic/result/ca_certificate.pem', 'cert_reqs': <VerifyMode.CERT_REQUIRED: ...e/tls-gen/basic/result/client_certificate.pem', 'keyfile': '/home/cameron/Defense/tls-gen/basic/result/client_key.pem'}

def _wrap_socket(self, sock, context=None, **sslopts):
    if context:
        return self._wrap_context(sock, sslopts, **context)

  return self._wrap_socket_sni(sock, **sslopts)

E TypeError: _wrap_socket_sni() got an unexpected keyword argument 'ca_certs'

/usr/local/lib/python3.8/dist-packages/amqp/transport.py:330: TypeError
----------------------------------------------------------- Captured stdout call -----------------------------------------------------------
2020-10-28 16:46:54: INFO: Consumer successfully connected to kombu server at localhost:5671
2020-10-28 16:46:54: ERROR: Failed to initialize rabbit consumer connection:
Traceback (most recent call last):
TypeError: _wrap_socket_sni() got an unexpected keyword argument 'ca_certs'`

Looking at py-amqp/amqp/transport.py, the parameters for the method _wrap_socket_sni for amqp==5.0.1, it doesn't include ca_certs. On amqp==5.0.0b1, the method _wrap_socket_snidoes includeca_certs` as a parameter. Is this a bug or was this left out by accident?

Here are the links to amqp version 5.0.1 and 5.0.0b1 of py-amqp/amqp/transport.py with the line numbers highlighting the method:
amqp==5.0.1:

py-amqp/amqp/transport.py

Line 337 in 93e4f3a

def _wrap_socket_sni(self, sock, keyfile=None, certfile=None,

amqp==5.0.0b1:

py-amqp/amqp/transport.py

Line 339 in c5fe7da

ca_certs=None, do_handshake_on_connect=False,

[thedrow]

It was removed as part of #327 since _wrap_socket_sni() no longer needs it (unless we were wrong and I believe we weren't).

It seems like we lack proper coverage in this branch as this should have been caught.

Do you have a suggestion or should I investigate myself?

[Cameron-Calpin]

My suggestion is to restore the missing fields if possible.

Just need the ca_cert filed to be put back into amqp/transport.py#339 (the _wrap_socket_sni method) that was removed in commit ad9697ab2eaa97e153211c82e4bad8d655e63591

This will allow it to be compatible with Python's SSLContext.load_verify_locations(cafile=None, capath=None, cadata=None)

I also suggest adding back the ciphers field as I believe users should have the option to specify what ciphers are allowed and allow support for Python's SSLContext.set_ciphers(ciphers) that replaces the deprecated ssl.wrap_socket

[thedrow]

If you can contribute a fix, that would be most welcome.
I'm currently busy with the Celery 5.0.2 release which contains very urgent fixes and after that, I have to take care of the Django integrations.

[thedrow]

@4383 Can you please assist?

[4383]

@thedrow sure I'll take a look

[4383]

@Cameron-Calpin I think we shouldn't restore this param, it correspond to a param consumed by a deprecated method that
will be removed from CPython soon [1]. The removing of this param is legit.

We recently moved [2] to the recommended function to use in replacement [3] of the deprecated method, and this param no longer exist in the function's signature.

I suppose you need to update your code to adapt you to these changes.

This does raise questions though:

• Should we have to release a major version (6.0.0) to highlight these breaking changes that are not backward compat? I guess we could say probably yes.
• Should we have to deprecate it first and lets the both functions available in parallel? I guess we could say yes too.

Might some libs like debtcollector could help us to manage future similar patches where changes are not backward compatible. This could help us to manage smooth migrations/transitions.

Thoughts?

[1] https://docs.python.org/3/library/ssl.html#ssl.wrap_socket
[2] #327
[3] https://docs.python.org/3/library/ssl.html#ssl.SSLContext.wrap_socket

[matusvalo]

ssl.wrap_socket() internally passes cacerts parameter to SSLContext.load_verify_locations():

https://github.com/python/cpython/blob/7c01f1540f958d4f52188b28afca721a9a6925c3/Lib/ssl.py#L1400

I am not ssl expert but, does it make sence to reintroduce the cacert parameter and call SSLContext.load_verify_locations() directly from _wrap_socket_sni()?

[4383]

@matusvalo good point, I think that yes it could be feasible. ca_certs could be set to None by default and then a call to SSLContext.load_verify_locations could be made if its value is not None, or something like that.

[4383]

@moisesguimaraes any opinions about @matusvalo's suggestion?

[matusvalo]

I did more investigation and found out that SSLContext.load_verify_locations() checks remote server (in our case RMQ broker) against CA certificate installed on client. Hence if you are not using this call you are encrypting the trafic between client and server but you are not verifying identity of server (to avoid man in the middle attack). So I suppose argument ca_certs should be reintroduced.

[matusvalo]

I have created PR fixing this issue. @Cameron-Calpin , @4383 could you review this PR?

[moisesguimaraes]

@matusvalo is correct about the MITM possibility, The fix described by @Cameron-Calpin and implemented in #344 should fix this security hole.

[4383]

@moisesguimaraes thanks