wojtekmach/req

Redirect behavior is inconvenient with transport_opts overrides

liamwhite opened this issue · 9 comments

liamwhite commented

Currently when I make a request through Req, I have a setup where I parse the URL scheme in order to determine whether I need to apply a sha1withrsa override (see relevant otp report here for why I want to do that):

%{scheme: "https"} ->  
  [
    transport_opts: [
      customize_hostname_check: [
        match_fun: :public_key.pkix_verify_hostname_match_fun(:https)
      ],
      signature_algs_cert: :ssl.signature_algs(:default, :"tlsv1.3") ++ [sha: :rsa]
    ]
  ]

_ ->
  []

This is because the transport_opts passed to Mint are directly passed to gen_tcp or ssl, depending on the scheme.
When attempting to fetch a https URL that redirects to http (I created one on a link shortener that redirects to example.com for testing) with these transport_opts, the following error occurs:

iex(4)> get("https://t.ly/E8MCX")
[debug] redirecting to http://example.com
** (exit) :badarg
    (finch 0.18.0) lib/finch/http1/pool.ex:96: Finch.HTTP1.Pool.request/6
    (finch 0.18.0) lib/finch.ex:423: anonymous fn/6 in Finch.stream_while/5
    (telemetry 1.2.1) telemetry/src/telemetry.erl:321: :telemetry.span/3
    (req 0.5.0) lib/req/steps.ex:823: Req.Steps.finch_stream_into_fun/5
    (req 0.5.0) lib/req/request.ex:1007: Req.Request.run_request/1
    (req 0.5.0) lib/req/steps.ex:2047: Req.Steps.redirect/1
    (req 0.5.0) lib/req/request.ex:1024: anonymous fn/2 in Req.Request.run_response/2
    (elixir 1.17.0) lib/enum.ex:4858: Enumerable.List.reduce/3
    (elixir 1.17.0) lib/enum.ex:2585: Enum.reduce_while/3
    (req 0.5.0) lib/req/request.ex:952: Req.Request.run/1
    iex:4: (file)

A similar but more limited error occurs when a link to an insecure website redirects to a link to a secure website with a sha1 root or intermediate certificate in the chain. The absence of the override in transport_opts causes the https request to then fail due to the ssl module rejecting the connection.

What should req do about this

It seems unreasonable to modify the redirect step to try to handle this. Instead, it would be nice to have more configurability about the transport_opts passed on a per-scheme basis, rather than globally. This would also eliminate my need to parse the scheme to determine which transport_opts to apply.

Alternatively, if OTP 28 fixes ssl being inoperable by default on a large number of hosts, this issue may no longer be relevant.

wojtekmach commented

Instead, it would be nice to have more configurability about the transport_opts passed on a per-scheme basis, rather than globally

right, I believe you should be able to write a request step that does exactly that. It’s ok for steps to change options. Let me know how that goes!

liamwhite commented

I am not sure I can make this interact correctly with the redirect step unless I reimplement the entire redirect step:

  @spec get(url()) :: result()
  def get(url) do
    request(:get, url)
  end

  @spec request(atom(), url(), iodata()) :: result()
  def request(method, url, body \\ []) do
    Req.new(method: method, url: url, body: body, max_redirects: 1)
    |> Req.Request.append_request_steps(connect: &inject_connect_options/1)
    |> Req.request()
  end

  defp inject_connect_options(request) do
    transport_opts =
      case request.url do
        %{scheme: "https"} ->
          [
            transport_opts: [
              customize_hostname_check: [
                match_fun: :public_key.pkix_verify_hostname_match_fun(:https)
              ],
              signature_algs_cert: :ssl.signature_algs(:default, :"tlsv1.3") ++ [sha: :rsa]
            ]
          ]

        _ ->
          []
      end

    Req.Request.merge_options(request, connect_options: transport_opts)
  end
iex(5)> get("https://t.ly/E8MCX")
[debug] redirecting to http://example.com
** (exit) :badarg
    (finch 0.18.0) lib/finch/http1/pool.ex:96: Finch.HTTP1.Pool.request/6
    (finch 0.18.0) lib/finch.ex:472: anonymous fn/4 in Finch.request/3
    (telemetry 1.2.1) telemetry/src/telemetry.erl:321: :telemetry.span/3
    (req 0.5.0) lib/req/steps.ex:953: Req.Steps.run_finch_request/3
    (req 0.5.0) lib/req/steps.ex:785: Req.Steps.run_finch/4
    (req 0.5.0) lib/req/request.ex:1007: Req.Request.run_request/1
    (req 0.5.0) lib/req/steps.ex:2047: Req.Steps.redirect/1
    (req 0.5.0) lib/req/request.ex:1024: anonymous fn/2 in Req.Request.run_response/2
    (elixir 1.17.0) lib/enum.ex:4858: Enumerable.List.reduce/3
    (elixir 1.17.0) lib/enum.ex:2585: Enum.reduce_while/3
    (req 0.5.0) lib/req/request.ex:952: Req.Request.run/1
    iex:5: (file)
wojtekmach commented

Do you know where :badarg is coming from?

btw I believe your :customize_hostname_check value is the default so I don’t think it needs to be set explicitly.

wojtekmach commented

Btw can you update to Req 0.5.1 just in case? There was a bug related to picking http1/2 pool though I don’t think it is relevant here.

liamwhite commented

The badarg is coming from here https://github.com/elixir-mint/mint/blob/main/lib/mint/core/transport/tcp.ex#L33

iex(5)> :gen_tcp.connect(~c"example.com", 80, [signature_algs_cert: :ssl.signature_algs(:default, :"tlsv1.3") ++ [sha: :rsa]])
** (exit) :badarg
    (kernel 10.0) gen_tcp.erl:578: :gen_tcp.connect/4
    iex:5: (file)

Updating req didn't have any effect, btw

wojtekmach commented

Oh because it’s using ssl options on :gen_tcp, right?

Perhaps it’s a bug in starting custom finch pool (per custom :connect_options)

could you create one finch pool for http and another for https (with those transport opts) and pick them in your custom step?

liamwhite commented 
      {Finch, name: HttpFinch},
      {Finch,
       name: HttpsFinch,
       pools: %{
         default: [
           conn_opts: [
             transport_opts: [
               customize_hostname_check: [
                 match_fun: :public_key.pkix_verify_hostname_match_fun(:https)
               ],
               signature_algs_cert: :ssl.signature_algs(:default, :"tlsv1.3") ++ [sha: :rsa]
             ]
           ]
         ]
       }},
  defp inject_connect_options(request) do
    connect_options =
      case request.url do
        %{scheme: "https"} ->
          [finch: HttpsFinch]

        _ ->
          [finch: HttpFinch]
      end

    Req.Request.merge_options(request, connect_options)
  end
iex(1)> get("https://t.ly/E8MCX")
[debug] redirecting to http://example.com
** (exit) :badarg
    (finch 0.18.0) lib/finch/http1/pool.ex:96: Finch.HTTP1.Pool.request/6
    (finch 0.18.0) lib/finch.ex:472: anonymous fn/4 in Finch.request/3
    (telemetry 1.2.1) telemetry/src/telemetry.erl:321: :telemetry.span/3
    (req 0.5.1) lib/req/steps.ex:977: Req.Steps.run_finch_request/3
    (req 0.5.1) lib/req/steps.ex:809: Req.Steps.run_finch/4
    (req 0.5.1) lib/req/request.ex:1101: Req.Request.run_request/1
    (req 0.5.1) lib/req/steps.ex:2073: Req.Steps.redirect/1
    (req 0.5.1) lib/req/request.ex:1118: anonymous fn/2 in Req.Request.run_response/2
    (elixir 1.17.0) lib/enum.ex:4858: Enumerable.List.reduce/3
    (elixir 1.17.0) lib/enum.ex:2585: Enum.reduce_while/3
    (req 0.5.1) lib/req/request.ex:1045: Req.Request.run/1
    iex:1: (file)
wojtekmach commented

Thank you for all the help. I was able to reproduce this with a test: https://github.com/wojtekmach/req/compare/wm-ssl#diff-dc476e4c9226d38862002c18df4835a1d51d98448df84548cd4e144fae0ec2e9

The thing is the custom step was only applied on the first (to https://) request and not on the subsequent redirected request (to http://) so we never chose the proper pool.

I can see how sometimes we may not necessarily want to re-apply request steps on redirects/retries (when steps are not idempotent) but for this one we definitely would have wanted that. I'll look more into this. Thanks again.

liamwhite commented

The original OTP behavior that necessitated overriding transport_opts for my use has been fixed as part of the solution to erlang/otp#8588. I will leave this issue open since I feel it still is a limitation of Req.