Add an identity component for unique/opaque url::Origins.

Add an identity component for unique/opaque url::Origins.

BUG=many

[dcheng, 2017-02-23 23:18:13]

dcheng@chromium.org changed reviewers:
+ mkwst@chromium.org

[dcheng, 2017-02-23 23:18:14]

Good idea, terrible idea? I tried to union it originally to save space, but that
turned out poorly. It's still possible to union, but then I need to change the
scheme/host/suborigin accessors to return a StringPiece instead.

Note that this patch is incomplete without:
- Plumbing it through ParamTraits
- Plumbing it through StructTraits
- Plumbing it through Blink

But this will let us get rid of pointer identity origin comparisons in Blink,
and fix issues with unique (opaque?) origin checks with url::Origin.

If no one feels too negatively about it, I'll add the Blink-side plumbing and
make it possible to roundtrip.

[dcheng, 2017-02-23 23:26:12]

+juncai, +ortuno as well

I need to update this CL, but with tweaks to operator<, we can also support
multiple unique origins in a map or set.

[ortuno, 2017-02-23 23:42:25]

On 2017/02/23 at 23:26:12, dcheng wrote:
> +juncai, +ortuno as well
> 
> I need to update this CL, but with tweaks to operator<, we can also support
multiple unique origins in a map or set.

Oh cool. I would have to check if PermissionManager supports opaque origins
before allowing them in web bluetooth.

[ortuno, 2017-02-24 01:59:01]

On 2017/02/23 at 23:42:25, ortuno wrote:
> On 2017/02/23 at 23:26:12, dcheng wrote:
> > +juncai, +ortuno as well
> > 
> > I need to update this CL, but with tweaks to operator<, we can also support
multiple unique origins in a map or set.
> 
> Oh cool. I would have to check if PermissionManager supports opaque origins
before allowing them in web bluetooth.

I see this is using randbytes. Shouldn't we try to make the id stable? Otherwise
any permission request for unique/opaque origins will look like a new request
after restart.

[palmer, 2017-02-24 02:03:03]

If possible, include a few actual bugs in the BUGS= line.

It'd be good to document precisely what attack 16-byte cryptographic identifiers
defend against. For example, are we concerned about a compromised renderer
guessing, by brute-force, the unique ID that the browser process is using to
make some security distinction? I imagine some future maintainer might come
along and, without a detailed comment to the contrary, try to save some memory
by reducing the size of the ID.

[dcheng, 2017-02-24 02:41:48]

On 2017/02/24 01:59:01, ortuno wrote:
> On 2017/02/23 at 23:42:25, ortuno wrote:
> > On 2017/02/23 at 23:26:12, dcheng wrote:
> > > +juncai, +ortuno as well
> > > 
> > > I need to update this CL, but with tweaks to operator<, we can also
support
> multiple unique origins in a map or set.
> > 
> > Oh cool. I would have to check if PermissionManager supports opaque origins
> before allowing them in web bluetooth.
> 
> I see this is using randbytes. Shouldn't we try to make the id stable?
Otherwise
> any permission request for unique/opaque origins will look like a new request
> after restart.

Hmm... unique origins aren't persistable ATM, and to my knowledge, that's
working as intended. If we wanted to change that, that would be a separate CL
with massive implications.

[Mike West, 2017-02-24 07:58:50]

I like the direction, but I'm a bit concerned about the implementation as it
seems like it's going to continue to diverge from Blink's understanding of
'equal' uniquenesses. Do you have a proposal for ensuring that an opaque
`url::Origin` creates a `SecurityOrigin` that it compares to as `==`? Or
vice-versa?

This seems like a fine approach for everything outside of Blink, but it seems
valuable and important to plan for linking them up in some way.

https://codereview.chromium.org/2714813003/diff/20001/url/origin.cc
File url/origin.cc (right):

https://codereview.chromium.org/2714813003/diff/20001/url/origin.cc#newcode112
url/origin.cc:112: suborigin_ = std::move(suborigin);
I wonder if suborigins should have an effect on the equality of opaque origins?
(+jochen@, who is now responsible for all of this ;) )

https://codereview.chromium.org/2714813003/diff/20001/url/origin.h
File url/origin.h (right):

https://codereview.chromium.org/2714813003/diff/20001/url/origin.h#newcode53
url/origin.h:53: //   serializations must not be relied upon for security
checks.
Please update these comments as well.

[Mike West, 2017-02-24 07:59:28]

mkwst@chromium.org changed reviewers:
+ iclelland@chromium.org

[Mike West, 2017-02-24 07:59:29]

Also, +iclelland@, whose https://codereview.chromium.org/2714813003 might be
obviated by this change.

[dcheng, 2017-02-24 08:19:30]

On 2017/02/24 07:58:50, Mike West (sloooooow) wrote:
> I like the direction, but I'm a bit concerned about the implementation as it
> seems like it's going to continue to diverge from Blink's understanding of
> 'equal' uniquenesses. Do you have a proposal for ensuring that an opaque
> `url::Origin` creates a `SecurityOrigin` that it compares to as `==`? Or
> vice-versa?

Yes, it needs to round trip through IPC and conversions to/from Blink. The
latest PS adds the unique token to Blink's SecurityOrigin, but doesn't update
the conversion operators yet.

I'm trying to figure out an implementation strategy that will avoid exposing the
unique token outside the class, though that's a bit tricky with three different
classes in three different layers...

> 
> This seems like a fine approach for everything outside of Blink, but it seems
> valuable and important to plan for linking them up in some way.
> 
> https://codereview.chromium.org/2714813003/diff/20001/url/origin.cc
> File url/origin.cc (right):
> 
> https://codereview.chromium.org/2714813003/diff/20001/url/origin.cc#newcode112
> url/origin.cc:112: suborigin_ = std::move(suborigin);
> I wonder if suborigins should have an effect on the equality of opaque
origins?
> (+jochen@, who is now responsible for all of this ;) )
> 
> https://codereview.chromium.org/2714813003/diff/20001/url/origin.h
> File url/origin.h (right):
> 
> https://codereview.chromium.org/2714813003/diff/20001/url/origin.h#newcode53
> url/origin.h:53: //   serializations must not be relied upon for security
> checks.
> Please update these comments as well.

I'll respond to the rest of the comments after I figure out conversions.

[Mike West, 2017-02-24 08:29:26]

On 2017/02/24 at 08:19:30, dcheng wrote:
> On 2017/02/24 07:58:50, Mike West (sloooooow) wrote:
> > I like the direction, but I'm a bit concerned about the implementation as it
> > seems like it's going to continue to diverge from Blink's understanding of
> > 'equal' uniquenesses. Do you have a proposal for ensuring that an opaque
> > `url::Origin` creates a `SecurityOrigin` that it compares to as `==`? Or
> > vice-versa?
> 
> Yes, it needs to round trip through IPC and conversions to/from Blink. The
latest PS adds the unique token to Blink's SecurityOrigin, but doesn't update
the conversion operators yet.
> 
> I'm trying to figure out an implementation strategy that will avoid exposing
the unique token outside the class, though that's a bit tricky with three
different classes in three different layers...

Can we not use three different classes in three different layers? Like, could
`SecurityOrigin` wrap `url::Origin` in the same way `KURL` wraps `url::Parsed`?
That might give us a path to getting rid of the distinctions at some point in
The Glorious Future.

[dcheng, 2017-02-24 08:33:29]

On 2017/02/24 08:29:26, Mike West (sloooooow) wrote:
> On 2017/02/24 at 08:19:30, dcheng wrote:
> > On 2017/02/24 07:58:50, Mike West (sloooooow) wrote:
> > > I like the direction, but I'm a bit concerned about the implementation as
it
> > > seems like it's going to continue to diverge from Blink's understanding of
> > > 'equal' uniquenesses. Do you have a proposal for ensuring that an opaque
> > > `url::Origin` creates a `SecurityOrigin` that it compares to as `==`? Or
> > > vice-versa?
> > 
> > Yes, it needs to round trip through IPC and conversions to/from Blink. The
> latest PS adds the unique token to Blink's SecurityOrigin, but doesn't update
> the conversion operators yet.
> > 
> > I'm trying to figure out an implementation strategy that will avoid exposing
> the unique token outside the class, though that's a bit tricky with three
> different classes in three different layers...
> 
> Can we not use three different classes in three different layers? Like, could
> `SecurityOrigin` wrap `url::Origin` in the same way `KURL` wraps
`url::Parsed`?
> That might give us a path to getting rid of the distinctions at some point in
> The Glorious Future.

That would certainly make this patch easier, but as comments in
WebSecurityOrigin note, url::Origin support is missing a number of other things,
such as document.domain, a bit for whether or not a unique origin is potentially
trustworthy, etc.

I /think/ it might be easier to bring url::Origin up to feature parity before
trying to swap out the implementation of SecurityOrigin to delegate to
url::Origin.

[Mike West, 2017-02-24 08:40:43]

On 2017/02/24 at 08:33:29, dcheng wrote:
> I /think/ it might be easier to bring url::Origin up to feature parity before
trying to swap out the implementation of SecurityOrigin to delegate to
url::Origin.

I'll defer to you since you're doing the work, but it seems just as easy to
layer `SecurityOrigin` on top of `url::Origin`, and merge the layers over time.

(Also, `base::UnguessableToken` is awesome. ;) )

[dcheng, 2017-02-24 08:48:46]

On 2017/02/24 08:40:43, Mike West (sloooooow) wrote:
> On 2017/02/24 at 08:33:29, dcheng wrote:
> > I /think/ it might be easier to bring url::Origin up to feature parity
before
> trying to swap out the implementation of SecurityOrigin to delegate to
> url::Origin.
> 
> I'll defer to you since you're doing the work, but it seems just as easy to
> layer `SecurityOrigin` on top of `url::Origin`, and merge the layers over
time.
> 
> (Also, `base::UnguessableToken` is awesome. ;) )

Ah, I misunderstood what you meant. That's an interesting idea... how much do we
care about the cost of conversions? One potential advantage is getting
scheme()/host()/suborigin() are essentially free now (they copy the String, but
it's just a refcount increment in Blink). If we implemented something like
KURL+url::Parsed, we'd have to construct a new String for each call.

[Charlie Harrison, 2017-02-24 12:57:47]

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

[Charlie Harrison, 2017-02-24 12:57:48]

Making conversions fast is pretty important to me, especially if we care about
making tons of resource requests.

Note that we also do Origin->GURL conversions which aren't super cheap either,
so I was actually considering making Origin / SecurityOrigin both do a
url::Parsed thing, so we don't have to build up a std::string using append for
Origin->GURL conversion.