Skip to content

Add a new keyLog property to HttpClient #48093

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.

Sign up for GitHub

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

Jump to bottom
Closed
brianquinlan opened this issue Jan 7, 2022 · 12 comments
Closed

Add a new keyLog property to HttpClient #48093

brianquinlan opened this issue Jan 7, 2022 · 12 comments
Assignees
@brianquinlan
Labels
area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. breaking-change-approved breaking-change-request This tracks requests for feedback on breaking changes library-io
Milestone
February Release ...

Comments

@brianquinlan
Copy link
Contributor

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.
@brianquinlan brianquinlan self-assigned this Jan 7, 2022
@brianquinlan brianquinlan added area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. breaking-change-request This tracks requests for feedback on breaking changes library-io labels Jan 7, 2022
@zanderso
Copy link
Member

zanderso commented Jan 7, 2022

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

@devoncarew
Copy link
Member

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

@grouma
Copy link
Member

grouma commented Jan 28, 2022

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

@grouma
Copy link
Member

grouma commented Jan 28, 2022

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
Copy link
Contributor

Hixie commented Jan 28, 2022

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
Copy link
Member

vsmenon commented Feb 1, 2022

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

@brianquinlan
Copy link
Contributor Author

@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
Copy link
Member

vsmenon commented Feb 1, 2022

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

@brianquinlan
Copy link
Contributor Author

@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
Copy link
Member

vsmenon commented Feb 1, 2022

Thanks - lgtm

@devoncarew
Copy link
Member

@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 brianquinlan mentioned this issue Feb 8, 2022
Merged
@brianquinlan
Copy link
Contributor Author

Fixed in 917ae52

@brianquinlan brianquinlan mentioned this issue Mar 9, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@brianquinlan brianquinlan

Labels
area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. breaking-change-approved breaking-change-request This tracks requests for feedback on breaking changes library-io
Projects
None yet
Milestone
February Release (Dart 2.17 beta 2)
Development

No branches or pull requests
7 participants
@Hixie @devoncarew @grouma @vsmenon @zanderso @a-siva @brianquinlan