Add support for fetching Certificate Transparency SCTs over a TLS extension

Add support for fetching Certificate Transparency SCTs over a TLS extension

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

[ekasper, 2013-11-22 15:42:24]

This patch is on shaky ground until the CT code that uses it has landed but I
think that the core client socket and test server changes are stable enough for
review.

Things that I expect to change (and welcome suggestions for how to):
- I've protected the TLS extension with a command-line switch until I find out
what the overall roll-out plan is, and how creepy you think this change is. (And
I've brought up a browser instance, flipped the flag and got it to receive SCTs
from OpenSSL s_server, whee!)

- While Eran is working on the code that parses SCTs and exposes them back up
(via SSLInfo?), I've added some temporary glue to the socket to export an "SCTs
received" bit, so that I can unit test the code.

[wtc, 2013-11-26 17:32:55]

Patch set 1 LGTM.

1. I found a bug (marked with "BUG"). There is another minor bug in
ssl_config_service.cc.

2. It's not clear whether the signed_cert_timestamps command-line option for the
Python test server must be a text string. If so, we need to specify the format
of the string (base64-encoded or hex string), and the fake signed certificate
timestamp in the unit tests will need to be in that format. (Right now it is
"test".)

https://codereview.chromium.org/83333003/diff/1/chrome/browser/net/ssl_config...
File chrome/browser/net/ssl_config_service_manager_pref.cc (right):

https://codereview.chromium.org/83333003/diff/1/chrome/browser/net/ssl_config...
chrome/browser/net/ssl_config_service_manager_pref.cc:180: BooleanPrefMember
signed_cert_timestamps_enabled_;

Nit: I suggest inserting the signed_cert_timestamps_enabled_ member in an order
that matches the order of corresponding members in ssl_config_service.h.

https://codereview.chromium.org/83333003/diff/1/chrome/browser/prefs/command_...
File chrome/browser/prefs/command_line_pref_store.cc (right):

https://codereview.chromium.org/83333003/diff/1/chrome/browser/prefs/command_...
chrome/browser/prefs/command_line_pref_store.cc:63:
prefs::kEnableSignedCertTimestamps, false },

BUG: I think the third element should be true when the command-line switch and
the preference have the same sense (e.g., when both are "enable ...").

https://codereview.chromium.org/83333003/diff/1/net/socket/ssl_client_socket_...
File net/socket/ssl_client_socket_nss.cc (right):

https://codereview.chromium.org/83333003/diff/1/net/socket/ssl_client_socket_...
net/socket/ssl_client_socket_nss.cc:771: void UpdateSignedCertTimestamps();

Nit: declare this immediately after UpdateServerCert() because they are related.

https://codereview.chromium.org/83333003/diff/1/net/socket/ssl_client_socket_...
net/socket/ssl_client_socket_nss.cc:1664: UpdateSignedCertTimestamps();

Nit: call this immediately after UpdateServerCert() because they are related.

https://codereview.chromium.org/83333003/diff/1/net/socket/ssl_client_socket_...
net/socket/ssl_client_socket_nss.cc:2539: signed_cert_timestamps->len);

This should be aligned with reinterpret_cast. (I guess you want to highlight the
common "signed_cert_timestamps->" part of these two arguments.

https://codereview.chromium.org/83333003/diff/1/net/socket/ssl_client_socket_...
net/socket/ssl_client_socket_nss.cc:3200: // aren't guarded (ChannelID).

You can remove this whole comment. Correct, we no longer add #ifdef guard for
libssl-only changes.

https://codereview.chromium.org/83333003/diff/1/net/socket/ssl_client_socket_...
net/socket/ssl_client_socket_nss.cc:3205: "SSL_ENABLE_SIGNED_CERT_TIMESTAMPS");

Add curly braces (because the if statement's body spans multiple lines).

https://codereview.chromium.org/83333003/diff/1/net/socket/ssl_client_socket_...
File net/socket/ssl_client_socket_unittest.cc (right):

https://codereview.chromium.org/83333003/diff/1/net/socket/ssl_client_socket_...
net/socket/ssl_client_socket_unittest.cc:1849: SpawnedTestServer
test_server(SpawnedTestServer::TYPE_HTTPS,

The test_server in this unit test should also have a
ssl_options.signed_cert_timestamp as in the previous test, so that the only
difference between the two tests is whether the client advertises the TLS
extension or not.

https://codereview.chromium.org/83333003/diff/1/net/ssl/ssl_config_service.cc
File net/ssl/ssl_config_service.cc (right):

https://codereview.chromium.org/83333003/diff/1/net/ssl/ssl_config_service.cc...
net/ssl/ssl_config_service.cc:167: (orig_config.false_start_enabled !=
new_config.false_start_enabled) ||

Please add a check for signed_cert_timestamps_enabled here.

https://codereview.chromium.org/83333003/diff/1/net/ssl/ssl_config_service.h
File net/ssl/ssl_config_service.h (right):

https://codereview.chromium.org/83333003/diff/1/net/ssl/ssl_config_service.h#...
net/ssl/ssl_config_service.h:180: //     false_start_enabled

We may need to list signed_cert_timestamps_enabled here as well.

https://codereview.chromium.org/83333003/diff/1/net/test/spawned_test_server/...
File net/test/spawned_test_server/base_test_server.h (left):

https://codereview.chromium.org/83333003/diff/1/net/test/spawned_test_server/...
net/test/spawned_test_server/base_test_server.h:263: 

Some of these lines at the end of files may have been added to fix cpplint
warnings about a file missing a LF character at the end of file. It is best not
to remove them unless it is suggested by an automated tool such as clang format.

https://codereview.chromium.org/83333003/diff/1/net/tools/testserver/testserv...
File net/tools/testserver/testserver.py (right):

https://codereview.chromium.org/83333003/diff/1/net/tools/testserver/testserv...
net/tools/testserver/testserver.py:2080: default='',

I assume the option's value is a string. What is the format? Base64-encoded, or
a hex string such as "1A35FD08..."?

https://codereview.chromium.org/83333003/diff/1/third_party/tlslite/tlslite/T...
File third_party/tlslite/tlslite/TLSConnection.py (right):

https://codereview.chromium.org/83333003/diff/1/third_party/tlslite/tlslite/T...
third_party/tlslite/tlslite/TLSConnection.py:1009: verification.

Nit: we probably should document the signedCertTimestamps parameter here.

https://codereview.chromium.org/83333003/diff/1/third_party/tlslite/tlslite/c...
File third_party/tlslite/tlslite/constants.py (right):

https://codereview.chromium.org/83333003/diff/1/third_party/tlslite/tlslite/c...
third_party/tlslite/tlslite/constants.py:35: signed_cert_timestamps = 18  #
signed_certificate_timestamp extension

This constant has a self-explanatory name, so it doesn't seem to need a comment.
Did you add the comment with the unabbreviated extension name so that people can
find it when doing a source code search? Just wondering.

To me, a reference to RFC 6962 would be useful.

https://codereview.chromium.org/83333003/diff/1/third_party/tlslite/tlslite/m...
File third_party/tlslite/tlslite/messages.py (right):

https://codereview.chromium.org/83333003/diff/1/third_party/tlslite/tlslite/m...
third_party/tlslite/tlslite/messages.py:177: elif extType == 7:

Unrelated problem: this is strange. Extension number 6 is user_mapping and 7 is
client_authz. I don't know why tlslite maps them to srp_username and
certificate_types.

https://codereview.chromium.org/83333003/diff/1/third_party/tlslite/tlslite/m...
third_party/tlslite/tlslite/messages.py:182: self.signed_cert_timestamps = True

Should we verify that extension_data is empty? The RFC does not require the
server to check this.

https://codereview.chromium.org/83333003/diff/1/third_party/tlslite/tlslite/m...
third_party/tlslite/tlslite/messages.py:230: self.signed_cert_timestamps = None

Nit: it is a little confusing that signed_cert_timestamps is a boolean in
ClientHello but is a string in ServerHello. No need to change this.

https://codereview.chromium.org/83333003/diff/1/third_party/tlslite/tlslite/m...
third_party/tlslite/tlslite/messages.py:298:
w.addVarSeq(stringToBytes(self.signed_cert_timestamps), 1, 2)

I am not familiar with the stringToBytes function, but I noticed that it is used
here, but not used on line 281 when we calculate the length of extension_data.
Is this OK?

[ekasper, 2013-11-26 19:33:54]

Thanks!

Bugs fixed, and I've made the testserver command-line option base64-encoded.

Could you (either Ryan or Wan-Teh) guide me on the flag setup?

Eran (cc'd) says the rollout plan is to "turn CT on". If there's no overall
switch for CT then having a command-line switch for the TLS extension alone is
probably also pointless... So I'm wondering if I should
- flip the default in SSLConfig to true?
- flip the command line switch to kDisable...?
- remove the user-facing command line switch altogether?

https://codereview.chromium.org/83333003/diff/1/chrome/browser/net/ssl_config...
File chrome/browser/net/ssl_config_service_manager_pref.cc (right):

https://codereview.chromium.org/83333003/diff/1/chrome/browser/net/ssl_config...
chrome/browser/net/ssl_config_service_manager_pref.cc:180: BooleanPrefMember
signed_cert_timestamps_enabled_;
On 2013/11/26 17:32:55, wtc wrote:
> 
> Nit: I suggest inserting the signed_cert_timestamps_enabled_ member in an
order
> that matches the order of corresponding members in ssl_config_service.h.

Done.

https://codereview.chromium.org/83333003/diff/1/chrome/browser/prefs/command_...
File chrome/browser/prefs/command_line_pref_store.cc (right):

https://codereview.chromium.org/83333003/diff/1/chrome/browser/prefs/command_...
chrome/browser/prefs/command_line_pref_store.cc:63:
prefs::kEnableSignedCertTimestamps, false },
On 2013/11/26 17:32:55, wtc wrote:
> 
> BUG: I think the third element should be true when the command-line switch and
> the preference have the same sense (e.g., when both are "enable ...").

Yes, my bad - the third value should indicate the pref value when the switch is
present. Fixed.

https://codereview.chromium.org/83333003/diff/1/net/socket/ssl_client_socket_...
File net/socket/ssl_client_socket_nss.cc (right):

https://codereview.chromium.org/83333003/diff/1/net/socket/ssl_client_socket_...
net/socket/ssl_client_socket_nss.cc:771: void UpdateSignedCertTimestamps();
On 2013/11/26 17:32:55, wtc wrote:
> 
> Nit: declare this immediately after UpdateServerCert() because they are
related.

Done.

https://codereview.chromium.org/83333003/diff/1/net/socket/ssl_client_socket_...
net/socket/ssl_client_socket_nss.cc:1664: UpdateSignedCertTimestamps();
On 2013/11/26 17:32:55, wtc wrote:
> 
> Nit: call this immediately after UpdateServerCert() because they are related.

Done.

https://codereview.chromium.org/83333003/diff/1/net/socket/ssl_client_socket_...
net/socket/ssl_client_socket_nss.cc:2539: signed_cert_timestamps->len);
On 2013/11/26 17:32:55, wtc wrote:
> 
> This should be aligned with reinterpret_cast. (I guess you want to highlight
the
> common "signed_cert_timestamps->" part of these two arguments.

Done. (No reason other than my own sloppiness for the bad indent.)

https://codereview.chromium.org/83333003/diff/1/net/socket/ssl_client_socket_...
net/socket/ssl_client_socket_nss.cc:3200: // aren't guarded (ChannelID).
On 2013/11/26 17:32:55, wtc wrote:
> 
> You can remove this whole comment. Correct, we no longer add #ifdef guard for
> libssl-only changes.

Done.

https://codereview.chromium.org/83333003/diff/1/net/socket/ssl_client_socket_...
net/socket/ssl_client_socket_nss.cc:3205: "SSL_ENABLE_SIGNED_CERT_TIMESTAMPS");
On 2013/11/26 17:32:55, wtc wrote:
> 
> Add curly braces (because the if statement's body spans multiple lines).

Done.

https://codereview.chromium.org/83333003/diff/1/net/socket/ssl_client_socket_...
File net/socket/ssl_client_socket_unittest.cc (right):

https://codereview.chromium.org/83333003/diff/1/net/socket/ssl_client_socket_...
net/socket/ssl_client_socket_unittest.cc:1849: SpawnedTestServer
test_server(SpawnedTestServer::TYPE_HTTPS,
On 2013/11/26 17:32:55, wtc wrote:
> 
> The test_server in this unit test should also have a
> ssl_options.signed_cert_timestamp as in the previous test, so that the only
> difference between the two tests is whether the client advertises the TLS
> extension or not.

Good point - I wanted to test that no "SCTs" were mistakenly set - but this way
we also cover that no SCTs were requested. Done.

https://codereview.chromium.org/83333003/diff/1/net/ssl/ssl_config_service.cc
File net/ssl/ssl_config_service.cc (right):

https://codereview.chromium.org/83333003/diff/1/net/ssl/ssl_config_service.cc...
net/ssl/ssl_config_service.cc:167: (orig_config.false_start_enabled !=
new_config.false_start_enabled) ||
On 2013/11/26 17:32:55, wtc wrote:
> 
> Please add a check for signed_cert_timestamps_enabled here.

Done, and added to the test.

https://codereview.chromium.org/83333003/diff/1/net/test/spawned_test_server/...
File net/test/spawned_test_server/base_test_server.h (left):

https://codereview.chromium.org/83333003/diff/1/net/test/spawned_test_server/...
net/test/spawned_test_server/base_test_server.h:263: 
On 2013/11/26 17:32:55, wtc wrote:
> 
> Some of these lines at the end of files may have been added to fix cpplint
> warnings about a file missing a LF character at the end of file. It is best
not
> to remove them unless it is suggested by an automated tool such as clang
format.

Done.

https://codereview.chromium.org/83333003/diff/1/net/tools/testserver/testserv...
File net/tools/testserver/testserver.py (right):

https://codereview.chromium.org/83333003/diff/1/net/tools/testserver/testserv...
net/tools/testserver/testserver.py:2080: default='',
On 2013/11/26 17:32:55, wtc wrote:
> 
> I assume the option's value is a string. What is the format? Base64-encoded,
or
> a hex string such as "1A35FD08..."?

I've made them base64-encoded.

https://codereview.chromium.org/83333003/diff/1/third_party/tlslite/tlslite/T...
File third_party/tlslite/tlslite/TLSConnection.py (right):

https://codereview.chromium.org/83333003/diff/1/third_party/tlslite/tlslite/T...
third_party/tlslite/tlslite/TLSConnection.py:1009: verification.
On 2013/11/26 17:32:55, wtc wrote:
> 
> Nit: we probably should document the signedCertTimestamps parameter here.

I was badly fashioning after another patch. Done.

https://codereview.chromium.org/83333003/diff/1/third_party/tlslite/tlslite/c...
File third_party/tlslite/tlslite/constants.py (right):

https://codereview.chromium.org/83333003/diff/1/third_party/tlslite/tlslite/c...
third_party/tlslite/tlslite/constants.py:35: signed_cert_timestamps = 18  #
signed_certificate_timestamp extension
On 2013/11/26 17:32:55, wtc wrote:
> 
> This constant has a self-explanatory name, so it doesn't seem to need a
comment.
> Did you add the comment with the unabbreviated extension name so that people
can
> find it when doing a source code search? Just wondering.
> 
> To me, a reference to RFC 6962 would be useful.

Yep, to map to the official name. Added a reference.

https://codereview.chromium.org/83333003/diff/1/third_party/tlslite/tlslite/m...
File third_party/tlslite/tlslite/messages.py (right):

https://codereview.chromium.org/83333003/diff/1/third_party/tlslite/tlslite/m...
third_party/tlslite/tlslite/messages.py:177: elif extType == 7:
On 2013/11/26 17:32:55, wtc wrote:
> 
> Unrelated problem: this is strange. Extension number 6 is user_mapping and 7
is
> client_authz. I don't know why tlslite maps them to srp_username and
> certificate_types.

Hmm. It's probably experimental code that was simply grabbing the next
"available" slot.

This patch doesn't seem the right place to go fixing it but I've added a note.

https://codereview.chromium.org/83333003/diff/1/third_party/tlslite/tlslite/m...
third_party/tlslite/tlslite/messages.py:182: self.signed_cert_timestamps = True
On 2013/11/26 17:32:55, wtc wrote:
> 
> Should we verify that extension_data is empty? The RFC does not require the
> server to check this.

Good point. In a correct implementation, all extensions should at least verify
they consume extLength bytes. I've added an explicit check for our case just to
set an example.

(None of this is particularly robust: if you look at the codec you'll find that
e.g. getFixBytes silently returns a truncated slice if there are not enough
bytes left...)

https://codereview.chromium.org/83333003/diff/1/third_party/tlslite/tlslite/m...
third_party/tlslite/tlslite/messages.py:230: self.signed_cert_timestamps = None
On 2013/11/26 17:32:55, wtc wrote:
> 
> Nit: it is a little confusing that signed_cert_timestamps is a boolean in
> ClientHello but is a string in ServerHello. No need to change this.

I've changed it anyway, to support_signed_cert_timestamps in the ClientHello.

https://codereview.chromium.org/83333003/diff/1/third_party/tlslite/tlslite/m...
third_party/tlslite/tlslite/messages.py:298:
w.addVarSeq(stringToBytes(self.signed_cert_timestamps), 1, 2)
On 2013/11/26 17:32:55, wtc wrote:
> 
> I am not familiar with the stringToBytes function, but I noticed that it is
used
> here, but not used on line 281 when we calculate the length of extension_data.
> Is this OK?

Yeah, this is python strings for you: stringToBytes simply converts a python
8-bit string to an array of bytes. The length doesn't change.

[wtc, 2013-11-26 22:46:12]

Patch set 3 LGTM.

On 2013/11/26 19:33:54, ekasper wrote:
> 
> Could you (either Ryan or Wan-Teh) guide me on the flag setup?
> 
> Eran (cc'd) says the rollout plan is to "turn CT on". If there's no overall
> switch for CT then having a command-line switch for the TLS extension alone is
> probably also pointless... So I'm wondering if I should
> - flip the default in SSLConfig to true?
> - flip the command line switch to kDisable...?
> - remove the user-facing command line switch altogether?

I agree that if there is no overall command-line switch for CT, then having a
command-line switch for the TLS extension is pointless.

I probably would flip the default in SSLConfig to true, and remove the
command-line switch.

It seems fine for Chrome to advertise the signed_certificate_timestamp TLS
extension even when CT is disabled. Ideally the flag in SSLConfig should be set
to false if CT is disabled.

https://codereview.chromium.org/83333003/diff/60001/chrome/browser/net/ssl_co...
File chrome/browser/net/ssl_config_service_manager_pref.cc (right):

https://codereview.chromium.org/83333003/diff/60001/chrome/browser/net/ssl_co...
chrome/browser/net/ssl_config_service_manager_pref.cc:218:
signed_cert_timestamps_enabled_.Init(

Nit: move this up to follow ssl_record_splitting_disabled_.Init().

https://codereview.chromium.org/83333003/diff/60001/chrome/browser/net/ssl_co...
chrome/browser/net/ssl_config_service_manager_pref.cc:314:
config->signed_cert_timestamps_enabled =

Nit: also move this up.

https://codereview.chromium.org/83333003/diff/60001/net/socket/ssl_client_soc...
File net/socket/ssl_client_socket_nss.cc (right):

https://codereview.chromium.org/83333003/diff/60001/net/socket/ssl_client_soc...
net/socket/ssl_client_socket_nss.cc:2530: void
SSLClientSocketNSS::Core::UpdateSignedCertTimestamps() {

Nit: move the definition of the UpdateSignedCertTimestamps() up to line 2423, to
follow the definition of the UpdateServerCert() method.

https://codereview.chromium.org/83333003/diff/60001/net/test/spawned_test_ser...
File net/test/spawned_test_server/base_test_server.cc (right):

https://codereview.chromium.org/83333003/diff/60001/net/test/spawned_test_ser...
net/test/spawned_test_server/base_test_server.cc:403:
ssl_options_.signed_cert_timestamps);

An alternative design is to let ssl_options_.signed_cert_timestamps be a binary
string (so your unit tests can just use "test"), and perform the base64 encoding
here, when passing the second argument to arguments->SetString().

https://codereview.chromium.org/83333003/diff/60001/third_party/tlslite/tlsli...
File third_party/tlslite/tlslite/messages.py (right):

https://codereview.chromium.org/83333003/diff/60001/third_party/tlslite/tlsli...
third_party/tlslite/tlslite/messages.py:178: # extension 7 is client_authz

Sorry about the confusion. I didn't mean to ask you to comment this. I was just
writing down my thoughts as I reviewed this code.

This code has been fixed in the current version of tlslite to use
ExtensionType.srp (12) and ExtensionType.cert_type (9), respectively:
https://github.com/trevp/tlslite/blob/master/tlslite/messages.py

So I suggest we remove this comment.

[ekasper, 2013-11-27 14:09:03]

I've reverted the command-line switch, and flipped the default in the config to
true.

(I've also added the missing def to export_win.def.)

https://codereview.chromium.org/83333003/diff/60001/chrome/browser/net/ssl_co...
File chrome/browser/net/ssl_config_service_manager_pref.cc (right):

https://codereview.chromium.org/83333003/diff/60001/chrome/browser/net/ssl_co...
chrome/browser/net/ssl_config_service_manager_pref.cc:218:
signed_cert_timestamps_enabled_.Init(
On 2013/11/26 22:46:12, wtc wrote:
> 
> Nit: move this up to follow ssl_record_splitting_disabled_.Init().

No longer applies: this part is reverted.

https://codereview.chromium.org/83333003/diff/60001/chrome/browser/net/ssl_co...
chrome/browser/net/ssl_config_service_manager_pref.cc:314:
config->signed_cert_timestamps_enabled =
On 2013/11/26 22:46:12, wtc wrote:
> 
> Nit: also move this up.

Ditto.

https://codereview.chromium.org/83333003/diff/60001/net/socket/ssl_client_soc...
File net/socket/ssl_client_socket_nss.cc (right):

https://codereview.chromium.org/83333003/diff/60001/net/socket/ssl_client_soc...
net/socket/ssl_client_socket_nss.cc:2530: void
SSLClientSocketNSS::Core::UpdateSignedCertTimestamps() {
On 2013/11/26 22:46:12, wtc wrote:
> 
> Nit: move the definition of the UpdateSignedCertTimestamps() up to line 2423,
to
> follow the definition of the UpdateServerCert() method.

Done.

https://codereview.chromium.org/83333003/diff/60001/net/test/spawned_test_ser...
File net/test/spawned_test_server/base_test_server.cc (right):

https://codereview.chromium.org/83333003/diff/60001/net/test/spawned_test_ser...
net/test/spawned_test_server/base_test_server.cc:403:
ssl_options_.signed_cert_timestamps);
On 2013/11/26 22:46:12, wtc wrote:
> 
> An alternative design is to let ssl_options_.signed_cert_timestamps be a
binary
> string (so your unit tests can just use "test"), and perform the base64
encoding
> here, when passing the second argument to arguments->SetString().

That's much better, thanks! Done.

https://codereview.chromium.org/83333003/diff/60001/third_party/tlslite/tlsli...
File third_party/tlslite/tlslite/messages.py (right):

https://codereview.chromium.org/83333003/diff/60001/third_party/tlslite/tlsli...
third_party/tlslite/tlslite/messages.py:178: # extension 7 is client_authz
On 2013/11/26 22:46:12, wtc wrote:
> 
> Sorry about the confusion. I didn't mean to ask you to comment this. I was
just
> writing down my thoughts as I reviewed this code.
> 
> This code has been fixed in the current version of tlslite to use
> ExtensionType.srp (12) and ExtensionType.cert_type (9), respectively:
> https://github.com/trevp/tlslite/blob/master/tlslite/messages.py
> 
> So I suggest we remove this comment.

Done.

[wtc, 2013-11-27 15:08:39]

Patch set 6 LGTM.

[commit-bot: I haz the power, 2013-11-28 12:00:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ekasper@google.com/83333003/140001

[commit-bot: I haz the power, 2013-11-28 13:43:29]

Change committed as 237775