Attach security info to redirect responses

Attach security info to redirect responses

The DevTools security panel should show SSL information for each
subresource on the page, including redirects. This CL attaches security
info to the ResourceResponseHead for redirect responses, so that the
security info for redirect responses can be sent to devtools when
enabled.

BUG=519120
Committed: https://crrev.com/a7140bc381590def065f0ecf9c4b9ba202c501a1
Cr-Commit-Position: refs/heads/master@{#343041}

[estark, 2015-08-11 06:13:15]

estark@chromium.org changed reviewers:
+ mmenke@chromium.org

[estark, 2015-08-11 06:13:16]

mmenke, could you please take a (preliminary) look? I think this could use a
unit test, but I wanted to check first if this seems viable to you. We are
building the devtools security panel and want to show SSL information for every
resource, including redirect responses, in devtools. SSL information gets sent
to devtools from the renderer, using the security_info on the
ResourceResponseInfo, but it seems that security_info isn't getting set for
redirect responses, so this CL does that. See the bug in the description for
more context.

Does this approach seem okay/correct to you? If yes, I'll add a unit test.
Thanks!

[Ryan Sleevi, 2015-08-11 07:16:45]

On 2015/08/11 06:13:16, estark wrote:
> mmenke, could you please take a (preliminary) look? I think this could use a
> unit test, but I wanted to check first if this seems viable to you. We are
> building the devtools security panel and want to show SSL information for
every
> resource, including redirect responses, in devtools. SSL information gets sent
> to devtools from the renderer, using the security_info on the
> ResourceResponseInfo, but it seems that security_info isn't getting set for
> redirect responses, so this CL does that. See the bug in the description for
> more context.
> 
> Does this approach seem okay/correct to you? If yes, I'll add a unit test.
> Thanks!

Won't we still end up with redirects with empty info (e.g. synthetic HSTS
redirects)? Would it be better to just expect there is a class of requests
without such info?

Doesn't this also seem the wrong layer to ensure this guarantee?

[estark, 2015-08-11 14:16:15]

On 2015/08/11 07:16:45, Ryan Sleevi wrote:
> On 2015/08/11 06:13:16, estark wrote:
> > mmenke, could you please take a (preliminary) look? I think this could use a
> > unit test, but I wanted to check first if this seems viable to you. We are
> > building the devtools security panel and want to show SSL information for
> every
> > resource, including redirect responses, in devtools. SSL information gets
sent
> > to devtools from the renderer, using the security_info on the
> > ResourceResponseInfo, but it seems that security_info isn't getting set for
> > redirect responses, so this CL does that. See the bug in the description for
> > more context.
> > 
> > Does this approach seem okay/correct to you? If yes, I'll add a unit test.
> > Thanks!
> 
> Won't we still end up with redirects with empty info (e.g. synthetic HSTS
> redirects)?

Do HSTS redirects happen purely in //net without //content learning about them?
I don't know much about how that works but would like to learn. I seem to recall
hearing that devtools doesn't see anything about HSTS redirects, so at some
point we'll probably want to investigate if there's anything we can do about
that.

> Would it be better to just expect there is a class of requests
> without such info?

We are also doing that, yes (https://codereview.chromium.org/1275513006/), but
this change would catch a big common case.

> 
> Doesn't this also seem the wrong layer to ensure this guarantee?

I'm not trying to ensure any guarantee at the moment, but rather just to get
security info attached to a big set of responses that aren't currently getting
security info attached, which I think will improve the devtools UX
significantly. Since ResourceLoader is the layer that attaches security info,
I'm not sure at what other layer we would do this?

[mmenke, 2015-08-11 14:20:08]

mmenke@chromium.org changed reviewers:
+ davidben@chromium.org

[mmenke, 2015-08-11 14:20:09]

David's probably a better owner reviewer for this than I am.

On 2015/08/11 14:16:15, estark wrote:
> On 2015/08/11 07:16:45, Ryan Sleevi wrote:
> > On 2015/08/11 06:13:16, estark wrote:
> > > mmenke, could you please take a (preliminary) look? I think this could use
a
> > > unit test, but I wanted to check first if this seems viable to you. We are
> > > building the devtools security panel and want to show SSL information for
> > every
> > > resource, including redirect responses, in devtools. SSL information gets
> sent
> > > to devtools from the renderer, using the security_info on the
> > > ResourceResponseInfo, but it seems that security_info isn't getting set
for
> > > redirect responses, so this CL does that. See the bug in the description
for
> > > more context.
> > > 
> > > Does this approach seem okay/correct to you? If yes, I'll add a unit test.
> > > Thanks!
> > 
> > Won't we still end up with redirects with empty info (e.g. synthetic HSTS
> > redirects)?
> 
> Do HSTS redirects happen purely in //net without //content learning about
them?
> I don't know much about how that works but would like to learn. I seem to
recall
> hearing that devtools doesn't see anything about HSTS redirects, so at some
> point we'll probably want to investigate if there's anything we can do about
> that.

HSTS redirects look mostly like normal redirects...  But HSTS redirects only
redirect us from HTTP to HTTPS, anyways, right?  So they shouldn't have any
security info (Just like normal HTTP requests, I assume).  Redirects created by
extensions or internal chrome modules before requests are actually issued may
have this problem, however.

> 
> > Would it be better to just expect there is a class of requests
> > without such info?
> 
> We are also doing that, yes (https://codereview.chromium.org/1275513006/), but
> this change would catch a big common case.
> 
> > 
> > Doesn't this also seem the wrong layer to ensure this guarantee?
> 
> I'm not trying to ensure any guarantee at the moment, but rather just to get
> security info attached to a big set of responses that aren't currently getting
> security info attached, which I think will improve the devtools UX
> significantly. Since ResourceLoader is the layer that attaches security info,
> I'm not sure at what other layer we would do this?

[estark, 2015-08-11 14:34:47]

On 2015/08/11 14:20:09, mmenke wrote:
> David's probably a better owner reviewer for this than I am.

Thanks. David, this is sort of a follow-up to
https://codereview.chromium.org/1275513006/. That is,
https://codereview.chromium.org/1275513006/ copes with empty security info on
HTTPS responses, and this change is to add security info to redirect responses
so that empty security info happens less often.

Please also see my comments to mmenke at the beginning of this thread about
needing a unit test but wanted to get a read on the approach first.

> 
> On 2015/08/11 14:16:15, estark wrote:
> > On 2015/08/11 07:16:45, Ryan Sleevi wrote:
> > > On 2015/08/11 06:13:16, estark wrote:
> > > > mmenke, could you please take a (preliminary) look? I think this could
use
> a
> > > > unit test, but I wanted to check first if this seems viable to you. We
are
> > > > building the devtools security panel and want to show SSL information
for
> > > every
> > > > resource, including redirect responses, in devtools. SSL information
gets
> > sent
> > > > to devtools from the renderer, using the security_info on the
> > > > ResourceResponseInfo, but it seems that security_info isn't getting set
> for
> > > > redirect responses, so this CL does that. See the bug in the description
> for
> > > > more context.
> > > > 
> > > > Does this approach seem okay/correct to you? If yes, I'll add a unit
test.
> > > > Thanks!
> > > 
> > > Won't we still end up with redirects with empty info (e.g. synthetic HSTS
> > > redirects)?
> > 
> > Do HSTS redirects happen purely in //net without //content learning about
> them?
> > I don't know much about how that works but would like to learn. I seem to
> recall
> > hearing that devtools doesn't see anything about HSTS redirects, so at some
> > point we'll probably want to investigate if there's anything we can do about
> > that.
> 
> HSTS redirects look mostly like normal redirects...  But HSTS redirects only
> redirect us from HTTP to HTTPS, anyways, right?  So they shouldn't have any
> security info (Just like normal HTTP requests, I assume).  Redirects created
by
> extensions or internal chrome modules before requests are actually issued may
> have this problem, however.
> 
> > 
> > > Would it be better to just expect there is a class of requests
> > > without such info?
> > 
> > We are also doing that, yes (https://codereview.chromium.org/1275513006/),
but
> > this change would catch a big common case.
> > 
> > > 
> > > Doesn't this also seem the wrong layer to ensure this guarantee?
> > 
> > I'm not trying to ensure any guarantee at the moment, but rather just to get
> > security info attached to a big set of responses that aren't currently
getting
> > security info attached, which I think will improve the devtools UX
> > significantly. Since ResourceLoader is the layer that attaches security
info,
> > I'm not sure at what other layer we would do this?

[davidben, 2015-08-11 15:15:59]

I think this change makes sense. Certainly, as Matt and Ryan mentioned, there
will still be responses which lack SSLInfo, notably extension-induced redirects.
But when we do have SSLInfo, we ought to route it through ResourceLoader through
redirects and final responsese alike.

https://codereview.chromium.org/1275743005/diff/1/content/browser/loader/reso...
File content/browser/loader/resource_loader.cc (right):

https://codereview.chromium.org/1275743005/diff/1/content/browser/loader/reso...
content/browser/loader/resource_loader.cc:298: }
Why not put this code into PopulateResourceResponse so it's shared between
OnReceivedRedirect and CompleteResponseStarted?

[estark, 2015-08-12 07:02:26]

Thanks, David. Added a unit test.

https://codereview.chromium.org/1275743005/diff/1/content/browser/loader/reso...
File content/browser/loader/resource_loader.cc (right):

https://codereview.chromium.org/1275743005/diff/1/content/browser/loader/reso...
content/browser/loader/resource_loader.cc:298: }
On 2015/08/11 15:15:59, David Benjamin wrote:
> Why not put this code into PopulateResourceResponse so it's shared between
> OnReceivedRedirect and CompleteResponseStarted?

Done.

[davidben, 2015-08-12 16:33:42]

lgtm. And thanks for the test!

https://codereview.chromium.org/1275743005/diff/20001/content/browser/loader/...
File content/browser/loader/resource_loader_unittest.cc (left):

https://codereview.chromium.org/1275743005/diff/20001/content/browser/loader/...
content/browser/loader/resource_loader_unittest.cc:623: private:
Stray removal? (Looks like you added an accessor anyway.)

https://codereview.chromium.org/1275743005/diff/20001/content/browser/loader/...
File content/browser/loader/resource_loader_unittest.cc (right):

https://codereview.chromium.org/1275743005/diff/20001/content/browser/loader/...
content/browser/loader/resource_loader_unittest.cc:216: "HTTP/1.1 302 MOVED\0"
Nit: It seems "Found" is the canonical description.

https://tools.ietf.org/html/rfc7231#section-6.4.3

(Not that we're remotely consistent here:
https://code.google.com/p/chromium/codesearch#search/&q=HTTP/1.1%5C%20302&sq=...
)

[estark, 2015-08-12 16:42:27]

https://codereview.chromium.org/1275743005/diff/20001/content/browser/loader/...
File content/browser/loader/resource_loader_unittest.cc (left):

https://codereview.chromium.org/1275743005/diff/20001/content/browser/loader/...
content/browser/loader/resource_loader_unittest.cc:623: private:
On 2015/08/12 16:33:41, David Benjamin wrote:
> Stray removal? (Looks like you added an accessor anyway.)

Done.

https://codereview.chromium.org/1275743005/diff/20001/content/browser/loader/...
File content/browser/loader/resource_loader_unittest.cc (right):

https://codereview.chromium.org/1275743005/diff/20001/content/browser/loader/...
content/browser/loader/resource_loader_unittest.cc:216: "HTTP/1.1 302 MOVED\0"
On 2015/08/12 16:33:41, David Benjamin wrote:
> Nit: It seems "Found" is the canonical description.
> 
> https://tools.ietf.org/html/rfc7231#section-6.4.3
> 
> (Not that we're remotely consistent here:
>
https://code.google.com/p/chromium/codesearch#search/&q=HTTP/1.1%5C%20302&sq=...
> )

Done.

[estark, 2015-08-12 16:42:33]

The CQ bit was checked by estark@chromium.org

[estark, 2015-08-12 16:42:34]

The patchset sent to the CQ was uploaded after l-g-t-m from
davidben@chromium.org
Link to the patchset: https://codereview.chromium.org/1275743005/#ps40001
(title: "davidben comments")

[commit-bot: I haz the power, 2015-08-12 16:42:41]

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

[estark, 2015-08-12 16:42:55]

The CQ bit was unchecked by estark@chromium.org

[estark, 2015-08-12 16:43:48]

The CQ bit was checked by estark@chromium.org

[estark, 2015-08-12 16:43:48]

The patchset sent to the CQ was uploaded after l-g-t-m from
davidben@chromium.org
Link to the patchset: https://codereview.chromium.org/1275743005/#ps60001
(title: "fix 302 Found capitalization")

[commit-bot: I haz the power, 2015-08-12 16:43:55]

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

[commit-bot: I haz the power, 2015-08-12 17:29:46]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-08-12 17:30:22]

Patchset 4 (id:??) landed as
https://crrev.com/a7140bc381590def065f0ecf9c4b9ba202c501a1
Cr-Commit-Position: refs/heads/master@{#343041}