http icon indicating copy to clipboard operation
http copied to clipboard

Added easy-to-use callback to handle custom invalid certificate acceptance

Open Mereep opened this issue 5 years ago • 8 comments

Fixes #458

This pull basically adds the following interface:

// set callback for specific client
ioClient.setBadCertificateCallback((cert, host, port) => true, false);

// or set it globally (which will even work for [http.get()] etc.
http.Client().setBadCertificateCallback((cr, host, port) => true);

where

(cert, host, port) => ...

is a callback which gets invoked if a https-request is made but the certificate appears to be invalid for any rason. If the function returns true the request is processed anyways.

The callback can be set for the current client only or also for all consecutive clients (second parameter; default: set for both the current and all consecutive clients)

This does not work for BrowserClient. I don't know yet if this can be implemented analogous. Trying to invoke this for any client other then IOClient will raise a UnimplementedError

What is done:

  • a new test case is added for all relevant new calls
  • an additional https server is added to the test suite. It behaves completely analogus to the existing (http) test server but having a security context set being initialized with an included self-signed dummy-certificate / private key.
  • the https mock server is under tests
  • no existing nor new tests break

that said, I didn't make any pull request yet. Please keep in touch for any problems. I will gladly try to fix. Opening this request marked as draft. If it is in any acceptable stage, I will mark it as mergeable. Just ping me for that also.

Mereep avatar Dec 06 '20 22:12 Mereep

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

:memo: Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

google-cla[bot] avatar Dec 06 '20 22:12 google-cla[bot]

Any feedback welcome...

Mereep avatar Dec 08 '20 13:12 Mereep

Anyone wants to check this / give feedback / merge this?

@kevmoo @natebosch @srawlins ?

Mereep avatar Dec 13 '20 23:12 Mereep

I don't like the idea of the forInstanceOnly argument and the global variable.

My bigger concern is having an IO specific API that leaks to http.Client() instead of only IOClient.

The supported approach here is to use http.IOClient(HttpClient()..badCertificatCallback = callback). You can then hold that client and use DI (or a global variable if you prefer) to reuse the same client.

If we specifically need calls like http.get to be configurable in code that you don't own we'd probably want to look at some kind of zone override API like

/// Runs [callback] in a Zone where top level http methods [get] etc use [client] instead of a default constructed client.
void runWithClient(void Function() callback, Client client);

cc @lrhn

natebosch avatar Dec 14 '20 20:12 natebosch

Thanks for looking into it @natebosch

Honestly, the sole idea was to provide an easy way of handling this common use case. After all, this will be an issue for any development-environment dealing with https connections. The ability to intercept with the certificate handling is kind of opaque (as of yet) and needs detailed knowledge about the internals of the package (i.e., that a thing like IOClient exists and how to configure it properly; or that there is a thing like HttpOverrides which intercepts with client creation).

Exposing the method to Client had multiple considerations: Other clients than IOClientmight be able to support this behaviour; also it provides auto-complete support and removes the necessity for explicit conversion (http.Client() returns type Client after all, which would hide it if only exposed to IOClient).

I mean the global variable (if you refer to the static one by that) could be just removed. It's just there to enable http.get()-calls also respect the setting.

Mereep avatar Dec 14 '20 22:12 Mereep

Other clients than IOClientmight be able to support this behaviour

BrowserClient can't support this because the browser doesn't support this. Keeping Client as only the cross-platform subset of the API is a critical part of the design for this package. We want to avoid the throw UnimplementedError anywhere in this package.

natebosch avatar Dec 14 '20 22:12 natebosch

Ok, I see.

So moving it up a level to IOClient, removing the static and documenting that feature with a concrete example (like var client = (http.Client() as IOClient)..setBadCertificateCallback(...);) as hint when the HandshakeException is thrown would be ok or you have reservations against that?

Mereep avatar Dec 14 '20 23:12 Mereep

This is really required. Need it so badly

SAGARSURI avatar Apr 05 '21 05:04 SAGARSURI

We do now have a runWithClient function to impact the behavior of the top level methods. https://pub.dev/documentation/http/latest/http/runWithClient.html

The supported way to handle this is to construct an IoClient with the bad certificate callback in place, and run the app in a zone with that client set. Other clients which support the configuration can also handle it at construction time - I don't think we need the setter in the public API.

natebosch avatar May 31 '23 15:05 natebosch