Add a new keyLog property to HttpClient

[brianquinlan]

Change

I propose that we add a new keyLog property to HttpClient with a signature like:

void keyLog=(
  void f(String line)?
)

This would allow users to log TLS keys for use with Wireshark and other analysis tools.

The approach was discussed in this code review

The non-breaking alternative would be to make keyLog an argument to the HttpClient constructor but that is not symmetrical with the HttpClient API (i.e. onBadCertificateCallback).

See related discussing on Add a new connectionFactory property to HttpClient.

Rationale

The HttpClient API already has multiple attributes that accept functions e.g.

void findProxy=(
  String f(Uri url)?)

void badCertificateCallback=(
  bool callback(
    X509Certificate cert,
    String host,
    int port
  )?
)

and implementing this as a constructor argument would be inconsistent with the rest of the API. Also, parameterizing all future functionality as constructor arguments does not seem scalable.

Impact

All classes that implements HttpClient (without extends Mock or equivalent noSuchMethod implementation) will need to be updated.

I suspect that there are not many implementations of HttpClient outside of mocks.

Mitigation

Users must implement the keyLog property.

[zanderso]

Please note https://github.com/flutter/flutter/blob/76cf452fe3d18e3765139190c13c2d52e0264310/packages/flutter_tools/test/src/fake_http_client.dart#L123

[devoncarew]

@vsm, @Hixie, and @grouma - can you review and sign off for Dart, Flutter, and AngularDart respectively? Thanks!

[grouma]

I couldn't find any usages of implements HttpClient in the Angular code base so this should have no impact.

[grouma]

Note that this may impact internal customer code. I did a quick spot check and it will impact the Assistant team but overall there aren't many usages of implements HttpClient.

[Hixie]

I don't object.

We should make sure we document that this is a very security-sensitive API though, and that applications should only use it during debugging.

[vsmenon]

Do we have summary on level of impact on internal and external code of this breaking change?

[brianquinlan]

@vsmenon For Google code, there are a few places that implement HttpClient without noSuchMethod or equivalent.

I checked some likely-to-be-affected external packages e.g. package:http, package:http2 but didn't find any that would be broken.

[vsmenon]

Thanks! For this (and #47887), what would downstream users need to do a minimum? E.g., implement an empty keyLog method?

[brianquinlan]

@vsmenon Yes, my plan was for the changelog to include instructions to add this to your implementation:

  set keyLog(String f(String line)) => throw UnsupportedError();

[vsmenon]

Thanks - lgtm

[devoncarew]

@brianquinlan - you're good to go with this change (see the 'Execution' steps here: https://github.com/dart-lang/sdk/blob/main/docs/process/breaking-changes.md#step-3-execution). Thanks for your patience!

[brianquinlan]

Fixed in 917ae52