Explicit HTTP persistent connection

[chulkilee]

Background

httpc and hackney implements HTTP persistent connection by themselves - not requiring passing any reference to existing connection, but using connection by host.

This results in strange behavior, such as not honoring other options (e.g. SSL), which is very bad for security.

Related:

• Secure by default #293
• insecure option for a host is persistent in a pool benoitc/hackney#570
• https://elixirforum.com/t/14-elixirconf-eu-2019-learn-you-some-ssl-for-much-security-bram-verburg/22495

Please note that mint is process-less, so it can not provide HTTP persistent connection unless someone (e.g. Tesla) provides pool management. Once we have connection pool logic in tesla, we may update all adapters to explicitly use the reference, not relying on behind-the-scene behavior of underlying libraries.

Why not just delegating to adapter/underlying library

From @teamon 's comment

Keeping connections alive is a low-level operation which by design is offloaded to adapters. At this point, I'm not sold on the idea of introducing connection pools to tesla.

I agree that "keeping connection a live " is a low-level operation and not a main job of tesla. However, at the same time, Tesla SHOULD not hide HTTP behavior. For example, two different tesla client should not share any connection - but they do.

Example

bad_url = "https://expired.badssl.com/"

secure_client =
  Tesla.client(
    [],
    {Tesla.Adapter.Hackney,
     ssl: [verify: :verify_peer, cacertfile: :certifi.cacertfile(), reuse_sessions: false]}
  )

insecure_client = Tesla.client([], {Tesla.Adapter.Hackney, ssl_options: [verify: :verify_none]})

# as expected
{:error, _} = Tesla.get(secure_client, bad_url)
# as expected
{:ok, _} = Tesla.get(insecure_client, bad_url)

# NOT AS EXPECTED!
{:ok, _} = Tesla.get(secure_client, bad_url)

This is very bad, when you need to build client and ssl options based on user input (e.g. say, your service takes webhook URL from user).

There are several workrounds

• use pool per host and SSL config
• disable HTTP persistent connection when not validating SSL

However, still, by default Tesla and httpc / hackney is insecure 😞

[teamon]

Regardless of adapter backend (stateless or not) it would require passing some kind of client identifier, which should be configurable.

Consider this two client:

def client1(url), do: Tesla.client([{BaseUrl, url}])
def client2(token), do: Tesla.client([{BaseUrl, "https://example.com"},{Headers, [{"Auth", token}]}])

Now, in a typical phoenix web app:

def some_action(conn, _params) do
  client = client1(...) # or client2(...)
end

In case of client1 we want to have a separate one for every call to Tesla.client, in case of client2 we want to have persistent connection to https://example.com as we only change the headers in subsequent requests.

Do you have any ideas how could the be solved on the interface side?

[chulkilee]

We may store connection ref (state) in Tesla.Env or Tesla.Client. I think Tesla.Client is better place but not 100% sure. Currently Tesla.Env contains Tesla.Client... so we may not need changes on interface but additional attribute on any of them.

# adapter should build blank state
client = Tesla.client([BaseUrl, url}, {PersistentAdapter, option_a: foo}]

# adapter should pass existing ref or new state so that Tesla.Client in the result result has the ref
{:ok, %{body: body, client: client}} = Tesla.get(client, path1)

# adapter should reuse existing ref in Tesla.Client
{:ok, %{body: body, client: client}} = Tesla.get(client, path2)

We may introduce it as Tesla.Client.adapter_state to wrap any state needed by adapter, instead of adding ref or conn to the struct.

[teamon]

This would break the user-friendly API, it's also not trivial to use in the (not only) webapp context as you don't have a good place to store client for future use, and we end up with some kind of pool implementation. I still think this should be handled on adapter level.

[Exadra37]

This would break the user-friendly API,

So I am confused... are you saying that developer convenience is more important that security in the library or that security must be delegated to the adapters?