Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dropping hackney dependency #123

Open
wojtekmach opened this issue Jul 24, 2021 · 10 comments
Open

Dropping hackney dependency #123

wojtekmach opened this issue Jul 24, 2021 · 10 comments

Comments

@wojtekmach
Copy link
Contributor

wojtekmach commented Jul 24, 2021

We need Hackney to make sure that tzdata we're downloading and unpacking has not been tampered with. However, hackney brings many transient dependencies and for that reason I believe it is not the best choice, it would be great to depend on as least deps as possible.

I think we have two options:

Option 1: httpc + castore. EEF Security WG has a nice guide how to give httpc secure settings. I think this will be pretty simple to implement and already be a big improvement.

Option 2: httpc + manually checking tzdata signature.

This would be similar to how mix local.hex works, it fetches https://repo.hex.pm/installs/hex-1.x.csv and https://repo.hex.pm/installs/hex-1.x.csv.signed and checks if it wasn't tampered with against Hex public key that is included in Elixir.

Turns out tzdata releases are signed with GPG using Paul Eggert's public key. If we include the key in tzdata and verify against that, we wouldn't even need castore which I think would be a small win.

I have a proof of concept:

defmodule Main do
  def main do
    url = "https://data.iana.org/time-zones/releases/tzdata2021a.tar.gz"
    tzdata = curl(url)
    tzdata_asc = curl(url <> ".asc")
    {public_key_string, 0} = System.cmd("gpg", ~w(--export -a [email protected]))

    # https://erlang.org/pipermail/erlang-questions/2021-July/101220.html
    [public_key] =
      :public_key.pem_decode(
        IO.iodata_to_binary([
          "-----BEGIN RSA PUBLIC KEY-----\n",
          public_key_string |> String.split("\n", trim: true) |> Enum.drop(1) |> Enum.drop(-2),
          "\n-----END RSA PUBLIC KEY-----"
        ])
      )

    IO.inspect(
      [
        tzdata: tzdata,
        tzdata_asc: tzdata_asc,
        public_key: public_key_string,
        public_key: public_key
      ],
      printable_limit: 100
    )
  end

  defp curl(url) do
    {out, 0} = System.cmd("curl", ["--fail", "--silent", url])
    out
  end
end

Main.main()
[
  tzdata: <<31, 139, 8, 0, 0, 0, 0, 0, 2, 3, 236, 125, 91, 119, 35, 71, 146,
    158, 94, 89, 191, 34, 23, 173, 53, 200, 49, 10, 4, 192, 59, 91, 212, 12, 72,
    130, 108, 170, 121, 51, 1, 170, 165, 30, 141, 225, 2, 144, 0, ...>>,
  tzdata_asc: "-----BEGIN PGP SIGNATURE-----\n\niQIzBAABCgAdFiEEfjeSqdis99YzvBWI7ZfpDmKqfjQFAmANxdIACgkQ7ZfpDmKq\nfjTn" <> ...,
  public_key: "-----BEGIN PGP PUBLIC KEY BLOCK-----\n\nmQINBEyAcmQBEADAAyH2xoTu7ppG5D3a8FMZEon74dCvc4+q1XA2J2tBy2pwaT" <> ...,
  public_key: {:RSAPublicKey,
   <<153, 2, 13, 4, 76, 128, 114, 100, 1, 16, 0, 192, 3, 33, 246, 198, 132, 238,
     238, 154, 70, 228, 61, 218, 240, 83, 25, 18, 137, 251, 225, 208, 175, 115,
     143, 170, 213, 112, 54, 39, 107, 65, 203, 106, ...>>, :not_encrypted}
]

the next step is to use :public_key.verify/4 but not sure how to transform the arguments appropriately so it accepts them.

Maybe @voltone could help with that :)

@bryannaegele
Copy link

Is this going to become a problem with Root CA file expiring at the end of the month since we can't pass hackney options down?

@voltone
Copy link

voltone commented Sep 19, 2021

Is this going to become a problem with Root CA file expiring at the end of the month since we can't pass hackney options down?

Should not be an issue, for two reasons:

  • Since no ssl options are being passed to Hackney, it will use its defaults which (at least with recent Hackney versions) will verify the server cert correctly before and after the DST Root CA cert expires
  • The server that Tzdata connects to does not use a Let's Encrypt certificate, so OTP's ssl should have no problem connecting to it in any case, regardless of OTP version or the client used

@lau
Copy link
Owner

lau commented Oct 26, 2021

Thanks @wojtekmach Option 2 sounds good, because it doesn't depend on any other packages.

:public_key.verify/4

This would replace the system call to gpg right?

Re: @voltone

Since no ssl options are being passed to Hackney, it will use its defaults which (at least with recent Hackney versions) will verify the server cert correctly before and after the DST Root CA cert expires

Sorry, maybe I'm missing something. Isn't the purpose is to use httpc and not depend on Hackney though? So there won't be any Hackney?

If we use option 2, we could theoretically even use http (without the s), since we would rely on the GPG signature to verify the downloaded file, correct?

@wojtekmach
Copy link
Contributor Author

:public_key.verify/4

This would replace the system call to gpg right?

Yeah, that's the idea. But I'm not sure if it is possible, I hope it is.

@voltone
Copy link

voltone commented Oct 26, 2021

Sorry, maybe I'm missing something. Isn't the purpose is to use httpc and not depend on Hackney though? So there won't be any Hackney?

Right, but I interpreted the question as referring to the current situation, which relies on server certificate verification. I suppose @bryannaegele wanted to know whether current applications might stop fetching tzdata updates as a result of the DST Root CA expiry.

If we use option 2, we could theoretically even use http (without the s), since we would rely on the GPG signature to verify the downloaded file, correct?

There is some value in using HTTPS even without the certificate verification. For one thing, the IANA repository might one day stop serving data over plain HTTP altogether. But yes, using HTTP would be possible.

Yeah, that's the idea. But I'm not sure if it is possible, I hope it is.

It should be possible in theory, but :public_key does not support PGP/GPG signatures, so it would require an implementation of RFC4880. I am not aware of a package that implements that, and doing that inside the Tzdata package might be a bit of a stretch...

@wojtekmach
Copy link
Contributor Author

wojtekmach commented Oct 26, 2021

RE RFC4880, got it, thanks Bram.

In that case, I believe the next best thing is to use httpc with secure options and castore or certifi, I think many users will appreciate tzdata bringing just one extra dep.

@lau
Copy link
Owner

lau commented Oct 26, 2021

See also this that I just wrote about alerting about new versions and then requiring data updates via hex packages: #127 Comments appreciated.

@bryannaegele
Copy link

Yes. My only concern was if the CA expiry was going to prevent downloading.

@aj-foster
Copy link

aj-foster commented Nov 4, 2021

As you approach this issue, please also consider corporate environments in which TLS interception is active, as described in hexpm/hex#727. Something analogous to HEX_CACERTS_PATH for injecting an additional valid certificate would be helpful. 🙂

@codeadict
Copy link

Also keep in mind that since OTP 25 there is support for native OS CA certificates that httpc can use and don't need the extra dependency

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants