use the secure and non-deprecated way of creating TLS sockets

[carlhoerberg]

And enable SNI by default, which is required for AMQP servers behind a TLS terminator/LB

https://docs.python.org/3/library/ssl.html#socket-creation

[carlhoerberg]

This also enables support for TLS 1.3, but disables the ability to choose a lower TLS version than what both the server and the client supports (is there a use case for that?)

[eandersson]

Thanks! Looks good. The only thing I am on the fence off is the hostname validation. I think that ideally should be optional.

Also, not sure why the tests aren't being integrated with the PR. https://travis-ci.org/github/eandersson/amqpstorm/jobs/669404108?utm_medium=notification&utm_source=github_status

btw this is how you use the slightly more modern (and safer) ssl.create_default_context. https://github.com/eandersson/amqpstorm/blob/master/examples/ssl_with_context.py

[carlhoerberg]

Yes but ssl.create_default_context doesn't exists in 2.7, that's why I didn't use it.

[carlhoerberg]

Imho hostname validation should be on by default but be possible to disable. Not doing hostname validation is the same as not doing any encryption at all as you don't know who you're talking to.

[carlhoerberg]

Will take a look at the failing tests

[eandersson]

Imho hostname validation should be on by default but be possible to disable. Not doing hostname validation is the same as not doing any encryption at all as you don't know who you're talking to.

Yea I agree with you, but it would be a breaking change. I would prefer to make such a change in a major release.

[carlhoerberg]

Ok, will amend that

[carlhoerberg]

The only failing test now is AMQPConnectionError: Could not connect to rmq.eandersson.net:5671 error: [SSL: CERTIFICATE_VERIFY_FAILED] certificate verify failed (_ssl.c:726) https://travis-ci.org/github/eandersson/amqpstorm/jobs/669861319

And i guess it's because the certificate is self signed. Would you like to disable all checks (valid cert chain, within issue dates etc), not just the name check?

[eandersson]

How about just exposing all of them when creating the Connection?

[carlhoerberg]

I'm not following? Now workring on exposing verify_mode as an option in ssl_options, that defaults to verify_none.

[codecov-io]

Codecov Report

Merging #79 into master will decrease coverage by 0.19%. The diff coverage is 80.00%.

@@             Coverage Diff             @@
##            master      #79      +/-   ##
===========================================
- Coverage   100.00%   99.80%   -0.20%     
===========================================
  Files           20       20              
  Lines         1000     1007       +7     
  Branches       117      117              
===========================================
+ Hits          1000     1005       +5     
- Misses           0        1       +1     
- Partials         0        1       +1     

Impacted Files	Coverage Δ
amqpstorm/io.py	98.83% <80.00%> (-1.17%)	:arrow_down:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update a3bfb58...eb143da. Read the comment docs.

[eandersson]

Looks good. Maybe a minor concern, but we defaulted to TLS 1.2 before. Is there a situation where it willcould default to 1.1 instead now?

[carlhoerberg]

No, this defaults to TLS 1.3

On Wed, 1 Apr 2020, 23:51 Erik Olof Gunnar Andersson, < notifications@github.com> wrote:

@eandersson approved this pull request.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/eandersson/amqpstorm/pull/79#pullrequestreview-385974951, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABL6TRVJTRUY5DRJCEFXGTRKOZMZANCNFSM4LX6KTWQ .

[eandersson]

Awesome. Thanks!