Configure networks requiring a certificate.

Configure networks requiring a certificate.

The current flow does not guarantee that a configured network will have
its tpm / pkcs11 credentials configured. We need to set these 
properties in order to reliably connect to networks requiring 
certificates. The patch also includes some minor changes to improve
error reporting and configuration triggering for failure cases.

BUG=247104
For /wifi_data_provider_chromeos_unittest.cc:
TBR=joth@chromium.org

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

[Greg Spencer (Chromium), 2013-06-06 01:11:46]

https://codereview.chromium.org/16512003/diff/2001/chromeos/network/network_c...
File chromeos/network/network_connection_handler.cc (right):

https://codereview.chromium.org/16512003/diff/2001/chromeos/network/network_c...
chromeos/network/network_connection_handler.cc:389: // TODO(stevenjb): Remove
this code once NetworkConfigurationHandler
So, why can't you just move it there now?  What's the sticking point (because
I'm sure there is one. :-))?

[stevenjb, 2013-06-06 01:16:46]

On 2013/06/06 01:11:46, Greg Spencer (Chromium) wrote:
>
https://codereview.chromium.org/16512003/diff/2001/chromeos/network/network_c...
> File chromeos/network/network_connection_handler.cc (right):
> 
>
https://codereview.chromium.org/16512003/diff/2001/chromeos/network/network_c...
> chromeos/network/network_connection_handler.cc:389: // TODO(stevenjb): Remove
> this code once NetworkConfigurationHandler
> So, why can't you just move it there now?  What's the sticking point (because
> I'm sure there is one. :-))?

For starters it's not enabled by default :)

More important, NetworkConfigView doesn't set these properties (or use
NetworkConfigurationHandler). Rather than modify NetworkConfigView, I would
rather do this here for now, then focus on the new UI to replace
NetworkConfigView. In the meanwhile these extra calls will merely be redundant.

I'm also not 100% certain that Shill persists these properties correctly, so we
will need to tread carefully when we do remove this configuration step.

[pneubeck (no reviews), 2013-06-06 20:41:20]

The CL seems to be a combination of two independent parts: ConfigureNetwork
function and the changes in NetworkConnectionHandler.

https://codereview.chromium.org/16512003/diff/6001/ash/system/chromeos/networ...
File ash/system/chromeos/network/network_connect.cc (right):

https://codereview.chromium.org/16512003/diff/6001/ash/system/chromeos/networ...
ash/system/chromeos/network/network_connect.cc:51: // configure UI and a
notification. See crbug.com/217033 for an example.
"configure UI" -> "configuration UI" or "settings UI" ?

https://codereview.chromium.org/16512003/diff/6001/ash/system/chromeos/networ...
File ash/system/chromeos/network/network_state_notifier.cc (right):

https://codereview.chromium.org/16512003/diff/6001/ash/system/chromeos/networ...
ash/system/chromeos/network/network_state_notifier.cc:95:
IDS_CHROMEOS_NETWORK_ERROR_IPSEC_CERT_AUTH_FAILED);
These errors are not related to IPsec only.
Currently more the opposite is the case, WiFi with EAP can lead to these errors.

https://codereview.chromium.org/16512003/diff/6001/chrome/browser/chromeos/op...
File chrome/browser/chromeos/options/network_connect.cc (right):

https://codereview.chromium.org/16512003/diff/6001/chrome/browser/chromeos/op...
chrome/browser/chromeos/options/network_connect.cc:172: if (!network)
Maybe LOG(ERROR)/WARNING.

https://codereview.chromium.org/16512003/diff/6001/chrome/browser/chromeos/op...
chrome/browser/chromeos/options/network_connect.cc:203:
ash::Shell::GetInstance()->system_tray_delegate()->ShowNetworkSettings(
This accesses ash to get the delegate that was previously set from
chrome/browser/chromeos,
only to again open the settings page living in chrome/browser/chromeos.
Although we're already in chrome/browser/chromeos.

Can't you access the settings page directly.
This call also again uses NetworkState handler, which seem to have avoided
above.

https://codereview.chromium.org/16512003/diff/6001/chrome/browser/chromeos/op...
File chrome/browser/chromeos/options/network_connect.h (right):

https://codereview.chromium.org/16512003/diff/6001/chrome/browser/chromeos/op...
chrome/browser/chromeos/options/network_connect.h:40: // * Configure and connect
to a network with an existing unconfigured cert.
What is an unconfigured cert? Do you mean that the network is not configured
with a cert yet?
Maybe:
"Configure the network with a matching certificate, if required and existent,
and connect to it."

https://codereview.chromium.org/16512003/diff/6001/chrome/browser/chromeos/op...
chrome/browser/chromeos/options/network_connect.h:41: // * Show the enrollment
dialog for a network.
in all cases it's /the/ network (service_path)

https://codereview.chromium.org/16512003/diff/6001/chrome/browser/chromeos/op...
chrome/browser/chromeos/options/network_connect.h:45: void
ConfigureNetwork(const std::string& service_path,
The function name should also indicate that it might connect, e.g.
ConfigureAndConnectNetwork(...)

https://codereview.chromium.org/16512003/diff/6001/chromeos/network/network_c...
File chromeos/network/network_connection_handler.cc (right):

https://codereview.chromium.org/16512003/diff/6001/chromeos/network/network_c...
chromeos/network/network_connection_handler.cc:185: certificates_loaded_ = true;
for now ok. later it should get a MockCertLoader that just returns true.

https://codereview.chromium.org/16512003/diff/6001/chromeos/network/network_c...
chromeos/network/network_connection_handler.cc:238: if
(HasConnectingNetwork(service_path)) {
shouldn't connection request be restricted to a certain number per network-type
and available devices of that type?
E.g. devices with one radio can only request one Wifi connection in parallel?

On the other hand, that might as well be handled on Shill's side.

https://codereview.chromium.org/16512003/diff/6001/chromeos/network/network_c...
chromeos/network/network_connection_handler.cc:363: if (ui_data &&
ui_data->certificate_type() == CLIENT_CERT_TYPE_PATTERN) {
Is it correct, that the new Handler doesn't support certificate references?
We might track that on the bugtracker.

https://codereview.chromium.org/16512003/diff/6001/chromeos/network/network_c...
chromeos/network/network_connection_handler.cc:423:
network_configuration_handler_->SetProperties(
I don't see what this block has to do with the configuration handler, except the
NetworkUIData from above.
These properties represent a user configuration, thus shouldn't be handled by
the ConfigHandler.

This block should be moved to the CertLoader (or maybe something called
NetworkCertHandler).
and network_configuration_handler_->SetProperties, should be replaced by a
direct ShillServiceClient call.

https://codereview.chromium.org/16512003/diff/6001/chromeos/network/network_c...
chromeos/network/network_connection_handler.cc:546: // We skip checking
certificate patterns for device policy ONC so that an
Please remove this clause, it's already verified on policy import/application
and I don't want to review it everytime we move this piece of code ;-)

https://codereview.chromium.org/16512003/diff/6001/chromeos/network/network_c...
File chromeos/network/network_connection_handler.h (right):

https://codereview.chromium.org/16512003/diff/6001/chromeos/network/network_c...
chromeos/network/network_connection_handler.h:110: // CertLoader::Observer
?

https://codereview.chromium.org/16512003/diff/6001/chromeos/network/network_c...
chromeos/network/network_connection_handler.h:150: bool
CertificateIsConfigured(NetworkUIData* ui_data, std::string* pkcs11_id);
I really have to hurry with migrating the ConnectionHandler to
(Managed)NetworkConfigurationHandler.
NetworkUIData shouldn't be used in the new implementations.

https://codereview.chromium.org/16512003/diff/6001/chromeos/network/network_c...
chromeos/network/network_connection_handler.h:175: // Map of pending connect
requests, used to prevent repeat attempts while
repeat -> repeated

https://codereview.chromium.org/16512003/diff/6001/chromeos/network/network_s...
File chromeos/network/network_state.cc (right):

https://codereview.chromium.org/16512003/diff/6001/chromeos/network/network_s...
chromeos/network/network_state.cc:220: security_);
nit: security()

[stevenjb, 2013-06-07 03:44:58]

https://codereview.chromium.org/16512003/diff/6001/ash/system/chromeos/networ...
File ash/system/chromeos/network/network_connect.cc (right):

https://codereview.chromium.org/16512003/diff/6001/ash/system/chromeos/networ...
ash/system/chromeos/network/network_connect.cc:51: // configure UI and a
notification. See crbug.com/217033 for an example.
On 2013/06/06 20:41:20, pneubeck wrote:
> "configure UI" -> "configuration UI" or "settings UI" ?

Done.

https://codereview.chromium.org/16512003/diff/6001/ash/system/chromeos/networ...
File ash/system/chromeos/network/network_state_notifier.cc (right):

https://codereview.chromium.org/16512003/diff/6001/ash/system/chromeos/networ...
ash/system/chromeos/network/network_state_notifier.cc:95:
IDS_CHROMEOS_NETWORK_ERROR_IPSEC_CERT_AUTH_FAILED);
On 2013/06/06 20:41:20, pneubeck wrote:
> These errors are not related to IPsec only.
> Currently more the opposite is the case, WiFi with EAP can lead to these
errors.
The message is "Failed to authenticate with provided certificate".
I renamed the ID to IDS_CHROMEOS_NETWORK_ERROR_CERT_AUTH_FAILED and combined the
ifs.
If Local/RemoteTlsFailed should have a different message, let me know what it
should (roughly) say.

https://codereview.chromium.org/16512003/diff/6001/chrome/browser/chromeos/op...
File chrome/browser/chromeos/options/network_connect.cc (right):

https://codereview.chromium.org/16512003/diff/6001/chrome/browser/chromeos/op...
chrome/browser/chromeos/options/network_connect.cc:172: if (!network)
On 2013/06/06 20:41:20, pneubeck wrote:
> Maybe LOG(ERROR)/WARNING.

Done.

https://codereview.chromium.org/16512003/diff/6001/chrome/browser/chromeos/op...
chrome/browser/chromeos/options/network_connect.cc:203:
ash::Shell::GetInstance()->system_tray_delegate()->ShowNetworkSettings(
On 2013/06/06 20:41:20, pneubeck wrote:
> This accesses ash to get the delegate that was previously set from
> chrome/browser/chromeos,
> only to again open the settings page living in chrome/browser/chromeos.
> Although we're already in chrome/browser/chromeos.
> 
> Can't you access the settings page directly.
> This call also again uses NetworkState handler, which seem to have avoided
> above.
Yeah, I agree, it's ugly. I really can't wait to remove this code entirely. I
was hoping to avoid some copy/paste code, but since it is going away...
Done.

btw, in case you're wondering - kInternetOptionsSubPage recognizes either a
string type or a numerical type. See
InternetOptionsHandler::NetworkCommandCallback.

https://codereview.chromium.org/16512003/diff/6001/chrome/browser/chromeos/op...
File chrome/browser/chromeos/options/network_connect.h (right):

https://codereview.chromium.org/16512003/diff/6001/chrome/browser/chromeos/op...
chrome/browser/chromeos/options/network_connect.h:40: // * Configure and connect
to a network with an existing unconfigured cert.
On 2013/06/06 20:41:20, pneubeck wrote:
> What is an unconfigured cert? Do you mean that the network is not configured
> with a cert yet?
> Maybe:
> "Configure the network with a matching certificate, if required and existent,
> and connect to it."

Done.

https://codereview.chromium.org/16512003/diff/6001/chrome/browser/chromeos/op...
chrome/browser/chromeos/options/network_connect.h:41: // * Show the enrollment
dialog for a network.
On 2013/06/06 20:41:20, pneubeck wrote:
> in all cases it's /the/ network (service_path)
Done

https://codereview.chromium.org/16512003/diff/6001/chrome/browser/chromeos/op...
chrome/browser/chromeos/options/network_connect.h:45: void
ConfigureNetwork(const std::string& service_path,
On 2013/06/06 20:41:20, pneubeck wrote:
> The function name should also indicate that it might connect, e.g.
> ConfigureAndConnectNetwork(...)
Renamed HandleUnconfiguredNetwork

https://codereview.chromium.org/16512003/diff/6001/chromeos/network/network_c...
File chromeos/network/network_connection_handler.cc (right):

https://codereview.chromium.org/16512003/diff/6001/chromeos/network/network_c...
chromeos/network/network_connection_handler.cc:185: certificates_loaded_ = true;
On 2013/06/06 20:41:20, pneubeck wrote:
> for now ok. later it should get a MockCertLoader that just returns true.
Added a TODO

https://codereview.chromium.org/16512003/diff/6001/chromeos/network/network_c...
chromeos/network/network_connection_handler.cc:238: if
(HasConnectingNetwork(service_path)) {
On 2013/06/06 20:41:20, pneubeck wrote:
> shouldn't connection request be restricted to a certain number per
network-type
> and available devices of that type?
> E.g. devices with one radio can only request one Wifi connection in parallel?
> 
> On the other hand, that might as well be handled on Shill's side.

It should be handled in Shill and/or in the UI. We can currently be in a
scenario where we are waiting (just a few frames) for one request to fail after
a second request is started. I think this is actually simpler than enforcing a
single request per type.

https://codereview.chromium.org/16512003/diff/6001/chromeos/network/network_c...
chromeos/network/network_connection_handler.cc:363: if (ui_data &&
ui_data->certificate_type() == CLIENT_CERT_TYPE_PATTERN) {
On 2013/06/06 20:41:20, pneubeck wrote:
> Is it correct, that the new Handler doesn't support certificate references?
> We might track that on the bugtracker.

My intention was to mimic the current behavior in NetworkLibrary. I don't see
CLIENT_CERT_TYPE_REF (assuming that's what you are referring to?) used anywhere
in the code. This sounds like something that can wait for the configuration
handler to deal with?

https://codereview.chromium.org/16512003/diff/6001/chromeos/network/network_c...
chromeos/network/network_connection_handler.cc:423:
network_configuration_handler_->SetProperties(
On 2013/06/06 20:41:20, pneubeck wrote:
> I don't see what this block has to do with the configuration handler, except
the
> NetworkUIData from above.
> These properties represent a user configuration, thus shouldn't be handled by
> the ConfigHandler.
> 
> This block should be moved to the CertLoader (or maybe something called
> NetworkCertHandler).
> and network_configuration_handler_->SetProperties, should be replaced by a
> direct ShillServiceClient call.

CertLoader doesn't and shouldn't know anything about Shill or the Network code.

My thinking was that pkcs11/tpm propeties should be configured once when a
network is configured and we shouldn't have to set them every time we connect. 

I suppose that might not handle the case where a matching certificate is removed
then re-added, so the network needs to be reconfigured but does not require the
user to set anything, but that still seems like something that a "Configuration
Handler" ought to handle.

We might have different notions of what NetworkConfigurationHandler should
provide, but my expectation was that eventually any setting of network
properties should go through NetworkConfigurationHandler.

https://codereview.chromium.org/16512003/diff/6001/chromeos/network/network_c...
chromeos/network/network_connection_handler.cc:546: // We skip checking
certificate patterns for device policy ONC so that an
On 2013/06/06 20:41:20, pneubeck wrote:
> Please remove this clause, it's already verified on policy import/application
> and I don't want to review it everytime we move this piece of code ;-)

Done.

https://codereview.chromium.org/16512003/diff/6001/chromeos/network/network_c...
File chromeos/network/network_connection_handler.h (right):

https://codereview.chromium.org/16512003/diff/6001/chromeos/network/network_c...
chromeos/network/network_connection_handler.h:110: // CertLoader::Observer
On 2013/06/06 20:41:20, pneubeck wrote:
> ?
Merge bug. Fixed.

https://codereview.chromium.org/16512003/diff/6001/chromeos/network/network_c...
chromeos/network/network_connection_handler.h:150: bool
CertificateIsConfigured(NetworkUIData* ui_data, std::string* pkcs11_id);
On 2013/06/06 20:41:20, pneubeck wrote:
> I really have to hurry with migrating the ConnectionHandler to
> (Managed)NetworkConfigurationHandler.
> NetworkUIData shouldn't be used in the new implementations.

Yeah, I was hoping to wait, but really need to move this forward. Sorry for the
extra future re-factoring.

https://codereview.chromium.org/16512003/diff/6001/chromeos/network/network_c...
chromeos/network/network_connection_handler.h:175: // Map of pending connect
requests, used to prevent repeat attempts while
On 2013/06/06 20:41:20, pneubeck wrote:
> repeat -> repeated
Actually technically 'repeat' can also be an adjective, but I'l change to the
more standardized form :)

https://codereview.chromium.org/16512003/diff/6001/chromeos/network/network_s...
File chromeos/network/network_state.cc (right):

https://codereview.chromium.org/16512003/diff/6001/chromeos/network/network_s...
chromeos/network/network_state.cc:220: security_);
On 2013/06/06 20:41:20, pneubeck wrote:
> nit: security()

Done.

[pneubeck (no reviews), 2013-06-07 12:05:47]

lgtm

https://codereview.chromium.org/16512003/diff/6001/ash/system/chromeos/networ...
File ash/system/chromeos/network/network_state_notifier.cc (right):

https://codereview.chromium.org/16512003/diff/6001/ash/system/chromeos/networ...
ash/system/chromeos/network/network_state_notifier.cc:95:
IDS_CHROMEOS_NETWORK_ERROR_IPSEC_CERT_AUTH_FAILED);
On 2013/06/07 03:44:58, stevenjb (chromium) wrote:
> On 2013/06/06 20:41:20, pneubeck wrote:
> > These errors are not related to IPsec only.
> > Currently more the opposite is the case, WiFi with EAP can lead to these
> errors.
> The message is "Failed to authenticate with provided certificate".
> I renamed the ID to IDS_CHROMEOS_NETWORK_ERROR_CERT_AUTH_FAILED and combined
the
> ifs.
> If Local/RemoteTlsFailed should have a different message, let me know what it
> should (roughly) say.
> 

Could be put into a follow-up CL:

Looking at the shill code,
EapLocalTlsFailed coulde be "EAP-TLS authentication failed locally." and
EapRemoteTlsFailed coulde be "EAP-TLS authentication failed remotely."

Alternatively, we could fallback to a generic error message like "EAP(-TLS)
authentication failed."

To be completely certain about the errors' meaning, we could contact Paul
Stewart.

https://codereview.chromium.org/16512003/diff/6001/chromeos/network/network_c...
File chromeos/network/network_connection_handler.cc (right):

https://codereview.chromium.org/16512003/diff/6001/chromeos/network/network_c...
chromeos/network/network_connection_handler.cc:363: if (ui_data &&
ui_data->certificate_type() == CLIENT_CERT_TYPE_PATTERN) {
On 2013/06/07 03:44:58, stevenjb (chromium) wrote:
> On 2013/06/06 20:41:20, pneubeck wrote:
> > Is it correct, that the new Handler doesn't support certificate references?
> > We might track that on the bugtracker.
> 
> My intention was to mimic the current behavior in NetworkLibrary. I don't see
> CLIENT_CERT_TYPE_REF (assuming that's what you are referring to?) used
anywhere
> in the code. This sounds like something that can wait for the configuration
> handler to deal with?

Interesting, I also don't find any implementation for that and I'm rather sure
that I didn't remove any.

so.. TBD.

https://codereview.chromium.org/16512003/diff/6001/chromeos/network/network_c...
chromeos/network/network_connection_handler.cc:423:
network_configuration_handler_->SetProperties(
On 2013/06/07 03:44:58, stevenjb (chromium) wrote:
> On 2013/06/06 20:41:20, pneubeck wrote:
> > I don't see what this block has to do with the configuration handler, except
> the
> > NetworkUIData from above.
> > These properties represent a user configuration, thus shouldn't be handled
by
> > the ConfigHandler.
> > 
> > This block should be moved to the CertLoader (or maybe something called
> > NetworkCertHandler).
> > and network_configuration_handler_->SetProperties, should be replaced by a
> > direct ShillServiceClient call.
> 
> CertLoader doesn't and shouldn't know anything about Shill or the Network
code.
> 
> My thinking was that pkcs11/tpm propeties should be configured once when a
> network is configured and we shouldn't have to set them every time we connect.

> 
> I suppose that might not handle the case where a matching certificate is
removed
> then re-added, so the network needs to be reconfigured but does not require
the
> user to set anything, but that still seems like something that a
"Configuration
> Handler" ought to handle.
> 
> We might have different notions of what NetworkConfigurationHandler should
> provide, but my expectation was that eventually any setting of network
> properties should go through NetworkConfigurationHandler.

I'm fine with what you plan with CertLoader.
About NetConfHandler however we really seem to have different notions of what it
should do. We should talk about that (before I write a lengthy thesis again ;-).

https://codereview.chromium.org/16512003/diff/19001/chrome/browser/chromeos/o...
File chrome/browser/chromeos/options/network_connect.cc (right):

https://codereview.chromium.org/16512003/diff/19001/chrome/browser/chromeos/o...
chrome/browser/chromeos/options/network_connect.cc:19: #include
"chrome/browser/chromeos/system/ash_system_tray_delegate.h"
unused?

https://codereview.chromium.org/16512003/diff/19001/chrome/browser/chromeos/o...
chrome/browser/chromeos/options/network_connect.cc:216:
"?servicePath=%s&networkType=%d&networkName=%s",
networkName is not used by internet_detail.js and can be removed.

https://codereview.chromium.org/16512003/diff/19001/chrome/browser/chromeos/o...
File chrome/browser/chromeos/options/network_connect.h (right):

https://codereview.chromium.org/16512003/diff/19001/chrome/browser/chromeos/o...
chrome/browser/chromeos/options/network_connect.h:41: //   picks11id and tpm pin
/ slot configured.
nit: picks11id -> pkcs11id

[Greg Spencer (Chromium), 2013-06-07 18:33:02]

lgtm

[stevenjb, 2013-06-07 18:50:39]

https://codereview.chromium.org/16512003/diff/19001/chrome/browser/chromeos/o...
File chrome/browser/chromeos/options/network_connect.cc (right):

https://codereview.chromium.org/16512003/diff/19001/chrome/browser/chromeos/o...
chrome/browser/chromeos/options/network_connect.cc:19: #include
"chrome/browser/chromeos/system/ash_system_tray_delegate.h"
On 2013/06/07 12:05:47, pneubeck wrote:
> unused?

Done.

https://codereview.chromium.org/16512003/diff/19001/chrome/browser/chromeos/o...
chrome/browser/chromeos/options/network_connect.cc:216:
"?servicePath=%s&networkType=%d&networkName=%s",
On 2013/06/07 12:05:47, pneubeck wrote:
> networkName is not used by internet_detail.js and can be removed.

It's still referenced there. I think I will leave it until / unless it is purged
(or more likely until this is all replaced).

https://codereview.chromium.org/16512003/diff/19001/chrome/browser/chromeos/o...
File chrome/browser/chromeos/options/network_connect.h (right):

https://codereview.chromium.org/16512003/diff/19001/chrome/browser/chromeos/o...
chrome/browser/chromeos/options/network_connect.h:41: //   picks11id and tpm pin
/ slot configured.
On 2013/06/07 12:05:47, pneubeck wrote:
> nit: picks11id -> pkcs11id

Done.

[commit-bot: I haz the power, 2013-06-07 18:54:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stevenjb@chromium.org/16512003/54001

[commit-bot: I haz the power, 2013-06-08 00:37:57]

Change committed as 204994