Add migration from CaCert NSS nicknames to PEM.

Add migration from CaCert NSS nicknames to PEM.

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

[pneubeck (no reviews), 2013-07-26 22:34:51]

@Steven, except the new files this touches mostly files that you know best.
Please take a look at these.

@Mattias, please take a look at the added network_cert_handler* files.

Thanks!

[pneubeck (no reviews), 2013-07-29 08:41:39]

https://codereview.chromium.org/20087002/diff/43001/chromeos/network/network_...
File chromeos/network/network_cert_handler.cc (right):

https://codereview.chromium.org/20087002/diff/43001/chromeos/network/network_...
chromeos/network/network_cert_handler.cc:24: typedef std::vector<std::string>
Nicknames;
Not used, to be removed.

[Mattias Nissler (ping if slow), 2013-07-29 10:08:15]

https://codereview.chromium.org/20087002/diff/43001/chromeos/dbus/shill_servi...
File chromeos/dbus/shill_service_client_stub.cc (left):

https://codereview.chromium.org/20087002/diff/43001/chromeos/dbus/shill_servi...
chromeos/dbus/shill_service_client_stub.cc:138: weak_ptr_factory_.GetWeakPtr(),
service_path, name));
Why did you remove this?

https://codereview.chromium.org/20087002/diff/43001/chromeos/dbus/shill_servi...
File chromeos/dbus/shill_service_client_stub.cc (right):

https://codereview.chromium.org/20087002/diff/43001/chromeos/dbus/shill_servi...
chromeos/dbus/shill_service_client_stub.cc:366: std::string property_changed;
nit: property_changed suggests a bool, how about changed_property

https://codereview.chromium.org/20087002/diff/43001/chromeos/network/network_...
File chromeos/network/network_cert_handler.cc (right):

https://codereview.chromium.org/20087002/diff/43001/chromeos/network/network_...
chromeos/network/network_cert_handler.cc:80: void GetPropertiesCallback(const
std::string& service_path,
This does too damn much given the unsuspecting name "GetPropertiesCallback".
Improve the naming or break up the function body.

https://codereview.chromium.org/20087002/diff/43001/chromeos/network/network_...
chromeos/network/network_cert_handler.cc:85: return;  // The CertHandler is
destroyed, stop.
Comment is redundant here, remove.

https://codereview.chromium.org/20087002/diff/43001/chromeos/network/network_...
chromeos/network/network_cert_handler.cc:88: if (call_status ==
DBUS_METHOD_CALL_FAILURE)
Should this have a VLOG as well?

https://codereview.chromium.org/20087002/diff/43001/chromeos/network/network_...
chromeos/network/network_cert_handler.cc:91: struct NssPem {
static

https://codereview.chromium.org/20087002/diff/43001/chromeos/network/network_...
chromeos/network/network_cert_handler.cc:97: {NULL,
flimflam::kEapCaCertNssProperty, shill::kEapCaCertPemProperty,
I'm not entirely sure, but I think style guide / consistency mandate a space
after {

https://codereview.chromium.org/20087002/diff/43001/chromeos/network/network_...
chromeos/network/network_cert_handler.cc:136: VLOG(2) << "PEM already exists,
clear NSS property.";
nit: s/clear/clearing/

https://codereview.chromium.org/20087002/diff/43001/chromeos/network/network_...
chromeos/network/network_cert_handler.cc:164: return;  // Didn't find a matching
certificate.
nit: remove redundant comment

https://codereview.chromium.org/20087002/diff/43001/chromeos/network/network_...
chromeos/network/network_cert_handler.cc:171: return;  // PEM encoding failed.
nit: remove redundant comment

https://codereview.chromium.org/20087002/diff/43001/chromeos/network/network_...
File chromeos/network/network_cert_handler.h (right):

https://codereview.chromium.org/20087002/diff/43001/chromeos/network/network_...
chromeos/network/network_cert_handler.h:18: // * Migration from deprecated
CaCertNSS properties to CaCertPEM.
Seems odd to have a single-item list :)

https://codereview.chromium.org/20087002/diff/43001/chromeos/network/network_...
chromeos/network/network_cert_handler.h:19: class CHROMEOS_EXPORT
NetworkCertHandler : public NetworkStateHandlerObserver,
If it's just doing migration, why not name it NetworkCertMigrationHelper or
similar?

https://codereview.chromium.org/20087002/diff/43001/chromeos/network/network_...
chromeos/network/network_cert_handler.h:41:
DISALLOW_COPY_AND_ASSIGN(NetworkCertHandler);
#include "base/basictypes.h"

https://codereview.chromium.org/20087002/diff/43001/chromeos/network/network_...
chromeos/network/network_cert_handler.h:47: 
remove trailing newline

https://codereview.chromium.org/20087002/diff/43001/chromeos/network/network_...
File chromeos/network/network_cert_handler_unittest.cc (right):

https://codereview.chromium.org/20087002/diff/43001/chromeos/network/network_...
chromeos/network/network_cert_handler_unittest.cc:228:
TEST_F(NetworkCertHandlerTest, MigrateNssOfOpenVpn) {
nit: MigrateOpenVpn

in my quest to reduce excessive usage of "of" :)

[pneubeck (no reviews), 2013-07-29 13:15:08]

PTAL

https://codereview.chromium.org/20087002/diff/43001/chromeos/dbus/shill_servi...
File chromeos/dbus/shill_service_client_stub.cc (left):

https://codereview.chromium.org/20087002/diff/43001/chromeos/dbus/shill_servi...
chromeos/dbus/shill_service_client_stub.cc:138: weak_ptr_factory_.GetWeakPtr(),
service_path, name));
On 2013/07/29 10:08:15, Mattias Nissler wrote:
> Why did you remove this?

NotifyObserversPropertyChanged always failed in this case, because the property
didn't exist anymore.
Shill also seems not to notify Chrome if ClearProperty is called.

Separated this change into another CL, as it's not necessary anymore for this
CL.

https://codereview.chromium.org/20087002/diff/43001/chromeos/dbus/shill_servi...
File chromeos/dbus/shill_service_client_stub.cc (right):

https://codereview.chromium.org/20087002/diff/43001/chromeos/dbus/shill_servi...
chromeos/dbus/shill_service_client_stub.cc:366: std::string property_changed;
On 2013/07/29 10:08:15, Mattias Nissler wrote:
> nit: property_changed suggests a bool, how about changed_property

Done.

https://codereview.chromium.org/20087002/diff/43001/chromeos/network/network_...
File chromeos/network/network_cert_handler.cc (right):

https://codereview.chromium.org/20087002/diff/43001/chromeos/network/network_...
chromeos/network/network_cert_handler.cc:24: typedef std::vector<std::string>
Nicknames;
On 2013/07/29 08:41:39, pneubeck wrote:
> Not used, to be removed.

Done.

https://codereview.chromium.org/20087002/diff/43001/chromeos/network/network_...
chromeos/network/network_cert_handler.cc:80: void GetPropertiesCallback(const
std::string& service_path,
On 2013/07/29 10:08:15, Mattias Nissler wrote:
> This does too damn much given the unsuspecting name "GetPropertiesCallback".
> Improve the naming or break up the function body.

Done.

https://codereview.chromium.org/20087002/diff/43001/chromeos/network/network_...
chromeos/network/network_cert_handler.cc:85: return;  // The CertHandler is
destroyed, stop.
On 2013/07/29 10:08:15, Mattias Nissler wrote:
> Comment is redundant here, remove.

Done.

https://codereview.chromium.org/20087002/diff/43001/chromeos/network/network_...
chromeos/network/network_cert_handler.cc:88: if (call_status ==
DBUS_METHOD_CALL_FAILURE)
On 2013/07/29 10:08:15, Mattias Nissler wrote:
> Should this have a VLOG as well?

Good point. Handling this error case like typical in the Network*Handler code.

https://codereview.chromium.org/20087002/diff/43001/chromeos/network/network_...
chromeos/network/network_cert_handler.cc:91: struct NssPem {
On 2013/07/29 10:08:15, Mattias Nissler wrote:
> static

No, doesn't add anything:
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/pF7nAmUQ2uw/abtbE...

https://codereview.chromium.org/20087002/diff/43001/chromeos/network/network_...
chromeos/network/network_cert_handler.cc:97: {NULL,
flimflam::kEapCaCertNssProperty, shill::kEapCaCertPemProperty,
On 2013/07/29 10:08:15, Mattias Nissler wrote:
> I'm not entirely sure, but I think style guide / consistency mandate a space
> after {

optional, so I'd like to keep the automatic formatting of the clang tool.

https://codereview.chromium.org/20087002/diff/43001/chromeos/network/network_...
chromeos/network/network_cert_handler.cc:136: VLOG(2) << "PEM already exists,
clear NSS property.";
On 2013/07/29 10:08:15, Mattias Nissler wrote:
> nit: s/clear/clearing/

Done.

https://codereview.chromium.org/20087002/diff/43001/chromeos/network/network_...
chromeos/network/network_cert_handler.cc:164: return;  // Didn't find a matching
certificate.
On 2013/07/29 10:08:15, Mattias Nissler wrote:
> nit: remove redundant comment

Done.

https://codereview.chromium.org/20087002/diff/43001/chromeos/network/network_...
chromeos/network/network_cert_handler.cc:171: return;  // PEM encoding failed.
On 2013/07/29 10:08:15, Mattias Nissler wrote:
> nit: remove redundant comment

Done.

https://codereview.chromium.org/20087002/diff/43001/chromeos/network/network_...
File chromeos/network/network_cert_handler.h (right):

https://codereview.chromium.org/20087002/diff/43001/chromeos/network/network_...
chromeos/network/network_cert_handler.h:18: // * Migration from deprecated
CaCertNSS properties to CaCertPEM.
On 2013/07/29 10:08:15, Mattias Nissler wrote:
> Seems odd to have a single-item list :)

I add first thought that it would make sense to add more functionality to this
class in upcoming CLs (like the client certificate pattern matching).
But I think that is a bad idea, now. E.g. the two tasks don't have so much in
common and testing will get more difficult.

Done.

https://codereview.chromium.org/20087002/diff/43001/chromeos/network/network_...
chromeos/network/network_cert_handler.h:19: class CHROMEOS_EXPORT
NetworkCertHandler : public NetworkStateHandlerObserver,
On 2013/07/29 10:08:15, Mattias Nissler wrote:
> If it's just doing migration, why not name it NetworkCertMigrationHelper or
> similar?

Done.

https://codereview.chromium.org/20087002/diff/43001/chromeos/network/network_...
chromeos/network/network_cert_handler.h:41:
DISALLOW_COPY_AND_ASSIGN(NetworkCertHandler);
On 2013/07/29 10:08:15, Mattias Nissler wrote:
> #include "base/basictypes.h"

Done.

https://codereview.chromium.org/20087002/diff/43001/chromeos/network/network_...
chromeos/network/network_cert_handler.h:47: 
On 2013/07/29 10:08:15, Mattias Nissler wrote:
> remove trailing newline

Done.

https://codereview.chromium.org/20087002/diff/43001/chromeos/network/network_...
File chromeos/network/network_cert_handler_unittest.cc (right):

https://codereview.chromium.org/20087002/diff/43001/chromeos/network/network_...
chromeos/network/network_cert_handler_unittest.cc:228:
TEST_F(NetworkCertHandlerTest, MigrateNssOfOpenVpn) {
On 2013/07/29 10:08:15, Mattias Nissler wrote:
> nit: MigrateOpenVpn
> 
> in my quest to reduce excessive usage of "of" :)

Done.

[stevenjb, 2013-07-29 17:53:03]

https://codereview.chromium.org/20087002/diff/85001/chromeos/dbus/shill_servi...
File chromeos/dbus/shill_service_client_stub.cc (right):

https://codereview.chromium.org/20087002/diff/85001/chromeos/dbus/shill_servi...
chromeos/dbus/shill_service_client_stub.cc:372: property ==
flimflam::kOpenVPNCaCertNSSProperty) {
Same property twice? I assume one should be kL2tpIpsecCaCertNssProperty. We
should probably at least handler 'kProvider[Host|Type|Name]Property' here also.
Would it be correct to handle all of 'L2TPIPSec.*' and 'OpenVPN.*' this way?

https://codereview.chromium.org/20087002/diff/85001/chromeos/network/network_...
File chromeos/network/network_cert_migrator.cc (right):

https://codereview.chromium.org/20087002/diff/85001/chromeos/network/network_...
chromeos/network/network_cert_migrator.cc:56: const
base::WeakPtr<NetworkCertMigrator>& cert_handler)
Calling a NetworkCertMigrator 'cert_handler' (when we have a CertHandler class)
is confusing.

https://codereview.chromium.org/20087002/diff/85001/chromeos/network/network_...
chromeos/network/network_cert_migrator.cc:57: : certs_(certs),
cert_handler_(cert_handler) {}
one member per line

https://codereview.chromium.org/20087002/diff/85001/chromeos/network/network_...
chromeos/network/network_cert_migrator.cc:65: ++it) {
nit: last 2 or 3 lines can be combined.

https://codereview.chromium.org/20087002/diff/85001/chromeos/network/network_...
chromeos/network/network_cert_migrator.cc:135: UMA_NETWORK_TYPE_EAP},
' ' after { and before }

https://codereview.chromium.org/20087002/diff/85001/chromeos/network/network_...
chromeos/network/network_cert_migrator.cc:139: shill::kOpenVPNCaCertPemProperty,
UMA_NETWORK_TYPE_OPENVPN}, };
last } on separate line

https://codereview.chromium.org/20087002/diff/85001/chromeos/network/network_...
chromeos/network/network_cert_migrator.cc:217: : network_state_handler_(NULL),
weak_ptr_factory_(this) {}
one member per line

https://codereview.chromium.org/20087002/diff/85001/chromeos/network/network_...
chromeos/network/network_cert_migrator.cc:245:
network_state_handler_->GetNetworkList(&networks);
This is going to happen a lot with the way Shill adds and removes visible
networks. I think we only really need to do this for Favorites?

I think it would be better if this were a NetworkProfileObserver to know when a
profile is added, and use the list of FavoriteStates. The boolean could then be
moved from NetworkState to FavoriteState.

https://codereview.chromium.org/20087002/diff/85001/chromeos/network/network_...
File chromeos/network/network_cert_migrator.h (right):

https://codereview.chromium.org/20087002/diff/85001/chromeos/network/network_...
chromeos/network/network_cert_migrator.h:25: void Init(NetworkStateHandler*
network_state_handler);
For consistency, make Init private and NetworkHandler (and any tests) friends.

https://codereview.chromium.org/20087002/diff/85001/chromeos/network/network_...
File chromeos/network/network_cert_migrator_unittest.cc (right):

https://codereview.chromium.org/20087002/diff/85001/chromeos/network/network_...
chromeos/network/network_cert_migrator_unittest.cc:93:
CertLoader::Get()->SetCryptoTaskRunner(message_loop_.message_loop_proxy());
I don't like the inconsistency here. Rather than forcing Initialze() to pass a
task runner, can we add a SetSlowTaskRunnerForTest() method that sets
worker_pool_task_runner_ and use that if not NULL (otherwise set it to 
base::WorkerPool::GetTaskRunner() as before)? That way we don't have to add that
dependency to chrome_browser_main_chromeos.cc.

https://codereview.chromium.org/20087002/diff/85001/chromeos/network/network_...
File chromeos/network/network_state.cc (right):

https://codereview.chromium.org/20087002/diff/85001/chromeos/network/network_...
chromeos/network/network_state.cc:231: return true;
For the connect code I added the complete properties DictionaryValue as a
parameter ti InitialPropertiesReceived(). Rather than storing 3 separate
booleans here, we should parse these properties there and set a single boolean. 

I will upload a CL with that change separately.

[pneubeck (no reviews), 2013-08-05 08:51:22]

PTAL

https://codereview.chromium.org/20087002/diff/85001/chromeos/dbus/shill_servi...
File chromeos/dbus/shill_service_client_stub.cc (right):

https://codereview.chromium.org/20087002/diff/85001/chromeos/dbus/shill_servi...
chromeos/dbus/shill_service_client_stub.cc:372: property ==
flimflam::kOpenVPNCaCertNSSProperty) {
On 2013/07/29 17:53:03, stevenjb (chromium) wrote:
> Same property twice? I assume one should be kL2tpIpsecCaCertNssProperty. We
> should probably at least handler 'kProvider[Host|Type|Name]Property' here
also.
> Would it be correct to handle all of 'L2TPIPSec.*' and 'OpenVPN.*' this way?
> 

Done.

https://codereview.chromium.org/20087002/diff/85001/chromeos/network/network_...
File chromeos/network/network_cert_migrator.cc (right):

https://codereview.chromium.org/20087002/diff/85001/chromeos/network/network_...
chromeos/network/network_cert_migrator.cc:56: const
base::WeakPtr<NetworkCertMigrator>& cert_handler)
On 2013/07/29 17:53:03, stevenjb (chromium) wrote:
> Calling a NetworkCertMigrator 'cert_handler' (when we have a CertHandler
class)
> is confusing.

Done. Missed it during the renaming.

https://codereview.chromium.org/20087002/diff/85001/chromeos/network/network_...
chromeos/network/network_cert_migrator.cc:57: : certs_(certs),
cert_handler_(cert_handler) {}
On 2013/07/29 17:53:03, stevenjb (chromium) wrote:
> one member per line

Done.

https://codereview.chromium.org/20087002/diff/85001/chromeos/network/network_...
chromeos/network/network_cert_migrator.cc:65: ++it) {
On 2013/07/29 17:53:03, stevenjb (chromium) wrote:
> nit: last 2 or 3 lines can be combined.

Done.

https://codereview.chromium.org/20087002/diff/85001/chromeos/network/network_...
chromeos/network/network_cert_migrator.cc:135: UMA_NETWORK_TYPE_EAP},
On 2013/07/29 17:53:03, stevenjb (chromium) wrote:
> ' ' after { and before }

Optional according to the style guide. Done.

https://codereview.chromium.org/20087002/diff/85001/chromeos/network/network_...
chromeos/network/network_cert_migrator.cc:139: shill::kOpenVPNCaCertPemProperty,
UMA_NETWORK_TYPE_OPENVPN}, };
On 2013/07/29 17:53:03, stevenjb (chromium) wrote:
> last } on separate line

Done.

https://codereview.chromium.org/20087002/diff/85001/chromeos/network/network_...
chromeos/network/network_cert_migrator.cc:217: : network_state_handler_(NULL),
weak_ptr_factory_(this) {}
On 2013/07/29 17:53:03, stevenjb (chromium) wrote:
> one member per line

Done.

https://codereview.chromium.org/20087002/diff/85001/chromeos/network/network_...
chromeos/network/network_cert_migrator.cc:245:
network_state_handler_->GetNetworkList(&networks);
On 2013/07/29 17:53:03, stevenjb (chromium) wrote:
> This is going to happen a lot with the way Shill adds and removes visible
> networks. I think we only really need to do this for Favorites?
> 
> I think it would be better if this were a NetworkProfileObserver to know when
a
> profile is added, and use the list of FavoriteStates. The boolean could then
be
> moved from NetworkState to FavoriteState.

As discussed, we can change this to FavoriteState/-List once it's stable enough.
This CL however is rather urgent, because without it, OpenVPN is broken for most
Googlers.

https://codereview.chromium.org/20087002/diff/85001/chromeos/network/network_...
File chromeos/network/network_cert_migrator.h (right):

https://codereview.chromium.org/20087002/diff/85001/chromeos/network/network_...
chromeos/network/network_cert_migrator.h:25: void Init(NetworkStateHandler*
network_state_handler);
On 2013/07/29 17:53:03, stevenjb (chromium) wrote:
> For consistency, make Init private and NetworkHandler (and any tests) friends.

I can do that and should then also make the cstor and dstor private. But AFAIU
this is not the way we usually choose visibility in Chrome/OS.
Visibility is used to separate between implementation details of this class and
the interface that can be used from the outside. It's not used to decide who
actually can access this class; for that we only have the separation into
components.

btw. this class is only exported because its currently required for the
unittests. If we would fix that, I could remove the CHROMEOS_EXPORT.

https://codereview.chromium.org/20087002/diff/85001/chromeos/network/network_...
File chromeos/network/network_cert_migrator_unittest.cc (right):

https://codereview.chromium.org/20087002/diff/85001/chromeos/network/network_...
chromeos/network/network_cert_migrator_unittest.cc:93:
CertLoader::Get()->SetCryptoTaskRunner(message_loop_.message_loop_proxy());
On 2013/07/29 17:53:03, stevenjb (chromium) wrote:
> I don't like the inconsistency here. Rather than forcing Initialze() to pass a
> task runner, can we add a SetSlowTaskRunnerForTest() method that sets
> worker_pool_task_runner_ and use that if not NULL (otherwise set it to 
> base::WorkerPool::GetTaskRunner() as before)? That way we don't have to add
that
> dependency to chrome_browser_main_chromeos.cc.

Done.

https://codereview.chromium.org/20087002/diff/85001/chromeos/network/network_...
File chromeos/network/network_state.cc (right):

https://codereview.chromium.org/20087002/diff/85001/chromeos/network/network_...
chromeos/network/network_state.cc:231: return true;
On 2013/07/29 17:53:03, stevenjb (chromium) wrote:
> For the connect code I added the complete properties DictionaryValue as a
> parameter ti InitialPropertiesReceived(). Rather than storing 3 separate
> booleans here, we should parse these properties there and set a single
boolean. 
> 
> I will upload a CL with that change separately.

Done.

[stevenjb, 2013-08-05 18:28:08]

lgtm

https://codereview.chromium.org/20087002/diff/85001/chromeos/network/network_...
File chromeos/network/network_cert_migrator.h (right):

https://codereview.chromium.org/20087002/diff/85001/chromeos/network/network_...
chromeos/network/network_cert_migrator.h:25: void Init(NetworkStateHandler*
network_state_handler);
When I bundled the Network*Handler classes to be owned by and accessed through
NetworkHandler, I made the constructor/Init methods private (and NetworkHandler
a friend) so that the classes would not get instantiated elsewhere (except in
tests which are also explicitly made friends). Maybe this was unnecessarily
paranoid, but it seemed prudent at the time. If this is going to be owned by
NetworkHandler I'd prefer to at least be consistent with other similar classes.

https://codereview.chromium.org/20087002/diff/160002/chromeos/cert_loader.cc
File chromeos/cert_loader.cc (right):

https://codereview.chromium.org/20087002/diff/160002/chromeos/cert_loader.cc#...
chromeos/cert_loader.cc:339: task_runner = slow_task_runner_for_test_.get();
Maybe else GetTaskRunner, instead of always calling that?

[pneubeck (no reviews), 2013-08-06 07:54:14]

https://codereview.chromium.org/20087002/diff/85001/chromeos/network/network_...
File chromeos/network/network_cert_migrator.h (right):

https://codereview.chromium.org/20087002/diff/85001/chromeos/network/network_...
chromeos/network/network_cert_migrator.h:25: void Init(NetworkStateHandler*
network_state_handler);
On 2013/08/05 18:28:09, stevenjb (chromium) wrote:
> When I bundled the Network*Handler classes to be owned by and accessed through
> NetworkHandler, I made the constructor/Init methods private (and
NetworkHandler
> a friend) so that the classes would not get instantiated elsewhere (except in
> tests which are also explicitly made friends). Maybe this was unnecessarily
> paranoid, but it seemed prudent at the time. If this is going to be owned by
> NetworkHandler I'd prefer to at least be consistent with other similar
classes.

Done.

https://codereview.chromium.org/20087002/diff/160002/chromeos/cert_loader.cc
File chromeos/cert_loader.cc (right):

https://codereview.chromium.org/20087002/diff/160002/chromeos/cert_loader.cc#...
chromeos/cert_loader.cc:339: task_runner = slow_task_runner_for_test_.get();
On 2013/08/05 18:28:09, stevenjb (chromium) wrote:
> Maybe else GetTaskRunner, instead of always calling that?

Done.

[commit-bot: I haz the power, 2013-08-06 11:32:25]

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

[commit-bot: I haz the power, 2013-08-06 12:08:02]

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

[commit-bot: I haz the power, 2013-08-06 12:53:53]

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

[commit-bot: I haz the power, 2013-08-06 17:15:50]

Change committed as 215914