Resolve certificate references in ONC by PEM.

Resolve certificate references in ONC by PEM.

In ONC, Server and CA certificates are referenced by GUID.
Before, the GUID was stored in the nickname of each certificate and used to identify each certificate.
After this change, the GUID is resolved and replaced by the PEM encoding of the certificate during import. The nickname is not used.

This commit only affects Server and CA certificates (including IssuerCARef in CertificatePatterns).
Client certificates are still identified by GUID.

This CL also 
- uses the new *CaCertPEMProperty fields of Shill.
- prepares for a list of CaCerts (for EAP, IPsec and OpenVPN)

Side-effect of this CL:
IssuerCARef is stored in the UIData service-property in Shill. Because this CL replaces IssuerCARef by IssuerCAPEMs, IssuerCARef entries of old UIData properties are ignored.
This may break network configurations which were configured via chrome://net-internals.
Reimporting such a configuration will fix the problem.

BUG=208986
TBR=eroman@chromium.org (for net_internals_ui.cc)

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

[pneubeck (no reviews), 2013-06-13 14:57:10]

Not finished and not tested yet.
So feel free to wait until tomorrow with the review.

[Mattias Nissler (ping if slow), 2013-06-14 12:56:44]

Looks reasonable, the only grief I have is that NSS cannot look up certificates
by fingerprints. Then again, we're going through the entire cert list to find a
suitable cert already anyhow, so it doesn't make a difference.

https://codereview.chromium.org/16946002/diff/5001/chrome/browser/chromeos/cr...
File chrome/browser/chromeos/cros/cert_library.cc (right):

https://codereview.chromium.org/16946002/diff/5001/chrome/browser/chromeos/cr...
chrome/browser/chromeos/cros/cert_library.cc:174: CertType type, const
std::string& fingerprint) const {
nit: one parameter per line

https://codereview.chromium.org/16946002/diff/5001/chrome/browser/chromeos/cr...
chrome/browser/chromeos/cros/cert_library.cc:182: LOG(ERROR) << "Certificate
fingerprints collided.";
I guess it's more likely that the cert is present twice in the list in this case
:)

https://codereview.chromium.org/16946002/diff/5001/chromeos/network/onc/onc_c...
File chromeos/network/onc/onc_certificate_importer.h (right):

https://codereview.chromium.org/16946002/diff/5001/chromeos/network/onc/onc_c...
chromeos/network/onc/onc_certificate_importer.h:33: // is stored together with
the certificate as Nickname.
you'll want to update this comment

https://codereview.chromium.org/16946002/diff/5001/net/cert/x509_certificate.cc
File net/cert/x509_certificate.cc (right):

https://codereview.chromium.org/16946002/diff/5001/net/cert/x509_certificate....
net/cert/x509_certificate.cc:685: return net::GetPEMEncoded(os_cert_handle(),
pem_encoded);
net qualifier not needed?

https://codereview.chromium.org/16946002/diff/5001/net/cert/x509_certificate....
net/cert/x509_certificate.cc:696: if
(!net::GetPEMEncoded(intermediate_ca_certs_[i], &pem_data))
no need for the net qualifier?

https://codereview.chromium.org/16946002/diff/5001/net/cert/x509_certificate.h
File net/cert/x509_certificate.h (right):

https://codereview.chromium.org/16946002/diff/5001/net/cert/x509_certificate....
net/cert/x509_certificate.h:350: // Encodes the this certificate (ignoring
intermediate certificates stored in
remove "the"

[pneubeck (no reviews), 2013-06-21 12:53:55]

https://codereview.chromium.org/16946002/diff/5001/chrome/browser/chromeos/cr...
File chrome/browser/chromeos/cros/cert_library.cc (right):

https://codereview.chromium.org/16946002/diff/5001/chrome/browser/chromeos/cr...
chrome/browser/chromeos/cros/cert_library.cc:174: CertType type, const
std::string& fingerprint) const {
On 2013/06/14 12:56:44, Mattias Nissler wrote:
> nit: one parameter per line

Done.

https://codereview.chromium.org/16946002/diff/5001/chrome/browser/chromeos/cr...
chrome/browser/chromeos/cros/cert_library.cc:182: LOG(ERROR) << "Certificate
fingerprints collided.";
On 2013/06/14 12:56:44, Mattias Nissler wrote:
> I guess it's more likely that the cert is present twice in the list in this
case
> :)

Done.

https://codereview.chromium.org/16946002/diff/5001/chromeos/network/onc/onc_c...
File chromeos/network/onc/onc_certificate_importer.h (right):

https://codereview.chromium.org/16946002/diff/5001/chromeos/network/onc/onc_c...
chromeos/network/onc/onc_certificate_importer.h:33: // is stored together with
the certificate as Nickname.
On 2013/06/14 12:56:44, Mattias Nissler wrote:
> you'll want to update this comment

Done.

https://codereview.chromium.org/16946002/diff/5001/net/cert/x509_certificate.cc
File net/cert/x509_certificate.cc (right):

https://codereview.chromium.org/16946002/diff/5001/net/cert/x509_certificate....
net/cert/x509_certificate.cc:685: return net::GetPEMEncoded(os_cert_handle(),
pem_encoded);
On 2013/06/14 12:56:44, Mattias Nissler wrote:
> net qualifier not needed?

Otherwise, GCC doesn't resolve to the function in the anonymous namespace.

https://codereview.chromium.org/16946002/diff/5001/net/cert/x509_certificate....
net/cert/x509_certificate.cc:696: if
(!net::GetPEMEncoded(intermediate_ca_certs_[i], &pem_data))
On 2013/06/14 12:56:44, Mattias Nissler wrote:
> no need for the net qualifier?

ditto.

https://codereview.chromium.org/16946002/diff/5001/net/cert/x509_certificate.h
File net/cert/x509_certificate.h (right):

https://codereview.chromium.org/16946002/diff/5001/net/cert/x509_certificate....
net/cert/x509_certificate.h:350: // Encodes the this certificate (ignoring
intermediate certificates stored in
On 2013/06/14 12:56:44, Mattias Nissler wrote:
> remove "the"

Done.

[pneubeck (no reviews), 2013-06-21 13:10:29]

Hi Steven, PTAL. Thanks!

[pneubeck (no reviews), 2013-06-21 13:12:25]

btw. I don't have the perfect testing environment available at home.
Thus I might not commit before testing it in the office.

[stevenjb, 2013-06-21 22:47:13]

I'm a little overwhelmed by this CL. I assume there is a pressing need for it,
rather than waiting until we can extract the ONC code from NetworkLibrary (which
seems like it would lead to a much simpler implementation)?

https://codereview.chromium.org/16946002/diff/79001/chrome/browser/chromeos/c...
File chrome/browser/chromeos/cros/native_network_parser.cc (right):

https://codereview.chromium.org/16946002/diff/79001/chrome/browser/chromeos/c...
chrome/browser/chromeos/cros/native_network_parser.cc:1248:
onc::DecodePEMCertificate(eap_server_ca_cert_pem);
How expensive is this? It would be better to defer any parsing, etc, especially
since we're in the process of replacing this code; I'd really prefer not to make
NetworkState dependent on net::X509Certificate.

https://codereview.chromium.org/16946002/diff/79001/chrome/browser/chromeos/c...
chrome/browser/chromeos/cros/native_network_parser.cc:1392:
onc::DecodePEMCertificate(ca_cert_pem);
Same comment about decoding here.

https://codereview.chromium.org/16946002/diff/79001/chrome/browser/chromeos/c...
File chrome/browser/chromeos/cros/network_library.h (right):

https://codereview.chromium.org/16946002/diff/79001/chrome/browser/chromeos/c...
chrome/browser/chromeos/cros/network_library.h:1676: fingerprint_to_pem) = 0;
Typedef the callback and document how it is used.

https://codereview.chromium.org/16946002/diff/79001/chrome/browser/chromeos/c...
File chrome/browser/chromeos/cros/network_library_impl_base.cc (right):

https://codereview.chromium.org/16946002/diff/79001/chrome/browser/chromeos/c...
chrome/browser/chromeos/cros/network_library_impl_base.cc:1110:
fingerprint_to_pem);
Passing the callback through here is pretty confusing to me. I really wish this
wasn't part of NetworkLibrary. Hopefully we will have have the equivalent of
CallConfigureService() and ForgetNetworksById() available in NetworkHandler soon
and we can move this. In the meanwhile, please add a comment.

https://codereview.chromium.org/16946002/diff/79001/chrome/browser/chromeos/p...
File chrome/browser/chromeos/policy/network_configuration_updater_impl_cros.cc
(right):

https://codereview.chromium.org/16946002/diff/79001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/network_configuration_updater_impl_cros.cc:191:
imported_server_and_ca_certs_list));
This feels awkward. Do we really need to abstract this, or can we just pass a
imported_server_and_ca_certs_list through and have LocalTranslator call
GetPEMEncodedCertFromFingerprint directly?

[pneubeck (no reviews), 2013-06-24 08:06:53]

On 2013/06/21 22:47:13, stevenjb (chromium) wrote:
> I'm a little overwhelmed by this CL. I assume there is a pressing need for it,
> rather than waiting until we can extract the ONC code from NetworkLibrary
(which
> seems like it would lead to a much simpler implementation)?

We have to support the new certificate chain of google ASAP, i.e. well tested in
M30. So, I didn't see an alternative to modifying NetworkLibrary. I also tried
to keep the change small by just replacing places with nickname by fingerprint.

> 
>
https://codereview.chromium.org/16946002/diff/79001/chrome/browser/chromeos/c...
> File chrome/browser/chromeos/cros/native_network_parser.cc (right):
> 
>
https://codereview.chromium.org/16946002/diff/79001/chrome/browser/chromeos/c...
> chrome/browser/chromeos/cros/native_network_parser.cc:1248:
> onc::DecodePEMCertificate(eap_server_ca_cert_pem);
> How expensive is this? It would be better to defer any parsing, etc,
especially
> since we're in the process of replacing this code; I'd really prefer not to
make
> NetworkState dependent on net::X509Certificate.
> 
>
https://codereview.chromium.org/16946002/diff/79001/chrome/browser/chromeos/c...
> chrome/browser/chromeos/cros/native_network_parser.cc:1392:
> onc::DecodePEMCertificate(ca_cert_pem);
> Same comment about decoding here.
> 
>
https://codereview.chromium.org/16946002/diff/79001/chrome/browser/chromeos/c...
> File chrome/browser/chromeos/cros/network_library.h (right):
> 
>
https://codereview.chromium.org/16946002/diff/79001/chrome/browser/chromeos/c...
> chrome/browser/chromeos/cros/network_library.h:1676: fingerprint_to_pem) = 0;
> Typedef the callback and document how it is used.
> 
>
https://codereview.chromium.org/16946002/diff/79001/chrome/browser/chromeos/c...
> File chrome/browser/chromeos/cros/network_library_impl_base.cc (right):
> 
>
https://codereview.chromium.org/16946002/diff/79001/chrome/browser/chromeos/c...
> chrome/browser/chromeos/cros/network_library_impl_base.cc:1110:
> fingerprint_to_pem);
> Passing the callback through here is pretty confusing to me. I really wish
this
> wasn't part of NetworkLibrary. Hopefully we will have have the equivalent of
> CallConfigureService() and ForgetNetworksById() available in NetworkHandler
soon
> and we can move this. In the meanwhile, please add a comment.
> 
>
https://codereview.chromium.org/16946002/diff/79001/chrome/browser/chromeos/p...
> File chrome/browser/chromeos/policy/network_configuration_updater_impl_cros.cc
> (right):
> 
>
https://codereview.chromium.org/16946002/diff/79001/chrome/browser/chromeos/p...
> chrome/browser/chromeos/policy/network_configuration_updater_impl_cros.cc:191:
> imported_server_and_ca_certs_list));
> This feels awkward. Do we really need to abstract this, or can we just pass a
> imported_server_and_ca_certs_list through and have LocalTranslator call
> GetPEMEncodedCertFromFingerprint directly?

[pneubeck (no reviews), 2013-06-24 08:12:30]

https://codereview.chromium.org/16946002/diff/79001/chrome/browser/chromeos/c...
File chrome/browser/chromeos/cros/native_network_parser.cc (right):

https://codereview.chromium.org/16946002/diff/79001/chrome/browser/chromeos/c...
chrome/browser/chromeos/cros/native_network_parser.cc:1248:
onc::DecodePEMCertificate(eap_server_ca_cert_pem);
On 2013/06/21 22:47:13, stevenjb (chromium) wrote:
> How expensive is this? It would be better to defer any parsing, etc,
especially
> since we're in the process of replacing this code; I'd really prefer not to
make
> NetworkState dependent on net::X509Certificate.

It is less expensive than the certificate import (happens during policy
application). The PEM decoding and fingerprint calculation shouldn't be
expensive. However, the certificate talks to NSS (e.g. CERT_NewTempCertificate)
and might be a bit slow (not sure).

What alternative would you propose? Storing the PEM also doesn't look ideal by
wasting more memory.

Btw.
I think this issue indicates that it's in general a bad idea to store
configuration in Shill and then read it back.

https://codereview.chromium.org/16946002/diff/79001/chrome/browser/chromeos/c...
File chrome/browser/chromeos/cros/network_library.h (right):

https://codereview.chromium.org/16946002/diff/79001/chrome/browser/chromeos/c...
chrome/browser/chromeos/cros/network_library.h:1676: fingerprint_to_pem) = 0;
On 2013/06/21 22:47:13, stevenjb (chromium) wrote:
> Typedef the callback and document how it is used.

Done.

https://codereview.chromium.org/16946002/diff/79001/chrome/browser/chromeos/p...
File chrome/browser/chromeos/policy/network_configuration_updater_impl_cros.cc
(right):

https://codereview.chromium.org/16946002/diff/79001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/network_configuration_updater_impl_cros.cc:191:
imported_server_and_ca_certs_list));
On 2013/06/21 22:47:13, stevenjb (chromium) wrote:
> This feels awkward. Do we really need to abstract this, or can we just pass a
> imported_server_and_ca_certs_list through and have LocalTranslator call
> GetPEMEncodedCertFromFingerprint directly?

This way it's easier to test the Translator and LoadOncNetworks. The
dependencies are also simpler.
Note, that X509Certificate is not an interface and does not provide a way for
Mocks/Stubs, which could have fake fingerprints.

[Mattias Nissler (ping if slow), 2013-06-24 12:45:09]

Still looks good to me in general, a couple nits inline.

One more thought: Fingerprints are probably nice for the ONC (but not strictly
necessary given we have GUIDs already), but why not use PEMs throughout shill
and Chrome? I can see that shill needs the PEM so it doesn't have to mess with
NSS, so it can't use fingerprints. Maybe it makes sense to use PEMs only on the
Chrome side as well for consistency? Just my random $0.02 here, feel free to
ignore if it doesn't make sense.

https://codereview.chromium.org/16946002/diff/87005/chrome/browser/chromeos/c...
File chrome/browser/chromeos/cros/network_library.h (right):

https://codereview.chromium.org/16946002/diff/87005/chrome/browser/chromeos/c...
chrome/browser/chromeos/cros/network_library.h:1673: // empty string, if the
reference cannot be resolved.
nit: remove comma

https://codereview.chromium.org/16946002/diff/87005/chromeos/network/onc/onc_...
File chromeos/network/onc/onc_certificate_importer.cc (right):

https://codereview.chromium.org/16946002/diff/87005/chromeos/network/onc/onc_...
chromeos/network/onc/onc_certificate_importer.cc:220: if
(!cert_database->SetCertTrust(
Not sure we should be doing this. What difference does it make btw.?

https://codereview.chromium.org/16946002/diff/87005/chromeos/network/onc/onc_...
File chromeos/network/onc/onc_translator_unittest.cc (right):

https://codereview.chromium.org/16946002/diff/87005/chromeos/network/onc/onc_...
chromeos/network/onc/onc_translator_unittest.cc:19: 
remove :)

https://codereview.chromium.org/16946002/diff/87005/chromeos/network/onc/onc_...
File chromeos/network/onc/onc_utils.cc (right):

https://codereview.chromium.org/16946002/diff/87005/chromeos/network/onc/onc_...
chromeos/network/onc/onc_utils.cc:366: LOG(ERROR) << "Fingerprint not unique in
list.";
Rather, cert present twice in list. I don't think returning error here is
reasonable, I'd just return the cert pointer for the first match.

https://codereview.chromium.org/16946002/diff/87005/chromeos/network/onc/onc_...
File chromeos/network/onc/onc_utils.h (right):

https://codereview.chromium.org/16946002/diff/87005/chromeos/network/onc/onc_...
chromeos/network/onc/onc_utils.h:120: // (see GetHexFingerprintOfCert). Return
true, if all references could be
nit: *Returns, no comma

https://codereview.chromium.org/16946002/diff/87005/chromeos/network/onc/onc_...
chromeos/network/onc/onc_utils.h:121: // resolved. Otherwise return false and
network configurations with
nit: *returns

[pneubeck (no reviews), 2013-06-24 15:35:41]

https://codereview.chromium.org/16946002/diff/87005/chrome/browser/chromeos/c...
File chrome/browser/chromeos/cros/network_library.h (right):

https://codereview.chromium.org/16946002/diff/87005/chrome/browser/chromeos/c...
chrome/browser/chromeos/cros/network_library.h:1673: // empty string, if the
reference cannot be resolved.
On 2013/06/24 12:45:09, Mattias Nissler wrote:
> nit: remove comma

Done.

https://codereview.chromium.org/16946002/diff/87005/chromeos/network/onc/onc_...
File chromeos/network/onc/onc_certificate_importer.cc (right):

https://codereview.chromium.org/16946002/diff/87005/chromeos/network/onc/onc_...
chromeos/network/onc/onc_certificate_importer.cc:220: if
(!cert_database->SetCertTrust(
On 2013/06/24 12:45:09, Mattias Nissler wrote:
> Not sure we should be doing this. What difference does it make btw.?

Good point.
This is required only to modify trust by user ONC import (through
about:net-internals).
We could remove this if that feature wouldn't be required (no idea if it is).
Instead, I ensure now that we don't try to call this function during policy
import (allow_trust_imports_ = false).

https://codereview.chromium.org/16946002/diff/87005/chromeos/network/onc/onc_...
File chromeos/network/onc/onc_translator_unittest.cc (right):

https://codereview.chromium.org/16946002/diff/87005/chromeos/network/onc/onc_...
chromeos/network/onc/onc_translator_unittest.cc:19: 
On 2013/06/24 12:45:09, Mattias Nissler wrote:
> remove :)

Done.

https://codereview.chromium.org/16946002/diff/87005/chromeos/network/onc/onc_...
File chromeos/network/onc/onc_utils.cc (right):

https://codereview.chromium.org/16946002/diff/87005/chromeos/network/onc/onc_...
chromeos/network/onc/onc_utils.cc:366: LOG(ERROR) << "Fingerprint not unique in
list.";
On 2013/06/24 12:45:09, Mattias Nissler wrote:
> Rather, cert present twice in list. I don't think returning error here is
> reasonable, I'd just return the cert pointer for the first match.

Done.

https://codereview.chromium.org/16946002/diff/87005/chromeos/network/onc/onc_...
File chromeos/network/onc/onc_utils.h (right):

https://codereview.chromium.org/16946002/diff/87005/chromeos/network/onc/onc_...
chromeos/network/onc/onc_utils.h:120: // (see GetHexFingerprintOfCert). Return
true, if all references could be
On 2013/06/24 12:45:09, Mattias Nissler wrote:
> nit: *Returns, no comma

Done.

https://codereview.chromium.org/16946002/diff/87005/chromeos/network/onc/onc_...
chromeos/network/onc/onc_utils.h:121: // resolved. Otherwise return false and
network configurations with
On 2013/06/24 12:45:09, Mattias Nissler wrote:
> nit: *returns

Done.

[pneubeck (no reviews), 2013-06-24 16:05:33]

On 2013/06/24 12:45:09, Mattias Nissler wrote:
> Still looks good to me in general, a couple nits inline.
> 
> One more thought: Fingerprints are probably nice for the ONC (but not strictly
> necessary given we have GUIDs already), but why not use PEMs throughout shill
> and Chrome? I can see that shill needs the PEM so it doesn't have to mess with
> NSS, so it can't use fingerprints. Maybe it makes sense to use PEMs only on
the
> Chrome side as well for consistency? Just my random $0.02 here, feel free to
> ignore if it doesn't make sense.

Parsing of PEM from Shill also only happens as long as we use NetworkLibrary,
because the new configuration handler stores the ONC (with fingerprints)
directly in UIData.

Summary from IM discussion:

PEM would work and reduce the number of representations and conversions, but at
the expense of increased memory usage.
Memory usage is a bit unpredictable as ONC is used in several places: Caching of
policy, storage of user settings and at some time it will be used in JS UI.

The right thing would be to store a X509Certificate*, but base::Value doesn't
support custom serializable values.

[stevenjb, 2013-06-24 20:55:47]

On 2013/06/24 16:05:33, pneubeck wrote:
> On 2013/06/24 12:45:09, Mattias Nissler wrote:
> > Still looks good to me in general, a couple nits inline.
> > 
> > One more thought: Fingerprints are probably nice for the ONC (but not
strictly
> > necessary given we have GUIDs already), but why not use PEMs throughout
shill
> > and Chrome? I can see that shill needs the PEM so it doesn't have to mess
with
> > NSS, so it can't use fingerprints. Maybe it makes sense to use PEMs only on
> the
> > Chrome side as well for consistency? Just my random $0.02 here, feel free to
> > ignore if it doesn't make sense.
> 
> Parsing of PEM from Shill also only happens as long as we use NetworkLibrary,
> because the new configuration handler stores the ONC (with fingerprints)
> directly in UIData.
> 
> Summary from IM discussion:
> 
> PEM would work and reduce the number of representations and conversions, but
at
> the expense of increased memory usage.
> Memory usage is a bit unpredictable as ONC is used in several places: Caching
of
> policy, storage of user settings and at some time it will be used in JS UI.
> 
> The right thing would be to store a X509Certificate*, but base::Value doesn't
> support custom serializable values.

I would much rather store the PEM and use a bit of extra memory to avoid adding
dependencies on net::X509Certificate (which, as you mention, is not especially
well abstracted and can not easily be stored or passed around), and to avoid
calls to DecodePEMCertificate during parsing (which makes NSS calls that, as you
also mention, could block, we don't have much transparency into that library).

In the grand scheme of things, we are not talking about very much memory. When
we complete the migration to the NetworkHandler code, we can make sure we do not
hold on to that data longer than we need to, at which point the amount of memory
becomes irrelevant. I would really rather see the decoding separated out from
parsing the Shill data.

[pneubeck (no reviews), 2013-06-27 16:12:06]

Removed the dependency on X509Certificate for parsing Shill properties.

Still missing: more tests, support for a list of CA certs (maybe better in a
separate CL).

[pneubeck (no reviews), 2013-06-27 16:13:29]

On 2013/06/27 16:12:06, pneubeck wrote:
> Removed the dependency on X509Certificate for parsing Shill properties.
> 
> Still missing: more tests, support for a list of CA certs (maybe better in a
> separate CL).

@Steven, PTAL

[stevenjb, 2013-06-27 16:50:03]

Greg, if you can take a look at this also that would be great, I'm really
fumbling around in the dark here.

This looks better, just a few nits really at this point.

https://codereview.chromium.org/16946002/diff/108054/chrome/browser/chromeos/...
File chrome/browser/chromeos/cros/native_network_parser.cc (left):

https://codereview.chromium.org/16946002/diff/108054/chrome/browser/chromeos/...
chrome/browser/chromeos/cros/native_network_parser.cc:1262: case
PROPERTY_INDEX_EAP_CA_CERT: {
Will removing this break any existing functionality?

https://codereview.chromium.org/16946002/diff/108054/chrome/browser/chromeos/...
File chrome/browser/chromeos/cros/network_library.cc (right):

https://codereview.chromium.org/16946002/diff/108054/chrome/browser/chromeos/...
chrome/browser/chromeos/cros/network_library.cc:721: if
(pem_encoded_cert.empty())
warning or error?

https://codereview.chromium.org/16946002/diff/108054/chrome/browser/chromeos/...
chrome/browser/chromeos/cros/network_library.cc:1204: if
(pem_encoded_cert.empty())
warning or error?

https://codereview.chromium.org/16946002/diff/108054/chrome/browser/chromeos/...
File chrome/browser/chromeos/cros/network_library_impl_base.h (right):

https://codereview.chromium.org/16946002/diff/108054/chrome/browser/chromeos/...
chrome/browser/chromeos/cros/network_library_impl_base.h:230:
fingerprint_to_pem) OVERRIDE;
Use typedef

[stevenjb, 2013-06-27 17:23:31]

https://codereview.chromium.org/16946002/diff/79001/chrome/browser/chromeos/p...
File chrome/browser/chromeos/policy/network_configuration_updater_impl_cros.cc
(right):

https://codereview.chromium.org/16946002/diff/79001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/network_configuration_updater_impl_cros.cc:191:
imported_server_and_ca_certs_list));
On 2013/06/24 08:12:31, pneubeck wrote:
> On 2013/06/21 22:47:13, stevenjb (chromium) wrote:
> > This feels awkward. Do we really need to abstract this, or can we just pass
a
> > imported_server_and_ca_certs_list through and have LocalTranslator call
> > GetPEMEncodedCertFromFingerprint directly?
> 
> This way it's easier to test the Translator and LoadOncNetworks. The
> dependencies are also simpler.
> Note, that X509Certificate is not an interface and does not provide a way for
> Mocks/Stubs, which could have fake fingerprints.

I looked at this more closely, and I still don't like it much. 

* We call LoadOncNetworks() from two non-test places; both bind this to
GetPEMEncodedCertFromFingerprint().
* NetworkLibraryStubTest calls LoadOncNetworks() with a test callback, but it
uses the Stub implementation, which could override LoadOncNetworks() (or add a
private GetPEMEncodedCertFromFingerprint() method to NetworkLibrary that has the
different implementations in the stub vs. cros impl).

onc::GetPEMEncodedCertFromFingerprint seems like an implementation detail that
the caller shouldn't need to know about. I would much rather see that detail
provided by the cros / stub implementation.

I guess I don't care too much because I am hoping to move LoadOncNetworks() out
of NetworkLibrary very soon, but generally speaking I do not think the added
code complexity is justified for the sake of a unit test. Stub implementations
or test setters seem like better options.

[Greg Spencer (Chromium), 2013-06-27 20:15:47]

So, I think I'm in agreement with Mattias about using PEM everywhere, and in
agreement with Steven that the translation between the two seems like an
implementation detail.

They're not that much larger than the fingerprints (maybe 10x), and since the
fingerprint is a hash of the PEM, it's a 1:1 mapping, and would make this code a
lot simpler (you wouldn't need to pass the conversion callback around, for
instance).

https://codereview.chromium.org/16946002/diff/108054/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/network_configuration_updater_impl.cc
(right):

https://codereview.chromium.org/16946002/diff/108054/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/network_configuration_updater_impl.cc:110: << "
could not be resolved.";
Shouldn't this also somehow bubble up to the import process so that it fails? 
Seems like a pretty bad failure, and it should probably prevent the ONC from
being imported (as being malformed somehow).

https://codereview.chromium.org/16946002/diff/108054/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/network_configuration_updater_impl_cros.cc
(right):

https://codereview.chromium.org/16946002/diff/108054/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/network_configuration_updater_impl_cros.cc:178:
<< " could not be resolved.";
Same comment here re: failure.

https://codereview.chromium.org/16946002/diff/108054/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/net_internals/net_internals_ui.cc (right):

https://codereview.chromium.org/16946002/diff/108054/chrome/browser/ui/webui/...
chrome/browser/ui/webui/net_internals/net_internals_ui.cc:1571: error = "Errors
occurred during the ONC parsing.";
nit: Add a space after the period so that if more errors occur, it'll look
nicer.

https://codereview.chromium.org/16946002/diff/108054/chrome/browser/ui/webui/...
chrome/browser/ui/webui/net_internals/net_internals_ui.cc:1579: error += "Some
certificates couldn't be imported.";
nit: Same here: add a space after the period so that if more errors occur, it'll
look nicer.

https://codereview.chromium.org/16946002/diff/108054/chromeos/network/certifi...
File chromeos/network/certificate_handler.h (right):

https://codereview.chromium.org/16946002/diff/108054/chromeos/network/certifi...
chromeos/network/certificate_handler.h:31: // certificates were imported
successfully.
Add a description of |imported_server_and_ca_certs| here.

[pneubeck (no reviews), 2013-06-28 09:06:25]

For the sake of completeness: one of the certs used by google is 1057 bytes
large, and it's fingerprint 20 bytes.

On 2013/06/27 20:15:47, Greg Spencer (Chromium) wrote:
> So, I think I'm in agreement with Mattias about using PEM everywhere, and in
> agreement with Steven that the translation between the two seems like an
> implementation detail.
> 
> They're not that much larger than the fingerprints (maybe 10x), and since the
> fingerprint is a hash of the PEM, it's a 1:1 mapping, and would make this code
a
> lot simpler (you wouldn't need to pass the conversion callback around, for
> instance).
> 
>
https://codereview.chromium.org/16946002/diff/108054/chrome/browser/chromeos/...
> File chrome/browser/chromeos/policy/network_configuration_updater_impl.cc
> (right):
> 
>
https://codereview.chromium.org/16946002/diff/108054/chrome/browser/chromeos/...
> chrome/browser/chromeos/policy/network_configuration_updater_impl.cc:110: << "
> could not be resolved.";
> Shouldn't this also somehow bubble up to the import process so that it fails? 
> Seems like a pretty bad failure, and it should probably prevent the ONC from
> being imported (as being malformed somehow).
> 
>
https://codereview.chromium.org/16946002/diff/108054/chrome/browser/chromeos/...
> File chrome/browser/chromeos/policy/network_configuration_updater_impl_cros.cc
> (right):
> 
>
https://codereview.chromium.org/16946002/diff/108054/chrome/browser/chromeos/...
> chrome/browser/chromeos/policy/network_configuration_updater_impl_cros.cc:178:
> << " could not be resolved.";
> Same comment here re: failure.
> 
>
https://codereview.chromium.org/16946002/diff/108054/chrome/browser/ui/webui/...
> File chrome/browser/ui/webui/net_internals/net_internals_ui.cc (right):
> 
>
https://codereview.chromium.org/16946002/diff/108054/chrome/browser/ui/webui/...
> chrome/browser/ui/webui/net_internals/net_internals_ui.cc:1571: error =
"Errors
> occurred during the ONC parsing.";
> nit: Add a space after the period so that if more errors occur, it'll look
> nicer.
> 
>
https://codereview.chromium.org/16946002/diff/108054/chrome/browser/ui/webui/...
> chrome/browser/ui/webui/net_internals/net_internals_ui.cc:1579: error += "Some
> certificates couldn't be imported.";
> nit: Same here: add a space after the period so that if more errors occur,
it'll
> look nicer.
> 
>
https://codereview.chromium.org/16946002/diff/108054/chromeos/network/certifi...
> File chromeos/network/certificate_handler.h (right):
> 
>
https://codereview.chromium.org/16946002/diff/108054/chromeos/network/certifi...
> chromeos/network/certificate_handler.h:31: // certificates were imported
> successfully.
> Add a description of |imported_server_and_ca_certs| here.

[pneubeck (no reviews), 2013-06-28 09:41:03]

https://codereview.chromium.org/16946002/diff/108054/chrome/browser/chromeos/...
File chrome/browser/chromeos/cros/network_library_impl_base.h (right):

https://codereview.chromium.org/16946002/diff/108054/chrome/browser/chromeos/...
chrome/browser/chromeos/cros/network_library_impl_base.h:230:
fingerprint_to_pem) OVERRIDE;
On 2013/06/27 16:50:04, stevenjb (chromium) wrote:
> Use typedef

Done.

https://codereview.chromium.org/16946002/diff/108054/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/network_configuration_updater_impl.cc
(right):

https://codereview.chromium.org/16946002/diff/108054/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/network_configuration_updater_impl.cc:110: << "
could not be resolved.";
On 2013/06/27 20:15:47, Greg Spencer (Chromium) wrote:
> Shouldn't this also somehow bubble up to the import process so that it fails? 
> Seems like a pretty bad failure, and it should probably prevent the ONC from
> being imported (as being malformed somehow).

I can/wanted to add such a check to the validator in another CL. But at this
place we can't do more than logging an error.

https://codereview.chromium.org/16946002/diff/108054/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/network_configuration_updater_impl_cros.cc
(right):

https://codereview.chromium.org/16946002/diff/108054/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/network_configuration_updater_impl_cros.cc:178:
<< " could not be resolved.";
On 2013/06/27 20:15:47, Greg Spencer (Chromium) wrote:
> Same comment here re: failure.

Ditto.

https://codereview.chromium.org/16946002/diff/108054/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/net_internals/net_internals_ui.cc (right):

https://codereview.chromium.org/16946002/diff/108054/chrome/browser/ui/webui/...
chrome/browser/ui/webui/net_internals/net_internals_ui.cc:1571: error = "Errors
occurred during the ONC parsing.";
On 2013/06/27 20:15:47, Greg Spencer (Chromium) wrote:
> nit: Add a space after the period so that if more errors occur, it'll look
> nicer.

Done.

https://codereview.chromium.org/16946002/diff/108054/chrome/browser/ui/webui/...
chrome/browser/ui/webui/net_internals/net_internals_ui.cc:1579: error += "Some
certificates couldn't be imported.";
On 2013/06/27 20:15:47, Greg Spencer (Chromium) wrote:
> nit: Same here: add a space after the period so that if more errors occur,
it'll
> look nicer.

Done.

https://codereview.chromium.org/16946002/diff/108054/chromeos/network/certifi...
File chromeos/network/certificate_handler.h (right):

https://codereview.chromium.org/16946002/diff/108054/chromeos/network/certifi...
chromeos/network/certificate_handler.h:31: // certificates were imported
successfully.
On 2013/06/27 20:15:47, Greg Spencer (Chromium) wrote:
> Add a description of |imported_server_and_ca_certs| here.

Done.

[pneubeck (no reviews), 2013-06-28 17:40:06]

I replaced all occurrences of fingerprint by PEM. I'm only concerned that after
a few months, we find out that fingerprints might be required.

PTAL

https://codereview.chromium.org/16946002/diff/108054/chrome/browser/chromeos/...
File chrome/browser/chromeos/cros/network_library.cc (right):

https://codereview.chromium.org/16946002/diff/108054/chrome/browser/chromeos/...
chrome/browser/chromeos/cros/network_library.cc:721: if
(pem_encoded_cert.empty())
On 2013/06/27 16:50:04, stevenjb (chromium) wrote:
> warning or error?

Done.

https://codereview.chromium.org/16946002/diff/108054/chrome/browser/chromeos/...
chrome/browser/chromeos/cros/network_library.cc:1204: if
(pem_encoded_cert.empty())
On 2013/06/27 16:50:04, stevenjb (chromium) wrote:
> warning or error?

Done.

[stevenjb, 2013-06-28 18:00:27]

LGTM

This seems like a much more straightforward transition to me, thanks for doing
the re-factoring. Once the NetworkHandler transition is complete we can evaluate
whether using fingerprints is necessary / useful and it should be much simpler
to make that change then. In the meanwhile storing an extra 1-2K of data per
certificate doesn't seem like a big deal in the grand scheme of things.

https://codereview.chromium.org/16946002/diff/140007/chromeos/network/onc/onc...
File chromeos/network/onc/onc_utils.cc (right):

https://codereview.chromium.org/16946002/diff/140007/chromeos/network/onc/onc...
chromeos/network/onc/onc_utils.cc:395: scoped_refptr<net::X509Certificate> >&
certs_by_guid,
Use typedef here and below

https://codereview.chromium.org/16946002/diff/140007/chromeos/network/onc/onc...
File chromeos/network/onc/onc_utils.h (right):

https://codereview.chromium.org/16946002/diff/140007/chromeos/network/onc/onc...
chromeos/network/onc/onc_utils.h:108: scoped_refptr<net::X509Certificate> >&
certs_by_guid,
These should use a typedef

[Greg Spencer (Chromium), 2013-06-28 18:25:49]

LGTM.  This is a lot more straightforward without the fingerprints.

https://codereview.chromium.org/16946002/diff/140007/chromeos/network/onc/onc...
File chromeos/network/onc/onc_translator_onc_to_shill.cc (right):

https://codereview.chromium.org/16946002/diff/140007/chromeos/network/onc/onc...
chromeos/network/onc/onc_translator_onc_to_shill.cc:29:
scoped_ptr<base::ListValue> SingletonStringList(const std::string& str) {
Singleton seems misleading here: people will assume you are referring to the
design pattern...

Maybe "SingleStringList"?

[pneubeck (no reviews), 2013-07-01 15:12:13]

Addressed all comments and also did more manual testing. I will add a few more
tests for the resolve function (tomorrow).

@Mattias, PTAL

@eroman, please owner-review net_internals_ui.*

https://codereview.chromium.org/16946002/diff/140007/chromeos/network/onc/onc...
File chromeos/network/onc/onc_translator_onc_to_shill.cc (right):

https://codereview.chromium.org/16946002/diff/140007/chromeos/network/onc/onc...
chromeos/network/onc/onc_translator_onc_to_shill.cc:29:
scoped_ptr<base::ListValue> SingletonStringList(const std::string& str) {
On 2013/06/28 18:25:50, Greg Spencer (Chromium) wrote:
> Singleton seems misleading here: people will assume you are referring to the
> design pattern...
> 
> Maybe "SingleStringList"?

Done.

https://codereview.chromium.org/16946002/diff/140007/chromeos/network/onc/onc...
File chromeos/network/onc/onc_utils.cc (right):

https://codereview.chromium.org/16946002/diff/140007/chromeos/network/onc/onc...
chromeos/network/onc/onc_utils.cc:395: scoped_refptr<net::X509Certificate> >&
certs_by_guid,
On 2013/06/28 18:00:27, stevenjb (chromium) wrote:
> Use typedef here and below

Done.

https://codereview.chromium.org/16946002/diff/140007/chromeos/network/onc/onc...
File chromeos/network/onc/onc_utils.h (right):

https://codereview.chromium.org/16946002/diff/140007/chromeos/network/onc/onc...
chromeos/network/onc/onc_utils.h:108: scoped_refptr<net::X509Certificate> >&
certs_by_guid,
On 2013/06/28 18:00:27, stevenjb (chromium) wrote:
> These should use a typedef

Done.

[pneubeck (no reviews), 2013-07-01 15:12:43]

@eroman, please owner-review net_internals_ui.*
Thanks!

[Mattias Nissler (ping if slow), 2013-07-02 13:00:35]

https://codereview.chromium.org/16946002/diff/193002/chrome/browser/chromeos/...
File chrome/browser/chromeos/cros/cert_library.cc (right):

https://codereview.chromium.org/16946002/diff/193002/chrome/browser/chromeos/...
chrome/browser/chromeos/cros/cert_library.cc:186: if (CertToPEM(*cert) !=
pem_encoded)
Is this fast enough for the UI thread? For large numbers of certificates, this
might be a problem. I guess this is the downside of not using fingerprints :)

https://codereview.chromium.org/16946002/diff/193002/chrome/browser/chromeos/...
File chrome/browser/chromeos/cros/network_library.h (right):

https://codereview.chromium.org/16946002/diff/193002/chrome/browser/chromeos/...
chrome/browser/chromeos/cros/network_library.h:662: return ca_cert_pem_;
nit: no need for line break.

https://codereview.chromium.org/16946002/diff/193002/chromeos/network/onc/onc...
File chromeos/network/onc/onc_constants.cc (right):

https://codereview.chromium.org/16946002/diff/193002/chromeos/network/onc/onc...
chromeos/network/onc/onc_constants.cc:240: namespace ipsec {
You should really try and break these unrelated changes out into separate CLs.

https://codereview.chromium.org/16946002/diff/193002/chromeos/test/data/netwo...
File chromeos/test/data/network/managed_toplevel1_with_cert_pems.onc (right):

https://codereview.chromium.org/16946002/diff/193002/chromeos/test/data/netwo...
chromeos/test/data/network/managed_toplevel1_with_cert_pems.onc:21:
"IssuerCAPEMs": [ "pem1" ],
so we're now inlining the PEMs? I don't think that's a good idea on the ONC
format side of things.

[pneubeck (no reviews), 2013-07-02 14:38:42]

Addressed/answered Mattias' comments.

https://codereview.chromium.org/16946002/diff/193002/chrome/browser/chromeos/...
File chrome/browser/chromeos/cros/cert_library.cc (right):

https://codereview.chromium.org/16946002/diff/193002/chrome/browser/chromeos/...
chrome/browser/chromeos/cros/cert_library.cc:186: if (CertToPEM(*cert) !=
pem_encoded)
On 2013/07/02 13:00:35, Mattias Nissler wrote:
> Is this fast enough for the UI thread? For large numbers of certificates, this
> might be a problem. I guess this is the downside of not using fingerprints :)

We _could_ calculate the fingerprint of pem_encoded here and compare it with
cert->fingerprint (which is precalculated). This could be done locally in this
function and without dependency on NSS (i.e. without creating a temporary cert
and without using NSS to calculate the SHA1).

Not sure that it's worth the effort.
However, we should better avoid using PEMs in the upcoming JS UI.

https://codereview.chromium.org/16946002/diff/193002/chrome/browser/chromeos/...
File chrome/browser/chromeos/cros/network_library.h (right):

https://codereview.chromium.org/16946002/diff/193002/chrome/browser/chromeos/...
chrome/browser/chromeos/cros/network_library.h:662: return ca_cert_pem_;
On 2013/07/02 13:00:35, Mattias Nissler wrote:
> nit: no need for line break.

Done.

https://codereview.chromium.org/16946002/diff/193002/chromeos/network/onc/onc...
File chromeos/network/onc/onc_constants.cc (right):

https://codereview.chromium.org/16946002/diff/193002/chromeos/network/onc/onc...
chromeos/network/onc/onc_constants.cc:240: namespace ipsec {
On 2013/07/02 13:00:35, Mattias Nissler wrote:
> You should really try and break these unrelated changes out into separate CLs.

Done.

https://codereview.chromium.org/16946002/diff/193002/chromeos/test/data/netwo...
File chromeos/test/data/network/managed_toplevel1_with_cert_pems.onc (right):

https://codereview.chromium.org/16946002/diff/193002/chromeos/test/data/netwo...
chromeos/test/data/network/managed_toplevel1_with_cert_pems.onc:21:
"IssuerCAPEMs": [ "pem1" ],
On 2013/07/02 13:00:35, Mattias Nissler wrote:
> so we're now inlining the PEMs? I don't think that's a good idea on the ONC
> format side of things.

This file is used for testing NetworkLibrary::LoadOncNetworks, which expects an
ONC in which GUID references are replaced by PEMs.

We continue to support GUID references from the policy sever.
The PEMs are meant to be used internally in Chrome. However, a side-effect of
this CL is that we also support PEMs in the policy ONC.

[Mattias Nissler (ping if slow), 2013-07-02 15:34:06]

LGTM from my side FWIW

https://codereview.chromium.org/16946002/diff/193002/chromeos/test/data/netwo...
File chromeos/test/data/network/managed_toplevel1_with_cert_pems.onc (right):

https://codereview.chromium.org/16946002/diff/193002/chromeos/test/data/netwo...
chromeos/test/data/network/managed_toplevel1_with_cert_pems.onc:21:
"IssuerCAPEMs": [ "pem1" ],
On 2013/07/02 14:38:43, pneubeck wrote:
> On 2013/07/02 13:00:35, Mattias Nissler wrote:
> > so we're now inlining the PEMs? I don't think that's a good idea on the ONC
> > format side of things.
> 
> This file is used for testing NetworkLibrary::LoadOncNetworks, which expects
an
> ONC in which GUID references are replaced by PEMs.
> 
> We continue to support GUID references from the policy sever.
> The PEMs are meant to be used internally in Chrome. However, a side-effect of
> this CL is that we also support PEMs in the policy ONC.

OK, all good if the existing GUID-based ONC stuff continues working. I suspect
we don't even want to change the spec to avoid people getting the impression
inline PEMs are the new hotness.

[stevenjb, 2013-07-02 16:16:25]

https://codereview.chromium.org/16946002/diff/193002/chrome/browser/chromeos/...
File chrome/browser/chromeos/cros/cert_library.cc (right):

https://codereview.chromium.org/16946002/diff/193002/chrome/browser/chromeos/...
chrome/browser/chromeos/cros/cert_library.cc:186: if (CertToPEM(*cert) !=
pem_encoded)
On 2013/07/02 14:38:43, pneubeck wrote:
> On 2013/07/02 13:00:35, Mattias Nissler wrote:
> > Is this fast enough for the UI thread? For large numbers of certificates,
this
> > might be a problem. I guess this is the downside of not using fingerprints
:)
> 
> We _could_ calculate the fingerprint of pem_encoded here and compare it with
> cert->fingerprint (which is precalculated). This could be done locally in this
> function and without dependency on NSS (i.e. without creating a temporary cert
> and without using NSS to calculate the SHA1).
> 
> Not sure that it's worth the effort.
> However, we should better avoid using PEMs in the upcoming JS UI.

As far as I recall, this function only gets called when we display a network
configuration dialog, at which point a minor block is reasonable, but we should
comment that in the header, and add a TODO to make this asynchronous or remove
it.

[pneubeck (no reviews), 2013-07-03 13:45:55]

@Mattias, can you please take a final look at the added test?
Thanks.

After that I would commit this.

https://codereview.chromium.org/16946002/diff/193002/chrome/browser/chromeos/...
File chrome/browser/chromeos/cros/cert_library.cc (right):

https://codereview.chromium.org/16946002/diff/193002/chrome/browser/chromeos/...
chrome/browser/chromeos/cros/cert_library.cc:186: if (CertToPEM(*cert) !=
pem_encoded)
On 2013/07/02 16:16:26, stevenjb (chromium) wrote:
> On 2013/07/02 14:38:43, pneubeck wrote:
> > On 2013/07/02 13:00:35, Mattias Nissler wrote:
> > > Is this fast enough for the UI thread? For large numbers of certificates,
> this
> > > might be a problem. I guess this is the downside of not using fingerprints
> :)
> > 
> > We _could_ calculate the fingerprint of pem_encoded here and compare it with
> > cert->fingerprint (which is precalculated). This could be done locally in
this
> > function and without dependency on NSS (i.e. without creating a temporary
cert
> > and without using NSS to calculate the SHA1).
> > 
> > Not sure that it's worth the effort.
> > However, we should better avoid using PEMs in the upcoming JS UI.
> 
> As far as I recall, this function only gets called when we display a network
> configuration dialog, at which point a minor block is reasonable, but we
should
> comment that in the header, and add a TODO to make this asynchronous or remove
> it.

Done.

[Mattias Nissler (ping if slow), 2013-07-03 15:13:57]

LGTM

[commit-bot: I haz the power, 2013-07-03 15:18:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pneubeck@chromium.org/16946002/197066

[commit-bot: I haz the power, 2013-07-03 15:30:48]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[commit-bot: I haz the power, 2013-07-03 15:31:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pneubeck@chromium.org/16946002/197066

[commit-bot: I haz the power, 2013-07-03 19:21:03]

Change committed as 210019