Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
92443002:
Extract Certificate Transparency SCTs from stapled OCSP responses (Closed)
|
Created:
7 years ago by ekasper Modified:
7 years ago CC:
chromium-reviews, cbentzel+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@extract_scts Visibility:
Public. |
DescriptionExtract Certificate Transparency SCTs from stapled OCSP responses
BUG=309578
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=240721
Patch Set 1 #Patch Set 2 : rebase and wire extracted SCTs to the CT verifier #
Total comments: 30
Patch Set 3 : review comments #
Total comments: 47
Patch Set 4 : redo ASN.1 #Patch Set 5 : format #
Total comments: 12
Patch Set 6 : comments #Patch Set 7 : fix win build #Patch Set 8 : clean up subs #
Total comments: 27
Patch Set 9 : review comments #
Total comments: 40
Patch Set 10 : more review comments #
Total comments: 2
Patch Set 11 : fix unit test #Patch Set 12 : rebase #
Total comments: 18
Patch Set 13 : rsleevi's comments #Patch Set 14 : rebase #Patch Set 15 : formatting nit #Patch Set 16 : add missing init #Patch Set 17 : indent #Patch Set 18 : rebase #
Total comments: 1
Patch Set 19 : fix template bug #Patch Set 20 : few more comments #
Total comments: 8
Patch Set 21 : fix format errors #
Total comments: 2
Patch Set 22 : Fix C++11 compile error #Messages
Total messages: 44 (0 generated)
The bad news is that this patch is a little more invasive than I'd hoped as I had to roll my own OCSP response decoder as well as do some ugly plumbing to get the test server to staple the responses. The good news is that this should be the last patch of this series from my side :)
Emilia: we need to use rsleevi's chromium.org email address for Chromium code reviews. I may not be able to review this CL this week. Today is the last working day of the week in the US.
Whoops, thanks for noticing the reviewer mix-up. I know it's a short week in the US, don't worry about getting to it today. Enjoy the holidays!
In case you'd already started looking at this CL, please note that I've now also wired the extracted SCTs to the CT verifier code that landed over the long weekend. It's a 2-line change - around line 3543 in ssl_client_socket_nss.cc - so I figured I'd sneak it in this CL. On 2013/11/27 20:32:22, ekasper wrote: > Whoops, thanks for noticing the reviewer mix-up. > > I know it's a short week in the US, don't worry about getting to it today. Enjoy > the holidays!
Review comments on patch set 2: The CL seems good, but I did a rather hurried review, so I want to take another look at it when my brain is in a better condition. https://codereview.chromium.org/92443002/diff/20001/net/cert/asn1_util.cc File net/cert/asn1_util.cc (right): https://codereview.chromium.org/92443002/diff/20001/net/cert/asn1_util.cc#new... net/cert/asn1_util.cc:353: return false; Nit: add curly braces {} because the conditional expression spans multiple lines. Please fix other instances of this problem in this function. https://codereview.chromium.org/92443002/diff/20001/net/cert/asn1_util.cc#new... net/cert/asn1_util.cc:373: 0x07, 0x30, 0x01, 0x01}; Nit: add a space after '{' and before '}'. https://codereview.chromium.org/92443002/diff/20001/net/cert/asn1_util.cc#new... net/cert/asn1_util.cc:431: while (responses.size() > 0) { Nit: this function is very long. If we want to break it into smaller functions, this while loop or line 426 may be a good point to start a new function that parses the SEQUENCE OF SingleResponse. https://codereview.chromium.org/92443002/diff/20001/net/cert/asn1_util.cc#new... net/cert/asn1_util.cc:552: } We should also return true at the end of this (inner) while loop, because we have found the right singleResponse but it doesn't have the SCT extension. One way to fix this is to change the "return true" statement on line 551 to a "break" statement, and then add a "return true" between lines 552 and 553. https://codereview.chromium.org/92443002/diff/20001/net/cert/asn1_util.cc#new... net/cert/asn1_util.cc:556: return true; With the change I proposed above, the only reason we get here is that the OCSP response does not have a singleResponse for |cert_serial_number|. We probably should return false in that case. https://codereview.chromium.org/92443002/diff/20001/net/cert/asn1_util.h File net/cert/asn1_util.h (right): https://codereview.chromium.org/92443002/diff/20001/net/cert/asn1_util.h#newc... net/cert/asn1_util.h:94: // |sct_list_out| is either empty (no response found), or points into |cert|. Typo: |cert| => |ocsp_response| https://codereview.chromium.org/92443002/diff/20001/net/cert/asn1_util.h#newc... net/cert/asn1_util.h:97: const base::StringPiece& cert_serial_number, Nit: this should be base::StringPiece, without the const ref. (We are not very consistent about this, but we should be consistent within a function.) https://codereview.chromium.org/92443002/diff/20001/net/cert/ct_objects_extra... File net/cert/ct_objects_extractor.h (right): https://codereview.chromium.org/92443002/diff/20001/net/cert/ct_objects_extra... net/cert/ct_objects_extractor.h:55: // Note that we do response matching by serial number only so if the response Nit: add a comma (,) after "by serial number only". https://codereview.chromium.org/92443002/diff/20001/net/cert/ct_objects_extra... net/cert/ct_objects_extractor.h:57: // this method does not work. However, repeating serial numbers within the same I don't think we need to worry about this, because certificate serial numbers are supposed to be unique under the same issuer. Unless an OCSP response is allowed to contain statuses for certificates issued by different CAs. Update: I checked the CertID ASN.1 type. I think we should also match issuerNameHash or issuerKeyHash. I agree the risk of not doing this is low. https://codereview.chromium.org/92443002/diff/20001/net/cert/ct_objects_extra... File net/cert/ct_objects_extractor_nss.cc (right): https://codereview.chromium.org/92443002/diff/20001/net/cert/ct_objects_extra... net/cert/ct_objects_extractor_nss.cc:281: base::StringPiece ocsp_resp(ocsp_response); Nit: it should be OK to just pass |ocsp_response|. https://codereview.chromium.org/92443002/diff/20001/net/cert/ct_objects_extra... net/cert/ct_objects_extractor_nss.cc:288: *sct_list = std::string(sct_list_out.data(), sct_list_out.size()); Nit: you can use the as_string() method: *sct_list = sct_list_out.as_string(); https://codereview.chromium.org/92443002/diff/20001/net/cert/ct_objects_extra... File net/cert/ct_objects_extractor_unittest.cc (right): https://codereview.chromium.org/92443002/diff/20001/net/cert/ct_objects_extra... net/cert/ct_objects_extractor_unittest.cc:127: TEST_F(CTObjectsExtractorTest, ExtractSCTListFromOCSPResponse) { Do we need to put this unit test inside #if !defined(USE_OPENSSL) ? https://codereview.chromium.org/92443002/diff/20001/net/socket/ssl_client_soc... File net/socket/ssl_client_socket_nss.cc (right): https://codereview.chromium.org/92443002/diff/20001/net/socket/ssl_client_soc... net/socket/ssl_client_socket_nss.cc:3220: // Request OCSP stapling even on platforms that don't support OCSP, in Nit: don't support OCSP => don't support OCSP stapling ? https://codereview.chromium.org/92443002/diff/20001/net/tools/testserver/mini... File net/tools/testserver/minica.py (right): https://codereview.chromium.org/92443002/diff/20001/net/tools/testserver/mini... net/tools/testserver/minica.py:171: Nit: don't add this blank line. https://codereview.chromium.org/92443002/diff/20001/third_party/tlslite/READM... File third_party/tlslite/README.chromium (right): https://codereview.chromium.org/92443002/diff/20001/third_party/tlslite/READM... third_party/tlslite/README.chromium:39: - patches/status_request.patch: add support for sending stapled OCSP responses. Nit: can you combine this patch with the previous patch? Ignore this suggestion if it's more work than it's worth.
https://codereview.chromium.org/92443002/diff/20001/net/cert/asn1_util.cc File net/cert/asn1_util.cc (right): https://codereview.chromium.org/92443002/diff/20001/net/cert/asn1_util.cc#new... net/cert/asn1_util.cc:353: return false; On 2013/12/03 01:18:06, wtc wrote: > > Nit: add curly braces {} because the conditional expression spans multiple > lines. > > Please fix other instances of this problem in this function. Done. https://codereview.chromium.org/92443002/diff/20001/net/cert/asn1_util.cc#new... net/cert/asn1_util.cc:373: 0x07, 0x30, 0x01, 0x01}; On 2013/12/03 01:18:06, wtc wrote: > > Nit: add a space after '{' and before '}'. Done. https://codereview.chromium.org/92443002/diff/20001/net/cert/asn1_util.cc#new... net/cert/asn1_util.cc:431: while (responses.size() > 0) { On 2013/12/03 01:18:06, wtc wrote: > > Nit: this function is very long. If we want to break it into smaller functions, > this while loop or line 426 may be a good point to start a new function that > parses the SEQUENCE OF SingleResponse. I've broken it up so that the seeking to the right extensions happens in separate methods. However I didn't use "seek" a la "SeekToSPKI" because I found the stringpiece naming to get too confusing. https://codereview.chromium.org/92443002/diff/20001/net/cert/asn1_util.cc#new... net/cert/asn1_util.cc:552: } On 2013/12/03 01:18:06, wtc wrote: > > We should also return true at the end of this (inner) while loop, because we > have found the right singleResponse but it doesn't have the SCT extension. > > One way to fix this is to change the "return true" statement on line 551 to a > "break" statement, and then add a "return true" between lines 552 and 553. Good catch, done as you suggested, break; followed by return true; after the loop. https://codereview.chromium.org/92443002/diff/20001/net/cert/asn1_util.cc#new... net/cert/asn1_util.cc:556: return true; On 2013/12/03 01:18:06, wtc wrote: > > With the change I proposed above, the only reason we get here is that the OCSP > response does not have a singleResponse for |cert_serial_number|. We probably > should return false in that case. Yes, a false is probably better in this case. This is now covered by ExtractExtensionsWithMatchingCertID which returns false if no matching response was found. https://codereview.chromium.org/92443002/diff/20001/net/cert/asn1_util.h File net/cert/asn1_util.h (right): https://codereview.chromium.org/92443002/diff/20001/net/cert/asn1_util.h#newc... net/cert/asn1_util.h:94: // |sct_list_out| is either empty (no response found), or points into |cert|. On 2013/12/03 01:18:06, wtc wrote: > > Typo: |cert| => |ocsp_response| Done. https://codereview.chromium.org/92443002/diff/20001/net/cert/asn1_util.h#newc... net/cert/asn1_util.h:97: const base::StringPiece& cert_serial_number, On 2013/12/03 01:18:06, wtc wrote: > > Nit: this should be base::StringPiece, without the const ref. (We are not very > consistent about this, but we should be consistent within a function.) The other one can't be const because it's modified; |cert_serial_number| is const. But, done. https://codereview.chromium.org/92443002/diff/20001/net/cert/ct_objects_extra... File net/cert/ct_objects_extractor.h (right): https://codereview.chromium.org/92443002/diff/20001/net/cert/ct_objects_extra... net/cert/ct_objects_extractor.h:55: // Note that we do response matching by serial number only so if the response On 2013/12/03 01:18:06, wtc wrote: > > Nit: add a comma (,) after "by serial number only". Done. https://codereview.chromium.org/92443002/diff/20001/net/cert/ct_objects_extra... net/cert/ct_objects_extractor.h:57: // this method does not work. However, repeating serial numbers within the same On 2013/12/03 01:18:06, wtc wrote: > > I don't think we need to worry about this, because certificate serial numbers > are supposed to be unique under the same issuer. > > Unless an OCSP response is allowed to contain statuses for certificates issued > by different CAs. > > Update: I checked the CertID ASN.1 type. I think we should also match > issuerNameHash or issuerKeyHash. I agree the risk of not doing this is low. In theory we should. In practice a collision here would happen if different CAs were delegating to the same OCSP responder, and these two CAs had certs with colliding serial numbers, and the responder was including the status for the second, presumably unrelated, certificate, in the response (which is a SHOULD NOT because the request for a stapled response would never ask for other certs). But I've added a TODO and will look into this. https://codereview.chromium.org/92443002/diff/20001/net/cert/ct_objects_extra... File net/cert/ct_objects_extractor_nss.cc (right): https://codereview.chromium.org/92443002/diff/20001/net/cert/ct_objects_extra... net/cert/ct_objects_extractor_nss.cc:281: base::StringPiece ocsp_resp(ocsp_response); On 2013/12/03 01:18:06, wtc wrote: > > Nit: it should be OK to just pass |ocsp_response|. Done. https://codereview.chromium.org/92443002/diff/20001/net/cert/ct_objects_extra... net/cert/ct_objects_extractor_nss.cc:288: *sct_list = std::string(sct_list_out.data(), sct_list_out.size()); On 2013/12/03 01:18:06, wtc wrote: > > Nit: you can use the as_string() method: > *sct_list = sct_list_out.as_string(); Done. https://codereview.chromium.org/92443002/diff/20001/net/cert/ct_objects_extra... File net/cert/ct_objects_extractor_unittest.cc (right): https://codereview.chromium.org/92443002/diff/20001/net/cert/ct_objects_extra... net/cert/ct_objects_extractor_unittest.cc:127: TEST_F(CTObjectsExtractorTest, ExtractSCTListFromOCSPResponse) { On 2013/12/03 01:18:06, wtc wrote: > > Do we need to put this unit test inside #if !defined(USE_OPENSSL) ? I think not - none of the objects extraction is implemented for OpenSSL so the whole test suite is excluded in net.gyp (line 2095). https://codereview.chromium.org/92443002/diff/20001/net/socket/ssl_client_soc... File net/socket/ssl_client_socket_nss.cc (right): https://codereview.chromium.org/92443002/diff/20001/net/socket/ssl_client_soc... net/socket/ssl_client_socket_nss.cc:3220: // Request OCSP stapling even on platforms that don't support OCSP, in On 2013/12/03 01:18:06, wtc wrote: > > Nit: don't support OCSP => don't support OCSP stapling ? I've simply made it "don't support it". https://codereview.chromium.org/92443002/diff/20001/net/tools/testserver/mini... File net/tools/testserver/minica.py (right): https://codereview.chromium.org/92443002/diff/20001/net/tools/testserver/mini... net/tools/testserver/minica.py:171: On 2013/12/03 01:18:06, wtc wrote: > > Nit: don't add this blank line. Done. https://codereview.chromium.org/92443002/diff/20001/third_party/tlslite/READM... File third_party/tlslite/README.chromium (right): https://codereview.chromium.org/92443002/diff/20001/third_party/tlslite/READM... third_party/tlslite/README.chromium:39: - patches/status_request.patch: add support for sending stapled OCSP responses. On 2013/12/03 01:18:06, wtc wrote: > > Nit: can you combine this patch with the previous patch? > > Ignore this suggestion if it's more work than it's worth. It's one 'git diff' but I don't think it makes sense as OCSP stapling is functionally independent from CT, and useful without it. If tlslite were to get OCSP stapling support upstream then we could drop this patch but not the other one, etc.
Emilia, apologies for the delays in getting to this. We should not be rolling our own OCSP response parser. We have NSS available on all platforms (mod Android) and should be using those ASN.1 functions. Can you provide more insight into why these didn't work? Cheers, Ryan https://codereview.chromium.org/92443002/diff/40001/net/cert/asn1_util.h File net/cert/asn1_util.h (right): https://codereview.chromium.org/92443002/diff/40001/net/cert/asn1_util.h#newc... net/cert/asn1_util.h:96: NET_EXPORT_PRIVATE bool ExtractSCTExtensionFromOCSPResponse( Not LGTM here. We should not be doing this using asn1_util. asn1_util is only meant to be used over trusted data (eg: validated to some root of trust). The SCT extraction, by definition, happens before the OCSP validity checks. https://codereview.chromium.org/92443002/diff/40001/net/cert/ct_objects_extra... File net/cert/ct_objects_extractor_nss.cc (right): https://codereview.chromium.org/92443002/diff/40001/net/cert/ct_objects_extra... net/cert/ct_objects_extractor_nss.cc:282: if (!asn1::ExtractSCTExtensionFromOCSPResponse(ocsp_response, We should be using the NSS ASN.1 functions for this. https://codereview.chromium.org/92443002/diff/40001/net/socket/ssl_client_soc... File net/socket/ssl_client_socket_nss.cc (right): https://codereview.chromium.org/92443002/diff/40001/net/socket/ssl_client_soc... net/socket/ssl_client_socket_nss.cc:771: #ifdef SSL_ENABLE_OCSP_STAPLING 1) This should be #if defined 2) We shouldn't be using an ifdef here, because this will always be true given our build time requirements (NSS 3.14.3+, but always using our own libssl)
Patch set 3 LGTM. I ran out of steam when I got to tlslite/messages.py, so you may want to review that file yourself. Please note the comments marked with "BUG" or "IMPORTANT". https://codereview.chromium.org/92443002/diff/40001/net/cert/asn1_util.cc File net/cert/asn1_util.cc (right): https://codereview.chromium.org/92443002/diff/40001/net/cert/asn1_util.cc#new... net/cert/asn1_util.cc:433: return false; Nit: you are meticulous in checking there is no junk after the expected elements. Here you are not checking that there is no junk in tbs_response_data after responseExtensions. We can add this check, or we can remove all the checks for extra junk to make these functions smaller. https://codereview.chromium.org/92443002/diff/40001/net/cert/asn1_util.cc#new... net/cert/asn1_util.cc:439: // ExtractExtensionsWithMatchingCertID extracts the 'singleExtensions' inner Nit: this comment says "inner contents", but the code on line 508 uses the variable name "single_extensions_outer". They seem to contradict each other. https://codereview.chromium.org/92443002/diff/40001/net/cert/asn1_util.cc#new... net/cert/asn1_util.cc:441: // identifiers. Returns False if ASN.1 parsing failed, or if the OCSP response Nit: "identifiers" => "identifier" or "serial number" (singular) https://codereview.chromium.org/92443002/diff/40001/net/cert/asn1_util.cc#new... net/cert/asn1_util.cc:449: while (single_responses.size() > 0) { Nit: use !single_responses.empty(), to be consistent with the check on line 565. https://codereview.chromium.org/92443002/diff/40001/net/cert/asn1_util.cc#new... net/cert/asn1_util.cc:525: bool ExtractSCTExtensionFromOCSPResponse( I found that NSS has a function for decoding an OCSP response: http://mxr.mozilla.org/nss/ident?i=CERT_DecodeOCSPResponse In particular, the singleExtensions is available in the decoded OCSP response: http://mxr.mozilla.org/nss/ident?i=singleExtensions Unfortunately, the decoded OCSP response is an opaque struct type because the header that defines those types is considered NSS internal. This can be fixed, but until then we'll need to write our own decoding function. It is worth considering using the NSS ASN.1 decoder to do this, essentially copying the code of CERT_DecodeOCSPResponse. But we'll still need your implementation for OpenSSL. Does OpenSSL also have a general-purpose ASN.1 decoder? https://codereview.chromium.org/92443002/diff/40001/net/cert/asn1_util.cc#new... net/cert/asn1_util.cc:550: return false; Nit: you didn't check for extra junk at the end of single_extensions here: if (!single_extensions.empty()) return false; Perhaps we should just remove all the checks for extra junk. https://codereview.chromium.org/92443002/diff/40001/net/cert/asn1_util.cc#new... net/cert/asn1_util.cc:579: extn_id.size() != 0)) { BUG: extn_id.size() != 0)) => extn_id.size()) != 0) By the way, a bug of this kind was just recently found by PVS-Studio in Chromium: https://codereview.chromium.org/99423002/ Otherwise I don't think I would have noticed it. https://codereview.chromium.org/92443002/diff/40001/net/cert/asn1_util.cc#new... net/cert/asn1_util.cc:584: if (!GetElement(&extension, kBOOLEAN, NULL)) IMPORTANT: Should we allow |critical| to be optional since it has a DEFAULT value? Compare this with lines 253-254. https://codereview.chromium.org/92443002/diff/40001/net/cert/ct_objects_extra... File net/cert/ct_objects_extractor.h (right): https://codereview.chromium.org/92443002/diff/40001/net/cert/ct_objects_extra... net/cert/ct_objects_extractor.h:64: X509Certificate::OSCertHandle leaf, Nit: If CT can also be used for CA certificates, I suggest renaming this parameter |cert| to be more general. If you rename it, you should update the comment "the serial number of the leaf certificate" on lines 50-51 accordingly. https://codereview.chromium.org/92443002/diff/40001/net/cert/ct_objects_extra... File net/cert/ct_objects_extractor_nss.cc (right): https://codereview.chromium.org/92443002/diff/40001/net/cert/ct_objects_extra... net/cert/ct_objects_extractor_nss.cc:276: NSSCertWrapper leaf_cert(leaf); Nit: this is an expensive way to get the serial number from the OSCertHandle |leaf|. NSSCertWrapper will not only decode the certificate but also add it to NSS's internal hash tables of certificates. This is OK for now. You can consider copying the first few lines of code from SeekToSPKI in net/cert/asn1_util.cc. (X509Certificate::GetDEREncoded can be used to get the DER-encoded certificate.) Also, if you actually have an X509Certificate object, you can use the serial_number() getter method. Update: I found that the two callers of this function (the unit test and ssl_client_socket_nss.cc) have an X509Certificate object. So I suggest that you change this function to take |cert_serial_number| instead of |leaf| as input, and change the callers to pass X509Certificate::serial_number() to this function. However, having |leaf| gives us more flexibility because we will be able to get the issuer from |leaf| for more accurate matching. https://codereview.chromium.org/92443002/diff/40001/net/cert/ct_objects_extra... net/cert/ct_objects_extractor_nss.cc:285: return false; Nit: add curly braces. https://codereview.chromium.org/92443002/diff/40001/net/socket/ssl_client_soc... File net/socket/ssl_client_socket_nss.cc (right): https://codereview.chromium.org/92443002/diff/40001/net/socket/ssl_client_soc... net/socket/ssl_client_socket_nss.cc:771: #ifdef SSL_ENABLE_OCSP_STAPLING Nit: can you take the opportunity to remove all #ifdef SSL_ENABLE_OCSP_STAPLING from this file? We used to be careful about that, so that we can compile this file against an older version of the system NSS SSL library. But we no longer support that. https://codereview.chromium.org/92443002/diff/40001/net/socket/ssl_client_soc... net/socket/ssl_client_socket_nss.cc:2426: ocsp_responses->items[0].len); We are copying an OCSP response here. This seems to argue for changing ct::ExtractSCTListFromOCSPResponse() to take a base::StringPiece instead of a std::string as the |ocsp_response| input. https://codereview.chromium.org/92443002/diff/40001/net/socket/ssl_client_soc... net/socket/ssl_client_socket_nss.cc:2436: if (nss_handshake_state_.server_cert) { Hmm... I wonder why we check nss_handshake_state_.server_cert for null in the OS_WIN case but not in the USE_NSS case. This comes from the original code, so you don't need to deal with this. https://codereview.chromium.org/92443002/diff/40001/net/socket/ssl_client_soc... File net/socket/ssl_client_socket_unittest.cc (right): https://codereview.chromium.org/92443002/diff/40001/net/socket/ssl_client_soc... net/socket/ssl_client_socket_unittest.cc:1906: ssl_options.signed_cert_timestamps_tls_ext = "test2"; BUG: I think you meant to assign "test2" to ssl_options.signed_cert_timestamps_ocsp. https://codereview.chromium.org/92443002/diff/40001/net/test/spawned_test_ser... File net/test/spawned_test_server/base_test_server.cc (right): https://codereview.chromium.org/92443002/diff/40001/net/test/spawned_test_ser... net/test/spawned_test_server/base_test_server.cc:405: return false; Nit: add curly braces here and also for the _ocsp code below. https://codereview.chromium.org/92443002/diff/40001/net/tools/testserver/mini... File net/tools/testserver/minica.py (right): https://codereview.chromium.org/92443002/diff/40001/net/tools/testserver/mini... net/tools/testserver/minica.py:169: # SignedCertificateTimestampList (RFC 6962) Nit: the comment should mention this OID is used with an OCSP extension. https://codereview.chromium.org/92443002/diff/40001/net/tools/testserver/mini... net/tools/testserver/minica.py:300: ])) Nit: the original code indents this line with two fewer spaces. https://codereview.chromium.org/92443002/diff/40001/net/tools/testserver/test... File net/tools/testserver/testserver.py (right): https://codereview.chromium.org/92443002/diff/40001/net/tools/testserver/test... net/tools/testserver/testserver.py:180: OCSPResponse = self.ocsp_response) Nit: should "OCSPResponse" start in lowercase "ocspResponse"? https://codereview.chromium.org/92443002/diff/40001/net/tools/testserver/test... net/tools/testserver/testserver.py:1933: # will simply ignore the extension. Is this comment true? If I get an OCSP response from an OCSP responder, should I ignore the SCT extension in the OCSP response? https://codereview.chromium.org/92443002/diff/40001/net/tools/testserver/test... net/tools/testserver/testserver.py:1953: ocsp_der) Should we use self.__ocsp_server.ocsp_response here instead? This will make it unnecessary to move the "ocsp_der = None" line to the beginning of this function. https://codereview.chromium.org/92443002/diff/40001/third_party/tlslite/tlsli... File third_party/tlslite/tlslite/TLSConnection.py (right): https://codereview.chromium.org/92443002/diff/40001/third_party/tlslite/tlsli... third_party/tlslite/tlslite/TLSConnection.py:941: OCSPResponse=None): Nit: I suggest "ocspResponse" to match the capitalization style of other parameters. https://codereview.chromium.org/92443002/diff/40001/third_party/tlslite/tlsli... third_party/tlslite/tlslite/TLSConnection.py:1021: Note that the response is sent independent of the ClientHello extension Nit: add "status_request" between "ClientHello" and "extension". Also on line 1024. https://codereview.chromium.org/92443002/diff/40001/third_party/tlslite/tlsli... third_party/tlslite/tlslite/TLSConnection.py:1045: OCSPResponse=None): Nit: ocspResponse. https://codereview.chromium.org/92443002/diff/40001/third_party/tlslite/tlsli... third_party/tlslite/tlslite/TLSConnection.py:1074: OCSPResponse): Nit: ocspResponse. https://codereview.chromium.org/92443002/diff/40001/third_party/tlslite/tlsli... third_party/tlslite/tlslite/TLSConnection.py:1451: if clientHello.status_request and OCSPResponse: Nit: we may be able to just test serverHello.status_request here.
https://codereview.chromium.org/92443002/diff/40001/net/socket/ssl_client_soc... File net/socket/ssl_client_socket_nss.cc (right): https://codereview.chromium.org/92443002/diff/40001/net/socket/ssl_client_soc... net/socket/ssl_client_socket_nss.cc:2436: if (nss_handshake_state_.server_cert) { On 2013/12/03 21:04:25, wtc wrote: > > Hmm... I wonder why we check nss_handshake_state_.server_cert for null in the > OS_WIN case but not in the USE_NSS case. > > This comes from the original code, so you don't need to deal with this. Because Windows will fail to parse certificates that don't map into Windows structures (eg: FILETIME date ranges), but NSS will. http://crbug.com/91341 Also, Chromoting (which doesn't have and can't use CAPI), but did use this code with pseudotcpsocket
Ryan, Wan-Teh, It's late here so I'll only get to addressing your comments tomorrow but I was hoping you could advise me on the ASN.1 approach meanwhile. Sorry for not being clear about the motivation behind rolling my own parser - the reason is that, while NSS does have an OCSP response decoder, it provides no API access to the internal response structure. (FYI, I've checked and OpenSSL, when we get to it, does have an API into response extensions.) If I'm not allowed to use asn1_util, then what do you recommend I do? Thanks, Emilia On 2013/12/03 21:07:15, Ryan Sleevi wrote: > https://codereview.chromium.org/92443002/diff/40001/net/socket/ssl_client_soc... > File net/socket/ssl_client_socket_nss.cc (right): > > https://codereview.chromium.org/92443002/diff/40001/net/socket/ssl_client_soc... > net/socket/ssl_client_socket_nss.cc:2436: if (nss_handshake_state_.server_cert) > { > On 2013/12/03 21:04:25, wtc wrote: > > > > Hmm... I wonder why we check nss_handshake_state_.server_cert for null in the > > OS_WIN case but not in the USE_NSS case. > > > > This comes from the original code, so you don't need to deal with this. > > Because Windows will fail to parse certificates that don't map into Windows > structures (eg: FILETIME date ranges), but NSS will. > > http://crbug.com/91341 > > Also, Chromoting (which doesn't have and can't use CAPI), but did use this code > with pseudotcpsocket
On Tue, Dec 3, 2013 at 2:13 PM, <ekasper@google.com> wrote: > Ryan, Wan-Teh, > > It's late here so I'll only get to addressing your comments tomorrow but I > was > hoping you could advise me on the ASN.1 approach meanwhile. > > Sorry for not being clear about the motivation behind rolling my own > parser - > the reason is that, while NSS does have an OCSP response decoder, it > provides no > API access to the internal response structure. > It looks like Wan-Teh made a similar comment. NSS exposes the ASN.1 functions (QuickDER et all). We should use those; if necessary, duplicating the bits from NSS in Chrome until we can land them upstream. I'm happy to help with those if you need. > > (FYI, I've checked and OpenSSL, when we get to it, does have an API into > response extensions.) > > If I'm not allowed to use asn1_util, then what do you recommend I do? > > Thanks, > Emilia > > > On 2013/12/03 21:07:15, Ryan Sleevi wrote: > > https://codereview.chromium.org/92443002/diff/40001/net/ > socket/ssl_client_socket_nss.cc > >> File net/socket/ssl_client_socket_nss.cc (right): >> > > > https://codereview.chromium.org/92443002/diff/40001/net/ > socket/ssl_client_socket_nss.cc#newcode2436 > >> net/socket/ssl_client_socket_nss.cc:2436: if >> > (nss_handshake_state_.server_cert) > >> { >> On 2013/12/03 21:04:25, wtc wrote: >> > >> > Hmm... I wonder why we check nss_handshake_state_.server_cert for null >> in >> > the > >> > OS_WIN case but not in the USE_NSS case. >> > >> > This comes from the original code, so you don't need to deal with this. >> > > Because Windows will fail to parse certificates that don't map into >> Windows >> structures (eg: FILETIME date ranges), but NSS will. >> > > http://crbug.com/91341 >> > > Also, Chromoting (which doesn't have and can't use CAPI), but did use this >> > code > >> with pseudotcpsocket >> > > > > https://codereview.chromium.org/92443002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Ryan, thanks a lot for offering help! I've attempted it myself, I would appreciate a careful review though :) A lot has changed so here's a guide: - I've lifted the (simplified) templates from NSS OCSP code into ct_objects_extractor_nss.cc - I also went ahead and implemented a proper issuer key hash check. Since we do not have an ordered chain until after cert verify is done, this means I had to move the extraction to a later stage. I think it fits there better, anyway. - Testing is now broken up more modularly: ssl_client_socket_unittest.cc tests that the appropriate blobs are received during the handshake; ct_objects_extractor_unittest.cc tests that the OCSP blob is appropriately decoded. I reverted, for now, the changes in minica.py to embed SCT lists in an OCSP response (right after creating my testdata :)). I've kept the stapling patch though as it allows to test that CT triggers a stapling request. Thanks a lot for all the guidance so far, Emilia https://codereview.chromium.org/92443002/diff/40001/net/cert/asn1_util.cc File net/cert/asn1_util.cc (right): https://codereview.chromium.org/92443002/diff/40001/net/cert/asn1_util.cc#new... net/cert/asn1_util.cc:579: extn_id.size() != 0)) { On 2013/12/03 21:04:25, wtc wrote: > > BUG: extn_id.size() != 0)) => extn_id.size()) != 0) > > By the way, a bug of this kind was just recently found by PVS-Studio in > Chromium: https://codereview.chromium.org/99423002/ > Otherwise I don't think I would have noticed it. This has gone away but... YIKES. Good catch. https://codereview.chromium.org/92443002/diff/40001/net/cert/asn1_util.cc#new... net/cert/asn1_util.cc:584: if (!GetElement(&extension, kBOOLEAN, NULL)) On 2013/12/03 21:04:25, wtc wrote: > > IMPORTANT: Should we allow |critical| to be optional since it has a DEFAULT > value? Compare this with lines 253-254. No, this was a bug that tests didn't catch because I introduced it both here and in the test data :( I've fixed the test data and the decoding template for extensions now comes directly from NSS. https://codereview.chromium.org/92443002/diff/40001/net/cert/ct_objects_extra... File net/cert/ct_objects_extractor.h (right): https://codereview.chromium.org/92443002/diff/40001/net/cert/ct_objects_extra... net/cert/ct_objects_extractor.h:64: X509Certificate::OSCertHandle leaf, On 2013/12/03 21:04:25, wtc wrote: > > Nit: If CT can also be used for CA certificates, I suggest renaming this > parameter |cert| to be more general. > > If you rename it, you should update the comment "the serial number of the leaf > certificate" on lines 50-51 accordingly. I've changed it to take a serial number directly. https://codereview.chromium.org/92443002/diff/40001/net/cert/ct_objects_extra... File net/cert/ct_objects_extractor_nss.cc (right): https://codereview.chromium.org/92443002/diff/40001/net/cert/ct_objects_extra... net/cert/ct_objects_extractor_nss.cc:276: NSSCertWrapper leaf_cert(leaf); On 2013/12/03 21:04:25, wtc wrote: > > Nit: this is an expensive way to get the serial number from the OSCertHandle > |leaf|. NSSCertWrapper will not only decode the certificate but also add it to > NSS's internal hash tables of certificates. > > This is OK for now. You can consider copying the first few lines of code from > SeekToSPKI in net/cert/asn1_util.cc. (X509Certificate::GetDEREncoded can be used > to get the DER-encoded certificate.) Also, if you actually have an > X509Certificate object, you can use the serial_number() getter method. > > Update: I found that the two callers of this function (the unit test and > ssl_client_socket_nss.cc) have an X509Certificate object. So I suggest that you > change this function to take |cert_serial_number| instead of |leaf| as input, > and change the callers to pass X509Certificate::serial_number() to this > function. > > However, having |leaf| gives us more flexibility because we will be able to get > the issuer from |leaf| for more accurate matching. I wasn't really thinking what I was doing :/ I've changed it to take a serial number and, separately, an issuer. https://codereview.chromium.org/92443002/diff/40001/net/cert/ct_objects_extra... net/cert/ct_objects_extractor_nss.cc:282: if (!asn1::ExtractSCTExtensionFromOCSPResponse(ocsp_response, On 2013/12/03 21:03:18, Ryan Sleevi wrote: > We should be using the NSS ASN.1 functions for this. Done... https://codereview.chromium.org/92443002/diff/40001/net/socket/ssl_client_soc... File net/socket/ssl_client_socket_nss.cc (right): https://codereview.chromium.org/92443002/diff/40001/net/socket/ssl_client_soc... net/socket/ssl_client_socket_nss.cc:771: #ifdef SSL_ENABLE_OCSP_STAPLING On 2013/12/03 21:04:25, wtc wrote: > > Nit: can you take the opportunity to remove all #ifdef SSL_ENABLE_OCSP_STAPLING > from this file? > > We used to be careful about that, so that we can compile this file against an > older version of the system NSS SSL library. But we no longer support that. Done. https://codereview.chromium.org/92443002/diff/40001/net/socket/ssl_client_soc... net/socket/ssl_client_socket_nss.cc:2426: ocsp_responses->items[0].len); On 2013/12/03 21:04:25, wtc wrote: > > We are copying an OCSP response here. This seems to argue for changing > ct::ExtractSCTListFromOCSPResponse() to take a base::StringPiece instead of a > std::string as the |ocsp_response| input. This has changed - I now store the raw response and parse it differently, later on. https://codereview.chromium.org/92443002/diff/40001/net/socket/ssl_client_soc... File net/socket/ssl_client_socket_unittest.cc (right): https://codereview.chromium.org/92443002/diff/40001/net/socket/ssl_client_soc... net/socket/ssl_client_socket_unittest.cc:1906: ssl_options.signed_cert_timestamps_tls_ext = "test2"; On 2013/12/03 21:04:25, wtc wrote: > > BUG: I think you meant to assign "test2" to > ssl_options.signed_cert_timestamps_ocsp. I did but this has gone away as I've reverted WereSignedCertTimestampsReceived() to test the TLS extension case only. https://codereview.chromium.org/92443002/diff/40001/net/test/spawned_test_ser... File net/test/spawned_test_server/base_test_server.cc (right): https://codereview.chromium.org/92443002/diff/40001/net/test/spawned_test_ser... net/test/spawned_test_server/base_test_server.cc:405: return false; On 2013/12/03 21:04:25, wtc wrote: > > Nit: add curly braces here and also for the _ocsp code below. Done. https://codereview.chromium.org/92443002/diff/40001/net/tools/testserver/test... File net/tools/testserver/testserver.py (right): https://codereview.chromium.org/92443002/diff/40001/net/tools/testserver/test... net/tools/testserver/testserver.py:180: OCSPResponse = self.ocsp_response) On 2013/12/03 21:04:25, wtc wrote: > > Nit: should "OCSPResponse" start in lowercase "ocspResponse"? Done. https://codereview.chromium.org/92443002/diff/40001/net/tools/testserver/test... net/tools/testserver/testserver.py:1933: # will simply ignore the extension. On 2013/12/03 21:04:25, wtc wrote: > > Is this comment true? If I get an OCSP response from an OCSP responder, should I > ignore the SCT extension in the OCSP response? I've reverted the extension code here as the testing is now broken up between receiving a stapled response and decoding it. But yes, this comment is true. CT must be side-channel free so we only accept stapled responses. https://codereview.chromium.org/92443002/diff/40001/net/tools/testserver/test... net/tools/testserver/testserver.py:1953: ocsp_der) On 2013/12/03 21:04:25, wtc wrote: > > Should we use self.__ocsp_server.ocsp_response here instead? This will make it > unnecessary to move the "ocsp_der = None" line to the beginning of this > function. Changed. https://codereview.chromium.org/92443002/diff/40001/third_party/tlslite/tlsli... File third_party/tlslite/tlslite/TLSConnection.py (right): https://codereview.chromium.org/92443002/diff/40001/third_party/tlslite/tlsli... third_party/tlslite/tlslite/TLSConnection.py:941: OCSPResponse=None): On 2013/12/03 21:04:25, wtc wrote: > > Nit: I suggest "ocspResponse" to match the capitalization style of other > parameters. Done. https://codereview.chromium.org/92443002/diff/40001/third_party/tlslite/tlsli... third_party/tlslite/tlslite/TLSConnection.py:1021: Note that the response is sent independent of the ClientHello extension On 2013/12/03 21:04:25, wtc wrote: > > Nit: add "status_request" between "ClientHello" and "extension". Also on line > 1024. Done. https://codereview.chromium.org/92443002/diff/40001/third_party/tlslite/tlsli... third_party/tlslite/tlslite/TLSConnection.py:1045: OCSPResponse=None): On 2013/12/03 21:04:25, wtc wrote: > > Nit: ocspResponse. Done. https://codereview.chromium.org/92443002/diff/40001/third_party/tlslite/tlsli... third_party/tlslite/tlslite/TLSConnection.py:1074: OCSPResponse): On 2013/12/03 21:04:25, wtc wrote: > > Nit: ocspResponse. Done. https://codereview.chromium.org/92443002/diff/40001/third_party/tlslite/tlsli... third_party/tlslite/tlslite/TLSConnection.py:1451: if clientHello.status_request and OCSPResponse: On 2013/12/03 21:04:25, wtc wrote: > > Nit: we may be able to just test serverHello.status_request here. Done.
I just focused on the NSS ASN.1 code. It matches what's in NSS (Wan-Teh, they begin around http://mxr.mozilla.org/nss/source/lib/certhigh/ocsp.c#1144 ) and look good. A few notes below. https://codereview.chromium.org/92443002/diff/80001/net/cert/ct_objects_extra... File net/cert/ct_objects_extractor_nss.cc (right): https://codereview.chromium.org/92443002/diff/80001/net/cert/ct_objects_extra... net/cert/ct_objects_extractor_nss.cc:212: struct ResponseBytes { nit: OcspResponseBytes? OCSPResponseBytes? https://codereview.chromium.org/92443002/diff/80001/net/cert/ct_objects_extra... net/cert/ct_objects_extractor_nss.cc:464: sct_list->clear(); Just as a minor nit, our normal pattern in net for the "things that take an out param" is "don't touch the out param unless things succeed" The benefit to *not* clearing is that you have the chance to avoid an additional memory allocation on line 542-544, since sct_list may have a buffer large enough to contain the new extension. If you're trying to minimize runtime memory overhead, however, we should be using the string-swap trick (per Meyers' "Effective STL", 6th printing, pp78). That is std::string result ... // hack hack hack sct_list->swap(result); return true; // sct_list is optimally sized to just large enough to hold the result, while result is deallocated. Otherwise, if |sct_list| was larger than |result| and you just did *sct_list = result, the copy would mean the original (larger) buffer of sct_list is still kept around https://codereview.chromium.org/92443002/diff/80001/net/cert/ct_objects_extra... net/cert/ct_objects_extractor_nss.cc:511: return false; If |issuer| has been verified as trusted already (which it should be, if we're at the point of SCT validation, right?), then we *can* use the asn1_util ExtractSPKIFromDERCert and avoid the NSS parse overhead. https://codereview.chromium.org/92443002/diff/80001/net/cert/ct_objects_extra... net/cert/ct_objects_extractor_nss.cc:521: std::string(reinterpret_cast<char*>(spk.data), spk.len)); This may/will change soon ( https://bugzilla.mozilla.org/show_bug.cgi?id=663315 ) https://codereview.chromium.org/92443002/diff/80001/net/cert/ct_objects_extra... net/cert/ct_objects_extractor_nss.cc:532: if (match == NULL) dominant style in net/ and base is if (!match) return false; See also line 506 That said, it's a nit. https://codereview.chromium.org/92443002/diff/80001/net/cert/ct_objects_extra... net/cert/ct_objects_extractor_nss.cc:540: wrap_cert.extensions = match->single_extensions; I'm really nervous about this, even though it will "just work" in the current version of NSS. I have no objections to making GetOctetStringExtension take CERTCertExtension** and just doing the GetExtension loop ourself See http://mxr.mozilla.org/nss/source/lib/certdb/certxutl.c#24
PTAL I'm still trying to figure out how to get the ASN.1 templates to link correctly on Win. Other than that, addressed all your comments. https://codereview.chromium.org/92443002/diff/80001/net/cert/ct_objects_extra... File net/cert/ct_objects_extractor_nss.cc (right): https://codereview.chromium.org/92443002/diff/80001/net/cert/ct_objects_extra... net/cert/ct_objects_extractor_nss.cc:212: struct ResponseBytes { On 2013/12/04 20:30:26, Ryan Sleevi wrote: > nit: OcspResponseBytes? OCSPResponseBytes? It's ResponseBytes in the RFC so I'd keep it; there isn't anything else in this file you could confuse it with afaict. https://codereview.chromium.org/92443002/diff/80001/net/cert/ct_objects_extra... net/cert/ct_objects_extractor_nss.cc:464: sct_list->clear(); Removed the clear() and GetOctetStringExtension does the rest. On 2013/12/04 20:30:26, Ryan Sleevi wrote: > Just as a minor nit, our normal pattern in net for the "things that take an out > param" is "don't touch the out param unless things succeed" > > The benefit to *not* clearing is that you have the chance to avoid an additional > memory allocation on line 542-544, since sct_list may have a buffer large enough > to contain the new extension. > > If you're trying to minimize runtime memory overhead, however, we should be > using the string-swap trick (per Meyers' "Effective STL", 6th printing, pp78). > That is > > std::string result > ... // hack hack hack > sct_list->swap(result); > return true; > > // sct_list is optimally sized to just large enough to hold the result, while > result is deallocated. Otherwise, if |sct_list| was larger than |result| and you > just did *sct_list = result, the copy would mean the original (larger) buffer of > sct_list is still kept around https://codereview.chromium.org/92443002/diff/80001/net/cert/ct_objects_extra... net/cert/ct_objects_extractor_nss.cc:511: return false; On 2013/12/04 20:30:26, Ryan Sleevi wrote: > If |issuer| has been verified as trusted already (which it should be, if we're > at the point of SCT validation, right?), then we *can* use the asn1_util > ExtractSPKIFromDERCert and avoid the NSS parse overhead. Right, I forgot about GetDEREncoded. Done, even though ExtractSPKI... and it's friend ExtractSubjectPublicKey... don't quite give me what I want so I had to involve another hack, sigh. https://codereview.chromium.org/92443002/diff/80001/net/cert/ct_objects_extra... net/cert/ct_objects_extractor_nss.cc:521: std::string(reinterpret_cast<char*>(spk.data), spk.len)); On 2013/12/04 20:30:26, Ryan Sleevi wrote: > This may/will change soon ( https://bugzilla.mozilla.org/show_bug.cgi?id=663315 > ) Aha. Added SHA-256. I hope this code will be coming from upstream by the time we have to worry about SHA-384. https://codereview.chromium.org/92443002/diff/80001/net/cert/ct_objects_extra... net/cert/ct_objects_extractor_nss.cc:532: if (match == NULL) On 2013/12/04 20:30:26, Ryan Sleevi wrote: > dominant style in net/ and base is > > if (!match) > return false; > > See also line 506 > > That said, it's a nit. Done. https://codereview.chromium.org/92443002/diff/80001/net/cert/ct_objects_extra... net/cert/ct_objects_extractor_nss.cc:540: wrap_cert.extensions = match->single_extensions; On 2013/12/04 20:30:26, Ryan Sleevi wrote: > I'm really nervous about this, even though it will "just work" in the current > version of NSS. > > I have no objections to making GetOctetStringExtension take CERTCertExtension** > and just doing the GetExtension loop ourself > > See http://mxr.mozilla.org/nss/source/lib/certdb/certxutl.c#24 I have to admit that, at that point yesterday, I was just tired of running into obstacles. Done, except I've discovered another inappropriate asn1_util use here, so I've kept the original for Cert and added a manual scan for OCSP. This should also make it easier to hopefully replace with upstream API again one day.
Aha, I think the culprit is missing defs for NSS_Get_CERT_SequenceOfCertExtensionTemplate NSS_Get_SEC_GeneralizedTimeTemplate_Util NSS_Get_SEC_IntegerTemplate_Util NSS_Get_SEC_OctetStringTemplate_Util in http://src.chromium.org/viewvc/chrome/trunk/deps/third_party/nss/nss/exports_... How would I go about adding those? On 2013/12/05 16:26:48, ekasper wrote: > PTAL > > I'm still trying to figure out how to get the ASN.1 templates to link correctly > on Win. Other than that, addressed all your comments. > > https://codereview.chromium.org/92443002/diff/80001/net/cert/ct_objects_extra... > File net/cert/ct_objects_extractor_nss.cc (right): > > https://codereview.chromium.org/92443002/diff/80001/net/cert/ct_objects_extra... > net/cert/ct_objects_extractor_nss.cc:212: struct ResponseBytes { > On 2013/12/04 20:30:26, Ryan Sleevi wrote: > > nit: OcspResponseBytes? OCSPResponseBytes? > > It's ResponseBytes in the RFC so I'd keep it; there isn't anything else in this > file you could confuse it with afaict. > > https://codereview.chromium.org/92443002/diff/80001/net/cert/ct_objects_extra... > net/cert/ct_objects_extractor_nss.cc:464: sct_list->clear(); > Removed the clear() and GetOctetStringExtension does the rest. > > On 2013/12/04 20:30:26, Ryan Sleevi wrote: > > Just as a minor nit, our normal pattern in net for the "things that take an > out > > param" is "don't touch the out param unless things succeed" > > > > The benefit to *not* clearing is that you have the chance to avoid an > additional > > memory allocation on line 542-544, since sct_list may have a buffer large > enough > > to contain the new extension. > > > > If you're trying to minimize runtime memory overhead, however, we should be > > using the string-swap trick (per Meyers' "Effective STL", 6th printing, pp78). > > That is > > > > std::string result > > ... // hack hack hack > > sct_list->swap(result); > > return true; > > > > // sct_list is optimally sized to just large enough to hold the result, while > > result is deallocated. Otherwise, if |sct_list| was larger than |result| and > you > > just did *sct_list = result, the copy would mean the original (larger) buffer > of > > sct_list is still kept around > > https://codereview.chromium.org/92443002/diff/80001/net/cert/ct_objects_extra... > net/cert/ct_objects_extractor_nss.cc:511: return false; > On 2013/12/04 20:30:26, Ryan Sleevi wrote: > > If |issuer| has been verified as trusted already (which it should be, if we're > > at the point of SCT validation, right?), then we *can* use the asn1_util > > ExtractSPKIFromDERCert and avoid the NSS parse overhead. > > Right, I forgot about GetDEREncoded. Done, even though ExtractSPKI... and it's > friend ExtractSubjectPublicKey... don't quite give me what I want so I had to > involve another hack, sigh. > > https://codereview.chromium.org/92443002/diff/80001/net/cert/ct_objects_extra... > net/cert/ct_objects_extractor_nss.cc:521: > std::string(reinterpret_cast<char*>(spk.data), spk.len)); > On 2013/12/04 20:30:26, Ryan Sleevi wrote: > > This may/will change soon ( > https://bugzilla.mozilla.org/show_bug.cgi?id=663315 > > ) > > Aha. Added SHA-256. I hope this code will be coming from upstream by the time we > have to worry about SHA-384. > > https://codereview.chromium.org/92443002/diff/80001/net/cert/ct_objects_extra... > net/cert/ct_objects_extractor_nss.cc:532: if (match == NULL) > On 2013/12/04 20:30:26, Ryan Sleevi wrote: > > dominant style in net/ and base is > > > > if (!match) > > return false; > > > > See also line 506 > > > > That said, it's a nit. > > Done. > > https://codereview.chromium.org/92443002/diff/80001/net/cert/ct_objects_extra... > net/cert/ct_objects_extractor_nss.cc:540: wrap_cert.extensions = > match->single_extensions; > On 2013/12/04 20:30:26, Ryan Sleevi wrote: > > I'm really nervous about this, even though it will "just work" in the current > > version of NSS. > > > > > I have no objections to making GetOctetStringExtension take > CERTCertExtension** > > and just doing the GetExtension loop ourself > > > > See http://mxr.mozilla.org/nss/source/lib/certdb/certxutl.c#24 > > I have to admit that, at that point yesterday, I was just tired of running into > obstacles. > > Done, except I've discovered another inappropriate asn1_util use here, so I've > kept the original for Cert and added a manual scan for OCSP. This should also > make it easier to hopefully replace with upstream API again one day.
On 2013/12/05 18:12:53, ekasper wrote: > Aha, I think the culprit is missing defs for > > NSS_Get_CERT_SequenceOfCertExtensionTemplate > NSS_Get_SEC_GeneralizedTimeTemplate_Util > NSS_Get_SEC_IntegerTemplate_Util > NSS_Get_SEC_OctetStringTemplate_Util > > in > > http://src.chromium.org/viewvc/chrome/trunk/deps/third_party/nss/nss/exports_... > > How would I go about adding those? It's a two-step process. I'll take care of that. The first step is at https://codereview.chromium.org/109413002/.
Review comments on patch set 8: I only reviewed ct_objects_extractor.h and ct_objects_extractor_nss.cc. https://codereview.chromium.org/92443002/diff/140001/net/cert/ct_objects_extr... File net/cert/ct_objects_extractor.h (right): https://codereview.chromium.org/92443002/diff/140001/net/cert/ct_objects_extr... net/cert/ct_objects_extractor.h:51: // and issuer, returns true, updating |*sct_list| to contain Nit: "issuer and serial number" is the common way to say this. It would be nice to also swap the first two parameters to match this order. https://codereview.chromium.org/92443002/diff/140001/net/cert/ct_objects_extr... File net/cert/ct_objects_extractor_nss.cc (right): https://codereview.chromium.org/92443002/diff/140001/net/cert/ct_objects_extr... net/cert/ct_objects_extractor_nss.cc:11: #include <secdert.h> The xxxt.h headers define types. The corresponding xxx.h headers always include xxxt.h. So <certt.h> and <secasn1t.h> are not needed. https://codereview.chromium.org/92443002/diff/140001/net/cert/ct_objects_extr... net/cert/ct_objects_extractor_nss.cc:75: // The wire form of the OID 1.3.6.1.4.1.11129.2.4.2. See Section 3.3 of Typo: the OCSP extension OID should be 1.3.6.1.4.1.11129.2.4.5 (ending in .5). https://codereview.chromium.org/92443002/diff/140001/net/cert/ct_objects_extr... net/cert/ct_objects_extractor_nss.cc:78: 0x79, 0x02, 0x04, 0x05}; Nit: it would be nice to fold the previous line before "0xD6," to match the formatting of lines 69-70. https://codereview.chromium.org/92443002/diff/140001/net/cert/ct_objects_extr... net/cert/ct_objects_extractor_nss.cc:117: // it is currently left as INVALID_CERT_EXTENSION. Nit: this comment doesn't really apply to the OCSP extension. It would be nice to update it. The main point is that we don't need NSS to use it. We just need NSS to be able to decode it. https://codereview.chromium.org/92443002/diff/140001/net/cert/ct_objects_extr... net/cert/ct_objects_extractor_nss.cc:165: // Obtains the data for an X.509v3 certificate extension identified by |oid| Typo: |oid| => |tag| https://codereview.chromium.org/92443002/diff/140001/net/cert/ct_objects_extr... net/cert/ct_objects_extractor_nss.cc:167: // |extensions|, updating |ext_data| to be the extension data after removing Typo: |ext_data| => |extension_data| https://codereview.chromium.org/92443002/diff/140001/net/cert/ct_objects_extr... net/cert/ct_objects_extractor_nss.cc:169: bool GetOctetStringExtension(PLArenaPool* arena, OCTET STRING is trivial to decode. If this function uses asn1::GetElement(asn1::kOCTETSTRING) to decode OCTET STRING, then this function won't need the |arena| parameter, and we can remove the GetCertOctetStringExtension function -- just call this function with |cert->extensions| as the extensions argument. https://codereview.chromium.org/92443002/diff/140001/net/cert/ct_objects_extr... net/cert/ct_objects_extractor_nss.cc:170: CERTCertExtension** extensions, Nit: this can be const: const CERTCertExtension** extensions, https://codereview.chromium.org/92443002/diff/140001/net/cert/ct_objects_extr... net/cert/ct_objects_extractor_nss.cc:175: SECOidData* oid = SECOID_FindOIDByTag(tag); If we directly pass the SECItem of oid bytes to this function (equivalent to &oid->oid), then we don't need to register the OID with NSS, and we won't need CTInitSingleton. This will require updating Eran's code, so you don't need to make this change. https://codereview.chromium.org/92443002/diff/140001/net/cert/ct_objects_extr... net/cert/ct_objects_extractor_nss.cc:179: CERTCertExtension *match = NULL; Nit: search for " *" in this file and move the '*' to be adjacent to the type. https://codereview.chromium.org/92443002/diff/140001/net/cert/ct_objects_extr... net/cert/ct_objects_extractor_nss.cc:184: if (result == SECEqual) { You can say if (SECITEM_ItemsAreEqual(&oid->oid, &ext->id)) { https://codereview.chromium.org/92443002/diff/140001/net/cert/ct_objects_extr... net/cert/ct_objects_extractor_nss.cc:207: Nit: delete one blank line. https://codereview.chromium.org/92443002/diff/140001/net/cert/ct_objects_extr... net/cert/ct_objects_extractor_nss.cc:593: if (CertIDsMatch(cert_serial_number, issuer_key_sha1_hash, With more work, we can compute the issuer key hash on demand and cache it. Right now the SHA-256 hash is computed in vain. You can just add a TODO comment.
https://codereview.chromium.org/92443002/diff/140001/net/cert/ct_objects_extr... File net/cert/ct_objects_extractor.h (right): https://codereview.chromium.org/92443002/diff/140001/net/cert/ct_objects_extr... net/cert/ct_objects_extractor.h:51: // and issuer, returns true, updating |*sct_list| to contain On 2013/12/08 04:52:45, wtc wrote: > > Nit: "issuer and serial number" is the common way to say this. > It would be nice to also swap the first two parameters to > match this order. Done. https://codereview.chromium.org/92443002/diff/140001/net/cert/ct_objects_extr... File net/cert/ct_objects_extractor_nss.cc (right): https://codereview.chromium.org/92443002/diff/140001/net/cert/ct_objects_extr... net/cert/ct_objects_extractor_nss.cc:11: #include <secdert.h> On 2013/12/08 04:52:45, wtc wrote: > > The xxxt.h headers define types. The corresponding xxx.h headers > always include xxxt.h. So <certt.h> and <secasn1t.h> are not needed. Done. secdert.h is also no longer needed in the latest patch. https://codereview.chromium.org/92443002/diff/140001/net/cert/ct_objects_extr... net/cert/ct_objects_extractor_nss.cc:75: // The wire form of the OID 1.3.6.1.4.1.11129.2.4.2. See Section 3.3 of On 2013/12/08 04:52:45, wtc wrote: > > Typo: the OCSP extension OID should be 1.3.6.1.4.1.11129.2.4.5 (ending in .5). Done. https://codereview.chromium.org/92443002/diff/140001/net/cert/ct_objects_extr... net/cert/ct_objects_extractor_nss.cc:78: 0x79, 0x02, 0x04, 0x05}; On 2013/12/08 04:52:45, wtc wrote: > > Nit: it would be nice to fold the previous line before "0xD6," to match the > formatting of lines 69-70. Done. https://codereview.chromium.org/92443002/diff/140001/net/cert/ct_objects_extr... net/cert/ct_objects_extractor_nss.cc:165: // Obtains the data for an X.509v3 certificate extension identified by |oid| On 2013/12/08 04:52:45, wtc wrote: > > Typo: |oid| => |tag| This has gone away. https://codereview.chromium.org/92443002/diff/140001/net/cert/ct_objects_extr... net/cert/ct_objects_extractor_nss.cc:167: // |extensions|, updating |ext_data| to be the extension data after removing On 2013/12/08 04:52:45, wtc wrote: > > Typo: |ext_data| => |extension_data| Done. Sorry for all the typos! https://codereview.chromium.org/92443002/diff/140001/net/cert/ct_objects_extr... net/cert/ct_objects_extractor_nss.cc:169: bool GetOctetStringExtension(PLArenaPool* arena, On 2013/12/08 04:52:45, wtc wrote: > > OCTET STRING is trivial to decode. If this function uses > asn1::GetElement(asn1::kOCTETSTRING) to decode OCTET STRING, then this function > won't need the |arena| parameter, and we can remove the > GetCertOctetStringExtension function -- just call this function with > |cert->extensions| as the extensions argument. I am not supposed to use asn1_util for these inputs and I think it's best to be consistent about this. So I think it's better to keep two version of the GetOctet... around - one for certificates that is going through the API, the ugly manual one for OCSP that is hopefully temporary and can one day be swapped for an API version. However, you make a very good point that the singleton is not needed anymore, so I've removed it, and even removed the OID from the args. This method has only one call site, and is factored out just for readability. https://codereview.chromium.org/92443002/diff/140001/net/cert/ct_objects_extr... net/cert/ct_objects_extractor_nss.cc:170: CERTCertExtension** extensions, On 2013/12/08 04:52:45, wtc wrote: > > Nit: this can be const: > const CERTCertExtension** extensions, Hm, that's not allowed, but it can be a const CERTExtension* const* extensions. https://codereview.chromium.org/92443002/diff/140001/net/cert/ct_objects_extr... net/cert/ct_objects_extractor_nss.cc:175: SECOidData* oid = SECOID_FindOIDByTag(tag); On 2013/12/08 04:52:45, wtc wrote: > > If we directly pass the SECItem of oid bytes to this function (equivalent to > &oid->oid), then we don't need to register the OID with NSS, and we won't need > CTInitSingleton. > > This will require updating Eran's code, so you don't need to make this change. I've done this for my case (see comment above). https://codereview.chromium.org/92443002/diff/140001/net/cert/ct_objects_extr... net/cert/ct_objects_extractor_nss.cc:179: CERTCertExtension *match = NULL; On 2013/12/08 04:52:45, wtc wrote: > > Nit: search for " *" in this file and move the '*' to be adjacent to the type. Done. https://codereview.chromium.org/92443002/diff/140001/net/cert/ct_objects_extr... net/cert/ct_objects_extractor_nss.cc:184: if (result == SECEqual) { On 2013/12/08 04:52:45, wtc wrote: > > You can say > if (SECITEM_ItemsAreEqual(&oid->oid, &ext->id)) { Done. https://codereview.chromium.org/92443002/diff/140001/net/cert/ct_objects_extr... net/cert/ct_objects_extractor_nss.cc:207: On 2013/12/08 04:52:45, wtc wrote: > > Nit: delete one blank line. Done. https://codereview.chromium.org/92443002/diff/140001/net/cert/ct_objects_extr... net/cert/ct_objects_extractor_nss.cc:593: if (CertIDsMatch(cert_serial_number, issuer_key_sha1_hash, On 2013/12/08 04:52:45, wtc wrote: > > With more work, we can compute the issuer key hash on demand and cache it. Right > now the SHA-256 hash is computed in vain. > > You can just add a TODO comment. Added a TODO.
Patch set 9 LGTM. 1. Let's wrap this CL up and save bigger changes for a follow-up CL. This will ensure the high quality of code review. 2. I checked the new NSS ASN.1 code very carefully. Good job! https://codereview.chromium.org/92443002/diff/160001/net/cert/ct_objects_extr... File net/cert/ct_objects_extractor_nss.cc (right): https://codereview.chromium.org/92443002/diff/160001/net/cert/ct_objects_extr... net/cert/ct_objects_extractor_nss.cc:74: // see Section 3.3 of RFC6962. Nit: the "see" on this line is a duplicate and should be removed. https://codereview.chromium.org/92443002/diff/160001/net/cert/ct_objects_extr... net/cert/ct_objects_extractor_nss.cc:78: const SECItem kOCSPExtensionItem = { Nit: this variable name should also contain "Oid": kOCSPExtensionOidItem https://codereview.chromium.org/92443002/diff/160001/net/cert/ct_objects_extr... net/cert/ct_objects_extractor_nss.cc:158: // Obtains the data for an X.509v3 certificate extension identified by Nit: it seems wrong to call this "an X.509v3 certificate extension". Should we say "an OCSP extension" instead? https://codereview.chromium.org/92443002/diff/160001/net/cert/ct_objects_extr... net/cert/ct_objects_extractor_nss.cc:315: SECItem next_update; Should this member be a pointer because it is optional? In the NSS CERTOCSPSingleResponseStr struct, nextUpdate is a pointer. Update: I think not using a pointer is fine. I found an example of this in CERTCrl: http://mxr.mozilla.org/nss/ident?i=CERTCrlStr NSS tests crl->nextUpdate.data && crl->nextUpdate.len to determine if crl->nextUpdate is present. I also found that NSS ASN.1 has a SEC_ASN1_SKIP flag to skip elements we are not interested in: http://mxr.mozilla.org/nss/ident?i=CERT_CrlTemplateEntriesOnly Don't try to use SEC_ASN1_SKIP in this CL. This can be a TODO if you think it's worth doing. https://codereview.chromium.org/92443002/diff/160001/net/cert/ct_objects_extr... net/cert/ct_objects_extractor_nss.cc:322: // ANY because (a) NSS ASN.1 doesn't support automatic CHOICE and Not sure what you meant by "automatic CHOICE". NSS ASN.1 has SEC_ASN1_CHOICE: http://mxr.mozilla.org/nss/ident?i=SEC_ASN1_CHOICE https://codereview.chromium.org/92443002/diff/160001/net/cert/ct_objects_extr... net/cert/ct_objects_extractor_nss.cc:393: const CertID& cert_id) { 1. The NSS version of this function (ocsp_CertIDsMatch) matches both the issuer_name_hash and issuer_key_hash. We should at least add a (TODO?) comment to point that we are not matching issuer_name_hash. 2. Nit: I suggest making |cert_id| the first argument, and renaming this function "CertIDMatches", or even making this function a method of the CertID class. https://codereview.chromium.org/92443002/diff/160001/net/cert/ct_objects_extr... net/cert/ct_objects_extractor_nss.cc:564: // always 0. Do you think this is a bug in asn1::ExtractSubjectPublicKeyFromSPKI? But we can't change it without updating the existing callers. We should mention that the leading byte is the number of unused bits in the last byte of the bit string. Nit: it's a little misleading to call it a pad byte. https://codereview.chromium.org/92443002/diff/160001/net/cert/ct_objects_extr... File net/cert/ct_objects_extractor_unittest.cc (right): https://codereview.chromium.org/92443002/diff/160001/net/cert/ct_objects_extr... net/cert/ct_objects_extractor_unittest.cc:164: ocsp_response, &extracted_sct_list)); I don't understand why this is expected to return false. These arguments look exactly the same as the arguments in the previous unit test. Update: I finally spotted the test_cert vs. test_cert_ difference. We should make the difference more prominent. https://codereview.chromium.org/92443002/diff/160001/net/cert/ct_verifier.h File net/cert/ct_verifier.h (right): https://codereview.chromium.org/92443002/diff/160001/net/cert/ct_verifier.h#n... net/cert/ct_verifier.h:25: // signed_certificate_timestamp TLS extension or OCSP on the given |cert| The comment should be updated because the second parameter is now an OCSP response rather than an SCT list. But I think taking sct_list_from_ocsp is better. https://codereview.chromium.org/92443002/diff/160001/net/socket/ssl_client_so... File net/socket/ssl_client_socket_nss.cc (right): https://codereview.chromium.org/92443002/diff/160001/net/socket/ssl_client_so... net/socket/ssl_client_socket_nss.cc:2418: ocsp_responses->items[0].len); Nit: this still copies the OCSP response, when all we need is the SCT list in the OCSP response. I guess you don't want to extract the SCT list from the OCSP response here because we don't have the issuer cert yet, right? https://codereview.chromium.org/92443002/diff/160001/net/socket/ssl_client_so... net/socket/ssl_client_socket_nss.cc:3540: core_->state().stapled_ocsp_response, Nit: from an API elegance perspective, I think we should extract the SCT list from stapled_ocsp_response here, and pass sct_list_from_ocsp to cert_transparency_verifier_->Verify. https://codereview.chromium.org/92443002/diff/160001/net/socket/ssl_client_so... File net/socket/ssl_client_socket_unittest.cc (right): https://codereview.chromium.org/92443002/diff/160001/net/socket/ssl_client_so... net/socket/ssl_client_socket_unittest.cc:1874: // Enabling Signed Cert Timestamps ensures we request stapled OCSP for Nit: stapled OCSP => OCSP stapling https://codereview.chromium.org/92443002/diff/160001/net/socket/ssl_client_so... net/socket/ssl_client_socket_unittest.cc:1876: // is able to process the OCSP status itself. Nit: OCSP status => OCSP response. https://codereview.chromium.org/92443002/diff/160001/net/test/spawned_test_se... File net/test/spawned_test_server/base_test_server.h (right): https://codereview.chromium.org/92443002/diff/160001/net/test/spawned_test_se... net/test/spawned_test_server/base_test_server.h:157: std::string signed_cert_timestamps_tls_ext; Nit: maybe no need to add "_tls_ext" now? https://codereview.chromium.org/92443002/diff/160001/net/tools/testserver/tes... File net/tools/testserver/testserver.py (right): https://codereview.chromium.org/92443002/diff/160001/net/tools/testserver/tes... net/tools/testserver/testserver.py:2098: dest='signed_cert_timestamps_tls_ext', Nit: it seems that we don't need to add "-tls-ext" and "_tls_ext" now.
https://codereview.chromium.org/92443002/diff/180001/net/cert/ct_objects_extr... File net/cert/ct_objects_extractor_unittest.cc (right): https://codereview.chromium.org/92443002/diff/180001/net/cert/ct_objects_extr... net/cert/ct_objects_extractor_unittest.cc:163: issuer_cert->os_cert_handle(), subject_cert->serial_number(), You cannot use subject_cert->serial_number() in this test :-)
wtc, I think I've either addressed or documented all your comments. Should I wait for rsleevi's stamp, too? https://codereview.chromium.org/92443002/diff/160001/net/cert/ct_objects_extr... File net/cert/ct_objects_extractor_nss.cc (right): https://codereview.chromium.org/92443002/diff/160001/net/cert/ct_objects_extr... net/cert/ct_objects_extractor_nss.cc:74: // see Section 3.3 of RFC6962. On 2013/12/10 04:23:17, wtc wrote: > > Nit: the "see" on this line is a duplicate and should be removed. Done. https://codereview.chromium.org/92443002/diff/160001/net/cert/ct_objects_extr... net/cert/ct_objects_extractor_nss.cc:78: const SECItem kOCSPExtensionItem = { On 2013/12/10 04:23:17, wtc wrote: > > Nit: this variable name should also contain "Oid": kOCSPExtensionOidItem Done. https://codereview.chromium.org/92443002/diff/160001/net/cert/ct_objects_extr... net/cert/ct_objects_extractor_nss.cc:158: // Obtains the data for an X.509v3 certificate extension identified by On 2013/12/10 04:23:17, wtc wrote: > > Nit: it seems wrong to call this "an X.509v3 certificate extension". Should we > say "an OCSP extension" instead? OCSP gets its extension definition from X509, which is why we can use CERTCertExtension but, ok. https://codereview.chromium.org/92443002/diff/160001/net/cert/ct_objects_extr... net/cert/ct_objects_extractor_nss.cc:315: SECItem next_update; On 2013/12/10 04:23:17, wtc wrote: > > Should this member be a pointer because it is optional? > > In the NSS CERTOCSPSingleResponseStr struct, nextUpdate is a pointer. > > Update: I think not using a pointer is fine. I found an example of this in > CERTCrl: > http://mxr.mozilla.org/nss/ident?i=CERTCrlStr > > NSS tests crl->nextUpdate.data && crl->nextUpdate.len to determine if > crl->nextUpdate is present. > > I also found that NSS ASN.1 has a SEC_ASN1_SKIP flag to skip elements we are not > interested in: > http://mxr.mozilla.org/nss/ident?i=CERT_CrlTemplateEntriesOnly > > Don't try to use SEC_ASN1_SKIP in this CL. This can be a TODO if you think it's > worth doing. It should be a pointer if we used this field, but we don't. Checking for .data and .len works too except you can't distinguish between an empty item, and a missing one. I've used SEC_ASN1_SKIP_REST for skipping an unused complex structure. I didn't think skipping simple fields was important - premature optimization and all that - but I've added a TODO. https://codereview.chromium.org/92443002/diff/160001/net/cert/ct_objects_extr... net/cert/ct_objects_extractor_nss.cc:322: // ANY because (a) NSS ASN.1 doesn't support automatic CHOICE and On 2013/12/10 04:23:17, wtc wrote: > > Not sure what you meant by "automatic CHOICE". NSS ASN.1 has SEC_ASN1_CHOICE: > http://mxr.mozilla.org/nss/ident?i=SEC_ASN1_CHOICE I assumed the comment in ocsp.c was accurate: * XXX Because the ASN.1 encoder and decoder currently do not provide * a way to automatically handle a CHOICE, we need to do it in two * steps, looking at the type tag and feeding the exact choice back * to the ASN.1 code. Since SEC_ASN1_CHOICE does exist, it would be prudent to validate the CHOICE tags but I'll leave this for a follow-up. https://codereview.chromium.org/92443002/diff/160001/net/cert/ct_objects_extr... net/cert/ct_objects_extractor_nss.cc:393: const CertID& cert_id) { On 2013/12/10 04:23:17, wtc wrote: > > 1. The NSS version of this function (ocsp_CertIDsMatch) matches both the > issuer_name_hash and issuer_key_hash. We should at least add a (TODO?) comment > to point that we are not matching issuer_name_hash. > > 2. Nit: I suggest making |cert_id| the first argument, and renaming this > function "CertIDMatches", or even making this function a method of the CertID > class. 1. Added a TODO 2. Not sure why this preference, but went with it anyway and swapped arguments. I prefer not to mix the structs effectively borrowed from NSS, and our methods, though. https://codereview.chromium.org/92443002/diff/160001/net/cert/ct_objects_extr... net/cert/ct_objects_extractor_nss.cc:564: // always 0. On 2013/12/10 04:23:17, wtc wrote: > > Do you think this is a bug in asn1::ExtractSubjectPublicKeyFromSPKI? But we > can't change it without updating the existing callers. > > We should mention that the leading byte is the number of unused bits in the last > byte of the bit string. > > Nit: it's a little misleading to call it a pad byte. Clarified the meaning of the leading byte. I think the bug is in the RFC that defines the public key as BIT STRING :) It's hard to handle this elegantly though I suppose, given that we know that we're dealing with byte-aligned public keys, stripping this byte should happen in asn1_util. https://codereview.chromium.org/92443002/diff/160001/net/cert/ct_objects_extr... File net/cert/ct_objects_extractor_unittest.cc (right): https://codereview.chromium.org/92443002/diff/160001/net/cert/ct_objects_extr... net/cert/ct_objects_extractor_unittest.cc:164: ocsp_response, &extracted_sct_list)); On 2013/12/10 04:23:17, wtc wrote: > > I don't understand why this is expected to return false. These arguments look > exactly the same as the arguments in the previous unit test. > > Update: I finally spotted the test_cert vs. test_cert_ difference. We should > make the difference more prominent. I've done test_cert -> subject_cert and added a comment. https://codereview.chromium.org/92443002/diff/160001/net/cert/ct_verifier.h File net/cert/ct_verifier.h (right): https://codereview.chromium.org/92443002/diff/160001/net/cert/ct_verifier.h#n... net/cert/ct_verifier.h:25: // signed_certificate_timestamp TLS extension or OCSP on the given |cert| On 2013/12/10 04:23:17, wtc wrote: > > The comment should be updated because the second parameter is now an OCSP > response rather than an SCT list. > > But I think taking sct_list_from_ocsp is better. The comment is also wrong because it's not either/or verification, so I've updated it. https://codereview.chromium.org/92443002/diff/160001/net/socket/ssl_client_so... File net/socket/ssl_client_socket_nss.cc (right): https://codereview.chromium.org/92443002/diff/160001/net/socket/ssl_client_so... net/socket/ssl_client_socket_nss.cc:2418: ocsp_responses->items[0].len); On 2013/12/10 04:23:17, wtc wrote: > > Nit: this still copies the OCSP response, when all we need is the SCT list in > the OCSP response. > > I guess you don't want to extract the SCT list from the OCSP response here > because we don't have the issuer cert yet, right? Correct, we can't extract it yet. https://codereview.chromium.org/92443002/diff/160001/net/socket/ssl_client_so... net/socket/ssl_client_socket_nss.cc:3540: core_->state().stapled_ocsp_response, On 2013/12/10 04:23:17, wtc wrote: > > Nit: from an API elegance perspective, I think we should extract the SCT list > from stapled_ocsp_response here, and pass sct_list_from_ocsp to > cert_transparency_verifier_->Verify. I'm not sure I agree - the extraction of SCTs embedded in the cert happens in the verifier, so I think it's in fact cleaner to keep the two extractions in the same place, and not have the socket concern itself with the semantics of CT responses. https://codereview.chromium.org/92443002/diff/160001/net/socket/ssl_client_so... File net/socket/ssl_client_socket_unittest.cc (right): https://codereview.chromium.org/92443002/diff/160001/net/socket/ssl_client_so... net/socket/ssl_client_socket_unittest.cc:1874: // Enabling Signed Cert Timestamps ensures we request stapled OCSP for On 2013/12/10 04:23:17, wtc wrote: > > Nit: stapled OCSP => OCSP stapling Done. https://codereview.chromium.org/92443002/diff/160001/net/socket/ssl_client_so... net/socket/ssl_client_socket_unittest.cc:1876: // is able to process the OCSP status itself. On 2013/12/10 04:23:17, wtc wrote: > > Nit: OCSP status => OCSP response. Hm, no, I meant "OCSP status" as in revocation status. https://codereview.chromium.org/92443002/diff/160001/net/test/spawned_test_se... File net/test/spawned_test_server/base_test_server.h (right): https://codereview.chromium.org/92443002/diff/160001/net/test/spawned_test_se... net/test/spawned_test_server/base_test_server.h:157: std::string signed_cert_timestamps_tls_ext; On 2013/12/10 04:23:17, wtc wrote: > > Nit: maybe no need to add "_tls_ext" now? Since we have three methods for embedding this list, I don't think it hurts to be explicit about which one it's for. https://codereview.chromium.org/92443002/diff/160001/net/tools/testserver/tes... File net/tools/testserver/testserver.py (right): https://codereview.chromium.org/92443002/diff/160001/net/tools/testserver/tes... net/tools/testserver/testserver.py:2098: dest='signed_cert_timestamps_tls_ext', On 2013/12/10 04:23:17, wtc wrote: > > Nit: it seems that we don't need to add "-tls-ext" and "_tls_ext" now. Ditto - being explicit doesn't hurt. https://codereview.chromium.org/92443002/diff/180001/net/cert/ct_objects_extr... File net/cert/ct_objects_extractor_unittest.cc (right): https://codereview.chromium.org/92443002/diff/180001/net/cert/ct_objects_extr... net/cert/ct_objects_extractor_unittest.cc:163: issuer_cert->os_cert_handle(), subject_cert->serial_number(), On 2013/12/10 14:32:43, wtc wrote: > > You cannot use subject_cert->serial_number() in this test :-) Thanks, I uploaded too early but the test was failing, obviously. Fixed.
Patch set 11 LGTM. Don't wait for Ryan's review. You addressed his objection to rolling your own OCSP response parser, and I checked the NSS ASN.1 code very carefully. He can still do a post-commit review. I will go over your comments and respond to them later.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ekasper@google.com/92443002/200001
Failed to apply patch for net/test/spawned_test_server/base_test_server.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file net/test/spawned_test_server/base_test_server.cc
Hunk #1 FAILED at 398.
1 out of 1 hunk FAILED -- saving rejects to file
net/test/spawned_test_server/base_test_server.cc.rej
Patch: net/test/spawned_test_server/base_test_server.cc
Index: net/test/spawned_test_server/base_test_server.cc
diff --git a/net/test/spawned_test_server/base_test_server.cc
b/net/test/spawned_test_server/base_test_server.cc
index
775341b608e6bd93ffe56712b1a676a3477b9bc5..6b570b814007ebd4c198015518832495b7a9bf92
100644
--- a/net/test/spawned_test_server/base_test_server.cc
+++ b/net/test/spawned_test_server/base_test_server.cc
@@ -398,12 +398,16 @@ bool
BaseTestServer::GenerateArguments(base::DictionaryValue* arguments) const {
arguments->Set("tls-intolerant",
new base::FundamentalValue(ssl_options_.tls_intolerant));
}
- if (!ssl_options_.signed_cert_timestamps.empty()) {
- std::string b64_scts;
- if (!base::Base64Encode(ssl_options_.signed_cert_timestamps, &b64_scts))
+ if (!ssl_options_.signed_cert_timestamps_tls_ext.empty()) {
+ std::string b64_scts_tls_ext;
+ if (!base::Base64Encode(ssl_options_.signed_cert_timestamps_tls_ext,
+ &b64_scts_tls_ext)) {
return false;
- arguments->SetString("signed-cert-timestamps", b64_scts);
+ }
+ arguments->SetString("signed-cert-timestamps-tls-ext", b64_scts_tls_ext);
}
+ if (ssl_options_.staple_ocsp_response)
+ arguments->Set("staple-ocsp-response", base::Value::CreateNullValue());
}
return GenerateAdditionalArguments(arguments);
FYI this looks bad but was just a rebase race - the return type of Base64Encode
changed.
On 2013/12/10 17:04:34, I haz the power (commit-bot) wrote:
> Failed to apply patch for net/test/spawned_test_server/base_test_server.cc:
> While running patch -p1 --forward --force --no-backup-if-mismatch;
> patching file net/test/spawned_test_server/base_test_server.cc
> Hunk #1 FAILED at 398.
> 1 out of 1 hunk FAILED -- saving rejects to file
> net/test/spawned_test_server/base_test_server.cc.rej
>
> Patch: net/test/spawned_test_server/base_test_server.cc
> Index: net/test/spawned_test_server/base_test_server.cc
> diff --git a/net/test/spawned_test_server/base_test_server.cc
> b/net/test/spawned_test_server/base_test_server.cc
> index
>
775341b608e6bd93ffe56712b1a676a3477b9bc5..6b570b814007ebd4c198015518832495b7a9bf92
> 100644
> --- a/net/test/spawned_test_server/base_test_server.cc
> +++ b/net/test/spawned_test_server/base_test_server.cc
> @@ -398,12 +398,16 @@ bool
> BaseTestServer::GenerateArguments(base::DictionaryValue* arguments) const {
> arguments->Set("tls-intolerant",
> new
base::FundamentalValue(ssl_options_.tls_intolerant));
> }
> - if (!ssl_options_.signed_cert_timestamps.empty()) {
> - std::string b64_scts;
> - if (!base::Base64Encode(ssl_options_.signed_cert_timestamps,
&b64_scts))
> + if (!ssl_options_.signed_cert_timestamps_tls_ext.empty()) {
> + std::string b64_scts_tls_ext;
> + if (!base::Base64Encode(ssl_options_.signed_cert_timestamps_tls_ext,
> + &b64_scts_tls_ext)) {
> return false;
> - arguments->SetString("signed-cert-timestamps", b64_scts);
> + }
> + arguments->SetString("signed-cert-timestamps-tls-ext",
b64_scts_tls_ext);
> }
> + if (ssl_options_.staple_ocsp_response)
> + arguments->Set("staple-ocsp-response", base::Value::CreateNullValue());
> }
>
> return GenerateAdditionalArguments(arguments);
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ekasper@google.com/92443002/220001
LGTM - Wan-Teh has naturally covered in far greater detail. I just have one nit and one design concern that shouldn't be addressed in this CL, but this CL exposes a bit of. https://codereview.chromium.org/92443002/diff/220001/net/cert/multi_log_ct_ve... File net/cert/multi_log_ct_verifier_unittest.cc (right): https://codereview.chromium.org/92443002/diff/220001/net/cert/multi_log_ct_ve... net/cert/multi_log_ct_verifier_unittest.cc:160: verifier_->Verify(chain_, "", sct_list, &result, BoundNetLog())); nit: use std::string() here, not "" https://codereview.chromium.org/92443002/diff/220001/net/socket/ssl_client_so... File net/socket/ssl_client_socket.h (right): https://codereview.chromium.org/92443002/diff/220001/net/socket/ssl_client_so... net/socket/ssl_client_socket.h:139: virtual bool WasStapledOCSPResponseReceived() const; I'm not sure why we're exposing either of these (the other being WereSCTReceived) on the socket object., when they only appear to be used for test code. Minimally, if the purpose is just for testing, we should move these to private and let FRIEND_TEST (and friends, HAH) cover this. Alternatively, we should let the unittests reach somewhat deeper into the socket object. My goal here is I don't know where there's a case where the caller would call either of these two methods and do something differently. Even in the EV-with-CT-only case, we wouldn't care about these boolean states - we'd care about the validated SCTs.
Retried try job too often on win_rel for step(s) net_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
A few more nits I missed from wtc's review. https://codereview.chromium.org/92443002/diff/220001/net/cert/ct_objects_extr... File net/cert/ct_objects_extractor.h (right): https://codereview.chromium.org/92443002/diff/220001/net/cert/ct_objects_extr... net/cert/ct_objects_extractor.h:57: const std::string& ocsp_response, std::string* sct_list); style nit: line break between params https://codereview.chromium.org/92443002/diff/220001/net/cert/ct_objects_extr... File net/cert/ct_objects_extractor_nss.cc (right): https://codereview.chromium.org/92443002/diff/220001/net/cert/ct_objects_extr... net/cert/ct_objects_extractor_nss.cc:156: // manually. comment nit: avoid pronouns // NSS offers CERT_FindCertExtension for certificates, but that only accepts CERTCertificate*s, rather // than CERTCertExtensions*s, which is what OCSP extensions use. https://codereview.chromium.org/92443002/diff/220001/net/cert/ct_objects_extr... net/cert/ct_objects_extractor_nss.cc:182: // QuickDER will point to |match|, so we don't have to free |contents|. comment nit: pronouns // With QuickDER, |contents| to point into |match|, so it's not necessary to free |contents|. https://codereview.chromium.org/92443002/diff/220001/net/cert/ct_objects_extr... net/cert/ct_objects_extractor_nss.cc:517: // ok for us as the decoded items never leave the method scope. This comment doesn't make much sense, because it's talking about the item on line 525. If anything, the comment should be moved there. // |response| will point directly into |src|, so it's not necessary to free, but may only be used // while |src| is valid (eg: this method) https://codereview.chromium.org/92443002/diff/220001/net/cert/ct_objects_extr... net/cert/ct_objects_extractor_nss.cc:540: sizeof(kBasicOCSPResponseOID)) != 0) { Would this make more sense as a synthetic SECITEM with SECITEM_ItemsAreEqual instead? This at least matches the change for the extension location code (line 172) https://codereview.chromium.org/92443002/diff/220001/net/cert/ct_objects_extr... net/cert/ct_objects_extractor_nss.cc:589: for (int i = 0; responses[i] != NULL; ++i) { s/int/size_t here https://codereview.chromium.org/92443002/diff/220001/net/cert/ct_verifier.h File net/cert/ct_verifier.h (right): https://codereview.chromium.org/92443002/diff/220001/net/cert/ct_verifier.h#n... net/cert/ct_verifier.h:28: // |stapled_ocsp_response| and |sct_list_from_tls_extension| should be left I don't understand the "unused" comment here, or the sentence preceding. // A given |cert| may have restrictions on which SCT methods it can be used with or is it // If no stapled OCSP response was available, |stapled_ocsp_response| should be an empty string. // If no SCT list extension was negotiated, |sct_list_from_tls_extension| should be an empty string.
PTAL I've addressed Ryan's comments. However, OCSP object extraction fails on Win (i.e., the compile now succeeds but the *test* fails) and, I'm afraid, I've currently no idea why this is. If you have any ideas, I'd appreciate them. https://codereview.chromium.org/92443002/diff/220001/net/cert/ct_objects_extr... File net/cert/ct_objects_extractor.h (right): https://codereview.chromium.org/92443002/diff/220001/net/cert/ct_objects_extr... net/cert/ct_objects_extractor.h:57: const std::string& ocsp_response, std::string* sct_list); On 2013/12/11 07:33:05, Ryan Sleevi wrote: > style nit: line break between params Done. https://codereview.chromium.org/92443002/diff/220001/net/cert/ct_objects_extr... File net/cert/ct_objects_extractor_nss.cc (right): https://codereview.chromium.org/92443002/diff/220001/net/cert/ct_objects_extr... net/cert/ct_objects_extractor_nss.cc:156: // manually. On 2013/12/11 07:33:05, Ryan Sleevi wrote: > comment nit: avoid pronouns > > // NSS offers CERT_FindCertExtension for certificates, but that only accepts > CERTCertificate*s, rather > // than CERTCertExtensions*s, which is what OCSP extensions use. I find the plural construct a little odd but I've reworded without pronouns. https://codereview.chromium.org/92443002/diff/220001/net/cert/ct_objects_extr... net/cert/ct_objects_extractor_nss.cc:182: // QuickDER will point to |match|, so we don't have to free |contents|. On 2013/12/11 07:33:05, Ryan Sleevi wrote: > comment nit: pronouns > > // With QuickDER, |contents| to point into |match|, so it's not necessary to > free |contents|. Done. https://codereview.chromium.org/92443002/diff/220001/net/cert/ct_objects_extr... net/cert/ct_objects_extractor_nss.cc:517: // ok for us as the decoded items never leave the method scope. On 2013/12/11 07:33:05, Ryan Sleevi wrote: > This comment doesn't make much sense, because it's talking about the item on > line 525. > > If anything, the comment should be moved there. > > // |response| will point directly into |src|, so it's not necessary to free, but > may only be used > // while |src| is valid (eg: this method) Done. https://codereview.chromium.org/92443002/diff/220001/net/cert/ct_objects_extr... net/cert/ct_objects_extractor_nss.cc:540: sizeof(kBasicOCSPResponseOID)) != 0) { On 2013/12/11 07:33:05, Ryan Sleevi wrote: > Would this make more sense as a synthetic SECITEM with SECITEM_ItemsAreEqual > instead? > > This at least matches the change for the extension location code (line 172) Done. https://codereview.chromium.org/92443002/diff/220001/net/cert/ct_objects_extr... net/cert/ct_objects_extractor_nss.cc:589: for (int i = 0; responses[i] != NULL; ++i) { On 2013/12/11 07:33:05, Ryan Sleevi wrote: > s/int/size_t here I've changed it to match the style of similar loops elsewhere instead. https://codereview.chromium.org/92443002/diff/220001/net/cert/ct_verifier.h File net/cert/ct_verifier.h (right): https://codereview.chromium.org/92443002/diff/220001/net/cert/ct_verifier.h#n... net/cert/ct_verifier.h:28: // |stapled_ocsp_response| and |sct_list_from_tls_extension| should be left On 2013/12/11 07:33:05, Ryan Sleevi wrote: > I don't understand the "unused" comment here, or the sentence preceding. > > // A given |cert| may have restrictions on which SCT methods it can be used with > > or is it > > // If no stapled OCSP response was available, |stapled_ocsp_response| should be > an empty string. > // If no SCT list extension was negotiated, |sct_list_from_tls_extension| should > be an empty string. Clarified. https://codereview.chromium.org/92443002/diff/220001/net/cert/multi_log_ct_ve... File net/cert/multi_log_ct_verifier_unittest.cc (right): https://codereview.chromium.org/92443002/diff/220001/net/cert/multi_log_ct_ve... net/cert/multi_log_ct_verifier_unittest.cc:160: verifier_->Verify(chain_, "", sct_list, &result, BoundNetLog())); On 2013/12/10 21:06:09, Ryan Sleevi wrote: > nit: use std::string() here, not "" I didn't introduce this but I've changed it everywhere in this file now. https://codereview.chromium.org/92443002/diff/220001/net/socket/ssl_client_so... File net/socket/ssl_client_socket.h (right): https://codereview.chromium.org/92443002/diff/220001/net/socket/ssl_client_so... net/socket/ssl_client_socket.h:139: virtual bool WasStapledOCSPResponseReceived() const; On 2013/12/10 21:06:09, Ryan Sleevi wrote: > I'm not sure why we're exposing either of these (the other being > WereSCTReceived) on the socket object., when they only appear to be used for > test code. > > Minimally, if the purpose is just for testing, we should move these to private > and let FRIEND_TEST (and friends, HAH) cover this. > > Alternatively, we should let the unittests reach somewhat deeper into the socket > object. > > My goal here is I don't know where there's a case where the caller would call > either of these two methods and do something differently. Even in the > EV-with-CT-only case, we wouldn't care about these boolean states - we'd care > about the validated SCTs. They were for testing indeed - I've removed them and given direct access to the private members via FRIEND_TEST_ALL_PREFIXES which appears to be the chromium version of FRIEND_TEST, right?
Emilia, I responded to your comments below. I'll let Ryan review your new patch sets. I am having reviewer's fatigue for this CL now :-) I'll try to debug the Windows test failure. https://codereview.chromium.org/92443002/diff/160001/net/cert/ct_objects_extr... File net/cert/ct_objects_extractor_nss.cc (right): https://codereview.chromium.org/92443002/diff/160001/net/cert/ct_objects_extr... net/cert/ct_objects_extractor_nss.cc:315: SECItem next_update; On 2013/12/10 14:45:20, ekasper wrote: > > Checking for .data and .len works too except you can't > distinguish between an empty item, and a missing one. Yes, I remember this now. I am impressed by how much you figured out about NSS ASN.1 templates on your own. > I didn't think skipping simple fields was important - > premature optimization and all that - but I've added a > TODO. I didn't realize you chose not to skip simple fields. You can remove the TODO comment. https://codereview.chromium.org/92443002/diff/160001/net/cert/ct_objects_extr... net/cert/ct_objects_extractor_nss.cc:322: // ANY because (a) NSS ASN.1 doesn't support automatic CHOICE and On 2013/12/10 14:45:20, ekasper wrote: > > I assumed the comment in ocsp.c was accurate: > * XXX Because the ASN.1 encoder and decoder currently do not provide > * a way to automatically handle a CHOICE, we need to do it in two > * steps, looking at the type tag and feeding the exact choice back > * to the ASN.1 code. I see. I found that this comment and the other XXX comments asserting the lack of CHOICE support all came from the initial open-source revision of NSS. So it's possible that this code was written before NSS could decode CHOICE, or simply that the author of this code didn't know CHOICE was supported. https://codereview.chromium.org/92443002/diff/160001/net/cert/ct_objects_extr... net/cert/ct_objects_extractor_nss.cc:393: const CertID& cert_id) { On 2013/12/10 14:45:20, ekasper wrote: > > 2. Not sure why this preference, but went with it anyway > and swapped arguments. When I first read the CertIDsMatch, I was confused why it's named "CertIDs (plural) match" even though there was only one CertID argument. That's why I thought the CertID argument should be listed more prominently because the function name said "CertIDs". Later I checked the NSS ocsp.c code and found this function was inspired by the NSS ocsp_CertIDsMatch function. Then it all became clear to me. But for the benefit of future maintainers don't know the history of this code, it's better for this function to have a name that matches its current form. https://codereview.chromium.org/92443002/diff/160001/net/cert/ct_objects_extr... net/cert/ct_objects_extractor_nss.cc:564: // always 0. I have to admit that when I checked the OCSP RFC 6960, I could not understand exactly what should be hashed: o issuerKeyHash is the hash of the issuer's public key. The hash shall be calculated over the value (excluding tag and length) of the subject public key field in the issuer's certificate. I later found a more accurate description of a similar hash in RFC 5280: (1) The keyIdentifier is composed of the 160-bit SHA-1 hash of the value of the BIT STRING subjectPublicKey (excluding the tag, length, and number of unused bits). It turns out the OCSP RFC also has something similar: KeyHash ::= OCTET STRING -- SHA-1 hash of responder's public key -- (i.e., the SHA-1 hash of the value of the -- BIT STRING subjectPublicKey [excluding -- the tag, length, and number of unused -- bits] in the responder's certificate) https://codereview.chromium.org/92443002/diff/160001/net/socket/ssl_client_so... File net/socket/ssl_client_socket_nss.cc (right): https://codereview.chromium.org/92443002/diff/160001/net/socket/ssl_client_so... net/socket/ssl_client_socket_nss.cc:3540: core_->state().stapled_ocsp_response, On 2013/12/10 14:45:20, ekasper wrote: > > I'm not sure I agree - the extraction of SCTs embedded in the cert happens in > the verifier, so I think it's in fact cleaner to keep the two extractions in the > same place, and not have the socket concern itself with the semantics of CT > responses. Your argument is also reasonable. Perhaps we should erase core_->state().stapled_ocsp_response and core_->state().sct_list_from_tls_extension after CT verification, to conserve memory? https://codereview.chromium.org/92443002/diff/160001/net/socket/ssl_client_so... File net/socket/ssl_client_socket_unittest.cc (right): https://codereview.chromium.org/92443002/diff/160001/net/socket/ssl_client_so... net/socket/ssl_client_socket_unittest.cc:1876: // is able to process the OCSP status itself. On 2013/12/10 14:45:20, ekasper wrote: > > Hm, no, I meant "OCSP status" as in revocation status. I see. I found "OCSP status" a little ambiguous because it could mean OCSPResponseStatus (successful, malformedRequest, etc.) or CertStatus (good, revoked, unknown). In any case, this is not a problem.
Emilia: I struggled a whole day to check out a Chromium tree successfully on Windows. I know how to fix the Windows unit test failure of CTObjectsExtractorTest.ExtractSCTListFromOCSPResponse. I hope that's the test failure you referred to. https://codereview.chromium.org/92443002/diff/340001/net/cert/ct_objects_extr... File net/cert/ct_objects_extractor_nss.cc (right): https://codereview.chromium.org/92443002/diff/340001/net/cert/ct_objects_extr... net/cert/ct_objects_extractor_nss.cc:346: SEC_ASN1_CONTEXT_SPECIFIC | 1, The test failed because of an assertion failure in NSS quickder.c. We need set SEC_ASN1_XTRN here. I think the rule is that whenever we use SEC_ASN1_SUB, we should also set SEC_ASN1_XTRN.
https://codereview.chromium.org/92443002/diff/160001/net/cert/ct_objects_extr... File net/cert/ct_objects_extractor_nss.cc (right): https://codereview.chromium.org/92443002/diff/160001/net/cert/ct_objects_extr... net/cert/ct_objects_extractor_nss.cc:315: SECItem next_update; On 2013/12/12 02:31:45, wtc wrote: > > On 2013/12/10 14:45:20, ekasper wrote: > > > > Checking for .data and .len works too except you can't > > distinguish between an empty item, and a missing one. > > Yes, I remember this now. I am impressed by how much you figured out about NSS > ASN.1 templates on your own. > > > I didn't think skipping simple fields was important - > > premature optimization and all that - but I've added a > > TODO. > > I didn't realize you chose not to skip simple fields. You > can remove the TODO comment. Done. https://codereview.chromium.org/92443002/diff/160001/net/cert/ct_objects_extr... net/cert/ct_objects_extractor_nss.cc:393: const CertID& cert_id) { On 2013/12/12 02:31:45, wtc wrote: > > On 2013/12/10 14:45:20, ekasper wrote: > > > > 2. Not sure why this preference, but went with it anyway > > and swapped arguments. > > When I first read the CertIDsMatch, I was confused why it's named "CertIDs > (plural) match" even though there was only one CertID argument. That's why I > thought the CertID argument should be listed more prominently because the > function name said "CertIDs". > > Later I checked the NSS ocsp.c code and found this function was inspired by the > NSS ocsp_CertIDsMatch function. Then it all became clear to me. But for the > benefit of future maintainers don't know the history of this code, it's better > for this function to have a name that matches its current form. Renamed to CertIDMatches. https://codereview.chromium.org/92443002/diff/160001/net/cert/ct_objects_extr... net/cert/ct_objects_extractor_nss.cc:564: // always 0. On 2013/12/12 02:31:45, wtc wrote: > > I have to admit that when I checked the OCSP RFC 6960, I could not understand > exactly what should be hashed: > > o issuerKeyHash is the hash of the issuer's public key. The hash > shall be calculated over the value (excluding tag and length) of > the subject public key field in the issuer's certificate. Yep, the RFC is ambiguous there. > > I later found a more accurate description of a similar hash in RFC 5280: > > (1) The keyIdentifier is composed of the 160-bit SHA-1 hash of the > value of the BIT STRING subjectPublicKey (excluding the tag, > length, and number of unused bits). > > It turns out the OCSP RFC also has something similar: > > KeyHash ::= OCTET STRING -- SHA-1 hash of responder's public key > -- (i.e., the SHA-1 hash of the value of the > -- BIT STRING subjectPublicKey [excluding > -- the tag, length, and number of unused > -- bits] in the responder's certificate) https://codereview.chromium.org/92443002/diff/160001/net/socket/ssl_client_so... File net/socket/ssl_client_socket_nss.cc (right): https://codereview.chromium.org/92443002/diff/160001/net/socket/ssl_client_so... net/socket/ssl_client_socket_nss.cc:3540: core_->state().stapled_ocsp_response, On 2013/12/12 02:31:45, wtc wrote: > > On 2013/12/10 14:45:20, ekasper wrote: > > > > I'm not sure I agree - the extraction of SCTs embedded in the cert happens in > > the verifier, so I think it's in fact cleaner to keep the two extractions in > the > > same place, and not have the socket concern itself with the semantics of CT > > responses. > > Your argument is also reasonable. > > Perhaps we should erase core_->state().stapled_ocsp_response and > core_->state().sct_list_from_tls_extension after CT verification, to conserve > memory? Good point, we could. I've added a TODO because I'm starting to get tunnel vision for this CL and don't want to make functional changes at the moment.
Wow, thanks so much! That was it. I was about to get a Win box myself - I got as far as realizing that the SEC_ASN1_SUB templates were corrupt but I just couldn't spot why. As far as I'm concerned, this should be good to go now but I'll wait for a fresh stamp from either of you. It would be nice to get it in today if at all possible, both to meet M33, and to avoid the patch going stale again. On 2013/12/13 03:32:50, wtc wrote: > Emilia: I struggled a whole day to check out a Chromium tree successfully on > Windows. I know how to fix the Windows unit test failure of > CTObjectsExtractorTest.ExtractSCTListFromOCSPResponse. I hope that's the test > failure you referred to. > > https://codereview.chromium.org/92443002/diff/340001/net/cert/ct_objects_extr... > File net/cert/ct_objects_extractor_nss.cc (right): > > https://codereview.chromium.org/92443002/diff/340001/net/cert/ct_objects_extr... > net/cert/ct_objects_extractor_nss.cc:346: SEC_ASN1_CONTEXT_SPECIFIC | 1, > > The test failed because of an assertion failure in NSS quickder.c. > > We need set SEC_ASN1_XTRN here. I think the rule is that whenever we use > SEC_ASN1_SUB, we should also set SEC_ASN1_XTRN.
Patch set 20 LGTM. https://codereview.chromium.org/92443002/diff/380001/net/socket/ssl_client_so... File net/socket/ssl_client_socket.h (right): https://codereview.chromium.org/92443002/diff/380001/net/socket/ssl_client_so... net/socket/ssl_client_socket.h:164: ConnectSignedCertTimestampsDisabled); Nit: add a blank line to separate the friend test declarations from the data members. https://codereview.chromium.org/92443002/diff/380001/net/test/spawned_test_se... File net/test/spawned_test_server/base_test_server.cc (right): https://codereview.chromium.org/92443002/diff/380001/net/test/spawned_test_se... net/test/spawned_test_server/base_test_server.cc:78: signed_cert_timestamps_tls_ext(std::string()), Delete the signed_cert_timestamps_tls_ext initializers in the SSLOptions constructors (lines 65 and this line). They are not necessary. https://codereview.chromium.org/92443002/diff/380001/net/test/spawned_test_se... File net/test/spawned_test_server/base_test_server.h (right): https://codereview.chromium.org/92443002/diff/380001/net/test/spawned_test_se... net/test/spawned_test_server/base_test_server.h:159: // a TLS extension. Delete these two lines (158-159). This is a merge error. https://codereview.chromium.org/92443002/diff/380001/net/test/spawned_test_se... net/test/spawned_test_server/base_test_server.h:167: // Whether to staple the OCSP response. Nit: add a blank line before this line.
https://codereview.chromium.org/92443002/diff/380001/net/socket/ssl_client_so... File net/socket/ssl_client_socket.h (right): https://codereview.chromium.org/92443002/diff/380001/net/socket/ssl_client_so... net/socket/ssl_client_socket.h:164: ConnectSignedCertTimestampsDisabled); On 2013/12/13 16:15:50, wtc wrote: > > Nit: add a blank line to separate the friend test declarations from the data > members. Done. https://codereview.chromium.org/92443002/diff/380001/net/test/spawned_test_se... File net/test/spawned_test_server/base_test_server.cc (right): https://codereview.chromium.org/92443002/diff/380001/net/test/spawned_test_se... net/test/spawned_test_server/base_test_server.cc:78: signed_cert_timestamps_tls_ext(std::string()), On 2013/12/13 16:15:50, wtc wrote: > > Delete the signed_cert_timestamps_tls_ext initializers in the SSLOptions > constructors (lines 65 and this line). They are not necessary. Done. https://codereview.chromium.org/92443002/diff/380001/net/test/spawned_test_se... File net/test/spawned_test_server/base_test_server.h (right): https://codereview.chromium.org/92443002/diff/380001/net/test/spawned_test_se... net/test/spawned_test_server/base_test_server.h:159: // a TLS extension. On 2013/12/13 16:15:50, wtc wrote: > > Delete these two lines (158-159). This is a merge error. Done. https://codereview.chromium.org/92443002/diff/380001/net/test/spawned_test_se... net/test/spawned_test_server/base_test_server.h:167: // Whether to staple the OCSP response. On 2013/12/13 16:15:50, wtc wrote: > > Nit: add a blank line before this line. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ekasper@google.com/92443002/400001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ekasper@google.com/92443002/400001
Message was sent while issue was closed.
Change committed as 240721
Message was sent while issue was closed.
On 2013/12/13 19:57:53, I haz the power (commit-bot) wrote: > Change committed as 240721 Does not compile on CrOS amd64: http://build.chromium.org/p/chromium.chromiumos/builders/ChromiumOS%20%28amd6... net/cert/ct_objects_extractor_nss.cc: In function 'bool net::ct::ExtractSCTListFromOCSPResponse(net::X509Certificate::OSCertHandle, const string&, const string&, std::string*)': net/cert/ct_objects_extractor_nss.cc:528:66: error: narrowing conversion of '(& ocsp_response)->std::basic_string<_CharT, _Traits, _Alloc>::size<char, std::char_traits<char>, std::allocator<char> >()' from 'std::basic_string<char>::size_type {aka long unsigned int}' to 'unsigned int' inside { } is ill-formed in C++11 [-Werror=narrowing] ocsp_response.data())), ocsp_response.size() };
Message was sent while issue was closed.
A revert of this CL has been created in https://codereview.chromium.org/108113006/ by szym@chromium.org. The reason for reverting is: Does not compile on CrOS amd64.
Message was sent while issue was closed.
This change broke the ChromiumOS (amd64) compile: http://build.chromium.org/p/chromium.chromiumos/builders/ChromiumOS%20%28amd6... The error is: chromeos-chrome-33.0.1738.0_alpha-r1: net/cert/ct_objects_extractor_nss.cc: In function 'bool net::ct::ExtractSCTListFromOCSPResponse(net::X509Certificate::OSCertHandle, const string&, const string&, std::string*)': chromeos-chrome-33.0.1738.0_alpha-r1: net/cert/ct_objects_extractor_nss.cc:528:66: error: narrowing conversion of '(& ocsp_response)->std::basic_string<_CharT, _Traits, _Alloc>::size<char, std::char_traits<char>, std::allocator<char> >()' from 'std::basic_string<char>::size_type {aka long unsigned int}' to 'unsigned int' inside { } is ill-formed in C++11 [-Werror=narrowing] chromeos-chrome-33.0.1738.0_alpha-r1: ocsp_response.data())), ocsp_response.size() }; I'll check with the troopers why this wasn't caught by any trybots.
Message was sent while issue was closed.
https://codereview.chromium.org/92443002/diff/400001/net/cert/ct_objects_extr... File net/cert/ct_objects_extractor_nss.cc (right): https://codereview.chromium.org/92443002/diff/400001/net/cert/ct_objects_extr... net/cert/ct_objects_extractor_nss.cc:528: ocsp_response.data())), ocsp_response.size() }; Emilia: you can add a static_cast<unsigned int> to ocsp_response.size(). If we want to be extra careful, we can add a check also: // A stapled OCSP response cannot overflow the 3-byte length. if (ocsp_response.size() > 0xffffff) // we can use a smaller max size. return false; Note also that Chromium's base/safe_numerics.h has base::checked_numeric_cast<>.
Message was sent while issue was closed.
The compile error should be fixed. Sorry about that! Is there a way to reopen this issue for try & commit, or should I create a clone? https://codereview.chromium.org/92443002/diff/400001/net/cert/ct_objects_extr... File net/cert/ct_objects_extractor_nss.cc (right): https://codereview.chromium.org/92443002/diff/400001/net/cert/ct_objects_extr... net/cert/ct_objects_extractor_nss.cc:528: ocsp_response.data())), ocsp_response.size() }; On 2013/12/15 05:31:14, wtc wrote: > > Emilia: you can add a static_cast<unsigned int> to ocsp_response.size(). If we > want to be extra careful, we can add a check also: > > // A stapled OCSP response cannot overflow the 3-byte length. > if (ocsp_response.size() > 0xffffff) // we can use a smaller max size. > return false; We know that we're safe given the source of those responses is NSS - but it doesn't cost anything to be correct so I've done it. > Note also that Chromium's base/safe_numerics.h has base::checked_numeric_cast<>. Ah, good to know. But this CHECKs so doesn't seem right for untrusted inputs. |
