Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10703189:
Add end to end client cert auth test for wss (Closed)
|
Created:
8 years, 5 months ago by Takashi Toyoshima Modified:
8 years, 2 months ago CC:
chromium-reviews, Ryan Sleevi Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd 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 --gtest_filter='SSLUITest.TestWSSClientCert'
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=162916
Patch Set 1 #Patch Set 2 : add cacert.pem #
Total comments: 9
Patch Set 3 : reflects review comments #Patch Set 4 : rebase #Patch Set 5 : try #Patch Set 6 : for review #
Total comments: 8
Patch Set 7 : 3) #
Total comments: 4
Patch Set 8 : revise #Patch Set 9 : rebase #Messages
Total messages: 20 (0 generated)
This is a very high-level test. While I understand wanting to test end-to-end, it seems like there should also be tests for the lower-layer parts of the stack, if at all possible. Or is the WebSocket implementation now fully within WebKit, and this is the only way to test? I think this should be safe to re-land, but some nits below: https://chromiumcodereview.appspot.com/10703189/diff/2001/chrome/browser/ssl/... File chrome/browser/ssl/ssl_browser_tests.cc (right): https://chromiumcodereview.appspot.com/10703189/diff/2001/chrome/browser/ssl/... chrome/browser/ssl/ssl_browser_tests.cc:608: #if defined(USE_NSS) nit: Add an explanation for why this is only for USE_NSS nit: // SSL client certificate tests are only enabled when using // NSS for private key storage, as only NSS can avoid // modifying global machine state when testing. // See http://crbug.com/51132 https://chromiumcodereview.appspot.com/10703189/diff/2001/chrome/browser/ssl/... chrome/browser/ssl/ssl_browser_tests.cc:614: // See also crypto/nss_util.h and http://crbug.com/136950#c5 . nit: // Open a temporary NSS DB for testing. // TODO(toyoshim): This currently intentionally leaks the // test database due to bugs within NSS. Once fixed, this // should be a scoped test database. See http://crbug.com/136950#c5 // for more details. https://chromiumcodereview.appspot.com/10703189/diff/2001/chrome/browser/ssl/... chrome/browser/ssl/ssl_browser_tests.cc:633: ui_test_utils::TestWebSocketServer wss_server; A random note (and not a pre-requisite for this CL), I find it odd that TestWebSocketServer does not follow the same model as the net::TestServer used extensively elsewhere. Specifically: 1) The net::TestServer is immutable once constructed, save for start/stop, to avoid tests getting in an inconsistent state (635-636) 2) Ports are /always/ randomized. All tests should be shardable. (634) 3) Utility function for building URLs based on the test servers' address and port (640-642) Just an inconsistent interface between these two core testing bits. https://chromiumcodereview.appspot.com/10703189/diff/2001/chrome/browser/ssl/... chrome/browser/ssl/ssl_browser_tests.cc:665: false, true); // Interstitial showing Since this is a browser test, you can/should be using TestRootCerts. The net::TestServer does this automatically when starting and stopping. By doing so, you're able to disable interstitials from even happening - the browser truly believes that the cert is trusted. https://chromiumcodereview.appspot.com/10703189/diff/2001/chrome/test/data/ss... File chrome/test/data/ssl/cacert.pem (right): https://chromiumcodereview.appspot.com/10703189/diff/2001/chrome/test/data/ss... chrome/test/data/ssl/cacert.pem:1: -----BEGIN CERTIFICATE----- As a general note, when using PEM certificates, I tend to request that they also have the human-friendly information in them as well (that is, the result of 'openssl x509 -text -inform pem -in cacert.pem -out cacert.pem') The benefit is that it means these certs will be indexed and searchable by both code search and existing tools, and that users can quickly view the source to understand problems (such as cert expirations)
Thank you for review. WebSocket has many high level tests as WebKit layout tests. But there we don't have a way to mimic UI action, or to use client cert for testing. On the other hands, we also have unit tests in net_unittests. But WebSocket, especially around SSL handling, codes are spreaded over many components. So this kind of unit tests are not enough. Thus I implement this test in browser tests. In browser tests, we can test cooperation of all components, and also we can inject hacking codes to import certs, and to mimic UI actions. https://chromiumcodereview.appspot.com/10703189/diff/2001/chrome/browser/ssl/... File chrome/browser/ssl/ssl_browser_tests.cc (right): https://chromiumcodereview.appspot.com/10703189/diff/2001/chrome/browser/ssl/... chrome/browser/ssl/ssl_browser_tests.cc:608: #if defined(USE_NSS) On 2012/07/13 19:25:06, Ryan Sleevi wrote: > nit: Add an explanation for why this is only for USE_NSS > > nit: > // SSL client certificate tests are only enabled when using > // NSS for private key storage, as only NSS can avoid > // modifying global machine state when testing. > // See http://crbug.com/51132 Done. https://chromiumcodereview.appspot.com/10703189/diff/2001/chrome/browser/ssl/... chrome/browser/ssl/ssl_browser_tests.cc:614: // See also crypto/nss_util.h and http://crbug.com/136950#c5 . On 2012/07/13 19:25:06, Ryan Sleevi wrote: > nit: > // Open a temporary NSS DB for testing. > // TODO(toyoshim): This currently intentionally leaks the > // test database due to bugs within NSS. Once fixed, this > // should be a scoped test database. See http://crbug.com/136950#c5 > // for more details. Done. https://chromiumcodereview.appspot.com/10703189/diff/2001/chrome/browser/ssl/... chrome/browser/ssl/ssl_browser_tests.cc:633: ui_test_utils::TestWebSocketServer wss_server; I filed a bug on this as a TODO. https://code.google.com/p/chromium/issues/detail?id=137639 https://chromiumcodereview.appspot.com/10703189/diff/2001/chrome/browser/ssl/... chrome/browser/ssl/ssl_browser_tests.cc:665: false, true); // Interstitial showing Currently, we use cert files in WebKit source code repos, because the test server is launched via a WebKit test infrastructure (new-run-webkit-websocket-server). I should remove dependency on WebKit to use TestRootCerts here. I'll fix TestWebSocketServer to share server implementation for HTTPS.
Ping? Is this still active?
Hi, Ryan. Sorry for being lazy on this change. I'd like to submit this change after pywebsocket relocation because using TestRootCerts is a little difficult to current situation where pywebsocket uses server certs provided by WebKit. We talked about pywebsocket relocation in the following thread. http://code.google.com/p/chromium/issues/detail?id=137639 So, can I start to import pywebsocket repository?
On 2012/08/09 08:54:00, toyoshim wrote: > Hi, Ryan. > Sorry for being lazy on this change. > > I'd like to submit this change after pywebsocket relocation because using > TestRootCerts is a little difficult to current situation where pywebsocket uses > server certs provided by WebKit. > > We talked about pywebsocket relocation in the following thread. > http://code.google.com/p/chromium/issues/detail?id=137639 > > So, can I start to import pywebsocket repository? I think that's fine, with the caveats from http://code.google.com/p/chromium/issues/detail?id=137639#c5 Perhaps discussing on chromium-dev might be good, to make sure those more familiar with the Webkit checkouts can comment about the dependencies - eg: if there is a (safe) way to avoid two checkouts. However, I don't think there is.
On 2012/08/09 18:36:46, Ryan Sleevi wrote: > On 2012/08/09 08:54:00, toyoshim wrote: > > Hi, Ryan. > > Sorry for being lazy on this change. > > > > I'd like to submit this change after pywebsocket relocation because using > > TestRootCerts is a little difficult to current situation where pywebsocket > uses > > server certs provided by WebKit. > > > > We talked about pywebsocket relocation in the following thread. > > http://code.google.com/p/chromium/issues/detail?id=137639 > > > > So, can I start to import pywebsocket repository? > > I think that's fine, with the caveats from > http://code.google.com/p/chromium/issues/detail?id=137639#c5 > > Perhaps discussing on chromium-dev might be good, to make sure those more > familiar with the Webkit checkouts can comment about the dependencies - eg: if > there is a (safe) way to avoid two checkouts. However, I don't think there is. Ping? It sounds like you've completed most of the pywebsocket changes, right?
Sorry, it's not finished. My last change to integrate pywebsocket into net::TestServer is reverted because of an issue on cros test infra and needs some investigation. I'm currently working on an urgent task and suspend this integration effort. But, I'll be back soon. Thank you for pinging me.
Hi Ryan, Finally, it's time to update this CL. Could you review Patch Set 6? I post try job on Patch Set 5 because Set 6 replaces a binary file, client_cert.p12. Set 5 is the same with Set 6 except for pointing old client_cert.p12 file. This CL depends on browser_tests migration CL you are reviewing, so will be landed after that. Thanks.
Suggestion: Consider breaking this up into three CLs - two of which you've already made 1) One CL to move the file from chrome/test/data into net/data (and update the test in chrome/browser/ssl to refer to the new path). Feel free to TBR that 2) The CL to update the test server (which you seem to have duplicated in other CLs) 3) The CL to do the unit test (aka this CL) Note that I'm not an appropriate reviewer here (not in chrome/browser/net/OWNERS), and you'll want phajdan.jr to review the CL #2. I've suggested wtc as a reviewer for you once you update this to just be the unit test update. https://chromiumcodereview.appspot.com/10703189/diff/16004/chrome/browser/ssl... File chrome/browser/ssl/ssl_browser_tests.cc (right): https://chromiumcodereview.appspot.com/10703189/diff/16004/chrome/browser/ssl... chrome/browser/ssl/ssl_browser_tests.cc:47: #endif // defined(USE_NSS) Please follow Chromium style for platform specific headers http://dev.chromium.org/developers/coding-style#TOC-Platform-specific-code This should be moved after line 48 (and with a newline between the blocks ending 48 and starting at 50) https://chromiumcodereview.appspot.com/10703189/diff/16004/chrome/browser/ssl... chrome/browser/ssl/ssl_browser_tests.cc:637: ASSERT_TRUE(crypto::OpenTestNSSDB()); Aside: Wondering if this should be a scoped RAII-like structure for now, so there's only a single place to fix once we require newer builds of NSS (I think 3.13.4?) https://chromiumcodereview.appspot.com/10703189/diff/16004/net/data/websocket... File net/data/websocket/cacert.pem (right): https://chromiumcodereview.appspot.com/10703189/diff/16004/net/data/websocket... net/data/websocket/cacert.pem:10: Not After : Oct 23 07:24:33 2039 GMT note: One suggestion would be to keep it < 2038, to avoid Y2038 issues with base::Time values on 32-bit systems. https://chromiumcodereview.appspot.com/10703189/diff/16004/net/test/base_test... File net/test/base_test_server.cc (right): https://chromiumcodereview.appspot.com/10703189/diff/16004/net/test/base_test... net/test/base_test_server.cc:377: if (type_ == TYPE_HTTPS) { Seems you have duplicated this change on other CLs. Seems like those should land first?
Thanks. I submit CL 1) as https://chromiumcodereview.appspot.com/11154032 with following fix. https://chromiumcodereview.appspot.com/10703189/diff/16004/net/data/websocket... File net/data/websocket/cacert.pem (right): https://chromiumcodereview.appspot.com/10703189/diff/16004/net/data/websocket... net/data/websocket/cacert.pem:10: Not After : Oct 23 07:24:33 2039 GMT OK. I remake CA and client certs which is valid for 7500 days.
2) https://codereview.chromium.org/11175002/ This CL contains all update on testserver.py and base_test_server.cc for this migration. https://chromiumcodereview.appspot.com/10703189/diff/16004/net/test/base_test... File net/test/base_test_server.cc (right): https://chromiumcodereview.appspot.com/10703189/diff/16004/net/test/base_test... net/test/base_test_server.cc:377: if (type_ == TYPE_HTTPS) { OK, I split changes on testserver.py and base_test_server.cc to another CL.
Hi, Ryan. I split ScopedTestNSSDB related change to https://codereview.chromium.org/11174006/ . https://chromiumcodereview.appspot.com/10703189/diff/16004/chrome/browser/ssl... File chrome/browser/ssl/ssl_browser_tests.cc (right): https://chromiumcodereview.appspot.com/10703189/diff/16004/chrome/browser/ssl... chrome/browser/ssl/ssl_browser_tests.cc:47: #endif // defined(USE_NSS) On 2012/10/15 17:04:52, Ryan Sleevi wrote: > Please follow Chromium style for platform specific headers > > http://dev.chromium.org/developers/coding-style#TOC-Platform-specific-code > > This should be moved after line 48 (and with a newline between the blocks ending > 48 and starting at 50) Done. https://chromiumcodereview.appspot.com/10703189/diff/16004/chrome/browser/ssl... chrome/browser/ssl/ssl_browser_tests.cc:637: ASSERT_TRUE(crypto::OpenTestNSSDB()); OK, I try it in another CL firstly.
Patch set 7 LGTM. Thanks! https://chromiumcodereview.appspot.com/10703189/diff/33001/chrome/browser/ssl... File chrome/browser/ssl/ssl_browser_tests.cc (right): https://chromiumcodereview.appspot.com/10703189/diff/33001/chrome/browser/ssl... chrome/browser/ssl/ssl_browser_tests.cc:636: // to bugs within NSS. Once fixed, this should be a scoped test database. Please replace "Once fixed" to "Once we require NSS 3.14" or "Once NSS 3.14 is widely available", because rsleevi has fixed this bug (in NSS 3.14). https://chromiumcodereview.appspot.com/10703189/diff/33001/chrome/browser/ssl... chrome/browser/ssl/ssl_browser_tests.cc:647: FILE_PATH_LITERAL("net/data/websocket/client_cert.p12")); It seems that you can put this client_cert.p12 file and the cacert.pem file under net/data/ssl/certificates? Would that be confusing?
I moved cert files. https://codereview.chromium.org/11192028 I'll land this once dependent CLs are submitted. https://chromiumcodereview.appspot.com/10703189/diff/33001/chrome/browser/ssl... File chrome/browser/ssl/ssl_browser_tests.cc (right): https://chromiumcodereview.appspot.com/10703189/diff/33001/chrome/browser/ssl... chrome/browser/ssl/ssl_browser_tests.cc:636: // to bugs within NSS. Once fixed, this should be a scoped test database. Thanks. Ryan suggested me to implement ScopedTestNSSDB and now it's ready for submission. I'll replace this with ScopedTestNSSDB, and remove this TODO away. https://chromiumcodereview.appspot.com/10703189/diff/33001/chrome/browser/ssl... chrome/browser/ssl/ssl_browser_tests.cc:647: FILE_PATH_LITERAL("net/data/websocket/client_cert.p12")); OK. I replace client_cert.p12 and cacert.pem to net/data/ssl/certificates. Also I'll update READ there. I think I should append some prefix to them, like websocket_cacert.pem.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/toyoshim@chromium.org/10703189/40002
Presubmit check for 10703189-40002 failed and returned exit status 1.
Running presubmit commit checks ...
** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
chrome/browser/ssl
Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!
+abarth for chrome/browser/ssl stamp. Could you take a look?
LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/toyoshim@chromium.org/10703189/40002
Change committed as 162916 |
