elixir-tesla / tesla

The flexible HTTP client library for Elixir, with support for middleware and multiple adapters.
MIT License
2.01k stars 348 forks source link

TLS httpc error: no relevant CRLs #700

Closed g-andrade closed 4 weeks ago

g-andrade commented 3 months ago

Hi,

Following #626 and the release of 1.12.0, I started getting the following error when talking to a particular endpoint:

 [notice] TLS :client: In state :wait_cert_cr at ssl_handshake.erl:2129 generated CLIENT ALERT: Fatal - Bad Certificate
 - {:bad_crls, :no_relevant_crls}

It comes down to the recently enabled CRL check: https://github.com/elixir-tesla/tesla/blob/9ec383835c1a8de02881f702f500fa12e7331a0c/lib/tesla/adapter/httpc.ex#L41-L42

From my understanding of the docs, the local ssl_crl_cache cache will be empty unless I provide it with some database, which "can be set up in many different ways".

If I read it right and this database is not ensured to be present, should the CRL check be enabled?

Relevant info: Elixir 1.16.0 / OTP 26.2.1 on GNU/Linux.

g-andrade commented 3 months ago

After some reading, this looks like a more general problem beyond Tesla. The CRL is downloaded & unpacked but it fails elsewhere. And I'm seeing the error pop up even for well known domains (for ex Google's).

I wonder if setting crl_check to best_effort would be a reasonable tradeoff.

lessless commented 3 months ago

We experienced a partial service outage because of this today - our service weren't able to communicate with the outer world.


root@48e2957c30e928:/app# curl https://<obfuscated>.fly.dev/warnings
{"detail":"Method Not Allowed"}

iex(<obfuscated>@fdaa:0:6924:a7b:18:6ddb:39cd:2)3> Tesla.get("https://<obfuscated>.fly.dev/warnings")
{:error, :econnrefused}

iex(<obfuscated>@fdaa:0:6924:a7b:18:6ddb:39cd:2)1> Req.get("https://<obfuscated>.fly.dev/warnings")
{:ok,
 %Req.Response{
   status: 405,
   headers: %{
     "allow" => ["POST"],
     "content-type" => ["application/json"],
     "date" => ["Mon, 05 Aug 2024 16:40:08 GMT"],
     "fly-request-id" => ["01J4HPK6RQ2RSY1ST7KAPVPBMY-lhr"],
     "server" => ["Fly/9fe23f3e1 (2024-07-31)"],
     "transfer-encoding" => ["chunked"],
     "via" => ["1.1 fly.io"]
   },
   body: %{"detail" => "Method Not Allowed"},
   trailers: %{},
   private: %{}
 }}

Reverting to Tesla {:tesla, "1.11.0"} fixed the issue.

@teamon I wonder how this issue cropped in and how are you planning to prevent this from happening in the future?

yordis commented 3 months ago

I am deprecating the version at the moment,

@lessless, I wonder, did you check the release notes under https://github.com/elixir-tesla/tesla/releases/tag/v1.12.0?

Screenshot 2024-08-05 at 1 15 51 PM

lessless commented 3 months ago

I am deprecating the version at the moment,

@lessless, I wonder, did you check the release notes under https://github.com/elixir-tesla/tesla/releases/tag/v1.12.0?

Screenshot 2024-08-05 at 1 15 51 PM

I didn't. That's on me.

But even if I checked it, it wouldn't raise any red flags for me. We already communicate with all third-party services through encrypted channels, so I don't think changing the default flag would make any difference.

yordis commented 3 months ago

I didn't. That's on me.

@lessless, I intend to focus on whatever caused you to miss the information; if there is anything I could do to mitigate the problem, please let me know.

so I don't think changing the default flag would make any difference.

That is the tricky situation with defaults generally; let me rethink the situation. Sorry for the inconvenience.

I rollback the changes and released a new version;

@teamon, is there any way I could tag 1.12.0 as deprecated in Hex? I do not have access to that.

yordis commented 3 months ago

Some people blame Tesla for the lack of proper security at the :httpc level, and if we try to fix it, there are plenty of folks who already built on top of some assumptions around :httpc

So what do I do? Damn, if I do, damn if I dont.

Those complaining about httpc not being secure by default can always pass their own ssl configuration or user another provider.

So I am a bit frustrated,

I feel this should be a httpc decision to make, not a middleware pipelining tool like Tesla ... this should be something that OTP itself fixes IMHO

@teamon I would say, document about httpc and SSL, and people opt-in; looking back, as much as a proper note of "Important" makes sense, still, I do not know what I do not know in terms of the assumptions make out there.

Chasing a technically correct situation at the cost of stability isn't a good idea.

yordis commented 3 months ago

@lessless by the way, I never used httpc with Tesla since Mint exists (which is the same client for Finch and therefore for Req); I, personally, would strongly recommend changing it if you can. Simply because of the maturity of httpc, nothing related to Tesla per se.

g-andrade commented 3 months ago

Some people blame Tesla for the lack of proper security at the :httpc level, and if we try to fix it, there are plenty of folks who already built on top of some assumptions around :httpc

So what do I do? Damn, if I do, damn if I dont.

Those complaining about httpc not being secure by default can always pass their own ssl configuration or user another provider.

So I am a bit frustrated

I hear you. I’m a bit of a security freak myself and 1.12.0 was, in my view, a bonafide effort at improving it. Tesla is an awesome project 💪

I don’t yet myself understand where the error comes from. I spent a few hours getting to know more about CRLs and how OTP deals with them. I may open an issue in its repo if I find it reasonable (i.e. when I’m sure I’m not misunderstanding how it’s supposed to work).

g-andrade commented 3 months ago

@IngelaAndin if you have the time, would be thankful for your thoughts on this

yordis commented 3 months ago

@g-andrade, if you have any recommendations, please send them over to https://github.com/elixir-tesla/tesla/pull/703

I would appreciate any support in the topic

IngelaAndin commented 3 months ago

I commented the issue mentioned above, hope it helps.