make the HTTP Manager configurable

[amesgen]

Closes #2022.

This description is partly outdated (see discussion below).

---

A LOT of functions now take IO Dhall.Import.Manager, where Manager = () on GHCJS or if with-http is disabled. The Manager is configurable via EvaluateSettings, and defaults to the previous default (default TLS Manager from http-client-tls with 30s timeout) when the use-http-client-tls flag is enabled, and the default plain Manager otherwise. This default is available in Dhall.Import.defaultNewManager.

Some remarks:

1. The remote import tests fail (as expected) with -fwith-http -f-use-http-client-tls due to TLS failures. Do you know any plain HTTP servers without HTTPS redirections for this use case?
2. When testing with -f-with-http, this url from dhall-lang/tests/import/success/unit/asLocation/RemoteChainEnvA.dhall does not seem to have been mocked.
3. cryptonite is a rather big dependency, and it is only used for SHA256. It is a transitive dep of http-client-tls, so maybe now (not necessarily in this PR) it would be a good idea to use cryptohash-sha256 instead.

In addition to bloat and security concerns, this PR enables to selectively disallow remote imports (e.g. in Dhall.Format).

Feel free to close this PR if the changes are too invasive or you see a different way to make the HTTP Manager configurable.

[amesgen]

I don't want to DOS the CI, is there an easy way to mirror a CI run locally (with cabal and without nix (man I should finally set up nix...))?

[Gabriella439]

@amesgen: The most accurate way is using Nix:

$ nix build --file ./release.nix

… but the closest thing using plain Cabal is:

$ cabal configure -f-use-http-client-tls
$ cabal test

[amesgen]

dhall-nixpkgs and dhall-try(?) seem to be missing from the root cabal.project. Should I add them in this PR or is this out of scope?

[Gabriella439]

@amesgen: Feel free to add them! 🙂

[amesgen]

Rebased on master. I removed dhall-try form cabal.project again as it requires GHCJS (and thus, a naive cabal build all fails), and most people interested in GHCJS probably already use nix.

[Gabriella439]

@amesgen: Instead of parametrizing each API function on a new Manager argument, would it be possible for the new cabal configure flag to bake in the insecure HTTP manager? That would avoid having to change all of the type signatures.

[amesgen]

If we did this, one could choose between these options:

1. disable with-http: no HTTP imports at all
2. enable with-http, but disable use-http-client-tls: only plain HTTP imports possible
3. enable with-http and use-http-client-tls: HTTP and HTTPS imports possible

Right now, 1. and 3. are already possible. In my original comment, I mentioned (first bullet point) that it is probably rare that 2. is actually useful (how often does one need HTTP imports, but not HTTPS imports?).

The second and third bullet point are only possible with changed type signatures, and they are much more useful:
If one does already depend on e.g. http-client-openssl, one can disable the http-client-tls dependency of dhall. And if one has e.g. security reservations, but wants HTTPS imports, one needs to supply a different Manager, which is only possible with changed type signatures.

[Gabriella439]

@amesgen: If the goal is to depend on http-client-openssl instead of http-client-tls, why not make a cabal configure flag for that instead? Switching out a dependency seems like a reasonable thing to do at package configuration time.

[amesgen]

That would have the disadvantage that possible future Manager backends can't be used/have to be given special treatment in dhall (and disables the (rare) use case of plain HTTP-only Managers).

Also, a configurable Manager has many other benefits apart from the http-client-tls vs. http-client-openssl thing (which was my original motivation indeed), e.g. one can configure proxy settings, connection timeouts (hardcoded right now), retry conditions, and one can share a Manager between different invocations of Dhall functions.

I understand that these type signature changes are not ideal. If you feel that the added flexibility of this PR is not worth the cost, I can create a new one with your suggestion to create a flag to switch between http-client-openssl and http-client-tls (which I also like, even though it is somewhat ad-hoc).

[Gabriella439]

@amesgen: How about preserving the original type signatures for the default functions, but add variations on them that accept a Manager as an argument?

[amesgen]

Good idea, what suffix should we use? ' , or withMgr, or withManager,...?

[Gabriella439]

@amesgen: …WithManager works fine for me

[amesgen]

I reverted the changes to all subprojects except dhall, as dhall-json etc. are probably not often used as libraries. Also, I did not create ...withManager variants for the functions in dhall which only serve as the implementation of specific subcommands for the same reason.

As a result, the diff is way smaller.

It might be a good idea to add a test that the Manager passed to EvaluateSettings is actually used in the import resolution, as it is imaginable that in a refactor, the Manager is not passed on due to the usage of a non-withManager-function.

[Gabriella439]

Looks great! I'm just waiting on CI

[Gabriella439]

@amesgen: Thank you for doing this! 🙂