Ban HTTP by default in HttpClient #40548
Comments
|
@natebosch @vsmenon PTAL |
|
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 The override flag could go in |
|
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
added
the
area-core-library
|
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. 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 Breaking existing code is bad, but if we haven't already, we could perhaps make the 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 It is technically a breaking change if the current behavior matches |
|
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. |
|
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 |
|
Would we consider changing the default externally in Dart 3 so that internal and external configurations match? |
|
If everyone is OK with this proposal, I will add comments and tests to the PR and send out for review. |
|
Let's ask @sortie as well. This reminds me of the "unsafe" annotation: #40595 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? 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 I'm fine with adding a general feature which allows you to disallow plain-text HTTP connections. 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.");
} |
|
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.
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. |
See the class sdk/sdk/lib/io/embedder_config.dart Line 17 in a75ffc8 For example, calls to |
|
@Hixie Is there a chance you would want to make this the default for all Flutter iOS/Android apps? This is the default behavior of iOS and Android platforms. On the other hand, this would to be a pretty big breaking change if apps are relying on it. |
|
I guess I can sell this so long as opting into http support is a one-liner. Re upgrades, I don't see how to do that in a secure way. I would not recommend that. But maybe I'm misunderstanding the idea. |
|
Sounds good. FTR: Runtime flags would not work without extensive changes. We don't currently have a mechanism in Flutter engine for configuring VM flags for release mode. Android relies on Intent extras whereas iOS takes arguments from process execution. These are only viable for development workflow. If we want these flags in release workflow, we would have to create a way to get VM flags via static configuration files (manifest on Android and Info.plist on iOS). I am not sure if we want to do that. @cbracken could you please let me know if there's an existing policy in place for allowing custom VM flags for release mode? I wonder if this was previously discussed. |
|
Personally I'd prefer some sort of code-level control than runtime flags.
Runtime flags are much more buried and mysterious and therefore much more
likely to be misused, misunderstood, or accidentally set incorrectly.
…On Tue, Feb 18, 2020 at 7:40 PM Mehmet Fidanboylu ***@***.***> wrote:
Sounds good.
FTR: Runtime flags would not work without extensive changes. We don't
currently have a mechanism in Flutter engine for configuring VM flags for
release mode. Android relies on Intent extras whereas iOS takes arguments
from process execution. These are only viable for development workflow. If
we want these flags in release workflow, we would have to create a way to
get VM flags via static configuration files (manifest on Android and
Info.plist on iOS). I am not sure if we want to do that.
@cbracken <https://github.com/cbracken> could you please let me know if
there's an existing policy in place for allowing custom VM flags for
release mode? I wonder if this was previously discussed.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#40548?email_source=notifications&email_token=AAEGSHFONMAKBU3I66LWME3RDSS4HA5CNFSM4KR5IKZKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEMGHRAI#issuecomment-588019841>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAEGSHG7QXHZIX2MMEAEYCLRDSS4HANCNFSM4KR5IKZA>
.
--
Ian Hickson
|
|
Having a platform level flag, no matter how it's integrated into the platform, is fine with me. If it can be applied at individual application compile-time, that's the most fine grained approach. Then Google internal builds can set it to false in the build system. Using a If it is applied to the run-time system when the embedder is built (that only works for native builds, not compilation to JavaScript, which also exposes HTTP requests), then we will have to figure out where to apply the flag (if the VM runtime snapshot is not built by the Google build system, then we can't put it into the snapshot). As long as the code can check this flag in some way rather than hard-coding where the restriction applies into the libraries themselves, then I am fine with it. Then we can keep it a non-breaking change for everyone who doesn't opt in to the more restrictive behavior. |
|
One thing I would encourage thinking about is whether this is a one-off situations, or we are expecting other similar restrictions to become issues in the future. If it's one-off, then the generality of the solution is not important. |
|
I'll take a closer look at the issue at hand and think about the best solution. My gut feeling right now is that this is probably best enforced by the embedder and could be controlled by some configuration file in a Flutter project, much like the existing app permissions and such already are. |
|
I am wondering whether these defaults should be configurable at the project level. My gut feeling right now is no. That's why I asked @Hixie above whether it would be fine making this proposal the default behavior for all Flutter apps. If defaults are not configurable, then I would love to have platform-specific configuration to core libs without involving the embedder. Cases like this are not specific to Flutter. They are specific to platform behavior. iOS and Android ban HTTP communication regardless of how Dart VM is being embedded. I also believe Fuchsia ban on I agree with @lrhn that peppering Platform.isXXX all over the place is too adhoc. However, I would love to have a central This decision would also be easy to reverse. If the use case arises that would require configurable defaults, we can get rid of |
|
If what we want is to have configurable defaults, then I am happy to collaborate on this @sortie but my hopes are not high. There's already a mechanism to pass VM flags in each embedder. However, as noted above, it is not designed to support passing configuration in release mode (and it would be undesirable to affect the behavior of VM via flags in release mode). That would mean creating an entirely different mechanism. For instance, the canonical way to represent static configuration on Android is via the manifest file. You would need to do the following to send this all the way to Dart:
This is just Android. You would need the equivalent of this for every platform. For covering a use case that does not yet exist, this is a tall order. |
|
The embedder (e.g. Flutter engine targeting iOS/Android) can set the value of a static getter in |
|
@zanderso What do you think about having Platform specific defaults in core libs itself? It feels unnecessary to me that OS-mandated behavior is configured statically by the embedder. For instance, why would we not do this: class _PlatformConfig {
final bool isolateCanExit;
final bool ...;
const _PlatformConfig(...);
}
const _PlatformConfig _fuchsiaConfig = _PlatformConfig(
isolateCanExit: false,
httpCleartextEnabled: true,
...
);
const _PlatformConfig _iOSConfig = _PlatformConfig(
isolateCanExit: true,
httpCleartextEnabled: false,
...
);
_PlatformConfig get platformConfig {
if (Platform.isIOS) return _iOSConfig;
...
}as opposed to using Tonic to reach in and change static values in various classes from the embedder? This only works if the defaults are not configurable and really are a property of the underlying platform (not the embedder). That's the only use case I have seen so far. |
|
I did. I think trying to create a "configuration object" and a set of known configurations is the wrong direction to take. Things that you might want to configure is an open-ended enum. It's not something that is best handled by code with a fixed API. A better design for the "configuration" would be a set of capabilities (not a The best way to introduce such a configuration is also not code. It should come from the environment in some way, whether that's the compilation environment or the run-time environment or the platform build system or hardcoded into each run-time binary ... whatever makes it work without having to fix any particular configuration into code. The general rule I'm leaning on here is that open ended enumeration should not be named in code because that makes them hard to grow from the outside. |
|
I was imagining something much simpler, where you just have HttpClient client = new HttpClient(allowInsecureHttp: true);...and without that argument, you can't use unencrypted HTTP. If google3 needs something stricter then we could add a lint that disallows using that flag, or something. |
|
@lrhn Sounds good. I would like to adhere to the principles that the SDK follows. I will add a capability similar to |
|
@Hixie while that's tempting, I would like to reduce the scope of this change to the intended platforms. We have many tools written in Dart that run on desktop. While it would be great if https was banned from all of them, I feel it is too big of a change. We can decide this later and just flip the default found in |
|
It's a very small change, just requires one new argument on each HttpClient constructor call site. Given the scope of other breaking changes we are considering on a regular basis (removing implicit casting, default non-nullability, lots of APIs changing to not return null anymore, etc), this seems like a more or less trivial change, and one that is easily handled by lints (e.g. we could add a default-on lint that warns you about the new argument if you don't specify it). |
|
My response to you was regarding platform defaults. I would like to enable this primarily for iOS and Android. I don't want to add a new argument to the HttpClient because many applications do not use dart:io directly. Instead they use package:http or |
|
I edited the proposal above to reflect the discussions. |
Invoking Zones on a security feature seems like a footgun, given the complexity of Zones. I'd really much prefer it just be a constructor argument. Ambient authority (which is what global zone variables, or indeed any global variant, essentially boil down to) generally leads to security bugs. I'm also concerned that setting _EmbedderConfig.allowClearText to false does not disallow cleartext. This also seems like a footgun. Having this differ from platform to platform also seems like a security problem waiting to happen. In general I am increasingly agitated the more I think about this. :-) I really would much rather, if we do anything here at all, that we just always default to In fact, it seems almost identical to the issue of self-signed certificates, for which we already have a solution. Maybe the right thing to do here is for us to actually use that same badCertificateCallback for all non-encrypted connections, but passing it a special cert instance that represents "no cert"? |
|
We definitely need to do something :-). I need @mit-mit or another Dart authority to make a decision on the defaults. I am abiding by policy here which only covers iOS and Android apps. Can you expand on the (I am still not convinced the benefits outweigh the costs of introducing a ctr argument; if it comes to that let's discuss offline.) |
|
I see https://api.dart.dev/stable/2.7.1/dart-io/HttpClient/badCertificateCallback.html. If you are directly using HttpClient, this is probably fine. It would mean it is not a breaking change for classes that implement |
|
I'm still very concerned that we might be addressing this at the wrong level. There is nothing inherently wrong with HTTP connections, but in some situations there is a policy disallowing them. That's business logic, not platform logic. We do need to support business logic choices, which means that the platform needs some hook that can intercept the undesired usage and prevent it if necessary. I would prefer if that logic was general enough that it could be used for other things than just throwing an error. So, if at all possible, I'd like what we do to be generalizable, not too specific. I'm also worried about the incentives that the design puts onto users. If we add the And finally, after reading this thread, the CL, and the linked document, I'm still not sure exactly what the precise requirements are that we are trying to solve for. Is the requirement:
? If that is the case, then we can add one deep hook in If not, what is the actual requirement? |
|
Lasse and I synced offline and are in agreement. His primary concern was exposure of Since the requirement is Flutter only (and the defaults are exposed only to embedders), the allow overrides functionality should only be exposed in embedders as well; not in core Dart. I will move that function to dart:ui or package:flutter. |
This can be configured by embedders by setting the `_embedderAllowsHttp` variable. It can also be overridden by client apps by introducing a `Zone` with the variable `#dart.library.io.allow_http` set to `true` or `false`. The default behavior in SDK is unchanged. Bug: #40548 Change-Id: Ifec0ad2d759de4bbb836644840d8c312e560f285 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/138911 Commit-Queue: Martin Kustermann <[email protected]> Reviewed-by: Jonas Termansen <[email protected]>
This reverts commit 6b44c63. Reason for revert: Tests timing out on Windows Original change's description: > [dart:_http] Allow the embedder to prohibit HTTP traffic. > > This can be configured by embedders by setting the `_embedderAllowsHttp` > variable. It can also be overridden by client apps by introducing a > `Zone` with the variable `#dart.library.io.allow_http` set to `true` or > `false`. > > The default behavior in SDK is unchanged. > > Bug: #40548 > Change-Id: Ifec0ad2d759de4bbb836644840d8c312e560f285 > Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/138911 > Commit-Queue: Martin Kustermann <[email protected]> > Reviewed-by: Jonas Termansen <[email protected]> [email protected],[email protected],[email protected],[email protected] Change-Id: I824a1845e82b5facb839109f74bb482e7095e313 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: #40548 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/142363 Reviewed-by: Aske Simon Christensen <[email protected]> Commit-Queue: Aske Simon Christensen <[email protected]>
This is a reland of 6b44c63 Original change's description: > [dart:_http] Allow the embedder to prohibit HTTP traffic. > > This can be configured by embedders by setting the `_embedderAllowsHttp` > variable. It can also be overridden by client apps by introducing a > `Zone` with the variable `#dart.library.io.allow_http` set to `true` or > `false`. > > The default behavior in SDK is unchanged. > > Bug: #40548 > Change-Id: Ifec0ad2d759de4bbb836644840d8c312e560f285 > Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/138911 > Commit-Queue: Martin Kustermann <[email protected]> > Reviewed-by: Jonas Termansen <[email protected]> Bug: #40548 Change-Id: I6ced6c1248b3b6687f6c7d998e5206b2b385f00b Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/142446 Reviewed-by: Martin Kustermann <[email protected]> Commit-Queue: Martin Kustermann <[email protected]>
… breaks valid local network http traffic Bug: flutter/flutter#72723 Change-Id: Ib5f809fccf1fad69add441fd40c463da8dc8be36 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/192953 Auto-Submit: Xiao Yu <[email protected]> Commit-Queue: Vyacheslav Egorov <[email protected]> Reviewed-by: Vyacheslav Egorov <[email protected]> Reviewed-by: Lasse R.H. Nielsen <[email protected]>
…ates and breaks valid local network http traffic" This reverts commit d84f359. Reason for revert: removed some private looking but actually vm-entrypoints used by Flutter engine https://buganizer.corp.google.com/issues/185462669 Original change's description: > Revert commits part of #40548 which still has some design debates and breaks valid local network http traffic > > Bug: flutter/flutter#72723 > Change-Id: Ib5f809fccf1fad69add441fd40c463da8dc8be36 > Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/192953 > Auto-Submit: Xiao Yu <[email protected]> > Commit-Queue: Vyacheslav Egorov <[email protected]> > Reviewed-by: Vyacheslav Egorov <[email protected]> > Reviewed-by: Lasse R.H. Nielsen <[email protected]> [email protected],[email protected],[email protected] Change-Id: Ie746805ab7ef214bc4ba6f3582eb1d32672360ee No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: flutter/flutter#72723 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/195484 Reviewed-by: Xiao Yu <[email protected]> Commit-Queue: Xiao Yu <[email protected]>
…ates and breaks valid local network http traffic" This is a reland of d84f359 Original change's description: > Revert commits part of #40548 which still has some design debates and breaks valid local network http traffic > > Bug: flutter/flutter#72723 > Change-Id: Ib5f809fccf1fad69add441fd40c463da8dc8be36 > Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/192953 > Auto-Submit: Xiao Yu <[email protected]> > Commit-Queue: Vyacheslav Egorov <[email protected]> > Reviewed-by: Vyacheslav Egorov <[email protected]> > Reviewed-by: Lasse R.H. Nielsen <[email protected]> Bug: flutter/flutter#72723 Change-Id: I466d888b3f324a996f0bef0463e7ab3df3179c56 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/195485 Reviewed-by: Vyacheslav Egorov <[email protected]> Reviewed-by: Lasse R.H. Nielsen <[email protected]> Commit-Queue: Xiao Yu <[email protected]>
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#HttpClientif 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#HttpClientperspective, this just means iOS and Android (not server). We should ban cleartext ifPlatform.isIOSorPlatform.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 ofHttpOverridesbeyond testing (it is already being used beyond testing by some clients).Combining these proposals, we get:
Modify
HttpOverridesto supportallowClearTextas a zone variable.Create an
_EmbedderConfigclass to contain a static configuration as a default. This will be overridden in embedders for iOS and Android.Modify
_HttpClientto ban HTTP scheme only if:See this internal design doc go/disable-http-flutter-dd for a more detailed discussion of alternatives.
The text was updated successfully, but these errors were encountered: