Check configuration for networks without UIData

chromeos/network/network_connection_handler.cc

 // Copyright (c) 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chromeos/network/network_connection_handler.h"
 
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/json/json_reader.h"
 #include "chromeos/chromeos_switches.h"
@@ skipping 396 matching lines @@
     if (vpn_provider_type == flimflam::kProviderOpenVpn)
       client_cert_type = client_cert::CONFIG_TYPE_OPENVPN;
     else
       client_cert_type = client_cert::CONFIG_TYPE_IPSEC;
   } else if (type == flimflam::kTypeWifi &&
              security == flimflam::kSecurity8021x) {
     client_cert_type = client_cert::CONFIG_TYPE_EAP;
   }
 
   base::DictionaryValue config_properties;
   if (client_cert_type != client_cert::CONFIG_TYPE_NONE) {

[pneubeck (no reviews), 2013/09/04 08:40:00]

If this condition holds, we only know that this network _can_ be configured with
a client cert, not that it requires a cert.

[stevenjb, 2013/09/04 17:58:13]

I see. I'll change the return value to kErrorConfigurationRequired.

     // If the client certificate must be configured, this will be set to a
     // non-empty string.
     std::string pkcs11_id;
 
     // Check certificate properties in kUIDataProperty if configured.
     // Note: Wifi/VPNConfigView set these properties explicitly, in which case
     //   only the TPM must be configured.
     scoped_ptr<NetworkUIData> ui_data =
         ManagedNetworkConfigurationHandler::GetUIData(service_properties);
     if (ui_data && ui_data->certificate_type() == CLIENT_CERT_TYPE_PATTERN) {

[pneubeck (no reviews), 2013/09/04 08:40:00]

In this branch we know that the policy configured the network so that it
requires a client cert.

[stevenjb, 2013/09/04 17:58:13]

Right...

       // User must be logged in to connect to a network requiring a certificate.
       if (!logged_in_ || !cert_loader_) {
         ErrorCallbackForPendingRequest(service_path, kErrorCertificateRequired);
         return;
       }
 
       // If certificates have not been loaded yet, queue the connect request.
       if (!certificates_loaded_) {
         ConnectRequest* request = GetPendingRequest(service_path);
         if (!request) {
           NET_LOG_ERROR("No pending request to queue", service_path);
           return;
         }
         NET_LOG_EVENT("Connect Request Queued", service_path);
         queued_connect_.reset(new ConnectRequest(
             service_path, request->success_callback, request->error_callback));
         pending_requests_.erase(service_path);
         return;
       }
 
       pkcs11_id = CertificateIsConfigured(ui_data.get());
       // Ensure the certificate is available and configured.
       if (!cert_loader_->IsHardwareBacked() || pkcs11_id.empty()) {
         ErrorCallbackForPendingRequest(service_path, kErrorCertificateRequired);
         return;
       }
+    } else {

[pneubeck (no reviews), 2013/08/31 05:31:30]

NIT: 'else if' and comment inside the clause.

[stevenjb, 2013/09/03 22:33:05]

Done.

[pneubeck (no reviews), 2013/09/04 08:40:00]

See my new comments above. I think, in this branch we don't know that a client
certificate is required.
Without having information from the server it might not be possible what
configuration is required.

However, there might be some other properties that strictly require a client
cert (e.g. maybe some specific EAP authentication method).

Maybe it's correct to require (client_cert || username)
But in case of OpenVPN it at least possible, I think, to not even require that.

[stevenjb, 2013/09/04 17:58:13]

So it sounds like for OpenVPN we never really know whether or not it is
configured, so IsCertificateConfigured() needs to always return false for
OpenVPN; I'll go ahead and change that and add a comment.

+      // Certificate is not configured in ui_data, check properties.
+      if (!client_cert::IsCertificateConfigured(
+              client_cert_type, service_properties)) {
+        ErrorCallbackForPendingRequest(service_path, kErrorCertificateRequired);
+        return;
+      }
     }
 
     // The network may not be 'Connectable' because the TPM properties are not
     // set up, so configure tpm slot/pin before connecting.
     if (cert_loader_ && cert_loader_->IsHardwareBacked()) {
       // Pass NULL if pkcs11_id is empty, so that it doesn't clear any
       // previously configured client cert.
       client_cert::SetShillProperties(client_cert_type,
                                       cert_loader_->tpm_token_slot(),
                                       cert_loader_->tpm_user_pin(),
@@ skipping 211 matching lines @@
 
 void NetworkConnectionHandler::HandleShillDisconnectSuccess(
     const std::string& service_path,
     const base::Closure& success_callback) {
   NET_LOG_EVENT("Disconnect Request Sent", service_path);
   if (!success_callback.is_null())
     success_callback.Run();
 }
 
 }  // namespace chromeos