Rearrange EAP setup so that it is more parameterizable for other EAP types

Rearrange EAP setup so that it is more parameterizable for other EAP types
and so that the certificate text can be reused, leading to
easier-to-parse configurations.

BUG=chromium-os:7101, chromium-os:10108, chromium-os:10206
TEST=rerun SecMat/019*, no functional change

Committed: http://chrome-svn/viewvc/chromeos?view=rev&revision=7d49819

[Paul Stewart, 2011-01-05 22:28:29]

I'm not sure I buy into this approach.  Unless all the other methods you intend
to use are certificate based, I don't think it makes sense to rename
site_eap_tls to site_eap and tack everything in there.

It seems to make more sense to me perhaps to have an "site_eap_certs" where
common certificates are kept (if folks other than eap_tls need them), then
create separate eap_blah modules for them.  One could even take an object
oriented approach for this.

It makes more sense to me to do a more structured approach to parameterizing
arguments to site_wlan_connect, and to use the "psk" parameter in the control
files wherever possible, instead of using the site_eap_*.py code to encode
passwords.

http://codereview.chromium.org/6030011/diff/2001/server/site_eap.py
File server/site_eap.py (right):

http://codereview.chromium.org/6030011/diff/2001/server/site_eap.py#newcode296
server/site_eap.py:296: args.append(info['password'])
Wouldn't it make more sense to send the password in as a parameter to the
'connect' just like it's done for WPA-PSK instead of using a file for this?  The
only reason keys are here in this separate file is to avoid needless duplication
of large blocks of data.

http://codereview.chromium.org/6030011/diff/2001/server/site_wlan_connect.py
File server/site_wlan_connect.py (right):

http://codereview.chromium.org/6030011/diff/2001/server/site_wlan_connect.py#...
server/site_wlan_connect.py:165: connection_settings['Passphrase'] =
cert_args[3]
Instead of continuing to tack on optional parameters, I'd prefer if you prepend
the eap type as the first argument, and then interpret the remaining arguments
specific to that type.

[Nathan Williams, 2011-01-05 22:38:44]

I originally started in the direction you suggest, splitting out into
site_eap_certs (since PEAP and EAP-TTLS both use the server certs), and then
having site_eap_tls, site_eap_peap, and site_eap_ttls. What I realized was that
most of the code was duplicated - the insert_conf_file() and router_config()
routines are common, and the differences were mostly client config. It didn't
seem too abusive to cover both the client cert and user/password case in there,
but I'll see if I can do some restructuring.

I do agree that parameterizing more of the variables in the individual test
files would be an improvement and I'll see what I can do there.

[Sam Leffler, 2011-01-06 04:26:08]

some nits while I wait for your update

http://codereview.chromium.org/6030011/diff/2001/server/site_eap.py
File server/site_eap.py (right):

http://codereview.chromium.org/6030011/diff/2001/server/site_eap.py#newcode201
server/site_eap.py:201: },
I suspect our style guide says you've got this indented one level too much (same
for each below)

http://codereview.chromium.org/6030011/diff/2001/server/site_eap.py#newcode253
server/site_eap.py:253: raise error.TestFail('EAP profile %s not in the
configuration' % cert)
s/cert/profile/

http://codereview.chromium.org/6030011/diff/2001/server/site_eap.py#newcode269
server/site_eap.py:269: raise error.TestFail("EAP profile %s not in the
configuration" % cert)
s/cert/profile/

[Nathan Williams, 2011-01-11 01:32:45]

PTAL, new approach. I don't like duplicating insert_file() but it wasn't clear
where to put it.

[Nathan Williams, 2011-01-12 21:32:35]

Ping.

[Paul Stewart, 2011-01-13 01:56:05]

http://codereview.chromium.org/6030011/diff/13001/server/site_tests/network_W...
File server/site_tests/network_WiFiSecMat/019CheckWPA_1x_AES (right):

http://codereview.chromium.org/6030011/diff/13001/server/site_tests/network_W...
server/site_tests/network_WiFiSecMat/019CheckWPA_1x_AES:18: "ca_cert" :
ca_cert_1,
If you're using a "files" parameter below in the client code, why not do
something identical here?  I'd prefer something that appeared consistent.

http://codereview.chromium.org/6030011/diff/13001/server/site_tests/network_W...
server/site_tests/network_WiFiSecMat/019CheckWPA_1x_AES:36: "psk" :
"EAP-TLS:chromeos:/tmp/pkg-client.pem:/tmp/ca-cert.pem"} ],
80 cols here and below.

http://codereview.chromium.org/6030011/diff/13001/server/site_wifitest.py
File server/site_wifitest.py (right):

http://codereview.chromium.org/6030011/diff/13001/server/site_wifitest.py#new...
server/site_wifitest.py:12: from autotest_lib.server.site_eap_certs import *
I'd prefer if you didn't do "import *" here, and referred to
"eap_certs.cert_blah" in the control file.

http://codereview.chromium.org/6030011/diff/13001/server/site_wifitest.py#new...
server/site_wifitest.py:321: 
It doesn't make sense to copy this function here and site_linux_router.  You
could either modify the host class and put this function there, or at the worst,
you could create a module for it.

http://codereview.chromium.org/6030011/diff/13001/server/site_wifitest.py#new...
server/site_wifitest.py:350: if 'files' in params:
Instead of having this as a parameter to "connect", wouldn't this make more
sense as a top-level function?

[Nathan Williams, 2011-01-13 22:56:27]

On 2011/01/13 01:56:05, Paul Stewart wrote:
>
http://codereview.chromium.org/6030011/diff/13001/server/site_tests/network_W...
> File server/site_tests/network_WiFiSecMat/019CheckWPA_1x_AES (right):
> 
>
http://codereview.chromium.org/6030011/diff/13001/server/site_tests/network_W...
> server/site_tests/network_WiFiSecMat/019CheckWPA_1x_AES:18: "ca_cert" :
> ca_cert_1,
> If you're using a "files" parameter below in the client code, why not do
> something identical here?  I'd prefer something that appeared consistent.

OK, done, and split out into a separate command, as mentioned below.

>
http://codereview.chromium.org/6030011/diff/13001/server/site_tests/network_W...
> server/site_tests/network_WiFiSecMat/019CheckWPA_1x_AES:36: "psk" :
> "EAP-TLS:chromeos:/tmp/pkg-client.pem:/tmp/ca-cert.pem"} ],
> 80 cols here and below.

OK.

> 
> http://codereview.chromium.org/6030011/diff/13001/server/site_wifitest.py
> File server/site_wifitest.py (right):
> 
>
http://codereview.chromium.org/6030011/diff/13001/server/site_wifitest.py#new...
> server/site_wifitest.py:12: from autotest_lib.server.site_eap_certs import *
> I'd prefer if you didn't do "import *" here, and referred to
> "eap_certs.cert_blah" in the control file.

OK.

> 
>
http://codereview.chromium.org/6030011/diff/13001/server/site_wifitest.py#new...
> server/site_wifitest.py:321: 
> It doesn't make sense to copy this function here and site_linux_router.  You
> could either modify the host class and put this function there, or at the
worst,
> you could create a module for it.

Splitting out install_files as a separate command obviated this.

PTAL.

[Paul Stewart, 2011-01-13 23:44:27]

Wow!  That looks way cleaner!  LGTM.