Check configuration for networks without UIData

Check configuration for networks without UIData

This avoids a doomed connect attempt for unconfigured networks.

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

[stevenjb, 2013-08-29 19:37:55]

I think this broke in one of the many re-factorings.

[pneubeck (no reviews), 2013-08-30 11:04:08]

lgtm except comment.

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

https://codereview.chromium.org/23583018/diff/3001/chromeos/network/network_c...
chromeos/network/network_connection_handler.cc:476: if (client_cert_type ==
client_cert::CONFIG_TYPE_OPENVPN) {
How about putting this into a
  client_cert::GetShillProperties
which would be the exact opposite of client_cert::SetShillProperties ?

https://codereview.chromium.org/23583018/diff/3001/chromeos/network/network_c...
chromeos/network/network_connection_handler.cc:478: service_properties,
flimflam::kOpenVPNClientCertIdProperty);
service_properties -> provider_properties
in case of OpenVPN and ipsec ?

(i'm currently in a hurry, so take it with a grain of salt.)

[stevenjb, 2013-08-30 17:01:21]

PTAL

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

https://codereview.chromium.org/23583018/diff/3001/chromeos/network/network_c...
chromeos/network/network_connection_handler.cc:476: if (client_cert_type ==
client_cert::CONFIG_TYPE_OPENVPN) {
On 2013/08/30 11:04:08, pneubeck wrote:
> How about putting this into a
>   client_cert::GetShillProperties
> which would be the exact opposite of client_cert::SetShillProperties ?

Yeah, I considered doing something like that originally, I'll go ahead and move
it. I'll call it IsCertificateConfigured() since it's not really 'Getting' the
properties, just checking them,

https://codereview.chromium.org/23583018/diff/3001/chromeos/network/network_c...
chromeos/network/network_connection_handler.cc:478: service_properties,
flimflam::kOpenVPNClientCertIdProperty);
On 2013/08/30 11:04:08, pneubeck wrote:
> service_properties -> provider_properties
> in case of OpenVPN and ipsec ?
> 
> (i'm currently in a hurry, so take it with a grain of salt.)

Bah, you're right, I keep reversing setting vs getting on those, we've got to
change that.
Done.

[pneubeck (no reviews), 2013-08-31 05:31:29]

https://codereview.chromium.org/23583018/diff/12001/chromeos/network/client_c...
File chromeos/network/client_cert_util.cc (right):

https://codereview.chromium.org/23583018/diff/12001/chromeos/network/client_c...
chromeos/network/client_cert_util.cc:263: *provider_properties,
flimflam::kOpenVPNUserProperty);
I'm find it irritating that here Username is checked and not tpm pin.

https://codereview.chromium.org/23583018/diff/12001/chromeos/network/client_c...
chromeos/network/client_cert_util.cc:271: std::string username =
GetStringFromDictionary(
and here, tpm pin + slot instead of username.

Is there a specific reason for the deviation from SetShillProperties above?

https://codereview.chromium.org/23583018/diff/12001/chromeos/network/network_...
File chromeos/network/network_connection_handler.cc (right):

https://codereview.chromium.org/23583018/diff/12001/chromeos/network/network_...
chromeos/network/network_connection_handler.cc:454: } else {
NIT: 'else if' and comment inside the clause.

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

https://codereview.chromium.org/23583018/diff/12001/chromeos/network/client_c...
File chromeos/network/client_cert_util.cc (right):

https://codereview.chromium.org/23583018/diff/12001/chromeos/network/client_c...
chromeos/network/client_cert_util.cc:263: *provider_properties,
flimflam::kOpenVPNUserProperty);
On 2013/08/31 05:31:30, pneubeck wrote:
> I'm find it irritating that here Username is checked and not tpm pin.

TPM Pin is something configured by Chrome, so it does not actually need to be
configured for us to be able to connect, i.e. Chrome can set it.

https://codereview.chromium.org/23583018/diff/12001/chromeos/network/client_c...
chromeos/network/client_cert_util.cc:271: std::string username =
GetStringFromDictionary(
On 2013/08/31 05:31:30, pneubeck wrote:
> and here, tpm pin + slot instead of username.
> 
> Is there a specific reason for the deviation from SetShillProperties above?

This isn't really intended to be the opposite of SetShillProperties, which is
why I named it differently. In practice, those properties would also be set, but
since they are well known values that Chrome can set if necessary, they are not
actually required. Whereas, unless I am mistaken, if CertId or Username are
unset, there is no possibility of connecting to the network.

https://codereview.chromium.org/23583018/diff/12001/chromeos/network/network_...
File chromeos/network/network_connection_handler.cc (right):

https://codereview.chromium.org/23583018/diff/12001/chromeos/network/network_...
chromeos/network/network_connection_handler.cc:454: } else {
On 2013/08/31 05:31:30, pneubeck wrote:
> NIT: 'else if' and comment inside the clause.

Done.

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

Overall I have the impression that we can at most give the user a hint that the
server might require a cert/username if the connection failed before.
But in general there is not strict requirement for certain credentials.

https://codereview.chromium.org/23583018/diff/12001/chromeos/network/client_c...
File chromeos/network/client_cert_util.cc (right):

https://codereview.chromium.org/23583018/diff/12001/chromeos/network/client_c...
chromeos/network/client_cert_util.cc:271: std::string username =
GetStringFromDictionary(
On 2013/09/03 22:33:05, stevenjb (chromium) wrote:
> On 2013/08/31 05:31:30, pneubeck wrote:
> > and here, tpm pin + slot instead of username.
> > 
> > Is there a specific reason for the deviation from SetShillProperties above?
> 
> This isn't really intended to be the opposite of SetShillProperties, which is
> why I named it differently. In practice, those properties would also be set,
but
> since they are well known values that Chrome can set if necessary, they are
not
> actually required. Whereas, unless I am mistaken, if CertId or Username are
> unset, there is no possibility of connecting to the network.
> 
> 

AFAIU, VPNs can be setup without Username or without client cert.

OpenVPN can be setup without client cert and instead with username/password: on
http://openvpn.net/index.php/open-source/documentation/howto.html
 grep for "client-cert-not-required".

In case of EAP, there are at least variants that don't require a client cert:
http://en.wikipedia.org/wiki/Extensible_Authentication_Protocol#EAP-TTLS
mentions "The client can, but does not have to be authenticated via a CA-signed
PKI certificate to the server"

Similar might be true for L2TP/IPsec.

https://codereview.chromium.org/23583018/diff/12001/chromeos/network/client_c...
File chromeos/network/client_cert_util.h (right):

https://codereview.chromium.org/23583018/diff/12001/chromeos/network/client_c...
chromeos/network/client_cert_util.h:57: // Returns false if any certificate
properties required for configuration are
according to your explanation, this comment should mention that it's concerned
about "properties that only the user can provide".

https://codereview.chromium.org/23583018/diff/12001/chromeos/network/network_...
File chromeos/network/network_connection_handler.cc (right):

https://codereview.chromium.org/23583018/diff/12001/chromeos/network/network_...
chromeos/network/network_connection_handler.cc:417: if (client_cert_type !=
client_cert::CONFIG_TYPE_NONE) {
If this condition holds, we only know that this network _can_ be configured with
a client cert, not that it requires a cert.

https://codereview.chromium.org/23583018/diff/12001/chromeos/network/network_...
chromeos/network/network_connection_handler.cc:427: if (ui_data &&
ui_data->certificate_type() == CLIENT_CERT_TYPE_PATTERN) {
In this branch we know that the policy configured the network so that it
requires a client cert.

https://codereview.chromium.org/23583018/diff/12001/chromeos/network/network_...
chromeos/network/network_connection_handler.cc:454: } else {
On 2013/09/03 22:33:05, stevenjb (chromium) wrote:
> On 2013/08/31 05:31:30, pneubeck wrote:
> > NIT: 'else if' and comment inside the clause.
> 
> Done.

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]

https://codereview.chromium.org/23583018/diff/12001/chromeos/network/client_c...
File chromeos/network/client_cert_util.cc (right):

https://codereview.chromium.org/23583018/diff/12001/chromeos/network/client_c...
chromeos/network/client_cert_util.cc:271: std::string username =
GetStringFromDictionary(
On 2013/09/04 08:40:00, pneubeck wrote:
> On 2013/09/03 22:33:05, stevenjb (chromium) wrote:
> > On 2013/08/31 05:31:30, pneubeck wrote:
> > > and here, tpm pin + slot instead of username.
> > > 
> > > Is there a specific reason for the deviation from SetShillProperties
above?
> > 
> > This isn't really intended to be the opposite of SetShillProperties, which
is
> > why I named it differently. In practice, those properties would also be set,
> but
> > since they are well known values that Chrome can set if necessary, they are
> not
> > actually required. Whereas, unless I am mistaken, if CertId or Username are
> > unset, there is no possibility of connecting to the network.
> > 
> > 
> 
> AFAIU, VPNs can be setup without Username or without client cert.
> 
> OpenVPN can be setup without client cert and instead with username/password:
on
> http://openvpn.net/index.php/open-source/documentation/howto.html
>  grep for "client-cert-not-required".
> 
> In case of EAP, there are at least variants that don't require a client cert:
> http://en.wikipedia.org/wiki/Extensible_Authentication_Protocol#EAP-TTLS
> mentions "The client can, but does not have to be authenticated via a
CA-signed
> PKI certificate to the server"
> 
> Similar might be true for L2TP/IPsec.

Until/unless Shill sets 'Configured' properly, all we can do is check the
situations where we are reasonably certain that the network is configured. It
sounds like for VPN we just need to always return 'false' here, which is the M29
behavior (always show the config UI first).

https://codereview.chromium.org/23583018/diff/12001/chromeos/network/network_...
File chromeos/network/network_connection_handler.cc (right):

https://codereview.chromium.org/23583018/diff/12001/chromeos/network/network_...
chromeos/network/network_connection_handler.cc:417: if (client_cert_type !=
client_cert::CONFIG_TYPE_NONE) {
On 2013/09/04 08:40:00, pneubeck wrote:
> If this condition holds, we only know that this network _can_ be configured
with
> a client cert, not that it requires a cert.

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

https://codereview.chromium.org/23583018/diff/12001/chromeos/network/network_...
chromeos/network/network_connection_handler.cc:427: if (ui_data &&
ui_data->certificate_type() == CLIENT_CERT_TYPE_PATTERN) {
On 2013/09/04 08:40:00, pneubeck wrote:
> In this branch we know that the policy configured the network so that it
> requires a client cert.
Right...

https://codereview.chromium.org/23583018/diff/12001/chromeos/network/network_...
chromeos/network/network_connection_handler.cc:454: } else {
On 2013/09/04 08:40:00, pneubeck wrote:
> On 2013/09/03 22:33:05, stevenjb (chromium) wrote:
> > On 2013/08/31 05:31:30, pneubeck wrote:
> > > NIT: 'else if' and comment inside the clause.
> > 
> > Done.
> 
> 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.

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.

[pneubeck (no reviews), 2013-09-05 07:45:03]

Thanks for clarifying.
I thought (again) at first that this check is interrupting any connection
attempt. But that's not the case. Maybe the usage of "required" vs. "may be
required" confused me :-)

I think it's good to go with the current CL and refine it later if we find cases
where popping up the dialog is to annoying.

https://codereview.chromium.org/23583018/diff/27001/chromeos/network/client_c...
File chromeos/network/client_cert_util.cc (right):

https://codereview.chromium.org/23583018/diff/27001/chromeos/network/client_c...
chromeos/network/client_cert_util.cc:258: // OpenVPN generally requires a
passphrase and we don't know whether or
nit: or -> or not

[pneubeck (no reviews), 2013-09-05 07:45:09]

lgtm

[stevenjb, 2013-09-05 16:29:34]

https://codereview.chromium.org/23583018/diff/27001/chromeos/network/client_c...
File chromeos/network/client_cert_util.cc (right):

https://codereview.chromium.org/23583018/diff/27001/chromeos/network/client_c...
chromeos/network/client_cert_util.cc:258: // OpenVPN generally requires a
passphrase and we don't know whether or
On 2013/09/05 07:45:04, pneubeck wrote:
> nit: or -> or not

Done.

[commit-bot: I haz the power, 2013-09-05 16:30:47]

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

[commit-bot: I haz the power, 2013-09-05 19:07:49]

Change committed as 221487