Plumb the net::SSLInfo to the browser process when it's using the network service.

Plumb the net::SSLInfo to the browser process when it's using the network service.

This fixes the page lock icon not showing the correct indicators. Since the data is large, only send it to the consumer if they request it at URLLoader creation time. Only the browser process would request it.

BUG=598073
Review-Url: https://codereview.chromium.org/2817033002
Cr-Commit-Position: refs/heads/master@{#464800}
Committed: https://chromium.googlesource.com/chromium/src/+/33d897e97d8a3fc40ddf1d0b9ca578041724873a

[jam, 2017-04-13 18:01:19]

The CQ bit was checked by jam@chromium.org to run a CQ dry run

[jam, 2017-04-13 18:01:20]

Patchset #1 (id:1) has been deleted

[commit-bot: I haz the power, 2017-04-13 18:02:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jam, 2017-04-13 19:48:49]

The CQ bit was checked by jam@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-13 19:49:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jam, 2017-04-13 19:54:49]

The CQ bit was checked by jam@chromium.org to run a CQ dry run

[jam, 2017-04-13 19:54:52]

Patchset #2 (id:40001) has been deleted

[commit-bot: I haz the power, 2017-04-13 19:55:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jam, 2017-04-13 19:58:04]

Description was changed from

==========
Plumb the net::SSLInfo to the browser process when it's using the network
service.

This fixes the page lock icon not showing the correct indicators. Since the data
is large, only send it to the consumer if they request it at URLLoader creation
time. Only the browser process would request it.

BUG=598073
==========

to

==========
Plumb the net::SSLInfo to the browser process when it's using the network
service.

This fixes the page lock icon not showing the correct indicators. Since the data
is large, only send it to the consumer if they request it at URLLoader creation
time. Only the browser process would request it.

BUG=598073
==========

[jam, 2017-04-13 19:58:05]

jam@chromium.org changed reviewers:
+ dcheng@chromium.org, yzshen@chromium.org

[jam, 2017-04-13 20:00:12]

The CQ bit was checked by jam@chromium.org to run a CQ dry run

[jam, 2017-04-13 20:00:21]

Patchset #2 (id:60001) has been deleted

[commit-bot: I haz the power, 2017-04-13 20:00:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jam, 2017-04-13 20:00:54]

Daniel: content/common
Yuzhu: the reste

[yzshen1, 2017-04-13 20:43:51]

https://codereview.chromium.org/2817033002/diff/80001/content/browser/service...
File content/browser/service_worker/service_worker_fetch_dispatcher.cc (right):

https://codereview.chromium.org/2817033002/diff/80001/content/browser/service...
content/browser/service_worker/service_worker_fetch_dispatcher.cc:164:
client_->OnReceiveResponse(head, std::move(ssl_info),
No need to use std::move().

https://codereview.chromium.org/2817033002/diff/80001/content/common/resource...
File content/common/resource_messages.cc (right):

https://codereview.chromium.org/2817033002/diff/80001/content/common/resource...
content/common/resource_messages.cc:54: cert->Persist(&temp);
This means the certificate is serialized twice (once here and one one line 62).
I am not sure how is the perf impact of doing this. Is there a cheap way to
calculate an upper bound?

https://codereview.chromium.org/2817033002/diff/80001/content/common/url_load...
File content/common/url_loader_factory.mojom (right):

https://codereview.chromium.org/2817033002/diff/80001/content/common/url_load...
content/common/url_loader_factory.mojom:12: enum URLLoadOptions {
Mojo enum is mapped to enum class, which we usually don't use as bit masks.

Please consider defining them as uint32_t constants. Something like:
uint32 kURLLoadOptionNone = 0;
uint32 kURLLoadOptionSendSSLInfo = 1;

https://codereview.chromium.org/2817033002/diff/80001/content/common/url_load...
content/common/url_loader_factory.mojom:25: int32 options,
- Please consider using uint32.
- Please add comments about what values are used in |options|.

[jam, 2017-04-13 21:07:23]

The CQ bit was checked by jam@chromium.org to run a CQ dry run

[jam, 2017-04-13 21:07:29]

https://codereview.chromium.org/2817033002/diff/80001/content/browser/service...
File content/browser/service_worker/service_worker_fetch_dispatcher.cc (right):

https://codereview.chromium.org/2817033002/diff/80001/content/browser/service...
content/browser/service_worker/service_worker_fetch_dispatcher.cc:164:
client_->OnReceiveResponse(head, std::move(ssl_info),
On 2017/04/13 20:43:50, yzshen1 wrote:
> No need to use std::move().

Done.

https://codereview.chromium.org/2817033002/diff/80001/content/common/resource...
File content/common/resource_messages.cc (right):

https://codereview.chromium.org/2817033002/diff/80001/content/common/resource...
content/common/resource_messages.cc:54: cert->Persist(&temp);
On 2017/04/13 20:43:50, yzshen1 wrote:
> This means the certificate is serialized twice (once here and one one line
62).
> I am not sure how is the perf impact of doing this. Is there a cheap way to
> calculate an upper bound?

Yes, that is how we implement GetCertSize for other classes as well (see above
in this file).

https://codereview.chromium.org/2817033002/diff/80001/content/common/url_load...
File content/common/url_loader_factory.mojom (right):

https://codereview.chromium.org/2817033002/diff/80001/content/common/url_load...
content/common/url_loader_factory.mojom:12: enum URLLoadOptions {
On 2017/04/13 20:43:51, yzshen1 wrote:
> Mojo enum is mapped to enum class, which we usually don't use as bit masks.
> 
> Please consider defining them as uint32_t constants. Something like:
> uint32 kURLLoadOptionNone = 0;
> uint32 kURLLoadOptionSendSSLInfo = 1;

Done.

https://codereview.chromium.org/2817033002/diff/80001/content/common/url_load...
content/common/url_loader_factory.mojom:25: int32 options,
On 2017/04/13 20:43:51, yzshen1 wrote:
> - Please consider using uint32.
> - Please add comments about what values are used in |options|.

Done.

[commit-bot: I haz the power, 2017-04-13 21:08:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dcheng, 2017-04-13 21:12:15]

https://codereview.chromium.org/2817033002/diff/80001/content/browser/loader/...
File content/browser/loader/mojo_async_resource_handler.cc (right):

https://codereview.chromium.org/2817033002/diff/80001/content/browser/loader/...
content/browser/loader/mojo_async_resource_handler.cc:199:
base::Optional<net::SSLInfo>(),
Nit: base::nullopt

https://codereview.chromium.org/2817033002/diff/80001/content/child/resource_...
File content/child/resource_dispatcher.cc (right):

https://codereview.chromium.org/2817033002/diff/80001/content/child/resource_...
content/child/resource_dispatcher.cc:768: base::Optional<net::SSLInfo>(),
Ditto with base::nullopt, here and elsewhere

https://codereview.chromium.org/2817033002/diff/80001/content/common/resource...
File content/common/resource_messages.cc (right):

https://codereview.chromium.org/2817033002/diff/80001/content/common/resource...
content/common/resource_messages.cc:54: cert->Persist(&temp);
On 2017/04/13 20:43:50, yzshen1 wrote:
> This means the certificate is serialized twice (once here and one one line
62).
> I am not sure how is the perf impact of doing this. Is there a cheap way to
> calculate an upper bound?

We could try seeing if https://crbug.com/681080 is feasible as well, right?

https://codereview.chromium.org/2817033002/diff/80001/content/common/resource...
content/common/resource_messages.cc:82: GetCertSize(s, p.cert.get());
I'm curious--do we want to be serializing the rest of this at all if is_valid()
is false?

https://codereview.chromium.org/2817033002/diff/80001/content/common/resource...
File content/common/resource_messages.h (right):

https://codereview.chromium.org/2817033002/diff/80001/content/common/resource...
content/common/resource_messages.h:148: IPC_ENUM_TRAITS(net::TokenBindingParam)
Can we add a MAX value for this enum?

https://codereview.chromium.org/2817033002/diff/80001/content/common/resource...
content/common/resource_messages.h:149:
IPC_ENUM_TRAITS(net::SSLInfo::HandshakeType)
Ditto for this and the other enums.

[yzshen1, 2017-04-13 21:19:34]

LGTM

https://codereview.chromium.org/2817033002/diff/80001/content/common/resource...
File content/common/resource_messages.cc (right):

https://codereview.chromium.org/2817033002/diff/80001/content/common/resource...
content/common/resource_messages.cc:54: cert->Persist(&temp);
On 2017/04/13 21:12:15, dcheng wrote:
> On 2017/04/13 20:43:50, yzshen1 wrote:
> > This means the certificate is serialized twice (once here and one one line
> 62).
> > I am not sure how is the perf impact of doing this. Is there a cheap way to
> > calculate an upper bound?
> 
> We could try seeing if https://crbug.com/681080 is feasible as well, right?

Yeah. It will make even more sense if we support using multiple buffers to back
a single message, without copying the data when we need to grow the buffer(s).
There was a discussion for that feature, but I couldn't find the bug link at the
moment.

Anyway, that is out of scope of this review thread, I think. :)

[mmenke, 2017-04-13 21:36:45]

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

[mmenke, 2017-04-13 21:36:46]

https://codereview.chromium.org/2817033002/diff/100001/content/network/url_lo...
File content/network/url_loader_unittest.cc (right):

https://codereview.chromium.org/2817033002/diff/100001/content/network/url_lo...
content/network/url_loader_unittest.cc:129:
ASSERT_TRUE(!!client.ssl_info()->cert);
Should we check that the data is correct as well?  EmbeddedTestServer lets us
grab its cert directly, I believe, and we can check the other fields of
ssl_info, too.

[jam, 2017-04-13 23:04:22]

https://codereview.chromium.org/2817033002/diff/80001/content/browser/loader/...
File content/browser/loader/mojo_async_resource_handler.cc (right):

https://codereview.chromium.org/2817033002/diff/80001/content/browser/loader/...
content/browser/loader/mojo_async_resource_handler.cc:199:
base::Optional<net::SSLInfo>(),
On 2017/04/13 21:12:15, dcheng wrote:
> Nit: base::nullopt

Done.

https://codereview.chromium.org/2817033002/diff/80001/content/child/resource_...
File content/child/resource_dispatcher.cc (right):

https://codereview.chromium.org/2817033002/diff/80001/content/child/resource_...
content/child/resource_dispatcher.cc:768: base::Optional<net::SSLInfo>(),
On 2017/04/13 21:12:15, dcheng wrote:
> Ditto with base::nullopt, here and elsewhere

Done.

https://codereview.chromium.org/2817033002/diff/80001/content/common/resource...
File content/common/resource_messages.cc (right):

https://codereview.chromium.org/2817033002/diff/80001/content/common/resource...
content/common/resource_messages.cc:54: cert->Persist(&temp);
On 2017/04/13 21:19:34, yzshen1 wrote:
> On 2017/04/13 21:12:15, dcheng wrote:
> > On 2017/04/13 20:43:50, yzshen1 wrote:
> > > This means the certificate is serialized twice (once here and one one line
> > 62).
> > > I am not sure how is the perf impact of doing this. Is there a cheap way
to
> > > calculate an upper bound?
> > 
> > We could try seeing if https://crbug.com/681080 is feasible as well, right?
> 
> Yeah. It will make even more sense if we support using multiple buffers to
back
> a single message, without copying the data when we need to grow the buffer(s).
> There was a discussion for that feature, but I couldn't find the bug link at
the
> moment.
> 
> Anyway, that is out of scope of this review thread, I think. :)

yep, agreed it's orthogonal :)

https://codereview.chromium.org/2817033002/diff/80001/content/common/resource...
content/common/resource_messages.cc:82: GetCertSize(s, p.cert.get());
On 2017/04/13 21:12:15, dcheng wrote:
> I'm curious--do we want to be serializing the rest of this at all if
is_valid()
> is false?

Good point, done

https://codereview.chromium.org/2817033002/diff/80001/content/common/resource...
File content/common/resource_messages.h (right):

https://codereview.chromium.org/2817033002/diff/80001/content/common/resource...
content/common/resource_messages.h:148: IPC_ENUM_TRAITS(net::TokenBindingParam)
On 2017/04/13 21:12:15, dcheng wrote:
> Can we add a MAX value for this enum?

I was avoiding changing src/net for now with these changes; however I switched
to the MAX_VALUE and put the last value for now. This is sometimes done with the
macro for enums with no max value.

https://codereview.chromium.org/2817033002/diff/100001/content/network/url_lo...
File content/network/url_loader_unittest.cc (right):

https://codereview.chromium.org/2817033002/diff/100001/content/network/url_lo...
content/network/url_loader_unittest.cc:129:
ASSERT_TRUE(!!client.ssl_info()->cert);
On 2017/04/13 21:36:46, mmenke wrote:
> Should we check that the data is correct as well?  EmbeddedTestServer lets us
> grab its cert directly, I believe, and we can check the other fields of
> ssl_info, too.

Sure, but I couldn't find a way to get the SSLInfo?

[jam, 2017-04-13 23:04:31]

The CQ bit was checked by jam@chromium.org to run a CQ dry run

[mmenke, 2017-04-13 23:08:45]

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

[mmenke, 2017-04-13 23:08:46]

[+davidben]:  Don't suppose you know how to very that we're getting a sane
SSLInfo from the TestServer, or know who would have ideas in that space? 
Context is that we're adding SSL info to the Mojo interface, and want to test
it.  See content/network/url_loader_unittest.cc

[commit-bot: I haz the power, 2017-04-13 23:32:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-13 23:37:46]

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

[commit-bot: I haz the power, 2017-04-13 23:37:47]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[jam, 2017-04-13 23:48:21]

The CQ bit was checked by jam@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-13 23:48:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dcheng, 2017-04-14 01:11:10]

ipc lgtm

I audited X509Certificate::CreateFromPickle as best as I could and it seems ok;
there's a few things that would normally be discouraged for IPC serialization,
such as checked_cast in the code, but happen to be OK here (it's a checked_cast
of size_t to unsigned, but the originally value is guaranteed to be a
non-negative int).

That being said, I'd like rsleevi@ to verify that this usage is OK and that
calling things like CERT_NewTempCertificate on arbitrary bytes is safe-ish.
Another question here is whether we're fuzzing these entry points to NSS (and if
we should be). As the network service is still behind a flag, it's OK to land
without blocking on rsleevi@ here, but these are things we should investigate in
parallel.

https://codereview.chromium.org/2817033002/diff/120001/content/common/resourc...
File content/common/resource_messages.cc (right):

https://codereview.chromium.org/2817033002/diff/120001/content/common/resourc...
content/common/resource_messages.cc:51: GetParamSize(s, !!cert);
Do we still need to handle nullptr here now that the serialization for SSLInfo
checks is_valid? If not, I'd prefer we remove it and DCHECK in GetCertSize and
WriteCert.

[commit-bot: I haz the power, 2017-04-14 01:37:54]

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

[commit-bot: I haz the power, 2017-04-14 01:37:55]

Dry run: This issue passed the CQ dry run.

[Ryan Sleevi, 2017-04-14 01:47:37]

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

[Ryan Sleevi, 2017-04-14 01:47:38]

I would prefer this not land as-is, as it seems a very dangerous pattern that
will limit the flexibility of //net by spreading through dependencies on
portions of the API that are not intended to be as a stable contract for
serialization.

I apologize if I don't understand some of the Mojo bits, but it would seem
desirable that this be somewhat closer to the //net code, or, if that's not
possible, that this be coded more defensively to assume the //net interfaces can
and will change, and this should still return appropriate responses.

Regarding the maliciousness point that dcheng@ raised, yes, the //net interfaces
need to be robust against malicious inputs because we will call these methods
directly on data received over the network that is unauthenticated (in the case
of TLS handshakes). So there's robustness there. We also fuzz many of the
BoringSSL and NSS entry-points via clusterfuzz or oss-fuzz, so we should have
robustness there as well.

It may be that my paranoia about change is unwarranted, but I do hope there's a
better way to defend against lower-level interfaces changing, especially one
which has seen changes. There have been 12 changes over the past 20 months that
would have broken this serialization, as written, and would not have been
detected by these tests, as far as I can tell.

Perhaps it would be appropriate to add a serialize/deserialize + equality check
to robustly defend against this? Is that the norm to make sure all objects you
don't "own" robustly roundtrip?

https://codereview.chromium.org/2817033002/diff/140001/content/common/resourc...
File content/common/resource_messages.cc (right):

https://codereview.chromium.org/2817033002/diff/140001/content/common/resourc...
content/common/resource_messages.cc:54: cert->Persist(&temp);
I'll see about updating the documentation. The Pickle implementation is intended
to be an internal detail of //net (related to how the disk cache is implemented)
moreso than a stable interface.

We can and have in the past changed the pickle format in conjunction with
changing the disk cache. Does that create issues? If so, we should be more
explicit here with serialization.

However, if this code doesn't have version skew issues (I thought Mojo tried to
allow version skew), then that's probably less of an issue.

https://codereview.chromium.org/2817033002/diff/140001/content/common/resourc...
content/common/resource_messages.cc:80: void
ParamTraits<net::SSLInfo>::GetSize(base::PickleSizer* s,
This makes me nervous, because we can and do add new fields to net::SSLInfo
here, and it would seem that these would be uninitialized by consumers if this
code wasn't updated.

What steps can we take to defend against the lower API changing, which would
result in a loss of information?

https://codereview.chromium.org/2817033002/diff/140001/content/common/resourc...
File content/common/resource_messages.h (right):

https://codereview.chromium.org/2817033002/diff/140001/content/common/resourc...
content/common/resource_messages.h:159: net::OCSPRevocationStatus::UNKNOWN)
Should these be in //net/interfaces? Why or why not?

I'm just curious because this creates a new API dependency that these max values
will always be the max value, but that's not an intended API contract from the
//net perspective, so I'm wondering how we can appropriately surface this
dependency?

https://codereview.chromium.org/2817033002/diff/140001/content/common/ssl_inf...
File content/common/ssl_info.typemap (right):

https://codereview.chromium.org/2817033002/diff/140001/content/common/ssl_inf...
content/common/ssl_info.typemap:6: public_headers = [ "//net/ssl/ssl_info.h" ]
Why not //net/interfaces ?

[jam, 2017-04-14 02:34:14]

https://codereview.chromium.org/2817033002/diff/140001/content/common/resourc...
File content/common/resource_messages.cc (right):

https://codereview.chromium.org/2817033002/diff/140001/content/common/resourc...
content/common/resource_messages.cc:54: cert->Persist(&temp);
On 2017/04/14 01:47:38, Ryan Sleevi wrote:
> I'll see about updating the documentation. The Pickle implementation is
intended
> to be an internal detail of //net (related to how the disk cache is
implemented)
> moreso than a stable interface.
> 
> We can and have in the past changed the pickle format in conjunction with
> changing the disk cache. Does that create issues? If so, we should be more
> explicit here with serialization.
> 
> However, if this code doesn't have version skew issues (I thought Mojo tried
to
> allow version skew), then that's probably less of an issue.

If the entire comment is only about version skew, then that's not an issue. We
have no plans in the next year or two of supporting different versions of these
APIs. If that changes, then at that point we would have to audit every data type
sent over APIs we want to freeze.

https://codereview.chromium.org/2817033002/diff/140001/content/common/resourc...
content/common/resource_messages.cc:80: void
ParamTraits<net::SSLInfo>::GetSize(base::PickleSizer* s,
On 2017/04/14 01:47:38, Ryan Sleevi wrote:
> This makes me nervous, because we can and do add new fields to net::SSLInfo
> here, and it would seem that these would be uninitialized by consumers if this
> code wasn't updated.

Well, the default constructor runs first, so at worst they should have whatever
that sets as default.

In general, the issue of having to match serialization code with structs is
something that has existed since the beginning of ipc but hasn't turned out to
be a problem in practice. i.e. this happens to a lot of structs defined in
cc/gpu/net/skia etc... that have traits in content/

> 
> What steps can we take to defend against the lower API changing, which would
> result in a loss of information?

https://codereview.chromium.org/2817033002/diff/140001/content/common/resourc...
File content/common/resource_messages.h (right):

https://codereview.chromium.org/2817033002/diff/140001/content/common/resourc...
content/common/resource_messages.h:159: net::OCSPRevocationStatus::UNKNOWN)
On 2017/04/14 01:47:38, Ryan Sleevi wrote:
> Should these be in //net/interfaces? Why or why not?

I don't know why the OOP proxy resolver had it's IPCs in net/interfaces. In
general though, we have traditionally put IPC traits used by content/ in
content, and avoided a dependency on /ipc by /net. i.e. see
https://cs.chromium.org/search/?q=file:src/content+file:traits+%22net::%22+pa...

> 
> I'm just curious because this creates a new API dependency that these max
values
> will always be the max value, but that's not an intended API contract from the
> //net perspective, so I'm wondering how we can appropriately surface this
> dependency?

I can add _MAX values to the net enums if you prefer that.

https://codereview.chromium.org/2817033002/diff/140001/content/common/ssl_inf...
File content/common/ssl_info.typemap (right):

https://codereview.chromium.org/2817033002/diff/140001/content/common/ssl_inf...
content/common/ssl_info.typemap:6: public_headers = [ "//net/ssl/ssl_info.h" ]
On 2017/04/14 01:47:38, Ryan Sleevi wrote:
> Why not //net/interfaces ?

A typemap's public_headers is a section which lists which headers describe the
typemap'd struct. See
https://chromium.googlesource.com/chromium/src/+/master/mojo/public/cpp/bindi...

[jam, 2017-04-14 02:40:29]

The CQ bit was checked by jam@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-14 02:40:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-14 04:34:22]

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

[commit-bot: I haz the power, 2017-04-14 04:34:23]

Dry run: This issue passed the CQ dry run.

[Ryan Sleevi, 2017-04-14 14:53:26]

Hi John,

I'm not really sure that addresses my comments. It feels like your response is
saying "Bugs happen, no big deal", but I don't think that's what you intended.

Given that this is used to make security determination on the page, it seems
important to get right. I don't think "This is how we do it" feels like a
satisfying answer.

Would you be able to add tests to this (and potentially other stuctures) to make
sure they round-trip appropriately? Thanks.

https://codereview.chromium.org/2817033002/diff/140001/content/common/resourc...
File content/common/resource_messages.cc (right):

https://codereview.chromium.org/2817033002/diff/140001/content/common/resourc...
content/common/resource_messages.cc:54: cert->Persist(&temp);
On 2017/04/14 02:34:14, jam wrote:
> On 2017/04/14 01:47:38, Ryan Sleevi wrote:
> > I'll see about updating the documentation. The Pickle implementation is
> intended
> > to be an internal detail of //net (related to how the disk cache is
> implemented)
> > moreso than a stable interface.
> > 
> > We can and have in the past changed the pickle format in conjunction with
> > changing the disk cache. Does that create issues? If so, we should be more
> > explicit here with serialization.
> > 
> > However, if this code doesn't have version skew issues (I thought Mojo tried
> to
> > allow version skew), then that's probably less of an issue.
> 
> If the entire comment is only about version skew, then that's not an issue. We
> have no plans in the next year or two of supporting different versions of
these
> APIs. If that changes, then at that point we would have to audit every data
type
> sent over APIs we want to freeze.

It is not.

[davidben, 2017-04-14 15:10:34]

I'm probably woefully out of date here, but doesn't URLResponseHead already
contain bits of this information? There's a bunch of SSL-related fields on
ResourceResponseInfo.

I would actually love for the work needed to plumb a new field in to be shorter.
I need to add another value here and have been dreading how 
much trouble it'll be to add it to every layer. Though I thought about whether
folding SSLInfo into ResourceResponseInfo was the right answer. Certainly it
seems odd that it's duplicated in SSLInfo and ResourceResponseInfo.

[mmenke, 2017-04-14 15:16:25]

On 2017/04/14 15:10:34, davidben wrote:
> I'm probably woefully out of date here, but doesn't URLResponseHead already
> contain bits of this information? There's a bunch of SSL-related fields on
> ResourceResponseInfo.

Do you mean content/public/common/resource_response_info.h (Which is a subclass
of ResourceResponseHead, which is what the mojo class URLResponseHead is
apparently an alias for)?  It does contain a "DER-encoded" cert chain, at least.
 No idea if that's sufficient for what's needed, though I'm certainly all for
minimizing the structures we expose outside the network service, whenever we
reasonably can.

> I would actually love for the work needed to plumb a new field in to be
shorter.
> I need to add another value here and have been dreading how 
> much trouble it'll be to add it to every layer. Though I thought about whether
> folding SSLInfo into ResourceResponseInfo was the right answer. Certainly it
> seems odd that it's duplicated in SSLInfo and ResourceResponseInfo.

[davidben, 2017-04-14 15:37:19]

On 2017/04/14 15:16:25, mmenke wrote:
> On 2017/04/14 15:10:34, davidben wrote:
> > I'm probably woefully out of date here, but doesn't URLResponseHead already
> > contain bits of this information? There's a bunch of SSL-related fields on
> > ResourceResponseInfo.
> 
> Do you mean content/public/common/resource_response_info.h (Which is a
subclass
> of ResourceResponseHead, which is what the mojo class URLResponseHead is
> apparently an alias for)?  It does contain a "DER-encoded" cert chain, at
least.
>  No idea if that's sufficient for what's needed, though I'm certainly all for
> minimizing the structures we expose outside the network service, whenever we
> reasonably can.

It's got that, cert_status, ssl_connection_status, ssl_key_exchange_group, and
signed_certificate_timestamps. (And then security_state::SecurityInfo has a
different subset and Blink has its own subset and it's all a giant mess.)

> > I would actually love for the work needed to plumb a new field in to be
> shorter.
> > I need to add another value here and have been dreading how 
> > much trouble it'll be to add it to every layer. Though I thought about
whether
> > folding SSLInfo into ResourceResponseInfo was the right answer. Certainly it
> > seems odd that it's duplicated in SSLInfo and ResourceResponseInfo.

[jam, 2017-04-14 15:38:26]

On 2017/04/14 14:53:26, Ryan Sleevi wrote:
> Hi John,
> 
> I'm not really sure that addresses my comments. It feels like your response is
> saying "Bugs happen, no big deal", but I don't think that's what you intended.

You had a few issues raised (i.e. location of the traits, whether version skew
is allowed, testing). Do you have more clarity now on the other issues that you
consider them addressed?

> 
> Given that this is used to make security determination on the page, it seems
> important to get right. I don't think "This is how we do it" feels like a
> satisfying answer.

Just to be clear, that wasn't what I meant :)

In general, we depend on tests. i.e. we have about 120 SSL* and SecurityState*
tests that operate on data derived from ne::SSLInfo. So if someone adds a new
field there and it affects the security state of the page, then there should be
a test added to verify that. Once this code is eventually running on a bot,
those tests will be exercised.

That's at the browser_test level.

For the URLLoader, I had added a test in this cl that verifies that the
certificate is serialized and deserialized correctly. Matt pointed it would be
better to also check if the SSLInfo is serialized correctly, I couldn't find a
way to get it from the http server.
> 
> Would you be able to add tests to this (and potentially other stuctures) to
make
> sure they round-trip appropriately? Thanks.

I don't think it's in scope for this cl to add tests for other structures. But
if you have specific ideas about a test that can be added with today's testing
infrastructure, then sure.

> 
>
https://codereview.chromium.org/2817033002/diff/140001/content/common/resourc...
> File content/common/resource_messages.cc (right):
> 
>
https://codereview.chromium.org/2817033002/diff/140001/content/common/resourc...
> content/common/resource_messages.cc:54: cert->Persist(&temp);
> On 2017/04/14 02:34:14, jam wrote:
> > On 2017/04/14 01:47:38, Ryan Sleevi wrote:
> > > I'll see about updating the documentation. The Pickle implementation is
> > intended
> > > to be an internal detail of //net (related to how the disk cache is
> > implemented)
> > > moreso than a stable interface.
> > > 
> > > We can and have in the past changed the pickle format in conjunction with
> > > changing the disk cache. Does that create issues? If so, we should be more
> > > explicit here with serialization.
> > > 
> > > However, if this code doesn't have version skew issues (I thought Mojo
tried
> > to
> > > allow version skew), then that's probably less of an issue.
> > 
> > If the entire comment is only about version skew, then that's not an issue.
We
> > have no plans in the next year or two of supporting different versions of
> these
> > APIs. If that changes, then at that point we would have to audit every data
> type
> > sent over APIs we want to freeze.
> 
> It is not.

I'm confused then. Your original comment did sound about pickle format changes
and mojo version skew. Those parts seem addressed from my comment?
If your concern that X509 certificates are being serialized and you only want
that to be called by the cache, then I'm not sure I follow. i.e. inherent of
running code out of process is that we will have to serialize data structures
that we currently were not. It seems that if we want to serialize the cert
class, having two different ways to serialize it would be suboptimal.

[jam, 2017-04-14 15:38:31]

On 2017/04/14 15:16:25, mmenke wrote:
> On 2017/04/14 15:10:34, davidben wrote:
> > I'm probably woefully out of date here, but doesn't URLResponseHead already
> > contain bits of this information? There's a bunch of SSL-related fields on
> > ResourceResponseInfo.
> 
> Do you mean content/public/common/resource_response_info.h (Which is a
subclass
> of ResourceResponseHead, which is what the mojo class URLResponseHead is
> apparently an alias for)?  It does contain a "DER-encoded" cert chain, at
least.
>  No idea if that's sufficient for what's needed, though I'm certainly all for
> minimizing the structures we expose outside the network service, whenever we
> reasonably can.
> 
> > I would actually love for the work needed to plumb a new field in to be
> shorter.
> > I need to add another value here and have been dreading how 
> > much trouble it'll be to add it to every layer. Though I thought about
whether
> > folding SSLInfo into ResourceResponseInfo was the right answer. Certainly it
> > seems odd that it's duplicated in SSLInfo and ResourceResponseInfo.

ResourceResponseInfo contains the minimal amount of information required at the
renderer.
This change is needed to plumb through the information needed at the browser.
Since a lot of the code depends on getting net::SSLInfo, I plumbed through the
structure instead of a subset.

[Ryan Sleevi, 2017-04-14 15:49:46]

On 2017/04/14 15:38:26, jam wrote:
> Just to be clear, that wasn't what I meant :)
> 
> In general, we depend on tests. i.e. we have about 120 SSL* and SecurityState*
> tests that operate on data derived from ne::SSLInfo. So if someone adds a new
> field there and it affects the security state of the page, then there should
be
> a test added to verify that. Once this code is eventually running on a bot,
> those tests will be exercised.

That suggests that you think integration tests should cover this, correct?

> I don't think it's in scope for this cl to add tests for other structures. But
> if you have specific ideas about a test that can be added with today's testing
> infrastructure, then sure.

So, you could test that your serialization code is correct by creating a test to
specifically exercise that, as follows:

Create a net::SSLInfo
Set all the fields on the net::SSLInfo
Serialize
Deserialize into a new net::SSLInfo
Test for equality

This is a unit test for your serialization code, by ensuring all fields of the
net::SSLInfo appropriately round-trip as specified. It does not address the case
of whether fields are added (but not serialized), but minimally ensures that
net::SSLInfo serialization code is fully covered and tested.

Regarding ensuring that net::SSLInfo code field changes are safe, I don't see a
good way to do this, short of using a static_assert on the sizeof(), to make
sure no fields are added or removed. This serves as an appropriate unit test
that the serialization code is insulated from changes on the lower layer, and
does not require an integration test of the interactions at the higher layer.

> I'm confused then. Your original comment did sound about pickle format changes
> and mojo version skew. Those parts seem addressed from my comment?

Apologies, I misread the comment. You are correct.

[jam, 2017-04-14 17:18:47]

The CQ bit was checked by jam@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-14 17:18:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jam, 2017-04-14 17:21:04]

On 2017/04/14 15:49:46, Ryan Sleevi wrote:
> On 2017/04/14 15:38:26, jam wrote:
> > Just to be clear, that wasn't what I meant :)
> > 
> > In general, we depend on tests. i.e. we have about 120 SSL* and
SecurityState*
> > tests that operate on data derived from ne::SSLInfo. So if someone adds a
new
> > field there and it affects the security state of the page, then there should
> be
> > a test added to verify that. Once this code is eventually running on a bot,
> > those tests will be exercised.
> 
> That suggests that you think integration tests should cover this, correct?

Right

> 
> > I don't think it's in scope for this cl to add tests for other structures.
But
> > if you have specific ideas about a test that can be added with today's
testing
> > infrastructure, then sure.
> 
> So, you could test that your serialization code is correct by creating a test
to
> specifically exercise that, as follows:
> 
> Create a net::SSLInfo
> Set all the fields on the net::SSLInfo
> Serialize
> Deserialize into a new net::SSLInfo
> Test for equality
> 
> This is a unit test for your serialization code, by ensuring all fields of the
> net::SSLInfo appropriately round-trip as specified. It does not address the
case
> of whether fields are added (but not serialized), but minimally ensures that
> net::SSLInfo serialization code is fully covered and tested.

Sure, done. Looks like that uncovered one missing write call.

> 
> Regarding ensuring that net::SSLInfo code field changes are safe, I don't see
a
> good way to do this, short of using a static_assert on the sizeof(), to make
> sure no fields are added or removed. This serves as an appropriate unit test
> that the serialization code is insulated from changes on the lower layer, and
> does not require an integration test of the interactions at the higher layer.

Unfortunately this isn't quite possible, as the serialized size of a type isn't
always the same size as the type. i.e. we add extra data for container size, and
types that have different size on 32 vs 64 bit are always sent as the 64 bit
size.

> 
> > I'm confused then. Your original comment did sound about pickle format
changes
> > and mojo version skew. Those parts seem addressed from my comment?
> 
> Apologies, I misread the comment. You are correct.

[Ryan Sleevi, 2017-04-14 19:00:09]

lgtm

[commit-bot: I haz the power, 2017-04-14 21:15:33]

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

[commit-bot: I haz the power, 2017-04-14 21:15:34]

Dry run: This issue passed the CQ dry run.

[jam, 2017-04-14 21:19:56]

The CQ bit was checked by jam@chromium.org

[jam, 2017-04-14 21:19:56]

The patchset sent to the CQ was uploaded after l-g-t-m from yzshen@chromium.org,
dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2817033002/#ps180001
(title: "add net::SSLInfo test")

[commit-bot: I haz the power, 2017-04-14 21:20:29]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-14 21:28:51]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1492204796328020,
"parent_rev": "fd475a9ac2f060c7d7ecc43b3ea2b526fa017dcb", "commit_rev":
"33d897e97d8a3fc40ddf1d0b9ca578041724873a"}

[commit-bot: I haz the power, 2017-04-14 21:29:41]

Description was changed from

==========
Plumb the net::SSLInfo to the browser process when it's using the network
service.

This fixes the page lock icon not showing the correct indicators. Since the data
is large, only send it to the consumer if they request it at URLLoader creation
time. Only the browser process would request it.

BUG=598073
==========

to

==========
Plumb the net::SSLInfo to the browser process when it's using the network
service.

This fixes the page lock icon not showing the correct indicators. Since the data
is large, only send it to the consumer if they request it at URLLoader creation
time. Only the browser process would request it.

BUG=598073

Review-Url: https://codereview.chromium.org/2817033002
Cr-Commit-Position: refs/heads/master@{#464800}
Committed:
https://chromium.googlesource.com/chromium/src/+/33d897e97d8a3fc40ddf1d0b9ca5...
==========

[commit-bot: I haz the power, 2017-04-14 21:29:44]

Committed patchset #7 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/33d897e97d8a3fc40ddf1d0b9ca5...