search

elixir-mint / mint

Functional HTTP client for Elixir with support for HTTP/1 and HTTP/2 🌱
Apache License 2.0
1.36k stars 112 forks source link

Implement cipher filtering based on TLS version and available crypto lib #317

Closed lukebakken closed 3 years ago

lukebakken commented 3 years ago

Fixes #316

lukebakken commented 3 years ago

@ericmj this is ready for review. Thanks!

ericmj commented 3 years ago

@voltone Can you take a look at this? This is the background: https://github.com/elixir-mint/mint/pull/314#discussion_r619790588, seems like we have to provide default ciphers to be compatible with TLS 1.3 out of the box.

lukebakken commented 3 years ago

we have to provide default ciphers to be compatible with TLS 1.3 out of the box

It's not just version 1.3 either. I used to work on the RabbitMQ team ... you would not believe all the time I spent with users and customers just to get TLS going for them. I'm impressed that this library strives to "do the right thing" by default.

At any rate, if a user does not specify ciphers themselves, and the OTP version supports it, the code in this PR enables all ciphers that are for the specified TLS versions and are supported by the linked crypto lib.

lukebakken commented 3 years ago

@ericmj @voltone @whatyouhide @sneako - I resolved conflicts now that #313 is merged. Thanks in advance for the reviews.

lukebakken commented 3 years ago

I would personally skip the renaming of module attributes, functions and variables with ssl in their name

Will do, that makes sense.

lukebakken commented 3 years ago

@voltone all set for re-review, thanks.

lukebakken commented 3 years ago

@whatyouhide @voltone -

we can fix that after merging the PR, no worries. Great catch :)

Just so you know, you can push changes to branches that are associated with a pull request. Just add my fork as a git remote and push away. I used to work on RabbitMQ and would usually make changes directly to contributions to skip some of the back-and-forth that happens during a PR review.