Ban HTTP by default in HttpClient

[mehmetf]

Goal

For Google's first party applications, using cleartext to communicate with a server is a launch blocker. To help Flutter applications detect such problems, we would like to explicitly ban cleartext http and only allow TLS traffic in HttpClient. Since there are legitimate use cases for cleartext transmission, it should also be possible to override this behavior via a security review.

Proposal

Many packages such as Flutter access dart:io directly for its network calls. So, if the app is loading network assets (such as an image), it could accidentally use cleartext without a problem. We can ensure that does not happen by banning HTTP in the lowest level possible in Dart. We could do this in the platform libraries by throwing an exception in dart:io#HttpClient if scheme is set to HTTP. There are several attributes of this requirement that shapes the proposed implementation.

This is for client apps only. From dart:io#HttpClient perspective, this just means iOS and Android (not server). We should ban cleartext if Platform.isIOS or Platform.isAndroid. Note that Web is out of scope and should be handled separately.

We need to allow overrides. I propose to create a new zone variable for "allowClearText" in HttpOverrides. This would be easy to use and readable. It also fits existing usage pattern if we expand the purpose of HttpOverrides beyond testing (it is already being used beyond testing by some clients).

Combining these proposals, we get:

• Modify HttpOverrides to support allowClearText as a zone variable.

• Create an _EmbedderConfig class to contain a static configuration as a default. This will be overridden in embedders for iOS and Android.

• Modify _HttpClient to ban HTTP scheme only if:

   (Zone.current[allowClearText] ?? _EmbedderConfig.allowClearText) == false

See this internal design doc go/disable-http-flutter-dd for a more detailed discussion of alternatives.

[mehmetf]

@natebosch @vsmenon PTAL

[zanderso]

Is it only cleartext http communication that is a launch blocker? If this also applies to other communication, then it may make sense to do roughly the same thing, but pushed down to the dart:io Socket API. It might be a bit simpler there too, since you can just make the non-Secure socket connect and bind methods throw rather than hooking into the middle of the HttpClient connection setup.

The override flag could go in IOOverrides: https://api.dart.dev/stable/2.7.1/dart-io/IOOverrides-class.html.

[mehmetf]

The internal documentation states HTTP only even though I am sure they would like to encrypt any traffic to/from an external service (FTP?). However, I am not sure if a similar ban exists for internal communication (IPCs?). It might be simpler to implement but I suspect it would be a bigger change to roll out.

PS: Would this break observatory? I know we currently use ws:// to connect but not sure if these are secure sockets or not.

[vsmenon]

@a-siva @lrhn @mit-mit - thoughts on this?

[lrhn]

Having different behavior in public vs. internal builds is an accident waiting to happen. It just takes one small bug in the transform and then the protection is gone from the internal Google version of Dart. Also, we cannot ensure that all Google Dart code runs on that version when we ship it to client devices.
Same for behavior which differs between platforms. If it works elsewhere, but not on iOS or Android, then it onlyr equires one mistake to enable it on iOS and Android again.

I'm not sure how I feel about using zone variables to override a default. Users are usually not very aware of zones, and it's easy to have code run in a different zone than you expect. If any zone enables the unsafe usage, the entire application should be thoroughly reviewed to ensure that that zone itself is contained.

Is it only the HttpClient class that we need to worry about?
If so, we could give it a constructor parameter which forces it to use an encrypted connection on every request, and fail if it can't get one. Then we can add a lint ensuring that code passes that parameter.

Breaking existing code is bad, but if we haven't already, we could perhaps make the HttpClient attempt to upgrade (https://en.wikipedia.org/wiki/HTTP/1.1_Upgrade_header) non-encrypted connections where possible (HTTPS everywhere FTW!)
If the upgrade succeeds, then nobody should have any issues.
If not, the constructor parameter comes into play.

So, maybe make it:

HttpClient({
  SecurityContext context, 
  HttpsUpgrade httpsUpgrade = HttpsUpgrade.whenPossible
}) ...

/// When an [HttpClient] upgrades an HTTP connection to HTTPS.
enum HttpsUpgrade {
  /// An HTTP connection is always upgraded to HTTPS.
  ///
  /// If the upgrade isn't possible, the connection fails.
  always,
  /// An HTTP connection is upgraded to HTTPS when the server allows it.
  ///
  /// An upgrade is always attempted, but it isn't possible, the connection is performed over HTTP.
  whenPossible,
  /// An HTTP request is always honored and never upgraded to HTTPS by the client.
  never
}

Then we can make a lint which requires the always option to be passed. You can ignore that using // ignore, as for any other lint, but it can be caught in code reviews if it isn't intended.

It is technically a breaking change if the current behavior matches never and we make the default whenPossible.

[mehmetf]

The upgrade option is interesting and should be implemented if it isn't already. I would like to keep that out of scope since that's not an allowable option internally.

I don't foresee code transform accidents being a problem since we can mitigate against it by putting in a few simple tests that block Dart rolls.

I had considered a constructor parameter but it ends up being a more complicated ecosystem change. HttpClient is being used directly in 30+ places internally (including in 5 third_party libraries). Each package will either need to expose the same option (and have its own lint) or needs to convert to being HTTPS-only. It is definitely doable but I feel like this solution is more manageable. Moreover, it fits to the intent of HttpOverrides as I understand it.

[mehmetf]

I am also aware that zones are tougher to use for most clients but we expect the overrides to be the exception not the norm. So from developer UX perspective, I am OK with them.

Overrides necessitate a thorough review of the code no matter what methodology is used. There are many different ways that people can shoot themselves in the foot (such as accidentally leaking an unsafe HttpClient instance).

[natebosch]

Would we consider changing the default externally in Dart 3 so that internal and external configurations match?

[mehmetf]

If everyone is OK with this proposal, I will add comments and tests to the PR and send out for review.

[lrhn]

Let's ask @sortie as well.

This reminds me of the "unsafe" annotation: #40595
You are saying that using a clear-text client is unsafe in some situations (in Flutter clients mainly).
However, you want to enforce this at run-time, not just show warnings at compile-time.

What if it was just a hint, but one that was enabled automatically for Flutter clients (they have a default analysis options file AFAIK)?

I really dislike hard-coding platform names in the code. What makes iOS and Android so special?
Is there something else we can do for those platforms (like set an environment variable by default while compiling or something)?
It's not like Windows is less of a client OS, it's just not a mobile OS (and not currently a Flutter compilation target, but there is no reason it can't be in the future).
Or, in short, we do not want to special case one particular use-case in the platform libraries.

Maybe we can make it a Dart embedder/compiler choice. But why just this one use-case then (plain-text HTTP). Are there other features that need to be handled as well?

So, I think that I'm fairly strongly against this change. It's to specific, too ad-hoc, to belong in the platform libraries as long as it's defined in terms of Platform.isIOS or Platform.isAndroid. That's not where that configuration should be located.

I'm fine with adding a general feature which allows you to disallow plain-text HTTP connections.
I'm also fine with allowing some way to enable it by default when compiling (perhaps use an environment variable, so compile with -Dhttp.allowCleartext=false).
Then you can have zone-based overrides as well.

Something like:

class HttpClient  ... {
  /// Runs [action] in a [Zone] where plain-text HTTP connections are allowed.
  ///
  /// Some programs or environments may disallow un-encrypted HTTP connections.
  /// In such programs, running the code creating the connection using this function will 
  /// allow unencrypted connections. This ensures that an unencrypted connection isn't
  /// created by accident, and it is visible in the code where it is being allowed.
  static Future<T> allowPlaintext<T>(FutureOr<T> action()) {
    return await runZoned(action, zoneVariables: {#_allowPlaintext, true});
  }
  bool get _allowsPlaintext => 
      const bool.fromEnvironment("http.allowPlaintext") || Zone.current[#_allowPlaintext] == true;

  ... where the test needs to be ...
  if (userRequestedPlaintext & !_allowsPlaintext) {
     throw StateError("Plain-text HTTP connections are disallowed.");
  }

[mehmetf]

Thanks for the comments.

This is necessarily platform specific whether we have the code in core libs or expose it as a compile-time flag. Android and iOS ban HTTP connections for apps at the OS level. Linux and Windows do not. That's what makes iOS and Android special. This might change in the future, at which point we can enable it for other platforms as well.

I am on board with not having platform-specific code in core libraries. I understand it is jarring. However, your suggestion exposed a problem in my plan. Today, we bring in pre-built core snapshots to google3. Neither my source transform idea nor your compile-time environment variable idea work in this situation since we don't build the VM or the core libs in google3.

Can this be a runtime flag to VM instead? We can have this passed in by the embedder of a particular platform and we can configure it in google3 specifically. Can core libraries read a VM flag? I would appreciate an example.

What if it was just a hint, but one that was enabled automatically for Flutter clients (they have a default analysis options file AFAIK)?

There will be a hint which will check whether you are explicitly allowing cleartext by setting a zone variable. We can indeed use unsafe_api hint to do it because that particular zone variable will be marked as unsafe and will require a security audit to use.

From your response, it also sounded like you were not sure why we need the runtime check if we have the hint above. Nothing prevents someone from attempting to pass in a http URL to a secure HttpClient since we cannot check the URL content at compile time. It is just an arbitrary string that can be constructed and passed around in many ways. We need this to fail at runtime.