Add end to end client cert auth test for wss

- add client cert authentication support in test websocket server
- add new browser test which access to a page which requires client cert

BUG=136950
TEST=browser_tests

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=146294

[Takashi Toyoshima, 2012-06-26 14:50:33]

Hi Jay, could you look this change to support client cert end to end test?

[Jay Civelli, 2012-06-26 16:37:17]

Hi,
I haven't looked at that code in a long time.
Wan-Teh might know a better person to look at this.
Let me know if he doesn't in which case I could take a look.

Jay

On Tue, Jun 26, 2012 at 7:50 AM, <toyoshim@chromium.org> wrote:

> Reviewers: Jay Civelli,
>
> Message:
> Hi Jay, could you look this change to support client cert end to end test?
>
> Description:
> - add client cert authentication support in test websocket server
> - add new browser test which access to a page which requires client cert
>
> BUG=none
> TEST=browser_tests
>
> Please review this at
https://chromiumcodereview.**appspot.com/10541168/<https://chromiumcodereview...
>
> SVN Base:
svn://svn.chromium.org/chrome/**trunk/src<http://svn.chromium.org/chrome/trunk/src>
>
> Affected files:
>  M chrome/browser/ssl/ssl_**browser_tests.cc
>  M chrome/test/base/ui_test_**utils.h
>  M chrome/test/base/ui_test_**utils.cc
>  A chrome/test/data/cacert.pem
>  A chrome/test/data/client_cert.**p12
>
>
>

[Takashi Toyoshima, 2012-06-27 03:29:59]

Jay,

OK, I ask Wan-Teh.

Hi Wan-Teh,

Could you look this change to introduce an end to end cert auth test for wss?
As you suggested, I use CertDatabase to import a client cert for test.
Unfortunately, automation of handling client cert selector dialog looks
difficult for now.
Then, I use the policy list feature to select imported cert automatically
without any UI interaction.
http://dev.chromium.org/administrators/policy-list-3#AutoSelectCertificateFor...

Thanks,

[Takashi Toyoshima, 2012-06-27 03:38:48]

Oops, I've just remembered that Wan-Teh is now in vacation.
But, this is not urgent change. I'll wait for a while.

[Takashi Toyoshima, 2012-07-02 08:12:07]

Hum... try fails because of missing client_cert.p12?
Should I need some special instruction for importing a binary file in try?

--- from the log of 'update' phase ---
/usr/bin/patch -p0 --remove-empty-files --force --forward
 in dir /mnt/data/b/build/slave/linux/build/src (timeout 300 secs)
 watching logfiles {}
 argv: ['/usr/bin/patch', '-p0', '--remove-empty-files', '--force', '--forward']
 environment:
  CHROME_HEADLESS=1
  CHROMIUM_GYP_SYNTAX_CHECK=1
  DEPOT_TOOLS_UPDATE=0
  DISPLAY=:0.0
  GYP_DEFINES=fastbuild=1 dcheck_always_on=1 component=static_library
  GYP_GENERATOR_FLAGS=
  HOME=/home/chrome-bot
  LANG=en_US.UTF-8
  LOGNAME=chrome-bot
  PAGER=cat
 
PATH=/mnt/data/b/depot_tools:/usr/bin:/usr/bin:/bin:/usr/sbin:/sbin:/usr/local/bin
  PWD=/mnt/data/b/build/slave/linux/build/src
 
PYTHONPATH=/mnt/data/b/build/site_config:/mnt/data/b/build/scripts:/mnt/data/b/build/scripts/release:/mnt/data/b/build/third_party:/mnt/data/b/build_internal/site_config:/mnt/data/b/build_internal/symsrc:/mnt/data/b/build/slave:/mnt/data/b/build/third_party/buildbot_7_12:/mnt/data/b/build/third_party/twisted_8_1:
  SHELL=/bin/bash
  USER=chrome-bot
 writing 7645 bytes to stdin
 closing stdin
 using PTY: False
patching file chrome/browser/ssl/ssl_browser_tests.cc
patching file chrome/test/base/ui_test_utils.cc
patching file chrome/test/base/ui_test_utils.h
patching file chrome/test/data/cacert.pem

[wtc, 2012-07-10 22:08:10]

On 2012/07/02 08:12:07, toyoshim wrote:
> Hum... try fails because of missing client_cert.p12?
> Should I need some special instruction for importing a binary file in try?

Try servers cannot handle new binary files.  To work around
this limitation, you have to commit the new binary files first,
in a separate CL.  So, you can create a separate CL for
chrome/test/data/client_cert.p12 and commit it first.

NOTE: chrome/test/data has several subdirectories.  You may want
to add cacert.pem and client_cert.p12 to the existing
chrome/test/data/ssl subdirectory or a new subdirectory for wss.

[Takashi Toyoshima, 2012-07-11 07:52:00]

Thanks.
Now I made a separated change to add the p12 file.

https://chromiumcodereview.appspot.com/10702140/

Patch Set 7 uses chrome/test/data/ssl as a data file path, and has a dependency
on that change.

[wtc, 2012-07-11 21:35:57]

Patch set 7 LGTM.

https://chromiumcodereview.appspot.com/10541168/diff/17003/chrome/browser/ssl...
File chrome/browser/ssl/ssl_browser_tests.cc (right):

https://chromiumcodereview.appspot.com/10541168/diff/17003/chrome/browser/ssl...
chrome/browser/ssl/ssl_browser_tests.cc:609: // cert will be selected
automatically, then a test whici uses WebSocket runs.

Typo: whici => which

https://chromiumcodereview.appspot.com/10541168/diff/17003/chrome/browser/ssl...
chrome/browser/ssl/ssl_browser_tests.cc:626: // Start pywebsocket with TLS and
client certs.

Nit: client certs => client cert authentication

Saying "client certs" may be misinterpreted as pywebsocket
having client certs, but it is the clients that have client
certs.

https://chromiumcodereview.appspot.com/10541168/diff/17003/chrome/browser/ssl...
chrome/browser/ssl/ssl_browser_tests.cc:664: // Test page run a WebSocket wss
connection test. The result will be shown

Typo: run => runs ?

https://chromiumcodereview.appspot.com/10541168/diff/17003/chrome/test/base/u...
File chrome/test/base/ui_test_utils.cc (right):

https://chromiumcodereview.appspot.com/10541168/diff/17003/chrome/test/base/u...
chrome/test/base/ui_test_utils.cc:897: secure_ = true;

Nit: this method probably should not set secure_ to true.

client_authentication_ does require secure_, but it seems
better to require them to be set separately.

https://chromiumcodereview.appspot.com/10541168/diff/17003/chrome/test/base/u...
chrome/test/base/ui_test_utils.cc:918: FilePath cacert_path(root_directory);

The declaration of the cacert_path variable can be moved
inside the "if (client_authentication_)" block.

https://chromiumcodereview.appspot.com/10541168/diff/17003/chrome/test/base/u...
chrome/test/base/ui_test_utils.cc:920: cmd_line->AppendArg("--ca-certificate");

It is strange that the --ca-certificate option alone causes
the test server to request client authentication.

https://chromiumcodereview.appspot.com/10541168/diff/17003/chrome/test/base/u...
File chrome/test/base/ui_test_utils.h (right):

https://chromiumcodereview.appspot.com/10541168/diff/17003/chrome/test/base/u...
chrome/test/base/ui_test_utils.h:363: // Serves with client cert authentication.

Nit: this comment should say:
  // Requests client cert authentication.

Please make a similar change to the comment on line 404.

[Takashi Toyoshima, 2012-07-12 05:20:13]

Thanks.
Also I added BUG number which is the same with one of preceding CL.

https://chromiumcodereview.appspot.com/10541168/diff/17003/chrome/browser/ssl...
File chrome/browser/ssl/ssl_browser_tests.cc (right):

https://chromiumcodereview.appspot.com/10541168/diff/17003/chrome/browser/ssl...
chrome/browser/ssl/ssl_browser_tests.cc:609: // cert will be selected
automatically, then a test whici uses WebSocket runs.
On 2012/07/11 21:35:57, wtc wrote:
> 
> Typo: whici => which

Done.

https://chromiumcodereview.appspot.com/10541168/diff/17003/chrome/browser/ssl...
chrome/browser/ssl/ssl_browser_tests.cc:626: // Start pywebsocket with TLS and
client certs.
On 2012/07/11 21:35:57, wtc wrote:
> 
> Nit: client certs => client cert authentication
> 
> Saying "client certs" may be misinterpreted as pywebsocket
> having client certs, but it is the clients that have client
> certs.

Done.

https://chromiumcodereview.appspot.com/10541168/diff/17003/chrome/browser/ssl...
chrome/browser/ssl/ssl_browser_tests.cc:664: // Test page run a WebSocket wss
connection test. The result will be shown
On 2012/07/11 21:35:57, wtc wrote:
> 
> Typo: run => runs ?

Done.

https://chromiumcodereview.appspot.com/10541168/diff/17003/chrome/test/base/u...
File chrome/test/base/ui_test_utils.cc (right):

https://chromiumcodereview.appspot.com/10541168/diff/17003/chrome/test/base/u...
chrome/test/base/ui_test_utils.cc:897: secure_ = true;
On 2012/07/11 21:35:57, wtc wrote:
> 
> Nit: this method probably should not set secure_ to true.
> 
> client_authentication_ does require secure_, but it seems
> better to require them to be set separately.

Done.

https://chromiumcodereview.appspot.com/10541168/diff/17003/chrome/test/base/u...
chrome/test/base/ui_test_utils.cc:918: FilePath cacert_path(root_directory);
On 2012/07/11 21:35:57, wtc wrote:
> 
> The declaration of the cacert_path variable can be moved
> inside the "if (client_authentication_)" block.

Done.

https://chromiumcodereview.appspot.com/10541168/diff/17003/chrome/test/base/u...
chrome/test/base/ui_test_utils.cc:920: cmd_line->AppendArg("--ca-certificate");
Currently, just adding '--ca-certificate <filename>' enables client cert
authentication in pywebsocket.
If you feel this is strange, I will fix this argument style in pywebsocket and
here.
So, how about adding '--request-client-cert-authentication' in addition to
'--ca-certificate'?

FYI, pywebsocket options for TLS follow,
Usage: standalone.py [options]

Options:
(snip)
  -t, --tls             use TLS (wss://)
  -k PRIVATE_KEY, --private-key=PRIVATE_KEY, --private_key=PRIVATE_KEY
                        TLS private key file.
  -c CERTIFICATE, --certificate=CERTIFICATE
                        TLS certificate file.
  --ca-certificate=CA_CERTIFICATE
                        TLS CA certificate file for client authentication.
(snip)

https://chromiumcodereview.appspot.com/10541168/diff/17003/chrome/test/base/u...
File chrome/test/base/ui_test_utils.h (right):

https://chromiumcodereview.appspot.com/10541168/diff/17003/chrome/test/base/u...
chrome/test/base/ui_test_utils.h:363: // Serves with client cert authentication.
On 2012/07/11 21:35:57, wtc wrote:
> 
> Nit: this comment should say:
>   // Requests client cert authentication.
> 
> Please make a similar change to the comment on line 404.

Done.

[commit-bot: I haz the power, 2012-07-12 05:20:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/toyoshim@chromium.org/10541168/16009

[commit-bot: I haz the power, 2012-07-12 06:47:14]

Change committed as 146294

[wtc, 2012-07-12 23:10:48]

https://chromiumcodereview.appspot.com/10541168/diff/17003/chrome/test/base/u...
File chrome/test/base/ui_test_utils.cc (right):

https://chromiumcodereview.appspot.com/10541168/diff/17003/chrome/test/base/u...
chrome/test/base/ui_test_utils.cc:920: cmd_line->AppendArg("--ca-certificate");

Thank you for the info.  I suggest that you use two separate
options for requesting client auth and for specifying the
CA certificate for client auth.

--ca-certificate should be renamed to indicate that it is
for client certificates.  I suggest you imitate the option
names of net/tools/testserver/testserver.py:

  option_parser.add_option('', '--ssl-client-auth', action='store_true',
                           help='Require SSL client auth on every connection.')
  option_parser.add_option('', '--ssl-client-ca', action='append', default=[],
                           help='Specify that the client certificate request '
                           'should include the CA named in the subject of '
                           'the DER-encoded certificate contained in the '
                           'specified file. This option may appear multiple '
                           'times, indicating multiple CA names should be '
                           'sent in the request.')

[Takashi Toyoshima, 2012-07-13 11:37:54]

Thank you.
I'll use --tls-client-* instead of --ssl-client-* because pywebsocket use a term
tls for other SSL related options. Is it acceptable difference?

[wtc, 2012-07-13 19:45:39]

On 2012/07/13 11:37:54, toyoshim wrote:
>
> I'll use --tls-client-* instead of --ssl-client-* because pywebsocket use a
term
> tls for other SSL related options. Is it acceptable difference?

Using "tls" instead of "ssl" in the comment line option names is fine.