Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
418723002:
Log the certificate subjects from the server certificate sent via QUIC. (Closed)
|
Created:
6 years, 5 months ago by Ryan Hamilton Modified:
6 years, 5 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, eroman, mmenke Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionLog the certificate subjects from the server certificate sent via QUIC.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=285446
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=285721
Patch Set 1 #Patch Set 2 : cleanup #
Total comments: 12
Patch Set 3 : fix comments #
Total comments: 4
Patch Set 4 : fix additional comments #Patch Set 5 : disable timing in hsts_view.js #
Total comments: 2
Patch Set 6 : Add comment #
Messages
Total messages: 25 (0 generated)
Patch set 2 LGTM. https://codereview.chromium.org/418723002/diff/20001/net/base/net_log_event_t... File net/base/net_log_event_type_list.h (right): https://codereview.chromium.org/418723002/diff/20001/net/base/net_log_event_t... net/base/net_log_event_type_list.h:1363: // "cert_subjects": <list of DNS names that the certs is valid for>, 1. Typo: certs => certificate 2. The certificate contains many fields. Why are you only interested in the subject names? Would it be better to dump the entire certificate chain? You can take a look at the NetLog::TYPE_SSL_CERTIFICATES_RECEIVED code in net/socket/ssl_client_socket_nss.cc. I guess you are investigating connection pooling opportunities? https://codereview.chromium.org/418723002/diff/20001/net/base/net_log_event_t... net/base/net_log_event_type_list.h:1365: EVENT_TYPE(QUIC_SESSION_CERTIFICATE_VERIFIED) Please list this event before QUIC_SESSION_PACKET_RECEIVED. (All the events in this section are named QUIC_SESSION_PACKET_xxx.) https://codereview.chromium.org/418723002/diff/20001/net/quic/quic_client_ses... File net/quic/quic_client_session.cc (right): https://codereview.chromium.org/418723002/diff/20001/net/quic/quic_client_ses... net/quic/quic_client_session.cc:671: logger_.OnCertificateVerified(cert_verify_result_->verified_cert); For future extensions, it would be better to pass const CertVerifyResult& to logger_.OnCertificateVerified so that the logger has access to all verification results. https://codereview.chromium.org/418723002/diff/20001/net/quic/quic_connection... File net/quic/quic_connection_logger.h (right): https://codereview.chromium.org/418723002/diff/20001/net/quic/quic_connection... net/quic/quic_connection_logger.h:75: void OnCertificateVerified(scoped_refptr<X509Certificate> cert); This seems to result in two or three unnecessary reference count increments and decrements. Maybe you can pass a const reference.
https://codereview.chromium.org/418723002/diff/20001/net/base/net_log_event_t... File net/base/net_log_event_type_list.h (right): https://codereview.chromium.org/418723002/diff/20001/net/base/net_log_event_t... net/base/net_log_event_type_list.h:1363: // "cert_subjects": <list of DNS names that the certs is valid for>, Nit: you can shorten this dictionary key to "subjects" because in this context the "cert" part is implied. But "subjects" could be a jargon to people unfamiliar with certificates and "cert" would help clarify it.
https://codereview.chromium.org/418723002/diff/20001/net/base/net_log_event_t... File net/base/net_log_event_type_list.h (right): https://codereview.chromium.org/418723002/diff/20001/net/base/net_log_event_t... net/base/net_log_event_type_list.h:1363: // "cert_subjects": <list of DNS names that the certs is valid for>, On 2014/07/24 17:17:01, wtc wrote: > > Nit: you can shorten this dictionary key to "subjects" because in this context > the "cert" part is implied. > > But "subjects" could be a jargon to people unfamiliar with certificates and > "cert" would help clarify it. Done. https://codereview.chromium.org/418723002/diff/20001/net/base/net_log_event_t... net/base/net_log_event_type_list.h:1363: // "cert_subjects": <list of DNS names that the certs is valid for>, On 2014/07/24 16:59:53, wtc wrote: > > 1. Typo: certs => certificate Done. > 2. The certificate contains many fields. Why are you only interested in the > subject names? Would it be better to dump the entire certificate chain? You can > take a look at the NetLog::TYPE_SSL_CERTIFICATES_RECEIVED code in > net/socket/ssl_client_socket_nss.cc. > > I guess you are investigating connection pooling opportunities? Right. For this usage, all I care about are the subjects for pooling. TYPE_SSL_CERTIFICATES_RECEIVED dumps the whole chain, but it dumps it as a PEM encoded blob which means it needs to be parsed externally before it can be used. I asked agl if we had any sort of function to dump an X509Certificate similar to openssl x509 -text, but he said we didn't. So I decided to just log the subject instead. Does this work for you? (I assume so since you LGTM'd) Alternatively, I could do both? https://codereview.chromium.org/418723002/diff/20001/net/base/net_log_event_t... net/base/net_log_event_type_list.h:1365: EVENT_TYPE(QUIC_SESSION_CERTIFICATE_VERIFIED) On 2014/07/24 16:59:53, wtc wrote: > > Please list this event before QUIC_SESSION_PACKET_RECEIVED. (All the events in > this section are named QUIC_SESSION_PACKET_xxx.) Done. https://codereview.chromium.org/418723002/diff/20001/net/quic/quic_client_ses... File net/quic/quic_client_session.cc (right): https://codereview.chromium.org/418723002/diff/20001/net/quic/quic_client_ses... net/quic/quic_client_session.cc:671: logger_.OnCertificateVerified(cert_verify_result_->verified_cert); On 2014/07/24 16:59:53, wtc wrote: > > For future extensions, it would be better to pass const CertVerifyResult& to > logger_.OnCertificateVerified so that the logger has access to all verification > results. Done. https://codereview.chromium.org/418723002/diff/20001/net/quic/quic_connection... File net/quic/quic_connection_logger.h (right): https://codereview.chromium.org/418723002/diff/20001/net/quic/quic_connection... net/quic/quic_connection_logger.h:75: void OnCertificateVerified(scoped_refptr<X509Certificate> cert); On 2014/07/24 16:59:53, wtc wrote: > > This seems to result in two or three unnecessary reference count increments and > decrements. Maybe you can pass a const reference. Done.
Patch set 3 LGTM. https://codereview.chromium.org/418723002/diff/20001/net/base/net_log_event_t... File net/base/net_log_event_type_list.h (right): https://codereview.chromium.org/418723002/diff/20001/net/base/net_log_event_t... net/base/net_log_event_type_list.h:1363: // "cert_subjects": <list of DNS names that the certs is valid for>, On 2014/07/24 17:24:46, Ryan Hamilton wrote: > > So I decided to just log the subject > instead. Does this work for you? (I assume so since you LGTM'd) This is fine by me. It's just that the reason for logging only the subjects is not obvious. I knew it because I discussed connection pooling with you recently. Perhaps we can add a comment to the logging function to note other fields can be logged if needed. https://codereview.chromium.org/418723002/diff/40001/net/base/net_log_event_t... File net/base/net_log_event_type_list.h (right): https://codereview.chromium.org/418723002/diff/40001/net/base/net_log_event_t... net/base/net_log_event_type_list.h:1356: // "cert_subjects": <list of DNS names that the certificate is valid for>, cert_subjects => subjects https://codereview.chromium.org/418723002/diff/40001/net/quic/quic_connection... File net/quic/quic_connection_logger.cc (right): https://codereview.chromium.org/418723002/diff/40001/net/quic/quic_connection... net/quic/quic_connection_logger.cc:696: base::Bind(&NetLogQuicCertificateVerifiedCallback, result.verified_cert)); Nit: if we pass a const const CertVerifyResult& to base::Bind(), will base::Bind() copy the entire CertVerifyResult object? If so, then passing a reference to result.verified_cert is cheaper.
Thanks! https://codereview.chromium.org/418723002/diff/20001/net/base/net_log_event_t... File net/base/net_log_event_type_list.h (right): https://codereview.chromium.org/418723002/diff/20001/net/base/net_log_event_t... net/base/net_log_event_type_list.h:1363: // "cert_subjects": <list of DNS names that the certs is valid for>, On 2014/07/24 17:34:35, wtc wrote: > > On 2014/07/24 17:24:46, Ryan Hamilton wrote: > > > > So I decided to just log the subject > > instead. Does this work for you? (I assume so since you LGTM'd) > > This is fine by me. It's just that the reason for logging only the subjects is > not obvious. I knew it because I discussed connection pooling with you recently. > Perhaps we can add a comment to the logging function to note other fields can be > logged if needed. Done. https://codereview.chromium.org/418723002/diff/40001/net/base/net_log_event_t... File net/base/net_log_event_type_list.h (right): https://codereview.chromium.org/418723002/diff/40001/net/base/net_log_event_t... net/base/net_log_event_type_list.h:1356: // "cert_subjects": <list of DNS names that the certificate is valid for>, On 2014/07/24 17:34:35, wtc wrote: > > cert_subjects => subjects Done. https://codereview.chromium.org/418723002/diff/40001/net/quic/quic_connection... File net/quic/quic_connection_logger.cc (right): https://codereview.chromium.org/418723002/diff/40001/net/quic/quic_connection... net/quic/quic_connection_logger.cc:696: base::Bind(&NetLogQuicCertificateVerifiedCallback, result.verified_cert)); On 2014/07/24 17:34:36, wtc wrote: > > Nit: if we pass a const const CertVerifyResult& to base::Bind(), will > base::Bind() copy the entire CertVerifyResult object? If so, then passing a > reference to result.verified_cert is cheaper. Yes, I believe that's exactly what will happen.
The CQ bit was checked by rch@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rch@chromium.org/418723002/60001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_rel_swarming on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel_sw...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_swarming on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel_sw...)
The CQ bit was checked by rch@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rch@chromium.org/418723002/60001
Message was sent while issue was closed.
Change committed as 285446
mmenke: can you confirm that this is the the right fix you proposed offline?
LGTM https://codereview.chromium.org/418723002/diff/80001/chrome/test/data/webui/n... File chrome/test/data/webui/net_internals/hsts_view.js (right): https://codereview.chromium.org/418723002/diff/80001/chrome/test/data/webui/n... chrome/test/data/webui/net_internals/hsts_view.js:57: // this.stsObserved_ = stsObserved; nit: Should add a link to the new bug here, once you've filed it.
The CQ bit was checked by rch@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rch@chromium.org/418723002/100001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_chromium_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
The CQ bit was checked by rch@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rch@chromium.org/418723002/100001
Message was sent while issue was closed.
Change committed as 285721
Message was sent while issue was closed.
https://codereview.chromium.org/418723002/diff/80001/chrome/test/data/webui/n... File chrome/test/data/webui/net_internals/hsts_view.js (right): https://codereview.chromium.org/418723002/diff/80001/chrome/test/data/webui/n... chrome/test/data/webui/net_internals/hsts_view.js:57: // this.stsObserved_ = stsObserved; On 2014/07/25 19:11:53, mmenke wrote: > nit: Should add a link to the new bug here, once you've filed it. Done. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
