Filter sensitive data from inspect(Tesla.Client.t)

[maltoe]

Hi :wave:

We recently discovered - luckily before going to production - that we leaked secret tokens for an API integration, as we were doing something like this

{:ok, _} = 
  [{Tesla.Middlewares.BearerAuth, token: "supersecret"}]
  |> Tesla.client()
  |> Tesla.post(...)

The resulting MatchError in case of network failures etc. would include the inspected Tesla.Client struct which in turn included the supersecret token.

We thought implementing Inspect within Tesla itself and filtering some options would be a more sensible default for all users, hence this PR.

Best, malte

Todos

• [ ] Add Tesla.Middleware.BasicAuth.Options struct
• [ ] Add Tesla.Middleware.BearerAuth.Options struct
• [ ] Add Tesla.Middleware.DigestAuth.Options struct

[yordis]

that we leaked secret tokens for an API integration

Do you mind expanding on that, do you mean you leaked to some 3rd party provider?

---

I am not opposed to the idea.

The curious part is that this issue would be all over Elixir code bases, so I am not sure what is the right call here.

As far as I can tell you would modify your logger to hide sensitive information, but I may be wrong.

Gonna lean on @teamon for some feedback about it.

[teamon]

I tend to have a custom implementation of Inspect protocol on a per-project basis, often something as simple as:

defimpl Inspect, for: Tesla.Client do
  @moduledoc """
  Implement custom Inspect protocol for Tesla.Client
  to prevent leaking secrets added with Tesla.Middleware.Headers
  to AppSignal or logs.
  """

  import Inspect.Algebra

  def inspect(client, opts) do
    attrs = client |> Map.take([:adapter]) |> Map.to_list()
    concat(["#Tesla.Client<", to_doc(attrs, opts), ">"])
  end
end

The problem with implementing it directly into tesla package is that as far as I know one wouldn't be able to override it anymore.

[yordis]

A solid argument is that Ecto does the same https://github.com/elixir-ecto/ecto/blob/d52039037ae8d3ba66034c35c08fc4d56c621447/lib/ecto/changeset.ex#L3134-L3172 (from Kyle Steger)

but then as @teamon pointed out, what happens when we have other middleware that wants to accomplish the same? We can't redact anymore something?

[kyleVsteger]

A solid argument is that Ecto does the same https://github.com/elixir-ecto/ecto/blob/d52039037ae8d3ba66034c35c08fc4d56c621447/lib/ecto/changeset.ex#L3134-L3172 (from Kyle Steger)

but then as @teamon pointed out, what happens when we have other middleware that wants to accomplish the same? We can't redact anymore something?

It's possible to write another middleware, Tesla.RedactValues or something of the like that takes a list of keys to redact. It looks like Tesla.post|patch|etc accept [option], which may be an OK place to add redact_keys: [:a, :b, :c].

I think what is proposed here provides immediate benefit and prevents known secret data from being leaked in logs. Gets a thumb in my book

[maltoe]

@yordis

Do you mind expanding on that, do you mean you leaked to some 3rd party provider?

Leaked to our application monitoring as part of its exception handling.

As far as I can tell you would modify your logger to hide sensitive information, but I may be wrong.

That's unfortunately not possible in this case. Exception is raised, Appsignal inspect()s it and we can't really redact it anymore (in a sane way) in the resulting string.

---

@teamon

it directly into tesla package is that as far as I know one wouldn't be able to override it anymore

How about hiding it behind a compile-time flag? I'd assume that the vast majority of users does not have a custom implementation, so defaulting it to true would make sense, IMO. Safety first.

config :tesla, implement_inspect_protocol: false

[maltoe]

Also, as a side note: We have since discovered that we also use the KeepRequest middleware in one case, so we would also need to purge the req_headers from the opts field of Tesla.Env :angry: - We're currently debating whether we want to implement Inspect for Tesla.Env as well (and optionally upstream it, too), or whether we can get rid of KeepRequest. Implementation of Inspect for Tesla.Env feels a bit brittle in this case. What do others think?

[yordis]

Safety first.

Agree with this, but having a solution when opting out without so much rework should be taken into consideration as well.

What do others think?

What if the input is an struct instead of a keyword list, then you implement the redacting for such struct, example:

{MyMiddlewareName, %MyMiddlewareName.Config{token: "..."}}

So you implement the protocol for MyMiddlewareName.Config

Would that work?

If that strategy work, we could decide to commit to what we have, and maybe recommend people to do such a thing if they want to control redacting things.

Thoughts?

[maltoe]

Gentlemen, any update on this? I'm happy to rework it in any way you conceive, but I do believe the status quo is an unnecessary pitfall / security risk for uncautious people (like me).

[yordis]

@maltoe sorry it took me this long to get back to this issue, but I have other duties.

From my perspective, the tricky situation with this PR is that we are taking over control of the Inspect for the entire Tesla.Client.

From the library perspective,

How do we ensure that this implementation of the Inspect protocol will work for all middleware (including the ones we did not write)?

That may not be a concern, but I am unsure.

This leads us to two possible solutions,

1. In your application code, you define the Inspect protocol (less ideal)
2. Use a struct as configuration (see my changes)

IMPORTANT

I pushed some changes showcasing the usage of Tesla.Middleware.BasicAuth.Options, which I made the rookie mistake of force-push because I rebased (my bad).

If you wish to continue helping in the subject, we must do the same for Tesla.Middleware.BearerAuth and Tesla.Middleware.DigestAuth thus far.

Let me know if you can help with the task. Otherwise, I can take over.

[maltoe]

@yordis

sorry it took me this long to get back to this issue, but I have other duties.

Sorry if my last comment came across too pushy. I'm sure you have and thank you for your work on tesla, it's an awesome tool :hammer_and_wrench: :purple_heart: !

From my perspective, the tricky situation with this PR is that we are taking over control of the Inspect for the entire Tesla.Client. From the library perspective, how do we ensure that this implementation of the Inspect protocol will work for all middleware (including the ones we did not write)?

My 2 cents: I'm wondering whether we're "overthinking" it - the original code in this PR was my attempt at fixing the "leak" while leaving some useful information in the inspect output. We have since refined it in our codebase and even expanded it to Tesla.Env (req_headers in case KeepRequest is used). But I'm more sceptical about its inclusion in Tesla itself now, as it has become a non-negligible amount of code, still doesn't cover all cases (e.g. third-party middlewares as you mentioned), and especially as I can't remember ever having needed the information in the inspect output at all.

Hence, how about going back to what @teamon originally said in https://github.com/elixir-tesla/tesla/pull/534#issuecomment-1164551535 and providing an extremely simplistic default implementation (e.g. hiding all middlewares including the internal ones) with an opt-out? That way you prevent leaks even from external middlewares, protect the innocents, and people don't have to rework much at all when they opt-out :grimacing:

Otherwise, I also like your struct approach (though req_headers in Tesla.Env needs to be considered as well). Unfortunately, I don't know how much time I'll be given to work on this any time soon, so can't promise much.

Thanks again for considering & best wishes.