Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
394013005:
Remove NSSCertDatabase from ClientCertStoreChromeOS unittest. (Closed)
|
Created:
6 years, 5 months ago by pneubeck (no reviews) Modified:
6 years, 5 months ago Reviewers:
Ryan Sleevi CC:
chromium-reviews, cbentzel+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionRemove NSSCertDatabase from ClientCertStoreChromeOS unittest.
The database was only used to import a PKCS#12 file. By changing to separate key (PKCS#8 format) and cert (X509 in PEM encoding), only dependencies on the lower level RSAPrivateKey, X509Certificate and PK11_* NSS functions are required.
Note this removes at the same time a call to the deprecated NSSCertDatabase::GetInstance().
Also
- fixes multi profile cases of the unit test and the CA matching (the latter is now identical to all other platforms).
- fixes a bug in the matching of client certs from software slots, because of reused cert database names
- gets rid of the error output that occurred during the PKCS12 import because the file contained also a CA cert:
[ERROR:nsPKCS12Blob.cpp(219)] Could not grab a handle to the certificate in the slot from the corresponding PKCS#12 DER certificate.
BUG=210525, 329735, 315285
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=284056
Patch Set 1 : #
Total comments: 20
Patch Set 2 : Addressed comments. Cleaned up the unit test further. #Patch Set 3 : Broke binary files into separate CL. Rebased. #
Total comments: 5
Patch Set 4 : Fixed gyp file. #
Total comments: 17
Patch Set 5 : Addressed comments. #
Messages
Total messages: 23 (0 generated)
ptal https://codereview.chromium.org/394013005/diff/40001/crypto/nss_util.cc File crypto/nss_util.cc (right): https://codereview.chromium.org/394013005/diff/40001/crypto/nss_util.cc#newco... crypto/nss_util.cc:60: const char kNSSDatabaseName[] = "Real NSS db"; I reduced the length of this string because the nickname was truncated on debug outputs. Not sure why.
Pretty sure you also need to update the .GN files. https://codereview.chromium.org/394013005/diff/40001/crypto/nss_util.cc File crypto/nss_util.cc (right): https://codereview.chromium.org/394013005/diff/40001/crypto/nss_util.cc#newco... crypto/nss_util.cc:60: const char kNSSDatabaseName[] = "Real NSS db"; On 2014/07/16 10:08:25, pneubeck wrote: > I reduced the length of this string because the nickname was truncated on debug > outputs. Not sure why. pedantry: db -> DB Nicknames have a fixed length. Could this be why? https://codereview.chromium.org/394013005/diff/40001/net/ssl/client_cert_stor... File net/ssl/client_cert_store_chromeos_unittest.cc (right): https://codereview.chromium.org/394013005/diff/40001/net/ssl/client_cert_stor... net/ssl/client_cert_store_chromeos_unittest.cc:46: << cert_filename; Note: This is generally a dangerous pattern to put EXPECT clauses within a sub-routine, since they can lead to subtle error when someone makes them ASSERT clauses (e.g. https://code.google.com/p/googletest/wiki/AdvancedGuide#Propagating_Fatal_Fai... ) https://codereview.chromium.org/394013005/diff/40001/net/ssl/client_cert_stor... net/ssl/client_cert_store_chromeos_unittest.cc:101: ASSERT_EQ(1u, request_1->client_certs.size()); Why... is this? Seems wrong to update the test expectation here. https://codereview.chromium.org/394013005/diff/40001/net/test/cert_test_util.h File net/test/cert_test_util.h (right): https://codereview.chromium.org/394013005/diff/40001/net/test/cert_test_util.... net/test/cert_test_util.h:16: #include "crypto/rsa_private_key.h" Forward declare RSAPrivateKey. The caller of this function will then include the header when they call this function. See http://www.chromium.org/developers/coding-style/cpp-dos-and-donts
https://codereview.chromium.org/394013005/diff/40001/net/ssl/client_cert_stor... File net/ssl/client_cert_store_chromeos_unittest.cc (right): https://codereview.chromium.org/394013005/diff/40001/net/ssl/client_cert_stor... net/ssl/client_cert_store_chromeos_unittest.cc:46: << cert_filename; On 2014/07/16 19:32:18, Ryan Sleevi wrote: > Note: This is generally a dangerous pattern to put EXPECT clauses within a > sub-routine, since they can lead to subtle error when someone makes them ASSERT > clauses (e.g. > https://code.google.com/p/googletest/wiki/AdvancedGuide#Propagating_Fatal_Fai... > ) I followed the existing style. I'd actually would prefer CHECKs, but these are also controversial in tests. So, I'll just remove the EXPECTs and ASSERT at the call site. https://codereview.chromium.org/394013005/diff/40001/net/ssl/client_cert_stor... net/ssl/client_cert_store_chromeos_unittest.cc:101: ASSERT_EQ(1u, request_1->client_certs.size()); On 2014/07/16 19:32:18, Ryan Sleevi wrote: > Why... is this? Seems wrong to update the test expectation here. Because they were wrong? :-) See Matt's comment above. Note that I didn't change it to 2, i.e. one of the certs is not matched. Previously no cert or both certs were matched which is not proving that the matching is actually working. https://codereview.chromium.org/394013005/diff/40001/net/test/cert_test_util.h File net/test/cert_test_util.h (right): https://codereview.chromium.org/394013005/diff/40001/net/test/cert_test_util.... net/test/cert_test_util.h:16: #include "crypto/rsa_private_key.h" On 2014/07/16 19:32:18, Ryan Sleevi wrote: > Forward declare RSAPrivateKey. > > The caller of this function will then include the header when they call this > function. > > See http://www.chromium.org/developers/coding-style/cpp-dos-and-donts Ops. Will change.
https://codereview.chromium.org/394013005/diff/40001/net/ssl/client_cert_stor... File net/ssl/client_cert_store_chromeos_unittest.cc (right): https://codereview.chromium.org/394013005/diff/40001/net/ssl/client_cert_stor... net/ssl/client_cert_store_chromeos_unittest.cc:46: << cert_filename; On 2014/07/16 19:49:44, pneubeck wrote: > On 2014/07/16 19:32:18, Ryan Sleevi wrote: > > Note: This is generally a dangerous pattern to put EXPECT clauses within a > > sub-routine, since they can lead to subtle error when someone makes them > ASSERT > > clauses (e.g. > > > https://code.google.com/p/googletest/wiki/AdvancedGuide#Propagating_Fatal_Fai... > > ) > I followed the existing style. > I'd actually would prefer CHECKs, but these are also controversial in tests. > > So, I'll just remove the EXPECTs and ASSERT at the call site. I think it's fine, but it means that you need to wrap these calls in the ASSERT_NO_FATAL_FAILURES macro, in order to prevent surprises. https://codereview.chromium.org/394013005/diff/40001/net/ssl/client_cert_stor... net/ssl/client_cert_store_chromeos_unittest.cc:101: ASSERT_EQ(1u, request_1->client_certs.size()); On 2014/07/16 19:49:44, pneubeck wrote: > On 2014/07/16 19:32:18, Ryan Sleevi wrote: > > Why... is this? Seems wrong to update the test expectation here. > > Because they were wrong? :-) > See Matt's comment above. > > Note that I didn't change it to 2, i.e. one of the certs is not matched. > Previously no cert or both certs were matched which is not proving that the > matching is actually working. I don't know what comment you're referring to.
https://codereview.chromium.org/394013005/diff/40001/net/ssl/client_cert_stor... File net/ssl/client_cert_store_chromeos_unittest.cc (right): https://codereview.chromium.org/394013005/diff/40001/net/ssl/client_cert_stor... net/ssl/client_cert_store_chromeos_unittest.cc:101: ASSERT_EQ(1u, request_1->client_certs.size()); On 2014/07/16 19:59:00, Ryan Sleevi wrote: > On 2014/07/16 19:49:44, pneubeck wrote: > > On 2014/07/16 19:32:18, Ryan Sleevi wrote: > > > Why... is this? Seems wrong to update the test expectation here. > > > > Because they were wrong? :-) > > See Matt's comment above. > > > > Note that I didn't change it to 2, i.e. one of the certs is not matched. > > Previously no cert or both certs were matched which is not proving that the > > matching is actually working. > > I don't know what comment you're referring to. Line 54 of the old file: // TODO(mattm): Do better testing of cert_authorities matching below. [...]
> Line 54 of the old file: > // TODO(mattm): Do better testing of cert_authorities matching below. [...] Rather than cheat and gchat, it would be better for this review to explain further, since I'm obviously confused. Hopefully put simply: Why is 1 now the correct answer, instead of 0? What of this change necessitates updating that expectation? How do we know that's the right number, than the wrong number. Any of those explanations will help understand the context :)
https://codereview.chromium.org/394013005/diff/40001/net/ssl/client_cert_stor... File net/ssl/client_cert_store_chromeos_unittest.cc (right): https://codereview.chromium.org/394013005/diff/40001/net/ssl/client_cert_stor... net/ssl/client_cert_store_chromeos_unittest.cc:101: ASSERT_EQ(1u, request_1->client_certs.size()); That's fair. My reasoning was: Matt commented that something about the CA matching is imperfect. The naming of the variable is following the patter *_1, *_2 and *_all where * is either a client cert, an authority, or a request. This indicates that Matt's original intention was that request_all should match both client certs and request_1 should match only cert_1. Independent from the author's intent, the question is what this unit test should test. IMO, it should verify that ClientCertStoreChromeOS is correctly delegating the CA matching to the base class ClientCertStoreNSS. One 'heuristic' could be to ensure that neither simply all certs are accepted or rejected. This can be ensured by testing a request that matches exactly 1 out of 2 client certs. And that would actually match the intent that I read from the existing test. The only thing that doesn't match is the old test assertion in line 90: ASSERT_EQ(0u, request_1->client_certs.size());
https://codereview.chromium.org/394013005/diff/40001/net/ssl/client_cert_stor... File net/ssl/client_cert_store_chromeos_unittest.cc (right): https://codereview.chromium.org/394013005/diff/40001/net/ssl/client_cert_stor... net/ssl/client_cert_store_chromeos_unittest.cc:101: ASSERT_EQ(1u, request_1->client_certs.size()); I guess this highlights poor documentation. I don't reach your same conclusions, though I understand how you got them. That is, I agree that kAuthority1DN is meant to correlate to the authority requested in cert_1. What's not clear to me is that the cert is intended to be returned. In particular, the only difference I see between the two tests is user.FinishInit() (line 108) The better explanation (it seems) from this is that it's an artifact of you changing the certs being tested themselves. In the old code, neither client.p12 nor websocket_client_cert.p12 matched the request. Is this true? https://codereview.chromium.org/394013005/diff/40001/net/ssl/client_cert_stor... net/ssl/client_cert_store_chromeos_unittest.cc:122: sizeof(kAuthority1DN))); Why not update this to match what you did above?
https://codereview.chromium.org/394013005/diff/40001/net/ssl/client_cert_stor... File net/ssl/client_cert_store_chromeos_unittest.cc (right): https://codereview.chromium.org/394013005/diff/40001/net/ssl/client_cert_stor... net/ssl/client_cert_store_chromeos_unittest.cc:101: ASSERT_EQ(1u, request_1->client_certs.size()); On 2014/07/16 20:46:07, Ryan Sleevi wrote: > I guess this highlights poor documentation. > > I don't reach your same conclusions, though I understand how you got them. That > is, I agree that kAuthority1DN is meant to correlate to the authority requested > in cert_1. > > What's not clear to me is that the cert is intended to be returned. > > In particular, the only difference I see between the two tests is > user.FinishInit() (line 108) Ok. Let me explain that part then too. Another thing that should be tested in this unit test is the asynchronous loading of the private slot (using crypto::GetPrivateSlotForChromeOSUser). Two possible control flows can occur: a) the slot is obtained synchronously b) the slot is obtained asynchronously via callback (note: this difference wouldn't have to be tested here, if GetPrivateSlotForChromeOSUser would only expose one way of returning the result) The first test seems to test b), as indicated by the test name WaitForNSSInit, the call order GetClientCerts() then user.FinishInit(), and the comment in line 95: // Callbacks won't be run until nss_util init finishes for the user. The second test seems to test a), as indicate by the test name NSSAlreadyInitialized and the call order user.FinishInit() then GetClientCerts(). If this were the only purpose of the these two tests (i.e. assuming my previous reasoning would be wrong), then it would be sufficient to have a single request per test. But there are two requests, which again support my previous reasoning that it was intended to test the delegation to ClientCertStoreNSS. > The better explanation (it seems) from this is that it's an artifact of you > changing the certs being tested themselves. My explanation was not about why the value of request_1->client_certs.size() changed, but about the reason why I changed the expectation and the used certificates. > In the old code, neither client.p12 nor websocket_client_cert.p12 matched the request. Is this true? Yes, neither matched. The changed result of request_1->client_certs.size() is because of the change from client.p12 to client_1.pem . The former does not have the same issuer CA as client_1.pem. The documentation of kAuthority1DN explicitly says that this DN is the issuer of the client_1 cert. Looking at the second sentence from Matt's comment (old line 55-56): // TODO(mattm): [...] Update net/data/ssl/scripts/generate-client-certificates.sh so that it actually // saves the .p12 files, and regenerate them. That script currently generates client_1.* and client_2.* with different CAs. I thus assumed, that Matt had planned to generate the client.p12 with the same client_1 or client_2 cert. From our previous discussion, we came to the conclusion that PKCS12 is unnecessarily complex and PKCS8 + a single X509 cert in PEM format are preferable. I updated the script accordingly. And yet another argument to support the change to the new ASSERT_EQ(1u, ....client_certs.size()): Other CertStore implementations are tested according to the tests defined in client_cert_store_unittest-inl.h . There client_1 and client_2 certs are used and it's verified that requesting for Authority1DN only returns client_1 (similary for client_2/Authority2DN). I just checked the original commit of this test, but there was no discussion or additional documentation of this test. https://codereview.chromium.org/112533002
I think we're in violent agreement, I'm just violently confused. I think the action items for this CL should be: 1) Add a comment to the WaitFor/After initialization tests that provide a simple description of what they're testing 2) Move the testing of the delegation of name matching to a separate test. That is, the moment I hear "this also tests X", I think it should be split into another test. It sounds like you want - A test that calls import, does a request, then does FinishInit, and makes sure the cert returned in the request is what is expected (i.e. that the request actually waited for async initialization) - A test that calls import, does a FinishInit, then does a request, and makes sure the cert returned in the request is what is expected (i.e. that the request handled sync initialization) - A test that does import of two certs, does a request, and makes sure only the certs matching the request are what are expected (this is the name matching TODO, I believe) - A test that does import of two certs to different stores, and ensures there is no store cross-talk (we already have this, I believe) I think the key point here is not conflating the name matching with the sync/async test. In that case, it shouldn't matter which cert you use for the sync/async, but it would matter for the name matching. I just like tests to test as little behaviour as possible-per-test, if that makes sense? The red flag was because I'm always nervous when test expectations change in the same CL as a test, since tests serve not only as a unit test, but also as a form of regression test. I didn't really see a clear explanation of why you were changing the expectations (and the test), but you've given that now, and so I follow. And there, it just highlights we should be doing that particular aspect in a different test. Sound good?
With that separation of tests mentioned, and to save you a roundtrip, I think it LGTM. You can wait to see if there are any nits to TBR, but I think the mechanical nature of the change LG, and it's just a matter of splitting up the tests. https://codereview.chromium.org/394013005/diff/40001/crypto/nss_util.cc File crypto/nss_util.cc (right): https://codereview.chromium.org/394013005/diff/40001/crypto/nss_util.cc#newco... crypto/nss_util.cc:474: std::string(kNSSDatabaseName) + " for " + username_hash; base::StringPrintF, rather than string concatting?
On 2014/07/16 21:38:40, Ryan Sleevi wrote: > I think we're in violent agreement, I'm just violently confused. > > I think the action items for this CL should be: > 1) Add a comment to the WaitFor/After initialization tests that provide a simple > description of what they're testing > 2) Move the testing of the delegation of name matching to a separate test. > > That is, the moment I hear "this also tests X", I think it should be split into > another test. I didn't even consider splitting the tests, because I prioritized keeping the change small and 'minimally invasive' if that makes sense. The cert loading I had to change, to get rid of the NSSCertDatabase dependency. But now that we have gone through the whole analysis of the tests, I'm happy to split/document the tests. > It sounds like you want > - A test that calls import, does a request, then does FinishInit, and makes sure > the cert returned in the request is what is expected (i.e. that the request > actually waited for async initialization) > - A test that calls import, does a FinishInit, then does a request, and makes > sure the cert returned in the request is what is expected (i.e. that the request > handled sync initialization) > - A test that does import of two certs, does a request, and makes sure only the > certs matching the request are what are expected (this is the name matching > TODO, I believe) > - A test that does import of two certs to different stores, and ensures there is > no store cross-talk (we already have this, I believe) > > I think the key point here is not conflating the name matching with the > sync/async test. In that case, it shouldn't matter which cert you use for the > sync/async, but it would matter for the name matching. > > I just like tests to test as little behaviour as possible-per-test, if that > makes sense? > > The red flag was because I'm always nervous when test expectations change in the > same CL as a test, since tests serve not only as a unit test, but also as a form > of regression test. I didn't really see a clear explanation of why you were > changing the expectations (and the test), but you've given that now, and so I > follow. And there, it just highlights we should be doing that particular aspect > in a different test. > > Sound good? Sounds good, and I fully support your initial concerns, reasoning and final conclusion.
On 2014/07/16 21:41:40, Ryan Sleevi wrote: > With that separation of tests mentioned, and to save you a roundtrip, I think it > LGTM. You can wait to see if there are any nits to TBR, but I think the > mechanical nature of the change LG, and it's just a matter of splitting up the > tests. Thanks a lot for the support. > > https://codereview.chromium.org/394013005/diff/40001/crypto/nss_util.cc > File crypto/nss_util.cc (right): > > https://codereview.chromium.org/394013005/diff/40001/crypto/nss_util.cc#newco... > crypto/nss_util.cc:474: std::string(kNSSDatabaseName) + " for " + username_hash; > base::StringPrintF, rather than string concatting?
https://codereview.chromium.org/394013005/diff/40001/crypto/nss_util.cc File crypto/nss_util.cc (right): https://codereview.chromium.org/394013005/diff/40001/crypto/nss_util.cc#newco... crypto/nss_util.cc:60: const char kNSSDatabaseName[] = "Real NSS db"; On 2014/07/16 19:32:18, Ryan Sleevi wrote: > On 2014/07/16 10:08:25, pneubeck wrote: > > I reduced the length of this string because the nickname was truncated on > debug > > outputs. Not sure why. > > pedantry: db -> DB > > Nicknames have a fixed length. Could this be why? shortened the name further and change it to indicate that it's a user's DB. https://codereview.chromium.org/394013005/diff/40001/crypto/nss_util.cc#newco... crypto/nss_util.cc:474: std::string(kNSSDatabaseName) + " for " + username_hash; On 2014/07/16 21:41:40, Ryan Sleevi wrote: > base::StringPrintF, rather than string concatting? Done. https://codereview.chromium.org/394013005/diff/40001/net/ssl/client_cert_stor... File net/ssl/client_cert_store_chromeos_unittest.cc (right): https://codereview.chromium.org/394013005/diff/40001/net/ssl/client_cert_stor... net/ssl/client_cert_store_chromeos_unittest.cc:46: << cert_filename; On 2014/07/16 19:59:00, Ryan Sleevi wrote: > On 2014/07/16 19:49:44, pneubeck wrote: > > On 2014/07/16 19:32:18, Ryan Sleevi wrote: > > > Note: This is generally a dangerous pattern to put EXPECT clauses within a > > > sub-routine, since they can lead to subtle error when someone makes them > > ASSERT > > > clauses (e.g. > > > > > > https://code.google.com/p/googletest/wiki/AdvancedGuide#Propagating_Fatal_Fai... > > > ) > > I followed the existing style. > > I'd actually would prefer CHECKs, but these are also controversial in tests. > > > > So, I'll just remove the EXPECTs and ASSERT at the call site. > > I think it's fine, but it means that you need to wrap these calls in the > ASSERT_NO_FATAL_FAILURES macro, in order to prevent surprises. Yeah, but that would still mean that there are redundant checks of the failure conditions (one in the EXPECT statement and one to return). I went ahead with the simpler and still safe variant to LOG(ERROR) and return. ASSERTion is done at the call-site. https://codereview.chromium.org/394013005/diff/40001/net/test/cert_test_util.h File net/test/cert_test_util.h (right): https://codereview.chromium.org/394013005/diff/40001/net/test/cert_test_util.... net/test/cert_test_util.h:16: #include "crypto/rsa_private_key.h" On 2014/07/16 19:49:44, pneubeck wrote: > On 2014/07/16 19:32:18, Ryan Sleevi wrote: > > Forward declare RSAPrivateKey. > > > > The caller of this function will then include the header when they call this > > function. > > > > See http://www.chromium.org/developers/coding-style/cpp-dos-and-donts > > Ops. Will change. Done.
since the change is not as trivial as probably anticipated, ptal. The unit test now covers exactly the cases that you listed yesterday. The verification whether 'cert filtering by CA' is correctly delegated to the base class is done by instantiating the tests from client_cert_store_unittest-inl.h (as Matt seems to have intended as well, see my other comment). https://codereview.chromium.org/394013005/diff/100001/net/ssl/client_cert_sto... File net/ssl/client_cert_store_chromeos.h (left): https://codereview.chromium.org/394013005/diff/100001/net/ssl/client_cert_sto... net/ssl/client_cert_store_chromeos.h:43: // Allows tests to initialize the cert store with the given slots. I just noticed today these two functions that Matt initially added (likely copy&pasted from the NSS implementation). This indicates that he planned to indeed reuse the test templates in client_cert_store_unittest-inl.h also for the ChromeOS implementation. I updated the ChromeOS unittest to apply these test. However, it wouldn't work to use these two functions only (i.e. don't import the client certs into a DB but directly pass them to SelectClientCertsForTesting), because the profile_filter_ does only accepts certs that _are_ stored in a specific slot. Thus, instead I removed these testing functions and test this class through its public interface only (which is the preferred way anyways). A minor glitch is that I have to import the private keys in advance, because the test template doesn't provide them (could be changed of course). In a follow up, I should update the NSS test to do the same. https://codereview.chromium.org/394013005/diff/100001/net/ssl/client_cert_sto... File net/ssl/client_cert_store_chromeos_unittest.cc (right): https://codereview.chromium.org/394013005/diff/100001/net/ssl/client_cert_sto... net/ssl/client_cert_store_chromeos_unittest.cc:36: std::string nickname = cert->GetDefaultNickname(USER_CERT); Changed the nickname from filename (because it's not provided anymore) to this DefaultNickname. https://codereview.chromium.org/394013005/diff/100001/net/ssl/client_cert_sto... net/ssl/client_cert_store_chromeos_unittest.cc:57: CHECK(user_.constructed_successfully()); very likely you don't like these CHECKs, although IMO this is OK because our test infrastructure automatically runs tests isolatedly in case of crashes. ASSERTs work neither because of the return type not being 'void'. Do you require this to be changed to EXPECT and a ASSERT_NO_FATAL_FAILURES wrapper at the call site? (I didn't try whether that actually works)
please also take a second look at the .gyp and .gn changes. Thanks!
Quick feedback since it may have larger implications. Will review more in depth later today. https://codereview.chromium.org/394013005/diff/100001/net/ssl/client_cert_sto... File net/ssl/client_cert_store_chromeos_unittest.cc (right): https://codereview.chromium.org/394013005/diff/100001/net/ssl/client_cert_sto... net/ssl/client_cert_store_chromeos_unittest.cc:57: CHECK(user_.constructed_successfully()); On 2014/07/17 13:40:50, pneubeck wrote: > very likely you don't like these CHECKs, although IMO this is OK because our > test infrastructure automatically runs tests isolatedly in case of crashes. > > ASSERTs work neither because of the return type not being 'void'. > Do you require this to be changed to EXPECT and a ASSERT_NO_FATAL_FAILURES > wrapper at the call site? (I didn't try whether that actually works) This is my preference, in part because I almost exclusively run tests without the multi-process harness, because it makes debugging IMPOSSIBLE. You can't quite wrapper a CTOR. Instead, it means an Init() method with a bool.
https://codereview.chromium.org/394013005/diff/100001/net/ssl/client_cert_sto... File net/ssl/client_cert_store_chromeos_unittest.cc (right): https://codereview.chromium.org/394013005/diff/100001/net/ssl/client_cert_sto... net/ssl/client_cert_store_chromeos_unittest.cc:57: CHECK(user_.constructed_successfully()); On 2014/07/17 18:42:20, Ryan Sleevi wrote: > On 2014/07/17 13:40:50, pneubeck wrote: > > very likely you don't like these CHECKs, although IMO this is OK because our > > test infrastructure automatically runs tests isolatedly in case of crashes. > > > > ASSERTs work neither because of the return type not being 'void'. > > Do you require this to be changed to EXPECT and a ASSERT_NO_FATAL_FAILURES > > wrapper at the call site? (I didn't try whether that actually works) > > This is my preference, in part because I almost exclusively run tests without > the multi-process harness, because it makes debugging IMPOSSIBLE. > > You can't quite wrapper a CTOR. Instead, it means an Init() method with a bool. I can do that tomorrow or in a follow up. Will mean adapting all the tests of the different platforms (mechanical change however). Would be good however to get this one landed at some finite time ;-)
LGTM, with caveat about init (e.g. fine to defer) https://codereview.chromium.org/394013005/diff/120001/net/ssl/client_cert_sto... File net/ssl/client_cert_store_chromeos_unittest.cc (right): https://codereview.chromium.org/394013005/diff/120001/net/ssl/client_cert_sto... net/ssl/client_cert_store_chromeos_unittest.cc:26: bool ImportClientCertToSlot(scoped_refptr<X509Certificate> cert, const scoped_refptr<>& https://codereview.chromium.org/394013005/diff/120001/net/ssl/client_cert_sto... net/ssl/client_cert_store_chromeos_unittest.cc:34: } Not sure why you do this check. It's not required that a cert be in the same key as its slot. https://codereview.chromium.org/394013005/diff/120001/net/ssl/client_cert_sto... net/ssl/client_cert_store_chromeos_unittest.cc:65: // are used during the test. This doesn't sound right, especially since NSS does violate this. https://codereview.chromium.org/394013005/diff/120001/net/ssl/client_cert_sto... net/ssl/client_cert_store_chromeos_unittest.cc:69: GetTestCertsDirectory(), "client_2.pk8", slot_.get()); One way to reduce refactoring would be to defer all this setup until SelectClientCerts() was called, and then return false if setup failed. https://codereview.chromium.org/394013005/diff/120001/net/ssl/client_cert_sto... net/ssl/client_cert_store_chromeos_unittest.cc:92: }; More documentation is needed - either in this class, on 72, or 94/95, to document what's going on and what the expectations are here. Most importantly is noting that the base test is defined somewhere else (took me a while to figure that out) https://codereview.chromium.org/394013005/diff/120001/net/ssl/client_cert_sto... net/ssl/client_cert_store_chromeos_unittest.cc:95: // base class ClientCertStoreNSS. Eh? This doesn't make sense, considering that you're using ClientCertStoreChromeOS everywhere. https://codereview.chromium.org/394013005/diff/120001/net/ssl/client_cert_sto... net/ssl/client_cert_store_chromeos_unittest.cc:130: return cert; |cert| may (and almost certainly will) refer to the old/global cert, not the cert in the slot. Not that this should matter in practice, but it does go back to NSS not caring about the slot. https://codereview.chromium.org/394013005/diff/120001/net/ssl/client_cert_sto... net/ssl/client_cert_store_chromeos_unittest.cc:135: IfRequestCertsBeforeNSSDBInitializedThenRequestWaitsForInitAndSucceeds) { I actually meant just include a more meaningful comment. ReallyLongSentencesWithCamelCapsCanBeHardToReadAndEvenHarderToTypeOnCommandLines What you did on 190-192 is good stuff and what I was asking for :) https://codereview.chromium.org/394013005/diff/120001/net/test/cert_test_util.h File net/test/cert_test_util.h (right): https://codereview.chromium.org/394013005/diff/120001/net/test/cert_test_util... net/test/cert_test_util.h:17: typedef struct PK11SlotInfoStr PK11SlotInfo; Usually list the NSS header, eg // From <pk11pub.h>
Added more documentation. https://codereview.chromium.org/394013005/diff/120001/net/ssl/client_cert_sto... File net/ssl/client_cert_store_chromeos_unittest.cc (right): https://codereview.chromium.org/394013005/diff/120001/net/ssl/client_cert_sto... net/ssl/client_cert_store_chromeos_unittest.cc:26: bool ImportClientCertToSlot(scoped_refptr<X509Certificate> cert, On 2014/07/17 22:18:52, Ryan Sleevi wrote: > const scoped_refptr<>& Done. https://codereview.chromium.org/394013005/diff/120001/net/ssl/client_cert_sto... net/ssl/client_cert_store_chromeos_unittest.cc:34: } On 2014/07/17 22:18:52, Ryan Sleevi wrote: > Not sure why you do this check. It's not required that a cert be in the same key > as its slot. Removed. https://codereview.chromium.org/394013005/diff/120001/net/ssl/client_cert_sto... net/ssl/client_cert_store_chromeos_unittest.cc:65: // are used during the test. On 2014/07/17 22:18:52, Ryan Sleevi wrote: > This doesn't sound right, especially since NSS does violate this. I never get used to the spares (or rather not existent) NSS documentation... Found finally that CK_INVALID_KEY and that the key is not required for the import. https://codereview.chromium.org/394013005/diff/120001/net/ssl/client_cert_sto... net/ssl/client_cert_store_chromeos_unittest.cc:69: GetTestCertsDirectory(), "client_2.pk8", slot_.get()); On 2014/07/17 22:18:51, Ryan Sleevi wrote: > One way to reduce refactoring would be to defer all this setup until > SelectClientCerts() was called, and then return false if setup failed. Done. https://codereview.chromium.org/394013005/diff/120001/net/ssl/client_cert_sto... net/ssl/client_cert_store_chromeos_unittest.cc:95: // base class ClientCertStoreNSS. On 2014/07/17 22:18:52, Ryan Sleevi wrote: > Eh? This doesn't make sense, considering that you're using > ClientCertStoreChromeOS everywhere. I don't see why it didn't make sense. But I made the explanation a bit more verbose. https://codereview.chromium.org/394013005/diff/120001/net/ssl/client_cert_sto... net/ssl/client_cert_store_chromeos_unittest.cc:130: return cert; On 2014/07/17 22:18:52, Ryan Sleevi wrote: > |cert| may (and almost certainly will) refer to the old/global cert, not the > cert in the slot. > > Not that this should matter in practice, but it does go back to NSS not caring > about the slot. Added a comment. https://codereview.chromium.org/394013005/diff/120001/net/ssl/client_cert_sto... net/ssl/client_cert_store_chromeos_unittest.cc:135: IfRequestCertsBeforeNSSDBInitializedThenRequestWaitsForInitAndSucceeds) { On 2014/07/17 22:18:52, Ryan Sleevi wrote: > I actually meant just include a more meaningful comment. > ReallyLongSentencesWithCamelCapsCanBeHardToReadAndEvenHarderToTypeOnCommandLines I blame TotT for that... > > What you did on 190-192 is good stuff and what I was asking for :) https://codereview.chromium.org/394013005/diff/120001/net/test/cert_test_util.h File net/test/cert_test_util.h (right): https://codereview.chromium.org/394013005/diff/120001/net/test/cert_test_util... net/test/cert_test_util.h:17: typedef struct PK11SlotInfoStr PK11SlotInfo; On 2014/07/17 22:18:52, Ryan Sleevi wrote: > Usually list the NSS header, eg > > // From <pk11pub.h> Taken from rsa_private_key.h where the header was not mentioned ;-)
The CQ bit was checked by pneubeck@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pneubeck@chromium.org/394013005/220001
Message was sent while issue was closed.
Change committed as 284056 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
