Skip to content

Comments

Add configuration option to disable automatically trusting all certificates#1

Open
aruh wants to merge 8 commits intolittleGnAl:mainfrom
aruh:cert-config
Open

Add configuration option to disable automatically trusting all certificates#1
aruh wants to merge 8 commits intolittleGnAl:mainfrom
aruh:cert-config

Conversation

@aruh
Copy link

@aruh aruh commented Dec 11, 2021

The problem

This library blindly trusts all server certificates by setting a badCertificateCallback that always returns true. The trust is not constrained to installed user certificates or programmatically configured certificates.
⚠️ This allows MITM attacks as it effectively disables certificate checks. This means that this library should never be used in production!

I recognize that this library was built to allow usage of debugging proxies like Charles or mitm-proxy. The README doesn't mention this restriction and doesn't inform about the danger of allowing MITM attacks. On the surface, it seems like a library to handle proxy settings in any context - not constrained to usage with debugging proxies.
It was used in production.

Proposition

The library provides the HttpOverrides that would also be handy in production to pick up on proxy settings of a device.
Therefore it would be nice to adjust this library to allow using it in production.

The proposed adjustments are:

  • Make the certificate ignoring behavior configurable. Then it can be switched on for easier development and switched off in production for proper certificate checks.
  • Add configuration option to allow setting a custom SecurityContext that can contain added certificates – e.g. self-signed certificates or the certificates of the debugging proxies.
    For example, this can be relevant in production for some company networks, where a proxy with an company-internal self-signed certificate is used.
  • Add documentation to inform library users about the risk of MITM attacks.

I guess that should allow using the library in production.

What does this PR change?

  • Adds ignoreBadCertificates option to allow switching the ignoring of certificate issues on and off.
  • Adds securityContext option to allow setting an adjusted SecurityContext that contains trusted certificates. The context is passed to the created HttpClient.
  • Adds and improves documentation

Full disclosure: I'm not an security expert nor a very experienced Flutter/Dart developer. It could very well be the case that there are security issues that would not allow usage in production.

Thank you for your work on http_proxy_override.

aruh added 8 commits October 10, 2021 14:43
This avoids a crash that occurs when a certificate is added a second time to a
SecurityContext. Instead the caller should add the certificates once to a SecurityContext
and that single instance is used every time a HttpClient is created.
The default proxy port of 8888 only seems to make sense for Charles - i don't know
whether other libraries also use this port as a default. But, as this library
allows setting general proxies - not constrained to Charles -, the default port of
8888 assumes that the user of this library is using Charles. This might not be the
case at all.
@aruh
Copy link
Author

aruh commented Dec 11, 2021

The fork was integrated in a project and the configuration options - both ignoreBadCertificates and a custom SecurityContext - were successfully tested on Android and iOS devices.

Nevertheless it would of course still be wise to test it again during the review of this PR.

environment['http_proxy'] = '$host:$port';
environment['https_proxy'] = '$host:$port';
} else {
environment['http_proxy'] = '$host:8888';
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, but I'm not sure if it can be set to null or not, or just do nothing

/// The port part of the proxy address.
final String? port;

static Future<HttpProxyOverride> createHttpProxy() async {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you should mark this function as @deprecated instead remove it directly, to avoid break change at this time

///
/// Supported platforms to read proxy settings from are **iOS** and
/// **Android**.
static Future<HttpProxyOverride> create(
Copy link
Owner

@littleGnAl littleGnAl Dec 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think since this library aims to use for debugging purposes

  1. the default value of ignoreBadCertificates should be true, if the user actually need to use it in production, they should set it to false
  2. it's better to make the return type to nullable and return null if ignoreBadCertificates is not true, so we do not "override" anything if ignoreBadCertificates is false

e.g.

static Future<HttpProxyOverride?> create(
      {bool ignoreBadCertificates = true,
      SecurityContext? securityContext}) async {
    if (!ignoreBadCertificates) return null;
    return HttpProxyOverride._(
        await _getProxyHost(),
        await _getProxyPort(),
        ignoreBadCertificates,
        securityContext ?? SecurityContext.defaultContext);
  }


@override
HttpClient createHttpClient(SecurityContext? context) {
if (context == null) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The SecurityContext should be handled when the HttpClient is created, see https://api.dart.dev/stable/2.14.4/dart-io/HttpClient/HttpClient.html, but not set by this library, say that the user should

  final context = SecurityContext.defaultContext;
  context.setTrustedCertificatesBytes(...);
  HttpClient httpClient = HttpClient(context: context);

  HttpProxyOverride httpProxyOverride = await HttpProxyOverride.create();

but not

  HttpClient httpClient = HttpClient();

  final context = SecurityContext.defaultContext;
  context.setTrustedCertificatesBytes(...);
  HttpProxyOverride httpProxyOverride = await HttpProxyOverride.create(securityContext: context);

@littleGnAl
Copy link
Owner

@aruh Thank you so much for your contribution, I have left some comments, btw can you add test cases for this PR?

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

Successfully merging this pull request may close these issues.

2 participants