Add match_fun clause to deal with ip addresses in TLS handshake

[mruoss]

Hey there

I think there's a bug in the OTP so I've opened an issue there: erlang/otp#7968. I'm not sure they see it as a bug and how long it may take to fix this. Maybe it is not a bug for Erlang after all but it sure is for the Elixir world. And Mint is one place where it could be fixed. There's also a discussion on the Elixir Forum about this. But let me quickly state the problem here, too.

Problem

The problem occurs when trying connect to an IP address over SSL where the server offers a certificate with its IP address in the "subject alternate names" extension. This is often the case when connecting to a Kubernetes API Server from within Kubernetes (hence the discussion on the flame_k8s_backend thread).

The TLS handshake should succeed. However, due to a mismatch in types (Erlang expects IP addresses to be in the form {127, 0, 0, 1}, Mint sends ~c"127.0.0.1"), We get a {:bad_cert, :hostname_check_failed}.

How to reproduce

You can generate a rootCA and a certificate using openssl (don't forget to add the IP: -addext "subjectAltName = IP:127.0.0.1") and run openssl s_server -accept 4443 -cert serverCert.pem -key serverKey.pem -CAfile rootCACert.pem -WWW.

Then the following code shows the problem:

ca_cert =
  "/path/to/rootCACert.pem"
  |> File.read!()
  |> :public_key.pem_decode()
  |> Enum.find_value(fn
    {:Certificate, data, _} -> data
    _ -> raise "Certificate data is missing"
  end)

Mint.HTTP.connect(:https, "127.0.0.1", 4443, transport_opts: [cacerts: [ca_cert]])

Solving the Problem

I guess there's more than one way to deal with this.

1. This PR (using a match_fun to validate the presented address)
2. Convert IP addresses to t:inet:ip_address() when passing them to :ssl.connect()
3. Let Erlang deal with the problem
4. Let the user or higher level libs deal with the problem

All are valid options. But I feel like this is a problem that needs to be fixed in one of the lower abstraction levels because it leads users to set verify: :verify_none where it's not needed.
In any case, mostly I wanted to let you guys know about the problem. Feel free to reject the PR if you find it should be fixed elsewhere. Or merge it to fix it here until it's fixed in the OTP...

[whatyouhide]

First of all, thank you so much for all the lovely context and the PR @mruoss 💟

I’m ok with the solution here. I also like 2., I don't really have a preference.

@voltone, I invoke your wisdom. Is the change in this PR a good idea, or should we go with converting IP-looking hostnames ("127.0.0.1" and friends) to Erlang-style IPs ({127, 0, 0, 1})?

[mruoss]

Unless we want to fix this for older OTP versions, we might also sit still for a bit as there's already a PR that seems to fix the problem in :ssl: erlang/otp#7974

[mruoss]

The fix was now merged to the OTP maint branch. If I understand correctly, this means it's going to be fixed in 26.3. Which means the changes in this PR are only required if we want to fix the issue for OTP < 26.3

[whatyouhide]

@mruoss I’m ok to fix this here for now, but can we add a test and a TODO comment that says to remove this once we depend on OTP 27+? 🙃

[mruoss]

Sure, will do.

[mruoss]

I'm a bit confused by the tests that were introduced here: #29 which seem to test that... somehow...

But I guess I'm just gonna add new tests.

[mruoss]

@whatyouhide This works. I have verified that the test actually fails on main ;)

[coveralls]

Pull Request Test Coverage Report for Build 5dd73bfeec9a709850df463999b8c7753aa0e3f5-PR-418

• 0 of 0 changed or added relevant lines in 0 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.02%) to 87.596%

---

Totals
Change from base Build 7bb9ee7b6cbda479bab0031ddacff2cb515156a2:	0.02%
Covered Lines:	1250
Relevant Lines:	1427

---

💛 - Coveralls

[whatyouhide]

Thank you @mruoss! 💟