Certificate Transparency: Threading the CT verifier into the SSL client socket.

Certificate Transparency: Threading the CT verifier into the SSL client socket.

With this patch, Basic Certificate Transparency status is available to the UI from the cert_status field of SSLInfo.

Please note this patch is based on https://codereview.chromium.org/67513008/ which has been LGTMed but not submitted yet.

Jam@: If you need any details on how it fits with the UI, it's outlined in issue https://codereview.chromium.org/27026002/

BUG=309578
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=237785

[Ryan Sleevi, 2013-11-25 06:49:31]

https://codereview.chromium.org/76443006/diff/120001/chrome/browser/io_thread.cc
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/76443006/diff/120001/chrome/browser/io_thread...
chrome/browser/io_thread.cc:576: " is 'description:base64_key')";
The DFATAL seems inappropriate here. The people most likely to get it wrong are
end users, and this won't really do anything special.

It seems like there are two possible behaviours here for invalid switches:
1) Ignore; fall back to default log. Let this be surfaced through net-internals,
and if logged, always at a LOG(ERROR).
2) Crash (aka CHECK), so indicate they've supplied the wrong value.

https://codereview.chromium.org/76443006/diff/120001/chrome/browser/io_thread...
chrome/browser/io_thread.cc:589: LOG(INFO) << "Using Certificate Transparency
log: " << log_description;
There's a push against LOG(INFO) in Chrome code, and reasonably so. Since this
is (or is going to be) surfaced through chrome://net-internals, let's drop this
entirely.

https://codereview.chromium.org/76443006/diff/120001/net/cert/cert_status_fla...
File net/cert/cert_status_flags.h (right):

https://codereview.chromium.org/76443006/diff/120001/net/cert/cert_status_fla...
net/cert/cert_status_flags.h:44: // from bit to state will not be
straightforward.
Why not? Can we not just encapsulate it in a helper function like we do for
CertStatusMinorError (which just handles masking)

https://codereview.chromium.org/76443006/diff/120001/net/cert/ct_known_logs_k...
File net/cert/ct_known_logs_keys.h (right):

https://codereview.chromium.org/76443006/diff/120001/net/cert/ct_known_logs_k...
net/cert/ct_known_logs_keys.h:10: static const char kGooglePilotLogKey[] =
This should be unsigned char; you should get a warning in one of the compilers
(IIRC, it's MSVC)

https://codereview.chromium.org/76443006/diff/120001/net/cert/ct_known_logs_k...
net/cert/ct_known_logs_keys.h:18: static const int kGooglePilotLogKeyLength =
arraysize(kGooglePilotLogKey) - 1;
So this file is a little weird. You're exposing these in the header, meaning
we're relying on duplicate variable folding. However, because they're exposed in
the header, the caller can always take arraysize(var) - 1, because it's known at
call time.

It would seem 'better' to take the approach of content_switches.h/.cc, and use

extern const unsigned char kGooglePilotLogKey[];
extern const size_t kGooglePilotLogKeyLength;

and then initialize in the .cc file.

Note also the conversion from int to size_t, since you're dealing with buffer
sizes.

https://codereview.chromium.org/76443006/diff/120001/net/cert/ct_known_logs_k...
net/cert/ct_known_logs_keys.h:20: static const char kGooglePilotLogName[] =
"Google CT Pilot";
I'm really uncomfortable with putting what will be user-visible strings in net/.
We already have this issue with some cert-related settings, and it's non-ideal.

We don't support doing localized strings in net_resources, but I suspect that we
need to move the I18N up a few layers.

https://codereview.chromium.org/76443006/diff/120001/net/socket/ssl_client_so...
File net/socket/ssl_client_socket_nss.cc (right):

https://codereview.chromium.org/76443006/diff/120001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_nss.cc:3492: "",  // SCT list from OCSP stapling
response
style: every "" should be std::string() instead

https://codereview.chromium.org/76443006/diff/120001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_nss.cc:3502:
ct_verify_result_.unknown_logs_scts.size();
The << call should be the first on the line - see
http://www.chromium.org/developers/coding-style#Code_Formatting

Should/will this be part of the BoundNetLog, rather than a VLOG?

https://codereview.chromium.org/76443006/diff/120001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_nss.cc:3518: CERT_STATUS_HAS_VALID_SCT |
CERT_STATUS_HAS_SCT_FROM_KNOWN_LOG;
style: four space indent

https://codereview.chromium.org/76443006/diff/120001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_nss.cc:3521: CERT_STATUS_HAS_SCT_FROM_KNOWN_LOG;
I don't understand how you can get into this state, given the condition on line
3515.

https://codereview.chromium.org/76443006/diff/120001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_nss.cc:3521: CERT_STATUS_HAS_SCT_FROM_KNOWN_LOG;
style: four space indent

[Eran M. (Google), 2013-11-25 17:18:32]

Addressed all comments, PTAL.

https://codereview.chromium.org/76443006/diff/120001/chrome/browser/io_thread.cc
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/76443006/diff/120001/chrome/browser/io_thread...
chrome/browser/io_thread.cc:576: " is 'description:base64_key')";
On 2013/11/25 06:49:31, Ryan Sleevi wrote:
> The DFATAL seems inappropriate here. The people most likely to get it wrong
are
> end users, and this won't really do anything special.
> 
> It seems like there are two possible behaviours here for invalid switches:
> 1) Ignore; fall back to default log. Let this be surfaced through
net-internals,
> and if logged, always at a LOG(ERROR).
> 2) Crash (aka CHECK), so indicate they've supplied the wrong value.
> 

I thought DFATAL crashes?

https://codereview.chromium.org/76443006/diff/120001/chrome/browser/io_thread...
chrome/browser/io_thread.cc:589: LOG(INFO) << "Using Certificate Transparency
log: " << log_description;
On 2013/11/25 06:49:31, Ryan Sleevi wrote:
> There's a push against LOG(INFO) in Chrome code, and reasonably so. Since this
> is (or is going to be) surfaced through chrome://net-internals, let's drop
this
> entirely.

Done.

https://codereview.chromium.org/76443006/diff/120001/net/cert/cert_status_fla...
File net/cert/cert_status_flags.h (right):

https://codereview.chromium.org/76443006/diff/120001/net/cert/cert_status_fla...
net/cert/cert_status_flags.h:44: // from bit to state will not be
straightforward.
On 2013/11/25 06:49:31, Ryan Sleevi wrote:
> Why not? Can we not just encapsulate it in a helper function like we do for
> CertStatusMinorError (which just handles masking)

Done, but the const names aren't pretty - PTAL.

https://codereview.chromium.org/76443006/diff/120001/net/cert/ct_known_logs_k...
File net/cert/ct_known_logs_keys.h (right):

https://codereview.chromium.org/76443006/diff/120001/net/cert/ct_known_logs_k...
net/cert/ct_known_logs_keys.h:10: static const char kGooglePilotLogKey[] =
On 2013/11/25 06:49:31, Ryan Sleevi wrote:
> This should be unsigned char; you should get a warning in one of the compilers
> (IIRC, it's MSVC)

As discussed offline, the warning is only if I use literals, which I'm not doing
here - keeping as char.

https://codereview.chromium.org/76443006/diff/120001/net/cert/ct_known_logs_k...
net/cert/ct_known_logs_keys.h:18: static const int kGooglePilotLogKeyLength =
arraysize(kGooglePilotLogKey) - 1;
On 2013/11/25 06:49:31, Ryan Sleevi wrote:
> So this file is a little weird. You're exposing these in the header, meaning
> we're relying on duplicate variable folding. However, because they're exposed
in
> the header, the caller can always take arraysize(var) - 1, because it's known
at
> call time.
> 
> It would seem 'better' to take the approach of content_switches.h/.cc, and use
> 
> extern const unsigned char kGooglePilotLogKey[];
> extern const size_t kGooglePilotLogKeyLength;
> 
> and then initialize in the .cc file.
> 
> Note also the conversion from int to size_t, since you're dealing with buffer
> sizes.

Done - applied both suggestions.

https://codereview.chromium.org/76443006/diff/120001/net/cert/ct_known_logs_k...
net/cert/ct_known_logs_keys.h:20: static const char kGooglePilotLogName[] =
"Google CT Pilot";
On 2013/11/25 06:49:31, Ryan Sleevi wrote:
> I'm really uncomfortable with putting what will be user-visible strings in
net/.
> We already have this issue with some cert-related settings, and it's
non-ideal.
> 
> We don't support doing localized strings in net_resources, but I suspect that
we
> need to move the I18N up a few layers.

What would you suggest?

In this case I'd argue this is an entity's name, not a string that should be
translated.
Personally, when reading a Hebrew UI, I do not expect entities' names to be
translated, and in the 'identity' section of the connection information dialogue
I'd already expect the CA's name to be in English regardless of the UI
localization (IMHO it makes sense as long as the directionality of the text is
not messed up when a non-English sentence contains names in English).

https://codereview.chromium.org/76443006/diff/120001/net/socket/ssl_client_so...
File net/socket/ssl_client_socket_nss.cc (right):

https://codereview.chromium.org/76443006/diff/120001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_nss.cc:3492: "",  // SCT list from OCSP stapling
response
On 2013/11/25 06:49:31, Ryan Sleevi wrote:
> style: every "" should be std::string() instead

Done.

https://codereview.chromium.org/76443006/diff/120001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_nss.cc:3502:
ct_verify_result_.unknown_logs_scts.size();
On 2013/11/25 06:49:31, Ryan Sleevi wrote:
> The << call should be the first on the line - see
> http://www.chromium.org/developers/coding-style#Code_Formatting
> 
> Should/will this be part of the BoundNetLog, rather than a VLOG?

Formatting done.
Yes, this will be part of the BoundNetLog, in the next patch. For now, this is
the only indication SCTs are validated - would be nice to keep it in.

https://codereview.chromium.org/76443006/diff/120001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_nss.cc:3518: CERT_STATUS_HAS_VALID_SCT |
CERT_STATUS_HAS_SCT_FROM_KNOWN_LOG;
On 2013/11/25 06:49:31, Ryan Sleevi wrote:
> style: four space indent

Done.

https://codereview.chromium.org/76443006/diff/120001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_nss.cc:3521: CERT_STATUS_HAS_SCT_FROM_KNOWN_LOG;
On 2013/11/25 06:49:31, Ryan Sleevi wrote:
> style: four space indent

Done.

https://codereview.chromium.org/76443006/diff/120001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_nss.cc:3521: CERT_STATUS_HAS_SCT_FROM_KNOWN_LOG;
On 2013/11/25 06:49:31, Ryan Sleevi wrote:
> I don't understand how you can get into this state, given the condition on
line
> 3515.

This is exactly the state where verified_scts.empty(), unknown_logs_scts.empty()
and unverified_scts is not empty: We have SCTs from logs we know but these SCTs
could not be verified.

I've reworked this if block according to your comment in cert_status_flags.h,
please let me know if it's clearer now.

[wtc, 2013-11-26 01:47:23]

Patch set 9 LGTM.

1. Please wait for Ryan's approval.

2. There are bugs (marked with "BUG") and a comment on a state machine design
issue (marked with "DESIGN").

3. The code that uses the new error codes needs to be re-applied to this CL.

https://codereview.chromium.org/76443006/diff/220001/chrome/browser/io_thread.cc
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/76443006/diff/220001/chrome/browser/io_thread...
chrome/browser/io_thread.cc:569: #if !defined(USE_OPENSSL)

Why don't you combine this #if block with the #if block on lines 542-561?

https://codereview.chromium.org/76443006/diff/220001/chrome/browser/io_thread...
chrome/browser/io_thread.cc:576: << " is 'description:base64_key')";

Line continuations are indented by four spaces.

Add a space after the << on line 575.

Nit: you can take advantage of C-preprocessor's concatenation of string literals
and use only one << operator:

    CHECK(delim_pos != std::string::npos)
        << "CT log description not provided (switch format"
           " is 'description:base64_key')";

https://codereview.chromium.org/76443006/diff/220001/chrome/browser/io_thread...
chrome/browser/io_thread.cc:590: << "builds using OpenSSL.";

Nit: same here: one << operator call is enough. The string literals will be
concatenated by the C preprocessor.

https://codereview.chromium.org/76443006/diff/220001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_impl_io_data.cc (right):

https://codereview.chromium.org/76443006/diff/220001/chrome/browser/profiles/...
chrome/browser/profiles/profile_impl_io_data.cc:375:
io_thread_globals->cert_transparency_verifier.get());

Recall that I suggested you imitate the code that handles
transport_security_state? I am wondering why this class is not getting
transport_security_state from io_thread_globals (see line 364).

https://codereview.chromium.org/76443006/diff/220001/chrome/common/chrome_swi...
File chrome/common/chrome_switches.cc (right):

https://codereview.chromium.org/76443006/diff/220001/chrome/common/chrome_swi...
chrome/common/chrome_switches.cc:152: // log_description:log_key

Nit: identing this line may make it look nicer.

https://codereview.chromium.org/76443006/diff/220001/net/base/net_error_list.h
File net/base/net_error_list.h (right):

https://codereview.chromium.org/76443006/diff/220001/net/base/net_error_list....
net/base/net_error_list.h:20: //   800-899 DNS resolver errors

Please document the new error code range here.

Are you sure you can't use the "200-299 Certificate errors" range?

https://codereview.chromium.org/76443006/diff/220001/net/base/net_error_list....
net/base/net_error_list.h:715: // a certificate.

This should fit on the previous line.

https://codereview.chromium.org/76443006/diff/220001/net/base/net_error_list....
net/base/net_error_list.h:718: // Serialization of the Precertificate LogEntry
or X.509 LogEntry failed

A period (.) is missing at the end of this sentence. Also on line 725.

https://codereview.chromium.org/76443006/diff/220001/net/base/net_error_list....
net/base/net_error_list.h:722: // not be created.

Delete this line?

https://codereview.chromium.org/76443006/diff/220001/net/cert/cert_status_fla...
File net/cert/cert_status_flags.h (right):

https://codereview.chromium.org/76443006/diff/220001/net/cert/cert_status_fla...
net/cert/cert_status_flags.h:46: //      0 |      1 | SCTs from unknown logs
were present

In this combination, are the SCTs verified? The name "HAS_GOOD_SCT" implies they
are verified. But can we verify SCTs from unknown logs?

Update: after reading the entire CL, I now understand these two bits don't have
independent meanings. Instead, the two bits are used to encode four possible
values (0, 1, 2, 3) as you tabulated here.

This is a little confusing because a reader of this file expects every cert
status bit can be tested independently. It may be better to imitate how this is
done in net/ssl/ssl_connection_status_flags.h. For example, it uses three bits
to encode the SSL/TLS protocol version.

https://codereview.chromium.org/76443006/diff/220001/net/cert/cert_status_fla...
net/cert/cert_status_flags.h:47: //      1 |      0 | SCTs from known logs
present but cannot be unverified

1. Is "unverified" a typo?

2. Does this combination imply an error? If so, this violates the comment on
line 38 that these bits are for non-error statuses.

https://codereview.chromium.org/76443006/diff/220001/net/cert/cert_status_fla...
net/cert/cert_status_flags.h:80: CertStatus cert_status);

Add a blank line after this line, and between the two functions.

https://codereview.chromium.org/76443006/diff/220001/net/cert/ct_known_logs_k...
File net/cert/ct_known_logs_keys.cc (right):

https://codereview.chromium.org/76443006/diff/220001/net/cert/ct_known_logs_k...
net/cert/ct_known_logs_keys.cc:6: #include "net/cert/ct_known_logs_keys.h"

The Style Guide recommends that this header be listed first.

https://codereview.chromium.org/76443006/diff/220001/net/cert/ct_known_logs_k...
File net/cert/ct_known_logs_keys.h (right):

https://codereview.chromium.org/76443006/diff/220001/net/cert/ct_known_logs_k...
net/cert/ct_known_logs_keys.h:6: #define NET_CERT_CT_KNOWN_LOGS_KEYS_H_

Nit: perhaps these files should be named ct_known_logs.h or ct_known_logs_data.h
because they also contain the log descriptions.

https://codereview.chromium.org/76443006/diff/220001/net/cert/ct_known_logs_k...
net/cert/ct_known_logs_keys.h:10: extern const char kGooglePilotLogKey[];

Should these constants be inside namespace ct?

https://codereview.chromium.org/76443006/diff/220001/net/cert/multi_log_ct_ve...
File net/cert/multi_log_ct_verifier.cc (right):

https://codereview.chromium.org/76443006/diff/220001/net/cert/multi_log_ct_ve...
net/cert/multi_log_ct_verifier.cc:64: return has_verified_scts ? OK :
ERR_FAILED;

Please re-apply the changes that use the new CT-related error codes.

https://codereview.chromium.org/76443006/diff/220001/net/socket/ssl_client_so...
File net/socket/ssl_client_socket_nss.cc (right):

https://codereview.chromium.org/76443006/diff/220001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_nss.cc:3470: GotoState(STATE_VERIFY_CT);

DESIGN: A state is necessary only if we need to wait for an async operation to
complete.

Since cert_transparency_verifier_->Verify() works synchronously, we don't need
new states for it. You can just add a VerifyCT() function that calls
cert_transparency_verifier_->Verify() and handles its results, and call
VerifyCT() here.

https://codereview.chromium.org/76443006/diff/220001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_nss.cc:3487: // Note that this is a completely
synchronic operation: The CT Log Verifier

synchronic => synchronous

https://codereview.chromium.org/76443006/diff/220001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_nss.cc:3489: // external communication

Nit: missing a period (.)

https://codereview.chromium.org/76443006/diff/220001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_nss.cc:3492: std::string(), // SCT list from OCSP
stapling response

Nit: remove "stapling"

https://codereview.chromium.org/76443006/diff/220001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_nss.cc:3493: std::string(),  // SCT list from TLS
handshake

Nit: TLS handshake => TLS extension

https://codereview.chromium.org/76443006/diff/220001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_nss.cc:3507: 

Nit: delete this blank line.

https://codereview.chromium.org/76443006/diff/220001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_nss.cc:3518: CERT_STATUS_HAS_SCT_FROM_KNOWN_LOG;

BUG: use |= instead of = here and on line 3526. (Line 3520 is correct.)

This line (a continuation line) should be indented by four spaces.

https://codereview.chromium.org/76443006/diff/220001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_nss.cc:3521: }

Omit curly braces when the if statement's body is one line.

https://codereview.chromium.org/76443006/diff/220001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_nss.cc:3524: // When this bit is set but
CERT_STATUS_HAS_SCT_FROM_KNOWN_LOG isn't

Nit: add a comma (,) at the end of this line.

https://codereview.chromium.org/76443006/diff/220001/net/socket/ssl_client_so...
File net/socket/ssl_client_socket_nss.h (right):

https://codereview.chromium.org/76443006/diff/220001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_nss.h:192: CTVerifier* cert_transparency_verifier_;

Nit: inside this class, I suggest we list these CT-related members right after
the CertVerifier-related members (lines 161-165).

[Ryan Sleevi, 2013-11-26 02:22:13]

Will take a final pass once a new version is uploaded

https://codereview.chromium.org/76443006/diff/220001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_impl_io_data.cc (right):

https://codereview.chromium.org/76443006/diff/220001/chrome/browser/profiles/...
chrome/browser/profiles/profile_impl_io_data.cc:375:
io_thread_globals->cert_transparency_verifier.get());
On 2013/11/26 01:47:23, wtc wrote:
> 
> Recall that I suggested you imitate the code that handles
> transport_security_state? I am wondering why this class is not getting
> transport_security_state from io_thread_globals (see line 364).

This is unrelated. But that's because transport_security_state contains
per-profile information (it's a persistent layer). The SCT verifier state does
not itself cache anything that would leak data between profiles.

[Eran M. (Google), 2013-11-26 14:45:53]

Addressed all comments, PTAL.
Notice the following major changes:

* net_error_list.h: I've left only two new error codes in the 2xx range. Does it
matter how fine-grained these errors are given the caller can inspect
CTVerifyResult to export more information to the CertStatus? 

* cert_status_flags.h: As suggested, stored the CT state in the two leftmost
bits, rather than two bits which look independent. Would it be better to have
(in addition) another bit in the error bits range indicating SCT validation
failure?

* Removed the added states in ssl_client_socket_nss.

https://codereview.chromium.org/76443006/diff/220001/chrome/browser/io_thread.cc
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/76443006/diff/220001/chrome/browser/io_thread...
chrome/browser/io_thread.cc:569: #if !defined(USE_OPENSSL)
On 2013/11/26 01:47:23, wtc wrote:
> 
> Why don't you combine this #if block with the #if block on lines 542-561?

Done.

https://codereview.chromium.org/76443006/diff/220001/chrome/browser/io_thread...
chrome/browser/io_thread.cc:576: << " is 'description:base64_key')";
On 2013/11/26 01:47:23, wtc wrote:
> 
> Line continuations are indented by four spaces.
> 
> Add a space after the << on line 575.
> 
> Nit: you can take advantage of C-preprocessor's concatenation of string
literals
> and use only one << operator:
> 
>     CHECK(delim_pos != std::string::npos)
>         << "CT log description not provided (switch format"
>            " is 'description:base64_key')";

Done.

https://codereview.chromium.org/76443006/diff/220001/chrome/browser/io_thread...
chrome/browser/io_thread.cc:590: << "builds using OpenSSL.";
On 2013/11/26 01:47:23, wtc wrote:
> 
> Nit: same here: one << operator call is enough. The string literals will be
> concatenated by the C preprocessor.

Done.

https://codereview.chromium.org/76443006/diff/220001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_impl_io_data.cc (right):

https://codereview.chromium.org/76443006/diff/220001/chrome/browser/profiles/...
chrome/browser/profiles/profile_impl_io_data.cc:375:
io_thread_globals->cert_transparency_verifier.get());
On 2013/11/26 02:22:14, Ryan Sleevi wrote:
> On 2013/11/26 01:47:23, wtc wrote:
> > 
> > Recall that I suggested you imitate the code that handles
> > transport_security_state? I am wondering why this class is not getting
> > transport_security_state from io_thread_globals (see line 364).
> 
> This is unrelated. But that's because transport_security_state contains
> per-profile information (it's a persistent layer). The SCT verifier state does
> not itself cache anything that would leak data between profiles.

Ack - I understand it should be left as it's now.

https://codereview.chromium.org/76443006/diff/220001/chrome/common/chrome_swi...
File chrome/common/chrome_switches.cc (right):

https://codereview.chromium.org/76443006/diff/220001/chrome/common/chrome_swi...
chrome/common/chrome_switches.cc:152: // log_description:log_key
On 2013/11/26 01:47:23, wtc wrote:
> 
> Nit: identing this line may make it look nicer.

Done, indented argument explanation as well.

https://codereview.chromium.org/76443006/diff/220001/net/base/net_error_list.h
File net/base/net_error_list.h (right):

https://codereview.chromium.org/76443006/diff/220001/net/base/net_error_list....
net/base/net_error_list.h:20: //   800-899 DNS resolver errors
On 2013/11/26 01:47:23, wtc wrote:
> 
> Please document the new error code range here.
> 
> Are you sure you can't use the "200-299 Certificate errors" range?

As suggested, added it to the 200-299 certificate error range (at the end of the
range).

https://codereview.chromium.org/76443006/diff/220001/net/base/net_error_list....
net/base/net_error_list.h:715: // a certificate.
On 2013/11/26 01:47:23, wtc wrote:
> 
> This should fit on the previous line.

Removed entirely.

https://codereview.chromium.org/76443006/diff/220001/net/base/net_error_list....
net/base/net_error_list.h:718: // Serialization of the Precertificate LogEntry
or X.509 LogEntry failed
On 2013/11/26 01:47:23, wtc wrote:
> 
> A period (.) is missing at the end of this sentence. Also on line 725.

Done.

https://codereview.chromium.org/76443006/diff/220001/net/base/net_error_list....
net/base/net_error_list.h:722: // not be created.
On 2013/11/26 01:47:23, wtc wrote:
> 
> Delete this line?

Done.

https://codereview.chromium.org/76443006/diff/220001/net/cert/cert_status_fla...
File net/cert/cert_status_flags.h (right):

https://codereview.chromium.org/76443006/diff/220001/net/cert/cert_status_fla...
net/cert/cert_status_flags.h:46: //      0 |      1 | SCTs from unknown logs
were present
On 2013/11/26 01:47:23, wtc wrote:
> 
> In this combination, are the SCTs verified? The name "HAS_GOOD_SCT" implies
they
> are verified. But can we verify SCTs from unknown logs?
> 
> Update: after reading the entire CL, I now understand these two bits don't
have
> independent meanings. Instead, the two bits are used to encode four possible
> values (0, 1, 2, 3) as you tabulated here.
> 
> This is a little confusing because a reader of this file expects every cert
> status bit can be tested independently. It may be better to imitate how this
is
> done in net/ssl/ssl_connection_status_flags.h. For example, it uses three bits
> to encode the SSL/TLS protocol version.

Done - used the two most-significant bits to store CT state as integer.

https://codereview.chromium.org/76443006/diff/220001/net/cert/cert_status_fla...
net/cert/cert_status_flags.h:47: //      1 |      0 | SCTs from known logs
present but cannot be unverified
On 2013/11/26 01:47:23, wtc wrote:
> 
> 1. Is "unverified" a typo?
> 
> 2. Does this combination imply an error? If so, this violates the comment on
> line 38 that these bits are for non-error statuses.

1. yes.
2. yes - but the CT state was now moved to a state of its own, so the comment
does not apply anymore.

https://codereview.chromium.org/76443006/diff/220001/net/cert/cert_status_fla...
net/cert/cert_status_flags.h:80: CertStatus cert_status);
On 2013/11/26 01:47:23, wtc wrote:
> 
> Add a blank line after this line, and between the two functions.

Removed these two functions, added a function to extract the CT state.

https://codereview.chromium.org/76443006/diff/220001/net/cert/ct_known_logs_k...
File net/cert/ct_known_logs_keys.cc (right):

https://codereview.chromium.org/76443006/diff/220001/net/cert/ct_known_logs_k...
net/cert/ct_known_logs_keys.cc:6: #include "net/cert/ct_known_logs_keys.h"
On 2013/11/26 01:47:23, wtc wrote:
> 
> The Style Guide recommends that this header be listed first.

Done.

https://codereview.chromium.org/76443006/diff/220001/net/cert/ct_known_logs_k...
File net/cert/ct_known_logs_keys.h (right):

https://codereview.chromium.org/76443006/diff/220001/net/cert/ct_known_logs_k...
net/cert/ct_known_logs_keys.h:6: #define NET_CERT_CT_KNOWN_LOGS_KEYS_H_
On 2013/11/26 01:47:23, wtc wrote:
> 
> Nit: perhaps these files should be named ct_known_logs.h or
ct_known_logs_data.h
> because they also contain the log descriptions.

Done.

https://codereview.chromium.org/76443006/diff/220001/net/cert/ct_known_logs_k...
net/cert/ct_known_logs_keys.h:10: extern const char kGooglePilotLogKey[];
On 2013/11/26 01:47:23, wtc wrote:
> 
> Should these constants be inside namespace ct?

Done.

https://codereview.chromium.org/76443006/diff/220001/net/cert/multi_log_ct_ve...
File net/cert/multi_log_ct_verifier.cc (right):

https://codereview.chromium.org/76443006/diff/220001/net/cert/multi_log_ct_ve...
net/cert/multi_log_ct_verifier.cc:64: return has_verified_scts ? OK :
ERR_FAILED;
On 2013/11/26 01:47:23, wtc wrote:
> 
> Please re-apply the changes that use the new CT-related error codes.

Done.

https://codereview.chromium.org/76443006/diff/220001/net/socket/ssl_client_so...
File net/socket/ssl_client_socket_nss.cc (right):

https://codereview.chromium.org/76443006/diff/220001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_nss.cc:3470: GotoState(STATE_VERIFY_CT);
On 2013/11/26 01:47:23, wtc wrote:
> 
> DESIGN: A state is necessary only if we need to wait for an async operation to
> complete.
> 
> Since cert_transparency_verifier_->Verify() works synchronously, we don't need
> new states for it. You can just add a VerifyCT() function that calls
> cert_transparency_verifier_->Verify() and handles its results, and call
> VerifyCT() here.

Done - note that as the return type of VerifyCT is not used, I've declared it a
void one. Later, when we have higher confidence in SCT validation we could
propagate the error from CTVerifier::Verify.

https://codereview.chromium.org/76443006/diff/220001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_nss.cc:3487: // Note that this is a completely
synchronic operation: The CT Log Verifier
On 2013/11/26 01:47:23, wtc wrote:
> 
> synchronic => synchronous

Done.

https://codereview.chromium.org/76443006/diff/220001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_nss.cc:3489: // external communication
On 2013/11/26 01:47:23, wtc wrote:
> 
> Nit: missing a period (.)

Done.

https://codereview.chromium.org/76443006/diff/220001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_nss.cc:3492: std::string(), // SCT list from OCSP
stapling response
On 2013/11/26 01:47:23, wtc wrote:
> 
> Nit: remove "stapling"

Done.

https://codereview.chromium.org/76443006/diff/220001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_nss.cc:3493: std::string(),  // SCT list from TLS
handshake
On 2013/11/26 01:47:23, wtc wrote:
> 
> Nit: TLS handshake => TLS extension

Done.

https://codereview.chromium.org/76443006/diff/220001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_nss.cc:3507: 
On 2013/11/26 01:47:23, wtc wrote:
> 
> Nit: delete this blank line.

Done.

https://codereview.chromium.org/76443006/diff/220001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_nss.cc:3518: CERT_STATUS_HAS_SCT_FROM_KNOWN_LOG;
On 2013/11/26 01:47:23, wtc wrote:
> 
> BUG: use |= instead of = here and on line 3526. (Line 3520 is correct.)
> 
> This line (a continuation line) should be indented by four spaces.

Done.

https://codereview.chromium.org/76443006/diff/220001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_nss.cc:3521: }
On 2013/11/26 01:47:23, wtc wrote:
> 
> Omit curly braces when the if statement's body is one line.

Done.

https://codereview.chromium.org/76443006/diff/220001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_nss.cc:3524: // When this bit is set but
CERT_STATUS_HAS_SCT_FROM_KNOWN_LOG isn't
On 2013/11/26 01:47:23, wtc wrote:
> 
> Nit: add a comma (,) at the end of this line.

Done.

https://codereview.chromium.org/76443006/diff/220001/net/socket/ssl_client_so...
File net/socket/ssl_client_socket_nss.h (right):

https://codereview.chromium.org/76443006/diff/220001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_nss.h:192: CTVerifier* cert_transparency_verifier_;
On 2013/11/26 01:47:23, wtc wrote:
> 
> Nit: inside this class, I suggest we list these CT-related members right after
> the CertVerifier-related members (lines 161-165).

Done.

[jam, 2013-11-26 17:20:36]

chrome rubberstamp lgtm

[wtc, 2013-11-27 16:45:56]

Review comments on patch set 12:

Eran: today is the last working day of this week in the US, so I'd like to send
you a comment first. I only looked at the net_error_list.h and
cert_status_flags.h changes in the CL.

https://codereview.chromium.org/76443006/diff/280001/net/base/net_error_list.h
File net/base/net_error_list.h (right):

https://codereview.chromium.org/76443006/diff/280001/net/base/net_error_list....
net/base/net_error_list.h:423: NET_ERROR(NO_SCTS_VERIFIED_OK, -299)

If we add these two CT errors as certificate errors, they need to be before
|CERT_END|, and corresponding *error* status bits (in bits 0 to 15) need to be
added to cert_status_flags.h.

So this part of the CL still needs work. I am sorry that I haven't spent the
time to study this issue and give you clear advices. So your attempts have been
repeatedly declined.

An error should be a certificate error (in the -2xx range) if it will trigger
the certificate error interstitial page. By this criterion, it seems that
CT_LOG_ENTRY_CREATION_FAILED is not a certificate error, but NO_SCTS_VERIFIED_OK
is.

[Eran M. (Google), 2013-11-27 19:05:47]

Removed changes to net_error_list.h, as discussed.

https://codereview.chromium.org/76443006/diff/280001/net/base/net_error_list.h
File net/base/net_error_list.h (right):

https://codereview.chromium.org/76443006/diff/280001/net/base/net_error_list....
net/base/net_error_list.h:423: NET_ERROR(NO_SCTS_VERIFIED_OK, -299)
On 2013/11/27 16:45:57, wtc wrote:
> 
> If we add these two CT errors as certificate errors, they need to be before
> |CERT_END|, and corresponding *error* status bits (in bits 0 to 15) need to be
> added to cert_status_flags.h.
> 
> So this part of the CL still needs work. I am sorry that I haven't spent the
> time to study this issue and give you clear advices. So your attempts have
been
> repeatedly declined.
> 
> An error should be a certificate error (in the -2xx range) if it will trigger
> the certificate error interstitial page. By this criterion, it seems that
> CT_LOG_ENTRY_CREATION_FAILED is not a certificate error, but
NO_SCTS_VERIFIED_OK
> is.

As we discussed offline, completely reverting these changes so this patch won't
be blocked on them. Let's discuss error handling later on.

[wtc, 2013-11-27 20:00:49]

Patch set 14 LGTM.

https://codereview.chromium.org/76443006/diff/320001/net/cert/cert_status_fla...
File net/cert/cert_status_flags.h (right):

https://codereview.chromium.org/76443006/diff/320001/net/cert/cert_status_fla...
net/cert/cert_status_flags.h:45: static const int
CERTIFICATE_TRANSPARENCY_STATUS_MASK = 3;

This mask should have the same type as CertStatus because it is bitwise AND'ed
with a CertStatus, so it should be a uint32.

https://codereview.chromium.org/76443006/diff/320001/net/cert/cert_status_fla...
net/cert/cert_status_flags.h:50: CERT_TRANSPARENCY_SCT_VALIDATED_OK = 3,

Nit: the four values should be documented.

https://codereview.chromium.org/76443006/diff/320001/net/cert/cert_status_fla...
net/cert/cert_status_flags.h:71: inline int
CertificateTransparencyStateFromCertStatus(CertStatus cert_status) {

It may be better to return uint32...

Document this function.

https://codereview.chromium.org/76443006/diff/320001/net/cert/ct_known_logs_d...
File net/cert/ct_known_logs_data.cc (right):

https://codereview.chromium.org/76443006/diff/320001/net/cert/ct_known_logs_d...
net/cert/ct_known_logs_data.cc:77: return net::CTLogVerifier::Create(key,
net::ct::kGoogleRocketeerLogName);

No need to use net:: and net::ct:: in this file.

https://codereview.chromium.org/76443006/diff/320001/net/cert/ct_known_logs_d...
File net/cert/ct_known_logs_data.h (right):

https://codereview.chromium.org/76443006/diff/320001/net/cert/ct_known_logs_d...
net/cert/ct_known_logs_data.h:24: NET_EXPORT scoped_ptr<net::CTLogVerifier>
CreateGoogleRocketeerLogVerifier();

1. Remove the net:: prefixes in this file.

2. Nit: now that this header no longer declare data, the name "ct_known_logs.h"
would be better.

https://codereview.chromium.org/76443006/diff/320001/net/socket/ssl_client_so...
File net/socket/ssl_client_socket_nss.cc (right):

https://codereview.chromium.org/76443006/diff/320001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_nss.cc:3500: int ct_state;

I suggest declaring this variable as a uint32 or CertStatus because it will be
bitwise OR'ed with a CertStatus.

https://codereview.chromium.org/76443006/diff/320001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_nss.cc:3508: }

Nit: if one branch of the if statement needs curly braces, all branches should
use curly braces.

https://codereview.chromium.org/76443006/diff/320001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_nss.cc:3511: CERTIFICATE_TRANSPARENCY_STATUS_SHIFT;

I don't think we need "& CERTIFICATE_TRANSPARENCY_STATUS_MASK".

This should work:

  server_cert_verify_result_.cert_status |=
      ct_state << CERTIFICATE_TRANSPARENCY_STATUS_SHIFT;

https://codereview.chromium.org/76443006/diff/320001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_nss.cc:3514: return;

Nit: the return statement is not necessary for a void function.

https://codereview.chromium.org/76443006/diff/320001/net/socket/ssl_client_so...
File net/socket/ssl_client_socket_nss.h (right):

https://codereview.chromium.org/76443006/diff/320001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_nss.h:139: void VerifyCT();

Nit: add a blank line before to separate this method from the other Do<State>
methods.

[wtc, 2013-11-27 20:13:53]

On 2013/11/26 14:45:53, eranm wrote:
>
> * net_error_list.h: I've left only two new error codes in the 2xx range. Does
it
> matter how fine-grained these errors are given the caller can inspect
> CTVerifyResult to export more information to the CertStatus?

If the caller has CTVerifyResult, then the error code returned by
ct_verifier->Verify() can be coarse-grained. I just don't want that error code
to be the default ERR_FAILED :-)

> * cert_status_flags.h: As suggested, stored the CT state in the two leftmost
> bits, rather than two bits which look independent. Would it be better to have
> (in addition) another bit in the error bits range indicating SCT validation
> failure?

An "error" bit is necessary to trigger the standard certificate error handling.
I suspect that is what you want for serious CT errors.

[Eran M. (Google), 2013-11-27 23:01:42]

Addressed all comments.
The only thing not done in patchset 15 is adding a general CT error to
net_error_list.h.
For now I will add it as -299, so it's in the certificate range but not included
there, later we could refine the exact value (my understanding is that this
error code is never persisted and so should be modifiable).

https://codereview.chromium.org/76443006/diff/320001/net/cert/cert_status_fla...
File net/cert/cert_status_flags.h (right):

https://codereview.chromium.org/76443006/diff/320001/net/cert/cert_status_fla...
net/cert/cert_status_flags.h:45: static const int
CERTIFICATE_TRANSPARENCY_STATUS_MASK = 3;
On 2013/11/27 20:00:49, wtc wrote:
> 
> This mask should have the same type as CertStatus because it is bitwise AND'ed
> with a CertStatus, so it should be a uint32.

Done.

https://codereview.chromium.org/76443006/diff/320001/net/cert/cert_status_fla...
net/cert/cert_status_flags.h:50: CERT_TRANSPARENCY_SCT_VALIDATED_OK = 3,
On 2013/11/27 20:00:49, wtc wrote:
> 
> Nit: the four values should be documented.

Done.

https://codereview.chromium.org/76443006/diff/320001/net/cert/cert_status_fla...
net/cert/cert_status_flags.h:71: inline int
CertificateTransparencyStateFromCertStatus(CertStatus cert_status) {
On 2013/11/27 20:00:49, wtc wrote:
> 
> It may be better to return uint32...
> 
> Document this function.

Gave a proper name to the enum,  added documentation.

https://codereview.chromium.org/76443006/diff/320001/net/cert/ct_known_logs_d...
File net/cert/ct_known_logs_data.cc (right):

https://codereview.chromium.org/76443006/diff/320001/net/cert/ct_known_logs_d...
net/cert/ct_known_logs_data.cc:77: return net::CTLogVerifier::Create(key,
net::ct::kGoogleRocketeerLogName);
On 2013/11/27 20:00:49, wtc wrote:
> 
> No need to use net:: and net::ct:: in this file.

Done.

https://codereview.chromium.org/76443006/diff/320001/net/cert/ct_known_logs_d...
File net/cert/ct_known_logs_data.h (right):

https://codereview.chromium.org/76443006/diff/320001/net/cert/ct_known_logs_d...
net/cert/ct_known_logs_data.h:24: NET_EXPORT scoped_ptr<net::CTLogVerifier>
CreateGoogleRocketeerLogVerifier();
On 2013/11/27 20:00:49, wtc wrote:
> 
> 1. Remove the net:: prefixes in this file.
> 
> 2. Nit: now that this header no longer declare data, the name
"ct_known_logs.h"
> would be better.

Done and Done.

https://codereview.chromium.org/76443006/diff/320001/net/socket/ssl_client_so...
File net/socket/ssl_client_socket_nss.cc (right):

https://codereview.chromium.org/76443006/diff/320001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_nss.cc:3500: int ct_state;
On 2013/11/27 20:00:49, wtc wrote:
> 
> I suggest declaring this variable as a uint32 or CertStatus because it will be
> bitwise OR'ed with a CertStatus.

Done - uint32 it is.

https://codereview.chromium.org/76443006/diff/320001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_nss.cc:3508: }
On 2013/11/27 20:00:49, wtc wrote:
> 
> Nit: if one branch of the if statement needs curly braces, all branches should
> use curly braces.

Done.

https://codereview.chromium.org/76443006/diff/320001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_nss.cc:3511: CERTIFICATE_TRANSPARENCY_STATUS_SHIFT;
On 2013/11/27 20:00:49, wtc wrote:
> 
> I don't think we need "& CERTIFICATE_TRANSPARENCY_STATUS_MASK".
> 
> This should work:
> 
>   server_cert_verify_result_.cert_status |=
>       ct_state << CERTIFICATE_TRANSPARENCY_STATUS_SHIFT;

Done.

https://codereview.chromium.org/76443006/diff/320001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_nss.cc:3514: return;
On 2013/11/27 20:00:49, wtc wrote:
> 
> Nit: the return statement is not necessary for a void function.

Oops! Done.

https://codereview.chromium.org/76443006/diff/320001/net/socket/ssl_client_so...
File net/socket/ssl_client_socket_nss.h (right):

https://codereview.chromium.org/76443006/diff/320001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_nss.h:139: void VerifyCT();
On 2013/11/27 20:00:49, wtc wrote:
> 
> Nit: add a blank line before to separate this method from the other Do<State>
> methods.

Done.

[Eran M. (Google), 2013-11-28 12:24:59]

As discussed offline, I have reverted the changes to cert_status_flags because
they are not necessary for displaying CT information in the UI once Al's patch
(88643002)  lands.
Since both patches are approved I can build the UI changes on top of both
patches, so no need for extra info in the cert_status word. I've saved you two
bits :)

I've also added an error code to net_error_list, will expand on error handling
in a follow-up CL, as discussed offline.

[commit-bot: I haz the power, 2013-11-28 13:44:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/eranm@google.com/76443006/360001

[commit-bot: I haz the power, 2013-11-28 15:11:57]

Change committed as 237785

[wtc, 2013-11-28 15:18:24]

Patch set 16 LGTM.

https://codereview.chromium.org/76443006/diff/360001/net/base/net_error_list.h
File net/base/net_error_list.h (right):

https://codereview.chromium.org/76443006/diff/360001/net/base/net_error_list....
net/base/net_error_list.h:413: NET_ERROR(CT_NO_SCTS_VERIFIED_OK, -299)

Let's work on this next week. I remember if an error is not < ERR_CERT_END, then
it won't trigger the standard certificate error handling.

This error should start with "CERT_". I suggest something like
"CERT_CT_VERIFY_FAILED" or "CERT_SCT_INVALID".

Note: Although error codes are not persisted, error code numbers are displayed
in error pages and may be cited in bug reports. So to avoid confusion, we don't
change error code values. But we have plenty of time to change this error before
the Chrome 33 branch is created.

https://codereview.chromium.org/76443006/diff/360001/net/cert/ct_known_logs.cc
File net/cert/ct_known_logs.cc (right):

https://codereview.chromium.org/76443006/diff/360001/net/cert/ct_known_logs.c...
net/cert/ct_known_logs.cc:65: scoped_ptr<net::CTLogVerifier>
CreateGoogleAviatorLogVerifier() {

Nit: remove net:: here and on line 71.