Don't persist HPKP if PrivacyMode is enabled.

Don't persist HPKP if PrivacyMode is enabled.

BUG=258667
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=224269

[mef, 2013-07-16 17:32:29]

Hi Chris & Ryan,

please take a look on my change to disable persisting of HPKP if privacy mode is
enabled for the request.

thanks,
-m

[palmer, 2013-07-16 18:19:34]

I think this CL is good as far as it goes, but unfortunately sleevi and I didn't
make it obvious that the problem applies to HSTS metadata as well as to HPKP
metadata. Sorry about that! But I think broadening your CL to include HSTS will
not be too hard.

[mef, 2013-07-16 18:28:15]

On 2013/07/16 18:19:34, Chromium Palmer wrote:
> I think this CL is good as far as it goes, but unfortunately sleevi and I
didn't
> make it obvious that the problem applies to HSTS metadata as well as to HPKP
> metadata. Sorry about that! But I think broadening your CL to include HSTS
will
> not be too hard.

Thanks! I think I might have misinterpreted Ryan's note. 

I've got few questions to clarify:

- In privacy mode we shouldn't persist not only HPKP, but the entire
DomainState, correct?
- In privacy mode do we still need to keep HSTS and HPKP in memory during
session?
- What should happen if DomainState has previously been persisted and now it is
being accessed in Privacy Mode?

thanks,
-m

[palmer, 2013-07-16 18:38:07]

> - In privacy mode we shouldn't persist not only HPKP, but the entire
> DomainState, correct?

Correct.

> - In privacy mode do we still need to keep HSTS and HPKP in memory during
> session?

Now that you mention it, I am inclined to think that all URLRequests that are in
privacy mode should not cause any updates to a DomainState for the relevant
host. I.e. you should avoid calling AddH{STS,PKP}Header at all. I think. There
is a risk that hosts seeking to do implicit tracking would be able to track the
UA even just in the one session, and not just across sessions via persisted
HSTS/HPKP/et c. metadata.

> - What should happen if DomainState has previously been persisted and now it
is
> being accessed in Privacy Mode?

I would say, apply the persisted state as normal, but don't do any updates to
that state.

However, let's wait and see what rsleevi thinks about all this. I am not 100%
sure I am right. I think he is back from vacation tomorrow.

[mef, 2013-07-16 18:44:29]

On 2013/07/16 18:38:07, Chromium Palmer wrote:
> > - In privacy mode we shouldn't persist not only HPKP, but the entire
> > DomainState, correct?
> 
> Correct.
> 
> > - In privacy mode do we still need to keep HSTS and HPKP in memory during
> > session?
> 
> Now that you mention it, I am inclined to think that all URLRequests that are
in
> privacy mode should not cause any updates to a DomainState for the relevant
> host. I.e. you should avoid calling AddH{STS,PKP}Header at all. I think. There
> is a risk that hosts seeking to do implicit tracking would be able to track
the
> UA even just in the one session, and not just across sessions via persisted
> HSTS/HPKP/et c. metadata.
> 
> > - What should happen if DomainState has previously been persisted and now it
> is
> > being accessed in Privacy Mode?
> 
> I would say, apply the persisted state as normal, but don't do any updates to
> that state.
> 
> However, let's wait and see what rsleevi thinks about all this. I am not 100%
> sure I am right. I think he is back from vacation tomorrow.

Thanks for clarification and sounds good to wait for Ryan. 

After reading
http://www.leviathansecurity.com/blog/the-double-edged-sword-of-hsts-persiste...

I agree that in privacy mode it would make sense not to update DomainState for
relevant host at all.


thanks,
-m

[Ryan Sleevi, 2013-07-17 22:16:31]

On 2013/07/16 18:38:07, Chromium Palmer wrote:
> > - In privacy mode we shouldn't persist not only HPKP, but the entire
> > DomainState, correct?
> 
> Correct.
> 
> > - In privacy mode do we still need to keep HSTS and HPKP in memory during
> > session?
> 
> Now that you mention it, I am inclined to think that all URLRequests that are
in
> privacy mode should not cause any updates to a DomainState for the relevant
> host. I.e. you should avoid calling AddH{STS,PKP}Header at all. I think. There
> is a risk that hosts seeking to do implicit tracking would be able to track
the
> UA even just in the one session, and not just across sessions via persisted
> HSTS/HPKP/et c. metadata.
> 
> > - What should happen if DomainState has previously been persisted and now it
> is
> > being accessed in Privacy Mode?
> 
> I would say, apply the persisted state as normal, but don't do any updates to
> that state.
> 
> However, let's wait and see what rsleevi thinks about all this. I am not 100%
> sure I am right. I think he is back from vacation tomorrow.

I'm always deeply confused about what 'privacy mode' is and how this relates to
actual requests. My ignorance knows no ends.

I (perhaps incorrectly) understand it to be the distinction between first
party/third-party requests. In the case of first party, we both send cookies and
respect Set-Cookie headers. In the case of third party, we send cookies (if
any), but we do not respect Set-Cookie headers.

If this is a correct understanding, then we should respect/use existing
DomainState, but we should not update (in memory or on disk) any new
DomainStates - eg: Don't use AddH{STS,PKP}Header, the same way we don't
AddCookies for Set-Cookie headers (however we handle that today).

If privacy mode means we do not *send* cookies AND we do not *set* cookies, then
it's a bit trickier - the response is contingent upon whether privacy mode
requests can trigger *other* privacy mode requests. If they cannot (that is,
each privacy mode request is 'independent', and always a 'child of' a
first-party request), then we should RESPECT HSTS/HPKP, but not PERSIST
HSTS/HPKP.

However, if privacy mode can trigger ADDITIONAL requests (eg: request A can
cause Request B and Request C to be issued, and can distinguish results), then I
think we're in a trickier position. This is because a hostile party (eg:
malicious advertiser) could, by causing a first party navigation, set an HSTS
identifier (using 32 subdomains). Then, when issued a third-party request, they
can kick off the 32 child requests and recover that identifier - in effect,
using it as a cookie.

I don't know all the privacy mode side, but hopefully that clarifies where the
risks are.

[palmer, 2013-07-17 22:58:36]

> I (perhaps incorrectly) understand it to be the distinction between first
> party/third-party requests. In the case of first party, we both send cookies
and
> respect Set-Cookie headers. In the case of third party, we send cookies (if
> any), but we do not respect Set-Cookie headers.

I also think it means that.

> If this is a correct understanding, then we should respect/use existing
> DomainState, but we should not update (in memory or on disk) any new
> DomainStates - eg: Don't use AddH{STS,PKP}Header, the same way we don't
> AddCookies for Set-Cookie headers (however we handle that today).

Agreed.

> If privacy mode means we do not *send* cookies AND we do not *set* cookies,
then
> it's a bit trickier - the response is contingent upon whether privacy mode
> requests can trigger *other* privacy mode requests. If they cannot (that is,
> each privacy mode request is 'independent', and always a 'child of' a
> first-party request), then we should RESPECT HSTS/HPKP, but not PERSIST
> HSTS/HPKP.

Inclined to agree.

> However, if privacy mode can trigger ADDITIONAL requests (eg: request A can
> cause Request B and Request C to be issued, and can distinguish results), then
I
> think we're in a trickier position. This is because a hostile party (eg:
> malicious advertiser) could, by causing a first party navigation, set an HSTS
> identifier (using 32 subdomains). Then, when issued a third-party request,
they
> can kick off the 32 child requests and recover that identifier - in effect,
> using it as a cookie.

In which case we should neither respect nor persist HSTS/HPKP.

mef, do you know which understanding of privacy mode is true? Sounds rsleevi and
I (and probably you) all agree, either way.

[mef, 2013-07-18 15:33:28]

Hi Ryan,

I've introduced the notion of Privacy Mode to allow granular usage of channel id
on different connections.
It is based on Dirk Balfanz's proposal here:
https://docs.google.com/a/google.com/document/d/1Tz9KEZwgQNWCGe6GnzpUbRxa1iK-...

It is based on cookie settings for particular url request taking in account
first and third party url and settings.

Privacy Mode is enabled (and channel id not sent) if user settings prohibit
cookies to be set or send for particular request. Privacy mode is disabled (and
channel id is sent) if cookies are allowed for particular request.

Basic idea is to prevent potential usage of channel id for tracking purpose if
user doesn't want to be tracked by particular host. This may or may not depend
on third party vs first party settings.

The original ticket is here: http://crbug.com/223191.

A bit later you've pointed out ssl session shards, and MattM brought up the idea
that the notion of Privacy Mode is also useful outside of SSL and channel id,
and should split http socket pools as well, so requests with cookies allowed and
disallowed wouldn't go over the same connection.

That is implemented here: http://crbug.com/244992.

Please also see my comments below.

> I'm always deeply confused about what 'privacy mode' is and how this relates
to
> actual requests. My ignorance knows no ends.
I think it is my fault, is it worth documenting 'privacy mode' better? If yes,
then how?

> I (perhaps incorrectly) understand it to be the distinction between first
> party/third-party requests. In the case of first party, we both send cookies
and
> respect Set-Cookie headers. In the case of third party, we send cookies (if
> any), but we do not respect Set-Cookie headers.
I'd say that Privacy Mode doesn't determine cookie policy, but uses it to enable
and disable other tracking features.

> If this is a correct understanding, then we should respect/use existing
> DomainState, but we should not update (in memory or on disk) any new
> DomainStates - eg: Don't use AddH{STS,PKP}Header, the same way we don't
> AddCookies for Set-Cookie headers (however we handle that today).
> 
> If privacy mode means we do not *send* cookies AND we do not *set* cookies,
then
> it's a bit trickier - the response is contingent upon whether privacy mode
> requests can trigger *other* privacy mode requests. If they cannot (that is,
> each privacy mode request is 'independent', and always a 'child of' a
> first-party request), then we should RESPECT HSTS/HPKP, but not PERSIST
> HSTS/HPKP.
I'd say that each request is independent in regards to 'privacy mode'.

> However, if privacy mode can trigger ADDITIONAL requests (eg: request A can
> cause Request B and Request C to be issued, and can distinguish results), then
I
> think we're in a trickier position. This is because a hostile party (eg:
> malicious advertiser) could, by causing a first party navigation, set an HSTS
> identifier (using 32 subdomains). Then, when issued a third-party request,
they
> can kick off the 32 child requests and recover that identifier - in effect,
> using it as a cookie.
It sounds like that if Privacy Mode is enabled for the request, then we
shouldn't neither RESPECT nor PERSIST HSTS/HPKP.

Does this make sense?

thanks,
-m

[mef, 2013-07-18 15:35:51]

On 2013/07/17 22:58:36, Chromium Palmer wrote:
> 
> In which case we should neither respect nor persist HSTS/HPKP.
> 
> mef, do you know which understanding of privacy mode is true? Sounds rsleevi
and
> I (and probably you) all agree, either way.

I agree. See my previous reply for some clarification of privacy mode.

[Ryan Sleevi, 2013-07-18 16:57:10]

On 2013/07/18 15:33:28, mef wrote:
> Hi Ryan,
> 
> I've introduced the notion of Privacy Mode to allow granular usage of channel
id
> on different connections.
> It is based on Dirk Balfanz's proposal here:
>
https://docs.google.com/a/google.com/document/d/1Tz9KEZwgQNWCGe6GnzpUbRxa1iK-...

Right, my confusion comes from the fact that Dirk's proposal was that we use
channel ID (eg: no privacy mode) for the send/don't accept case.

In the send/don't accept case, we should respect/not persist.

However...

> 
> It is based on cookie settings for particular url request taking in account
> first and third party url and settings.
> 
> Privacy Mode is enabled (and channel id not sent) if user settings prohibit
> cookies to be set or send for particular request. Privacy mode is disabled
(and
> channel id is sent) if cookies are allowed for particular request.
> 
> Basic idea is to prevent potential usage of channel id for tracking purpose if
> user doesn't want to be tracked by particular host. This may or may not depend
> on third party vs first party settings.
> 
> The original ticket is here: http://crbug.com/223191.
> 
> A bit later you've pointed out ssl session shards, and MattM brought up the
idea
> that the notion of Privacy Mode is also useful outside of SSL and channel id,
> and should split http socket pools as well, so requests with cookies allowed
and
> disallowed wouldn't go over the same connection.

This is the confusing part.

Here, you suggest send/accept == !privacy, and all else = privacy mode.

I understand that both of the don't-send cases == privacy. The question is for
the send/don't accept case.

> 
> That is implemented here: http://crbug.com/244992.
> 
> Please also see my comments below.
> 
> > I'm always deeply confused about what 'privacy mode' is and how this relates
> to
> > actual requests. My ignorance knows no ends.
> I think it is my fault, is it worth documenting 'privacy mode' better? If yes,
> then how?
> 
> > I (perhaps incorrectly) understand it to be the distinction between first
> > party/third-party requests. In the case of first party, we both send cookies
> and
> > respect Set-Cookie headers. In the case of third party, we send cookies (if
> > any), but we do not respect Set-Cookie headers.
> I'd say that Privacy Mode doesn't determine cookie policy, but uses it to
enable
> and disable other tracking features.
> 
> > If this is a correct understanding, then we should respect/use existing
> > DomainState, but we should not update (in memory or on disk) any new
> > DomainStates - eg: Don't use AddH{STS,PKP}Header, the same way we don't
> > AddCookies for Set-Cookie headers (however we handle that today).
> > 
> > If privacy mode means we do not *send* cookies AND we do not *set* cookies,
> then
> > it's a bit trickier - the response is contingent upon whether privacy mode
> > requests can trigger *other* privacy mode requests. If they cannot (that is,
> > each privacy mode request is 'independent', and always a 'child of' a
> > first-party request), then we should RESPECT HSTS/HPKP, but not PERSIST
> > HSTS/HPKP.
> I'd say that each request is independent in regards to 'privacy mode'.
> 
> > However, if privacy mode can trigger ADDITIONAL requests (eg: request A can
> > cause Request B and Request C to be issued, and can distinguish results),
then
> I
> > think we're in a trickier position. This is because a hostile party (eg:
> > malicious advertiser) could, by causing a first party navigation, set an
HSTS
> > identifier (using 32 subdomains). Then, when issued a third-party request,
> they
> > can kick off the 32 child requests and recover that identifier - in effect,
> > using it as a cookie.
> It sounds like that if Privacy Mode is enabled for the request, then we
> shouldn't neither RESPECT nor PERSIST HSTS/HPKP.
> 
> Does this make sense?
> 
> thanks,
> -m

[mef, 2013-07-23 16:10:08]

> > It is based on Dirk Balfanz's proposal here:
> >
>
https://docs.google.com/a/google.com/document/d/1Tz9KEZwgQNWCGe6GnzpUbRxa1iK-...
> 
> Right, my confusion comes from the fact that Dirk's proposal was that we use
> channel ID (eg: no privacy mode) for the send/don't accept case.
> 
> In the send/don't accept case, we should respect/not persist.

Right. That's what is implemented for the channel id.

> 
> However...
> 
> > 
> > It is based on cookie settings for particular url request taking in account
> > first and third party url and settings.
> > 
> > Privacy Mode is enabled (and channel id not sent) if user settings prohibit
> > cookies to be set or send for particular request. Privacy mode is disabled
> (and
> > channel id is sent) if cookies are allowed for particular request.
> > 
> > Basic idea is to prevent potential usage of channel id for tracking purpose
if
> > user doesn't want to be tracked by particular host. This may or may not
depend
> > on third party vs first party settings.
> > 
> > The original ticket is here: http://crbug.com/223191.
> > 
> > A bit later you've pointed out ssl session shards, and MattM brought up the
> idea
> > that the notion of Privacy Mode is also useful outside of SSL and channel
id,
> > and should split http socket pools as well, so requests with cookies allowed
> and
> > disallowed wouldn't go over the same connection.
> 
> This is the confusing part.
> 
> Here, you suggest send/accept == !privacy, and all else = privacy mode.
> 
> I understand that both of the don't-send cases == privacy. The question is for
> the send/don't accept case.

I think I've oversimplified the explanation as I didn't realize the importance
of this distinction for this conversation.

So, if we state that send/don't accept case is not privacy mode, then what
should HSTS and HPKP behavior be?

[mef, 2013-07-23 19:45:18]

Per conversation with Ryan:

If you set aside privacy mode entirely

cookies sent = use profile's TSS
cookies are not sent = use default TSS
cookies stored = persist into TSS
cookies are not stored = do not persist into TSS

he 'missing' part is that the use profile's TSS / use default TSS are tied to
the pool, NOT the HTTP request
so I think it's reasonable to say PrivacyMode Off = use profile's TSS,
PrivacyMode On = use default TSS.

I'll update the CL.

[mef, 2013-07-26 19:41:16]

Hi guys, PTAL. I've refactored the code to work according to following:

cookies sent = use profile's TSS
cookies are not sent = use default TSS
cookies stored = persist into TSS
cookies are not stored = do not persist into TSS

thanks,
-m

[palmer, 2013-07-29 21:44:49]

https://codereview.chromium.org/19269012/diff/44001/net/socket_stream/socket_...
File net/socket_stream/socket_stream.cc (right):

https://codereview.chromium.org/19269012/diff/44001/net/socket_stream/socket_...
net/socket_stream/socket_stream.cc:1314: delegate_->CanGetCookies(this, url_),
This stops requests from *using* dynamic state in privacy mode; but really, all
we want to do is to stop dynamic state from being created and stored as a result
of requests made in privacy mode. That seems wrong to me, because it means that
if we by some chance had good dynamic state for example.com (user had visited
the site before not in privacy mode), but we were loading an iframe for (say) an
ad from example.com, that we would load that ad without the benefit of HSTS or
HPKP, or with outdated static HSTS/HPKP (!).

Right? Or am I confused again. That happens.

[mef, 2013-07-30 16:04:52]

On 2013/07/29 21:44:49, Chromium Palmer wrote:

>
https://codereview.chromium.org/19269012/diff/44001/net/socket_stream/socket_...
> net/socket_stream/socket_stream.cc:1314: delegate_->CanGetCookies(this, url_),
> This stops requests from *using* dynamic state in privacy mode; but really,
all
> we want to do is to stop dynamic state from being created and stored as a
result
> of requests made in privacy mode. That seems wrong to me, because it means
that
> if we by some chance had good dynamic state for http://example.com (user had
visited
> the site before not in privacy mode), but we were loading an iframe for (say)
an
> ad from http://example.com, that we would load that ad without the benefit of
HSTS or
> HPKP, or with outdated static HSTS/HPKP (!).
> 
> Right? Or am I confused again. That happens.

AFAIU Ryan's point was that if we don't send cookies to the server, 
then we shouldn't use dynamic HSTS/HPKP as it could be misused for tracking.

We still should honor static HSTS/HPKP though.

[Ryan Sleevi, 2013-07-30 18:15:57]

On 2013/07/30 16:04:52, mef wrote:
> On 2013/07/29 21:44:49, Chromium Palmer wrote:
> 
> >
>
https://codereview.chromium.org/19269012/diff/44001/net/socket_stream/socket_...
> > net/socket_stream/socket_stream.cc:1314: delegate_->CanGetCookies(this,
url_),
> > This stops requests from *using* dynamic state in privacy mode; but really,
> all
> > we want to do is to stop dynamic state from being created and stored as a
> result
> > of requests made in privacy mode. That seems wrong to me, because it means
> that
> > if we by some chance had good dynamic state for http://example.com (user had
> visited
> > the site before not in privacy mode), but we were loading an iframe for
(say)
> an
> > ad from http://example.com, that we would load that ad without the benefit
of
> HSTS or
> > HPKP, or with outdated static HSTS/HPKP (!).
> > 
> > Right? Or am I confused again. That happens.
> 
> AFAIU Ryan's point was that if we don't send cookies to the server, 
> then we shouldn't use dynamic HSTS/HPKP as it could be misused for tracking.
> 
> We still should honor static HSTS/HPKP though.

Yes, mef@'s understanding is correct.

If we *respected* dynamic HSTS data, then when in first-party mode, the
'tracker' sets up the 32-bits, and then can recover them at any point in
third-party mode.

If we don't send cookies, we shouldn't leak those 32-bits to the tracker either.

[mef, 2013-08-02 14:45:56]

On 2013/07/30 18:15:57, Ryan Sleevi wrote:
> On 2013/07/30 16:04:52, mef wrote:
> > On 2013/07/29 21:44:49, Chromium Palmer wrote:
> > 
> > >
> >
>
https://codereview.chromium.org/19269012/diff/44001/net/socket_stream/socket_...
> > > net/socket_stream/socket_stream.cc:1314: delegate_->CanGetCookies(this,
> url_),
> > > This stops requests from *using* dynamic state in privacy mode; but
really,
> > all
> > > we want to do is to stop dynamic state from being created and stored as a
> > result
> > > of requests made in privacy mode. That seems wrong to me, because it means
> > that
> > > if we by some chance had good dynamic state for http://example.com (user
had
> > visited
> > > the site before not in privacy mode), but we were loading an iframe for
> (say)
> > an
> > > ad from http://example.com, that we would load that ad without the benefit
> of
> > HSTS or
> > > HPKP, or with outdated static HSTS/HPKP (!).
> > > 
> > > Right? Or am I confused again. That happens.
> > 
> > AFAIU Ryan's point was that if we don't send cookies to the server, 
> > then we shouldn't use dynamic HSTS/HPKP as it could be misused for tracking.
> > 
> > We still should honor static HSTS/HPKP though.
> 
> Yes, mef@'s understanding is correct.
> 
> If we *respected* dynamic HSTS data, then when in first-party mode, the
> 'tracker' sets up the 32-bits, and then can recover them at any point in
> third-party mode.
> 
> If we don't send cookies, we shouldn't leak those 32-bits to the tracker
either.

Excellent! With having this said, is there anything I should do with this CL to
make it look good?

[palmer, 2013-08-06 17:52:22]

LGTM

[mef1, 2013-08-06 18:14:21]

thanks!


On Tue, Aug 6, 2013 at 1:52 PM, <palmer@chromium.org> wrote:

> LGTM
>
>
https://codereview.chromium.**org/19269012/<https://codereview.chromium.org/1...
>

[Ryan Sleevi, 2013-08-06 22:32:11]

https://codereview.chromium.org/19269012/diff/44001/net/socket_stream/socket_...
File net/socket_stream/socket_stream.cc (right):

https://codereview.chromium.org/19269012/diff/44001/net/socket_stream/socket_...
net/socket_stream/socket_stream.cc:1314: delegate_->CanGetCookies(this, url_),
Are we sure that "url_" is correct here?

When looking at WebSockets, it seems it does some URL rewriting (see
https://code.google.com/p/chromium/codesearch#chromium/src/net/websockets/web...
), which this would be oblivious to.

https://codereview.chromium.org/19269012/diff/44001/net/socket_stream/socket_...
File net/socket_stream/socket_stream_job.cc (right):

https://codereview.chromium.org/19269012/diff/44001/net/socket_stream/socket_...
net/socket_stream/socket_stream_job.cc:33: delegate->CanGetCookies(NULL, url),
See remark about url needing to be scheme-fixed.

(It's also a really sucky layering here that StreamSocketJob has to know about
WebSockets, which sit atop net/socket_stream)

[mef, 2013-08-09 17:04:04]

Hi,

thank you for the review! Sorry for the delayed answer, I was preoccupied with
other stuff, and now I'm ready for vacation. :-)

In the meanwhile Ryan, could you elaborate on your concerns? Is it that
different url schema could result in different CanGetCookies result? 

thanks,
-m

https://codereview.chromium.org/19269012/diff/44001/net/socket_stream/socket_...
File net/socket_stream/socket_stream.cc (right):

https://codereview.chromium.org/19269012/diff/44001/net/socket_stream/socket_...
net/socket_stream/socket_stream.cc:1314: delegate_->CanGetCookies(this, url_),
On 2013/08/06 22:32:11, Ryan Sleevi wrote:
> Are we sure that "url_" is correct here?
> 
> When looking at WebSockets, it seems it does some URL rewriting (see
>
https://code.google.com/p/chromium/codesearch#chromium/src/net/websockets/web...
> ), which this would be oblivious to.

Hmm, the rewrite in WebSocketJob::GetURLForCookies() only seems to change the
schema, so it shouldn't affect result of CanGetCookies as that is based on
hostname pattern matching.

https://codereview.chromium.org/19269012/diff/44001/net/socket_stream/socket_...
File net/socket_stream/socket_stream_job.cc (right):

https://codereview.chromium.org/19269012/diff/44001/net/socket_stream/socket_...
net/socket_stream/socket_stream_job.cc:33: delegate->CanGetCookies(NULL, url),
On 2013/08/06 22:32:11, Ryan Sleevi wrote:
> See remark about url needing to be scheme-fixed.
> 
> (It's also a really sucky layering here that StreamSocketJob has to know about
> WebSockets, which sit atop net/socket_stream)

Sorry, could you elaborate a little? I'm sure you are right, I'm just not sure
what needs to be done. :-)

[Ryan Sleevi, 2013-08-09 17:42:29]

https://codereview.chromium.org/19269012/diff/44001/net/socket_stream/socket_...
File net/socket_stream/socket_stream.cc (right):

https://codereview.chromium.org/19269012/diff/44001/net/socket_stream/socket_...
net/socket_stream/socket_stream.cc:1314: delegate_->CanGetCookies(this, url_),
On 2013/08/09 17:04:05, mef wrote:
> On 2013/08/06 22:32:11, Ryan Sleevi wrote:
> > Are we sure that "url_" is correct here?
> > 
> > When looking at WebSockets, it seems it does some URL rewriting (see
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/net/websockets/web...
> > ), which this would be oblivious to.
> 
> Hmm, the rewrite in WebSocketJob::GetURLForCookies() only seems to change the
> schema, so it shouldn't affect result of CanGetCookies as that is based on
> hostname pattern matching. 

That's what doesn't make sense - why rewrite the URL scheme (which effectively
changes the origin) if the cookie subsystem is going to ignore origin?

I'll readily admit that I didn't dig too far into this, other than noting that
the existing behaviour of SocketStream in the presence of WSS is to call
Delegate::CanGetCookies() with GetURLForCookies(), and that's a behaviour that
isn't matched here (as a WSS socket will layer atop this and get this default
behaviour)

I don't have a good answer, other than asking you to try to trace through and
see what the implications are, and if it's a bug that we should fix (either
CanGetCookies() doesn't need GetURLForCookies(), and that should be changed, or
we need to fix this code to be consistent). I just want to make sure we're
reliably giving the same URL to CanGetCookies(), whether for the TSS data or for
actual cookies.

Hopefully that makes sense?

[mef, 2013-08-09 17:54:30]

On 2013/08/09 17:42:29, Ryan Sleevi wrote:
>
https://codereview.chromium.org/19269012/diff/44001/net/socket_stream/socket_...
> File net/socket_stream/socket_stream.cc (right):
> 
>
https://codereview.chromium.org/19269012/diff/44001/net/socket_stream/socket_...
> net/socket_stream/socket_stream.cc:1314: delegate_->CanGetCookies(this, url_),
> On 2013/08/09 17:04:05, mef wrote:
> > On 2013/08/06 22:32:11, Ryan Sleevi wrote:
> > > Are we sure that "url_" is correct here?
> > > 
> > > When looking at WebSockets, it seems it does some URL rewriting (see
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/net/websockets/web...
> > > ), which this would be oblivious to.
> > 
> > Hmm, the rewrite in WebSocketJob::GetURLForCookies() only seems to change
the
> > schema, so it shouldn't affect result of CanGetCookies as that is based on
> > hostname pattern matching. 
> 
> That's what doesn't make sense - why rewrite the URL scheme (which effectively
> changes the origin) if the cookie subsystem is going to ignore origin?
> 
> I'll readily admit that I didn't dig too far into this, other than noting that
> the existing behaviour of SocketStream in the presence of WSS is to call
> Delegate::CanGetCookies() with GetURLForCookies(), and that's a behaviour that
> isn't matched here (as a WSS socket will layer atop this and get this default
> behaviour)
> 
> I don't have a good answer, other than asking you to try to trace through and
> see what the implications are, and if it's a bug that we should fix (either
> CanGetCookies() doesn't need GetURLForCookies(), and that should be changed,
or
> we need to fix this code to be consistent). I just want to make sure we're
> reliably giving the same URL to CanGetCookies(), whether for the TSS data or
for
> actual cookies.
> 
> Hopefully that makes sense?

I see, I'm not familiar with WebSocket cookie specifics, 
so I'll have to exercise due diligence after return from Cape Cod. 

My hunch is that scheme is not important for 'CanGetCookies', 
but important for 'GetCookies' itself.

thanks,
-m

[mef, 2013-09-10 19:29:33]

Sorry for the delay, PTAL. 

I've finally processed your concerns, and figured that we cannot expect
delegate's CanGetCookie method to be aware of web socket schema. 
Fortunately StreamSocket is already aware of "ws/wss" schemas, so I've moved
GetURLForCookies method from WebSocketJob to SocketStream. 
I've made it static, so it could be used by SocketStreamJob before SocketStream
object is instantiated. 

thanks,
-m

[mef, 2013-09-13 21:31:36]

On 2013/09/10 19:29:33, mef wrote:
> Sorry for the delay, PTAL. 
> 
> I've finally processed your concerns, and figured that we cannot expect
> delegate's CanGetCookie method to be aware of web socket schema. 
> Fortunately StreamSocket is already aware of "ws/wss" schemas, so I've moved
> GetURLForCookies method from WebSocketJob to SocketStream. 
> I've made it static, so it could be used by SocketStreamJob before
SocketStream
> object is instantiated. 
> 
> thanks,
> -m

ping.

[Ryan Sleevi, 2013-09-13 22:14:48]

Sorry, I've unpaged a lot of this code, and haven't quite paged it back in.

I'm not entirely thrilled pushing some of this into SocketStream, but then
again, the whole relation between SocketStream and WebSockets is a non-ideal one
at this point. It just feels like we're leaking some abstraction details, even
if it is "tiny".

I think this would lg.tm, but a question below about whether an alternative
approach makes more sense / is cleaner. I'm not sure, but given that you've
mapped a lot of this in, I'm curious your thoguhts.

https://codereview.chromium.org/19269012/diff/44001/net/socket_stream/socket_...
File net/socket_stream/socket_stream_job.cc (right):

https://codereview.chromium.org/19269012/diff/44001/net/socket_stream/socket_...
net/socket_stream/socket_stream_job.cc:33: delegate->CanGetCookies(NULL, url),
On 2013/08/09 17:04:05, mef wrote:
> On 2013/08/06 22:32:11, Ryan Sleevi wrote:
> > See remark about url needing to be scheme-fixed.
> > 
> > (It's also a really sucky layering here that StreamSocketJob has to know
about
> > WebSockets, which sit atop net/socket_stream)
> 
> Sorry, could you elaborate a little? I'm sure you are right, I'm just not sure
> what needs to be done. :-)

Well, rather than having StreamSocket::CreateSocketStreamJob handle ws -> wss
fixup, the higher layer could do it.

Likewise, rather than having SocketStream fixup ws->http(s), have the delegate
do so.

https://codereview.chromium.org/19269012/diff/89001/net/socket_stream/socket_...
File net/socket_stream/socket_stream.h (right):

https://codereview.chromium.org/19269012/diff/89001/net/socket_stream/socket_...
net/socket_stream/socket_stream.h:184: // Replace ws(s) URL Schema with http(s)
for cookie policy checking.
1) Why is this public
2) The documentation describes what it does in terms of how it's implemented,
rather than how it should be used. ISTM the description should focus less on the
ws/https aspect.

[mef, 2013-09-16 15:00:07]

Hi Ryan,

thanks a lot for your comments! See my answers inline.

thanks,
-m

https://codereview.chromium.org/19269012/diff/44001/net/socket_stream/socket_...
File net/socket_stream/socket_stream_job.cc (right):

https://codereview.chromium.org/19269012/diff/44001/net/socket_stream/socket_...
net/socket_stream/socket_stream_job.cc:33: delegate->CanGetCookies(NULL, url),
> Well, rather than having StreamSocket::CreateSocketStreamJob handle ws -> wss
> fixup, the higher layer could do it.
Hmm, higher level from here is SocketStreamHost and I'm not sure why moving
ws->wss fix up there would make it less confusing. It doesn't seem to care about
url details at this moment.

> Likewise, rather than having SocketStream fixup ws->http(s), have the delegate
> do so.
AFAIU delegate can have multiple different implementations now and in the future
and I'm not sure that expecting it to handle ws(s) vs http(s) correctly is a
good idea.

https://codereview.chromium.org/19269012/diff/89001/net/socket_stream/socket_...
File net/socket_stream/socket_stream.h (right):

https://codereview.chromium.org/19269012/diff/89001/net/socket_stream/socket_...
net/socket_stream/socket_stream.h:184: // Replace ws(s) URL Schema with http(s)
for cookie policy checking.
On 2013/09/13 22:14:48, Ryan Sleevi wrote:
> 1) Why is this public
It is public to be accessible from SocketStreamJob::CreateSocketStreamJob.

> 2) The documentation describes what it does in terms of how it's implemented,
> rather than how it should be used. ISTM the description should focus less on
the
> ws/https aspect.
Should it be something like: 'Get URL for cookie policy checking based on Web
Socket URL'?

[Ryan Sleevi, 2013-09-17 18:53:02]

LGTM, mod nit.

https://codereview.chromium.org/19269012/diff/44001/net/socket_stream/socket_...
File net/socket_stream/socket_stream_job.cc (right):

https://codereview.chromium.org/19269012/diff/44001/net/socket_stream/socket_...
net/socket_stream/socket_stream_job.cc:33: delegate->CanGetCookies(NULL, url),
On 2013/09/16 15:00:08, mef wrote:
> > Well, rather than having StreamSocket::CreateSocketStreamJob handle ws ->
wss
> > fixup, the higher layer could do it.
> Hmm, higher level from here is SocketStreamHost and I'm not sure why moving
> ws->wss fix up there would make it less confusing. It doesn't seem to care
about
> url details at this moment.
> 
> > Likewise, rather than having SocketStream fixup ws->http(s), have the
delegate
> > do so.
> AFAIU delegate can have multiple different implementations now and in the
future
> and I'm not sure that expecting it to handle ws(s) vs http(s) correctly is a
> good idea. 
> 
> 

Well, the only implementation that would/should have to worry about ws(s) vs
http(s) would be ones that supported WebSockets - which would be the right
abstraction to keeping the websocket knowledge 'out' of this.

However, it's clear that this class is *only* used in context of WebSockets (yay
confusing/weird designs), so I'm OK with this - and looking forward to the point
where WebSockets are better integrated.

https://codereview.chromium.org/19269012/diff/89001/net/socket_stream/socket_...
File net/socket_stream/socket_stream.h (right):

https://codereview.chromium.org/19269012/diff/89001/net/socket_stream/socket_...
net/socket_stream/socket_stream.h:184: // Replace ws(s) URL Schema with http(s)
for cookie policy checking.
On 2013/09/16 15:00:08, mef wrote:
> On 2013/09/13 22:14:48, Ryan Sleevi wrote:
> > 1) Why is this public
> It is public to be accessible from SocketStreamJob::CreateSocketStreamJob.
> 
> > 2) The documentation describes what it does in terms of how it's
implemented,
> > rather than how it should be used. ISTM the description should focus less on
> the
> > ws/https aspect.
> Should it be something like: 'Get URL for cookie policy checking based on Web
> Socket URL'?
> 

// Returns the URL to be used for cookie policy checking. Note that
// this may be different than |url|; for example, the cookie policy
// for a ws:// URL is based upon the http:// scheme for that
// host:port pair.

[mef, 2013-09-17 20:38:37]

Ryan, thanks a lot!

Now I would like to humbly ask for OWNER's approvals:

mmenke@ - chrome/browser/net/transport_security_persister_unittest.cc
          chrome/browser/ui/webui/net_internals/net_internals_ui.cc

sky@ - chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc

erikwright@ - chrome_frame/test/net/fake_external_tab.cc

thanks,
-m

https://codereview.chromium.org/19269012/diff/89001/net/socket_stream/socket_...
File net/socket_stream/socket_stream.h (right):

https://codereview.chromium.org/19269012/diff/89001/net/socket_stream/socket_...
net/socket_stream/socket_stream.h:184: // Replace ws(s) URL Schema with http(s)
for cookie policy checking.
On 2013/09/17 18:53:03, Ryan Sleevi wrote:
> On 2013/09/16 15:00:08, mef wrote:
> > On 2013/09/13 22:14:48, Ryan Sleevi wrote:
> > > 1) Why is this public
> > It is public to be accessible from SocketStreamJob::CreateSocketStreamJob.
> > 
> > > 2) The documentation describes what it does in terms of how it's
> implemented,
> > > rather than how it should be used. ISTM the description should focus less
on
> > the
> > > ws/https aspect.
> > Should it be something like: 'Get URL for cookie policy checking based on
Web
> > Socket URL'?
> > 
> 
> // Returns the URL to be used for cookie policy checking. Note that
> // this may be different than |url|; for example, the cookie policy
> // for a ws:// URL is based upon the http:// scheme for that
> // host:port pair.

Done.

[mmenke, 2013-09-17 21:10:32]

net_internals and transport_security_persister_unittest LGTM

https://codereview.chromium.org/19269012/diff/112001/net/http/transport_secur...
File net/http/transport_security_state.h (right):

https://codereview.chromium.org/19269012/diff/112001/net/http/transport_secur...
net/http/transport_security_state.h:205: // otherwise only static state is
used..
nit:  Remove extra period.

[erikwright (departed), 2013-09-18 15:37:59]

chrome_frame LGTM.

[mef, 2013-09-19 20:20:22]

Thanks a lot to Matt and Erik!

sky@ or jam@, could your OWNER-review 

chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc

thanks,
-m

[jam, 2013-09-19 20:31:09]

On 2013/09/19 20:20:22, mef wrote:
> Thanks a lot to Matt and Erik!
> 
> sky@ or jam@, could your OWNER-review 
> 
> chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc
> 
> thanks,
> -m

lgtm

[commit-bot: I haz the power, 2013-09-20 00:06:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mef@chromium.org/19269012/112001

[commit-bot: I haz the power, 2013-09-20 03:33:45]

Change committed as 224269

[tkent, 2013-09-20 04:51:19]

On 2013/09/20 03:33:45, I haz the power (commit-bot) wrote:
> Change committed as 224269

Reverted by r224275 due to build failure.

[mef, 2013-09-20 15:53:29]

Hi Ryan, please review my change.
Others: FYI and I'm open for suggestions.

The commit has failed due to error in code inside of #if
defined(OFFICIAL_BUILD). I've fixed the compilation error, but I'm not sure
about right thing to do here (see my comment), so I'm looking for suggestions.

thanks,
-m

https://chromiumcodereview.appspot.com/19269012/diff/134001/net/socket/ssl_cl...
File net/socket/ssl_client_socket_nss.cc (right):

https://chromiumcodereview.appspot.com/19269012/diff/134001/net/socket/ssl_cl...
net/socket/ssl_client_socket_nss.cc:3446: domain_state.HasPublicKeyPins()) {
I'm not 100% sure that this is right thing to do here, but I don't know how to
get cookie settings to make a correct decision. It shouldn't affect HSTS as this
is already SSL, but checking pins could theoretically be done with or without
dynamic information, and choosing correct one would seem to require socket pool
sharding. I'm open for suggestions.

[mef, 2013-10-08 20:55:27]

On 2013/09/20 15:53:29, mef wrote:
> Hi Ryan, please review my change.
> Others: FYI and I'm open for suggestions.
> 
> The commit has failed due to error in code inside of #if
> defined(OFFICIAL_BUILD). I've fixed the compilation error, but I'm not sure
> about right thing to do here (see my comment), so I'm looking for suggestions.
> 
> thanks,
> -m
> 
>
https://chromiumcodereview.appspot.com/19269012/diff/134001/net/socket/ssl_cl...
> File net/socket/ssl_client_socket_nss.cc (right):
> 
>
https://chromiumcodereview.appspot.com/19269012/diff/134001/net/socket/ssl_cl...
> net/socket/ssl_client_socket_nss.cc:3446: domain_state.HasPublicKeyPins()) {
> I'm not 100% sure that this is right thing to do here, but I don't know how to
> get cookie settings to make a correct decision. It shouldn't affect HSTS as
this
> is already SSL, but checking pins could theoretically be done with or without
> dynamic information, and choosing correct one would seem to require socket
pool
> sharding. I'm open for suggestions.

ping

[palmer, 2014-05-07 00:05:58]

Also, be aware that you'll have to rebase and probably have conflicts after my
refactor (https://codereview.chromium.org/103803012/, currently in CQ) lands.
Sorry about that. :\ Also, that refactor might impinge, perhaps positively, on
your problem?

[mef, 2014-05-07 01:17:41]

Given 9 month gestation period of my CL any change is a good change. :-)


On Tue, May 6, 2014 at 8:05 PM, <palmer@chromium.org> wrote:

> Also, be aware that you'll have to rebase and probably have conflicts
> after my
> refactor (https://codereview.chromium.org/103803012/, currently in CQ)
> lands.
> Sorry about that. :\ Also, that refactor might impinge, perhaps
> positively, on
> your problem?
>
> https://codereview.chromium.org/19269012/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[palmer, 2014-10-16 17:25:21]

Ping! Any movement on this one?

[mef, 2014-10-16 17:41:13]

On 2014/10/16 17:25:21, Chromium Palmer wrote:
> Ping! Any movement on this one?

There isn't, but given that the sticking point is in
net/socket/ssl_client_socket_nss.cc and we are moving to boring ssl, may be I
should revisit?

WDYT?

[palmer, 2014-10-16 22:53:33]

> There isn't, but given that the sticking point is in
> net/socket/ssl_client_socket_nss.cc and we are moving to boring ssl, may be I
> should revisit?

That's a question for rsleevi and agl and davidben.

[palmer, 2014-10-16 22:54:49]

palmer@chromium.org changed reviewers:
+ agl@chromium.org, davidben@chromium.org

[palmer, 2014-10-16 22:54:50]

agl, davidben, rsleevi: Can you give mef some guidance on this?

[agl, 2014-10-16 23:00:05]

The change description need to be much longer.

If the aim is not to use dynamic state in Incognito mode then hard coding "true"
to the pinning check doesn't meet that goal. It would still be possible to probe
whether dynamic entries are in the cache.

We do already have plumbing to get a bit from Incognito to the SSL layer because
we use it for sharding the cache. That path seems like the right way to separate
out dynamic pins in Incognito mode, assuming that's the aim.