Allow trusted brokers to restrict connections for spawned applications to whitelisted applications …

Allow trusted brokers to restrict connections for spawned applications to whitelisted applications and interfaces.

Currently, filters are not inherited when a restricted application starts a new application. If I did that immediately there is an opportunity for races if a restricted app starts a trusted app before the trusted broker can. I need to think through that in more detail. For now, any filters must be explicitly specified with every call to ConnectToApplication.

I'm not handling content handlers properly yet in this round. TBD.

http://crbug.com/510210

Committed: https://crrev.com/2dd70fcff791aa32b77360c9c8ed413d791e5b47
Cr-Commit-Position: refs/heads/master@{#340352}

[Ben Goodger (Google), 2015-07-22 05:21:38]

ben@chromium.org changed reviewers:
+ jam@chromium.org, sky@chromium.org, yzshen@chromium.org

[Ben Goodger (Google), 2015-07-22 05:22:11]

OK this is a first cut. See the new unit test.

[sky, 2015-07-22 15:57:45]

https://codereview.chromium.org/1244233002/diff/120001/mojo/application/publi...
File mojo/application/public/cpp/application_impl.h (right):

https://codereview.chromium.org/1244233002/diff/120001/mojo/application/publi...
mojo/application/public/cpp/application_impl.h:86: CapabilityFilterPtr filter =
nullptr);
Style guide says no default values.

https://codereview.chromium.org/1244233002/diff/120001/mojo/application/publi...
File mojo/application/public/cpp/lib/application_impl.cc (right):

https://codereview.chromium.org/1244233002/diff/120001/mojo/application/publi...
mojo/application/public/cpp/lib/application_impl.cc:27: struct TypeConverter <
std::set<E>, Array<T> > {
You shouldn't need the extra spaces anymore, eg <std::set<E>, Array<T>> should
work fine with c++11.

https://codereview.chromium.org/1244233002/diff/120001/mojo/application/publi...
mojo/application/public/cpp/lib/application_impl.cc:161:
allowed_interfaces.To<std::set<std::string> >());
nit: '> >' -> '>>'

https://codereview.chromium.org/1244233002/diff/120001/mojo/application/publi...
File mojo/application/public/cpp/lib/service_registry.h (right):

https://codereview.chromium.org/1244233002/diff/120001/mojo/application/publi...
mojo/application/public/cpp/lib/service_registry.h:36: std::set<std::string>
allowed_interfaces);
const std::set<std::string>&

https://codereview.chromium.org/1244233002/diff/120001/mojo/application/publi...
mojo/application/public/cpp/lib/service_registry.h:65: std::set<std::string>
allowed_interfaces_;
nit: const

https://codereview.chromium.org/1244233002/diff/120001/mojo/application/publi...
File mojo/application/public/interfaces/shell.mojom (right):

https://codereview.chromium.org/1244233002/diff/120001/mojo/application/publi...
mojo/application/public/interfaces/shell.mojom:46: // corresponding set of
services to be exposed to the target application.
Should 'to be exposed' be 'that may be exposed'? Since ultimately exposing the
services is up to the other end and for whatever reason the other end may not
expose all services.
Have you considered a string mapping to an empty array should mean the set of
services is not limited? In fact I think the implementation has this behavior.

https://codereview.chromium.org/1244233002/diff/120001/mojo/runner/context.cc
File mojo/runner/context.cc (right):

https://codereview.chromium.org/1244233002/diff/120001/mojo/runner/context.cc...
mojo/runner/context.cc:197: NULL, request.Pass(), std::string(),
GURL("mojo:shell"),
NULL -> nullptr

https://codereview.chromium.org/1244233002/diff/120001/mojo/runner/context.cc...
mojo/runner/context.cc:312: NULL, request.Pass(), std::string(), GURL(""),
nullptr,
NULL -> nullptr

https://codereview.chromium.org/1244233002/diff/120001/mojo/runner/context.cc...
mojo/runner/context.cc:358: NULL, request.Pass(), std::string(), GURL(),
GetProxy(&services),
NULL -> nullptr

https://codereview.chromium.org/1244233002/diff/120001/mojo/runner/native_run...
File mojo/runner/native_runner_unittest.cc (right):

https://codereview.chromium.org/1244233002/diff/120001/mojo/runner/native_run...
mojo/runner/native_runner_unittest.cc:100: NULL, request.Pass(), std::string(),
GURL(), services.Pass(),
NULL -> nullptr

https://codereview.chromium.org/1244233002/diff/120001/mojo/runner/shell_test...
File mojo/runner/shell_test_base.cc (right):

https://codereview.chromium.org/1244233002/diff/120001/mojo/runner/shell_test...
mojo/runner/shell_test_base.cc:54: NULL, request.Pass(), std::string(), GURL(),
GetProxy(&services), nullptr,
NULL -> nullptr

https://codereview.chromium.org/1244233002/diff/120001/mojo/shell/application...
File mojo/shell/application_instance.cc (right):

https://codereview.chromium.org/1244233002/diff/120001/mojo/shell/application...
mojo/shell/application_instance.cc:95: }
Is it worth an else and a DVLOG/VLOG?

https://codereview.chromium.org/1244233002/diff/120001/mojo/shell/application...
File mojo/shell/application_instance.h (right):

https://codereview.chromium.org/1244233002/diff/120001/mojo/shell/application...
mojo/shell/application_instance.h:51: using AllowedInterfaces = std::set <
std::string > ;
nit: no need for spaces here and on next line.

https://codereview.chromium.org/1244233002/diff/120001/mojo/shell/capability_...
File mojo/shell/capability_filter_unittest.cc (right):

https://codereview.chromium.org/1244233002/diff/120001/mojo/shell/capability_...
mojo/shell/capability_filter_unittest.cc:1: // Copyright 2014 The Chromium
Authors. All rights reserved.
2015

https://codereview.chromium.org/1244233002/diff/120001/mojo/shell/content_han...
File mojo/shell/content_handler_connection.cc (right):

https://codereview.chromium.org/1244233002/diff/120001/mojo/shell/content_han...
mojo/shell/content_handler_connection.cc:27: NULL, request.Pass(), qualifier,
requestor_url, GetProxy(&services),
NULL->nullptr

[yzshen1, 2015-07-22 17:53:58]

https://codereview.chromium.org/1244233002/diff/120001/mojo/application/publi...
File mojo/application/public/cpp/lib/service_registry.cc (right):

https://codereview.chromium.org/1244233002/diff/120001/mojo/application/publi...
mojo/application/public/cpp/lib/service_registry.cc:42: if
(allowed_interfaces_.empty() ||
Please see my comment in the shell.mojom.

https://codereview.chromium.org/1244233002/diff/160001/mojo/application/publi...
File mojo/application/public/interfaces/shell.mojom (right):

https://codereview.chromium.org/1244233002/diff/160001/mojo/application/publi...
mojo/application/public/interfaces/shell.mojom:51: // any application entry in
the mapping presents an empty set of services,
I feel that it is possible that the trusted broker may allow an app A to connect
to another app B, but disallow B to expose any interface to A. (For example, A
exposes interfaces to B but not the other way around.)

In that case, it seems natural to pass {"B" -> *empty* or *null* Array} into 
ConnectToApplication when launching A, meaning *no* interface allowed.

I think we should have another way to express *all* allowed. Maybe we should use
an Array containing a "*"?

[Ben Goodger (Google), 2015-07-22 18:00:10]

On 2015/07/22 17:53:58, yzshen1 wrote:
>
https://codereview.chromium.org/1244233002/diff/120001/mojo/application/publi...
> File mojo/application/public/cpp/lib/service_registry.cc (right):
> 
>
https://codereview.chromium.org/1244233002/diff/120001/mojo/application/publi...
> mojo/application/public/cpp/lib/service_registry.cc:42: if
> (allowed_interfaces_.empty() ||
> Please see my comment in the shell.mojom.
> 
>
https://codereview.chromium.org/1244233002/diff/160001/mojo/application/publi...
> File mojo/application/public/interfaces/shell.mojom (right):
> 
>
https://codereview.chromium.org/1244233002/diff/160001/mojo/application/publi...
> mojo/application/public/interfaces/shell.mojom:51: // any application entry in
> the mapping presents an empty set of services,
> I feel that it is possible that the trusted broker may allow an app A to
connect
> to another app B, but disallow B to expose any interface to A. (For example, A
> exposes interfaces to B but not the other way around.)
> 
> In that case, it seems natural to pass {"B" -> *empty* or *null* Array} into 
> ConnectToApplication when launching A, meaning *no* interface allowed.
> 
> I think we should have another way to express *all* allowed. Maybe we should
use
> an Array containing a "*"?

Good point. I'll work on this now.

[jam, 2015-07-22 19:47:40]

please reference bug
lgtm

https://codereview.chromium.org/1244233002/diff/160001/mojo/shell/application...
File mojo/shell/application_instance.h (right):

https://codereview.chromium.org/1244233002/diff/160001/mojo/shell/application...
mojo/shell/application_instance.h:51: using AllowedInterfaces = std::set
<std::string> ;
nit: here and below extra spaces before and after brackets

https://codereview.chromium.org/1244233002/diff/160001/mojo/shell/application...
File mojo/shell/application_manager.cc (right):

https://codereview.chromium.org/1244233002/diff/160001/mojo/shell/application...
mojo/shell/application_manager.cc:531: ConnectToApplication(NULL,
request.Pass(), std::string(), GURL(),
nit: nullptr

https://codereview.chromium.org/1244233002/diff/160001/mojo/shell/test.mojom
File mojo/shell/test.mojom (right):

https://codereview.chromium.org/1244233002/diff/160001/mojo/shell/test.mojom#...
mojo/shell/test.mojom:27: // mojo:restricted, one is limited by the broker to
only see Exposed while the
nit: update comment to match names below

[Ben Goodger (Google), 2015-07-22 21:56:28]

OK, I think I got everything here, including the wildcards (and some new tests).

I'd like to add content handler support in a separate CL, since this one is
already pretty big.

Please take another look.

[yzshen1, 2015-07-22 22:26:47]

https://codereview.chromium.org/1244233002/diff/160001/mojo/shell/application...
File mojo/shell/application_instance.h (right):

https://codereview.chromium.org/1244233002/diff/160001/mojo/shell/application...
mojo/shell/application_instance.h:26: struct TypeConverter <std::set<E>,
Array<T>> {
Please consider share this code with application_impl.cc.

https://codereview.chromium.org/1244233002/diff/160001/mojo/shell/application...
File mojo/shell/application_manager.cc (right):

https://codereview.chromium.org/1244233002/diff/160001/mojo/shell/application...
mojo/shell/application_manager.cc:531: ConnectToApplication(NULL,
request.Pass(), std::string(), GURL(),
nit: please consider nullptr.

https://codereview.chromium.org/1244233002/diff/220001/mojo/application/publi...
File mojo/application/public/interfaces/shell.mojom (right):

https://codereview.chromium.org/1244233002/diff/220001/mojo/application/publi...
mojo/application/public/interfaces/shell.mojom:15: // may connect to any other
application and potentially any service that is
Should it be {"*": [ "*" ]} to mean that it allows to connect to any application
and any service that is exposed?

{"*": []} seems natural to mean that the app is allowed to connect to any app
but cannot get any service from them. (For example, this app can connect to any
app and provide service for them, but doesn't require any service in return.)

https://codereview.chromium.org/1244233002/diff/220001/mojo/shell/application...
File mojo/shell/application_instance.cc (right):

https://codereview.chromium.org/1244233002/diff/220001/mojo/shell/application...
mojo/shell/application_instance.cc:56: CapabilityFilterPtr filter) {
It seems this filter is ignored (either on line 69 or stored in
queued_client_requests_ but later ignored on line 158).

[yzshen1, 2015-07-22 23:46:21]

Hi, Ben.

I looked at mojo/application/public/cpp/BUILD.gn and it is already depending on
//mojo/common:
https://code.google.com/p/chromium/codesearch#chromium/src/mojo/application/p...

So it seems okay to put the type converter in 
//src/mojo/common/common_type_converters.h

What do you think? Thanks!

[Ben Goodger (Google), 2015-07-23 05:44:26]

On 2015/07/22 23:46:21, yzshen1 wrote:
> Hi, Ben.
> 
> I looked at mojo/application/public/cpp/BUILD.gn and it is already depending
on
> //mojo/common:
>
https://code.google.com/p/chromium/codesearch#chromium/src/mojo/application/p...
> 
> So it seems okay to put the type converter in 
> //src/mojo/common/common_type_converters.h
> 
> What do you think? Thanks!

Yeah I think mojo/application/public probably shouldn't depend on those. I need
to think about that a bit. When it was upstream it obviously didn't depend on
anything outside of mojo/public since it was part of the core SDK. I think it
began to when we forked & John moved some stuff around. 

BTW I just realized my tests are broken (in that they pass, but wrongly).
They're quitting too soon. I need to implement a better way of logging blocked
interfaces, working on that now.

[Ben Goodger (Google), 2015-07-24 03:28:04]

OK I redid the tests. PTAL.

[yzshen1, 2015-07-24 08:02:09]

LGTM

https://codereview.chromium.org/1244233002/diff/380001/mojo/shell/application...
File mojo/shell/application_instance.cc (right):

https://codereview.chromium.org/1244233002/diff/380001/mojo/shell/application...
mojo/shell/application_instance.cc:56: CapabilityFilterPtr filter) {
(I probably have missed something obvious...)

This |filter| is ignored -- even if we store it into queued_client_requests_, we
ignore it later when use the request. Is that intentional?

[sky, 2015-07-24 15:36:55]

https://codereview.chromium.org/1244233002/diff/380001/mojo/application/publi...
File mojo/application/public/cpp/application_connection.h (right):

https://codereview.chromium.org/1244233002/diff/380001/mojo/application/publi...
mojo/application/public/cpp/application_connection.h:107: virtual void
SetConnectionErrorHandler(const Closure& handler) = 0;
Do you think this should be SetServiceProviderConnectionHandler() to reinforce
what it relates to? The reason I suggest this name is your comment and name
imply the connection is no longer valid, which isn't right. For example, any
services vended from the remove service provider may still be valid. This error
handler just means the remote service provider is no longer valid.

https://codereview.chromium.org/1244233002/diff/380001/mojo/application/publi...
mojo/application/public/cpp/application_connection.h:116: virtual bool
SetServiceConnectorForName(ServiceConnector* service_connector,
Document return value.

https://codereview.chromium.org/1244233002/diff/380001/mojo/application/publi...
File mojo/application/public/cpp/lib/service_registry.h (right):

https://codereview.chromium.org/1244233002/diff/380001/mojo/application/publi...
mojo/application/public/cpp/lib/service_registry.h:29: // an application to
expose to another application. If this set is empty, the
Isn't allow_all only true if the set contains "*"?

https://codereview.chromium.org/1244233002/diff/380001/mojo/application/publi...
mojo/application/public/cpp/lib/service_registry.h:67: const bool
allow_all_interfaces_;
nit: As you can calculate allow_all_interfaces_ from allowed_interfaces_, and it
isn't that expensive, I would be inclined to nuke the member and instead have an
inline function allow_all_interfaces().

https://codereview.chromium.org/1244233002/diff/380001/mojo/application/publi...
File mojo/application/public/interfaces/application.mojom (right):

https://codereview.chromium.org/1244233002/diff/380001/mojo/application/publi...
mojo/application/public/interfaces/application.mojom:51: // application. When
this parameter is empty, this application is free to
Update comment about empty.

[Ben Goodger (Google), 2015-07-24 17:29:30]

Scott, updated PTAL

https://codereview.chromium.org/1244233002/diff/380001/mojo/shell/application...
File mojo/shell/application_instance.cc (right):

https://codereview.chromium.org/1244233002/diff/380001/mojo/shell/application...
mojo/shell/application_instance.cc:56: CapabilityFilterPtr filter) {
On 2015/07/24 08:02:09, yzshen1 wrote:
> (I probably have missed something obvious...)
> 
> This |filter| is ignored -- even if we store it into queued_client_requests_,
we
> ignore it later when use the request. Is that intentional?

I use it on line 151 below if the connection is broken. ConnectToApplication()
requires a valid filter in that case.

[Ben Goodger (Google), 2015-07-24 17:31:52]

The CQ bit was checked by ben@chromium.org

[Ben Goodger (Google), 2015-07-24 17:31:53]

The patchset sent to the CQ was uploaded after l-g-t-m from jam@chromium.org,
yzshen@chromium.org
Link to the patchset: https://codereview.chromium.org/1244233002/#ps400001
(title: ".")

[commit-bot: I haz the power, 2015-07-24 17:32:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1244233002/400001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1244233002/400001

[commit-bot: I haz the power, 2015-07-24 18:03:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-07-24 18:03:49]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Ben Goodger (Google), 2015-07-24 19:33:28]

The CQ bit was checked by ben@chromium.org

[commit-bot: I haz the power, 2015-07-24 19:34:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1244233002/400001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1244233002/400001

[commit-bot: I haz the power, 2015-07-24 20:31:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-07-24 20:31:07]

Try jobs failed on following builders:
  linux_chromium_gn_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Ben Goodger (Google), 2015-07-24 20:40:41]

The CQ bit was checked by ben@chromium.org

[Ben Goodger (Google), 2015-07-24 20:40:42]

The patchset sent to the CQ was uploaded after l-g-t-m from yzshen@chromium.org,
jam@chromium.org
Link to the patchset: https://codereview.chromium.org/1244233002/#ps420001
(title: ".")

[commit-bot: I haz the power, 2015-07-24 20:41:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1244233002/420001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1244233002/420001

[commit-bot: I haz the power, 2015-07-24 22:22:18]

Committed patchset #22 (id:420001)

[commit-bot: I haz the power, 2015-07-24 22:23:19]

Patchset 22 (id:??) landed as
https://crrev.com/2dd70fcff791aa32b77360c9c8ed413d791e5b47
Cr-Commit-Position: refs/heads/master@{#340352}

[xhwang, 2015-07-25 07:57:35]

xhwang@chromium.org changed reviewers:
+ xhwang@chromium.org

[xhwang, 2015-07-25 07:57:36]

https://codereview.chromium.org/1244233002/diff/420001/mojo/shell/application...
File mojo/shell/application_instance.cc (right):

https://codereview.chromium.org/1244233002/diff/420001/mojo/shell/application...
mojo/shell/application_instance.cc:147: manager->ConnectToApplication(this,
url.Pass(), std::string(),
It seems we are using |this| after it's deleted on line 140. This is causing
crashes in my CL (https://codereview.chromium.org/1249643004/).

I am not super familiar with this code so I don't what's the best fix. Shall we
just pass nullptr here?

[Ben Goodger (Google), 2015-07-27 17:47:47]

https://codereview.chromium.org/1244233002/diff/420001/mojo/shell/application...
File mojo/shell/application_instance.cc (right):

https://codereview.chromium.org/1244233002/diff/420001/mojo/shell/application...
mojo/shell/application_instance.cc:147: manager->ConnectToApplication(this,
url.Pass(), std::string(),
Hrm. Yes my code seems wrong. Seems more correct though to store the originator
in the ctor and pass it here. We would have to track the lifetime of the
originator.

[Ben Goodger (Google), 2015-07-27 19:11:03]

On 2015/07/27 17:47:47, Ben Goodger (Google) wrote:
>
https://codereview.chromium.org/1244233002/diff/420001/mojo/shell/application...
> File mojo/shell/application_instance.cc (right):
> 
>
https://codereview.chromium.org/1244233002/diff/420001/mojo/shell/application...
> mojo/shell/application_instance.cc:147: manager->ConnectToApplication(this,
> url.Pass(), std::string(),
> Hrm. Yes my code seems wrong. Seems more correct though to store the
originator
> in the ctor and pass it here. We would have to track the lifetime of the
> originator.

See: https://codereview.chromium.org/1257133003/

[xhwang, 2015-07-27 19:26:14]

On 2015/07/27 19:11:03, Ben Goodger (Google) wrote:
> On 2015/07/27 17:47:47, Ben Goodger (Google) wrote:
> >
>
https://codereview.chromium.org/1244233002/diff/420001/mojo/shell/application...
> > File mojo/shell/application_instance.cc (right):
> > 
> >
>
https://codereview.chromium.org/1244233002/diff/420001/mojo/shell/application...
> > mojo/shell/application_instance.cc:147: manager->ConnectToApplication(this,
> > url.Pass(), std::string(),
> > Hrm. Yes my code seems wrong. Seems more correct though to store the
> originator
> > in the ctor and pass it here. We would have to track the lifetime of the
> > originator.
> 
> See: https://codereview.chromium.org/1257133003/

Thanks!