Remove certificates from Channel ID

Remove certificates from Channel ID

When creating a new Channel ID, an X.509 certificate is created, even though
only the keypair is used. This CL removes the generation and storage of
certificates for Channel ID and updates the callsites to use the modified
API that only exposes a crypto::ECPrivateKey.

BUG=457566
Committed: https://crrev.com/2e171cf4d960a6c723a529b2a4cbceb075bcd306
Cr-Commit-Position: refs/heads/master@{#332260}

[nharper, 2015-04-09 22:19:41]

nharper@chromium.org changed reviewers:
+ mattm@chromium.org, rsleevi@chromium.org

[Ryan Sleevi, 2015-04-09 22:33:49]

rsleevi@chromium.org changed required reviewers:
+ mattm@chromium.org

[Ryan Sleevi, 2015-04-09 22:40:10]

Quick scan for issues; I'll leave Matt to do the major review.

https://codereview.chromium.org/1076063002/diff/1/net/extras/sqlite/sqlite_ch...
File net/extras/sqlite/sqlite_channel_id_store.cc (right):

https://codereview.chromium.org/1076063002/diff/1/net/extras/sqlite/sqlite_ch...
net/extras/sqlite/sqlite_channel_id_store.cc:55: */
This code is commented out.

https://codereview.chromium.org/1076063002/diff/1/net/extras/sqlite/sqlite_ch...
net/extras/sqlite/sqlite_channel_id_store.cc:212: if (!EnsureDatabaseVersion())
{
Why no InitTable() call?

https://codereview.chromium.org/1076063002/diff/1/net/extras/sqlite/sqlite_ch...
net/extras/sqlite/sqlite_channel_id_store.cc:231: return;
Doesn't this cause things to explode if no channel_ids exist (e.g. a new
profile)?

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

https://codereview.chromium.org/1076063002/diff/1/net/socket/ssl_client_socke...
net/socket/ssl_client_socket_nss.cc:2324: if (channel_id_key_.get() == nullptr)
if (!channel_id_key_)

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

https://codereview.chromium.org/1076063002/diff/1/net/ssl/channel_id_service....
net/ssl/channel_id_service.cc:98: if (!key.get()) {
if (!key)

https://codereview.chromium.org/1076063002/diff/1/net/ssl/channel_id_service....
net/ssl/channel_id_service.cc:123: int CreateECPrivateKeyFromSerializedKey(
return net::Error instead?

https://codereview.chromium.org/1076063002/diff/1/net/ssl/channel_id_service....
net/ssl/channel_id_service.cc:129: }
no braces

https://codereview.chromium.org/1076063002/diff/1/net/ssl/channel_id_service....
net/ssl/channel_id_service.cc:130: // crypto::ECPrivateKey expects
std::vector<uint8> instead of std::string.
newline + delete the comment

https://codereview.chromium.org/1076063002/diff/1/net/ssl/channel_id_service....
net/ssl/channel_id_service.cc:222: }
no braces

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

https://codereview.chromium.org/1076063002/diff/1/net/ssl/channel_id_service....
net/ssl/channel_id_service.h:216: std::string* private_key);
This is a little weird. Why isn't this part of the interface of
crypto::ECPrivateKey?

https://codereview.chromium.org/1076063002/diff/1/net/ssl/channel_id_service....
net/ssl/channel_id_service.h:227: scoped_ptr<crypto::ECPrivateKey>* key_out);
Why does the API require the public key to create the private key?

[nharper, 2015-04-10 00:32:09]

https://codereview.chromium.org/1076063002/diff/1/net/extras/sqlite/sqlite_ch...
File net/extras/sqlite/sqlite_channel_id_store.cc (right):

https://codereview.chromium.org/1076063002/diff/1/net/extras/sqlite/sqlite_ch...
net/extras/sqlite/sqlite_channel_id_store.cc:55: */
On 2015/04/09 22:40:09, Ryan Sleevi wrote:
> This code is commented out.

Sorry - removed.

https://codereview.chromium.org/1076063002/diff/1/net/extras/sqlite/sqlite_ch...
net/extras/sqlite/sqlite_channel_id_store.cc:212: if (!EnsureDatabaseVersion())
{
On 2015/04/09 22:40:09, Ryan Sleevi wrote:
> Why no InitTable() call?

It didn't make sense as its own function - I rolled its functionality into
EnsureDatabaseVersion. With this schema change, I'm actually creating a new
table (sqlite won't let you drop columns), migrating, and dropping the old one,
so it didn't fit the old pattern of InitTable() very well.

https://codereview.chromium.org/1076063002/diff/1/net/extras/sqlite/sqlite_ch...
net/extras/sqlite/sqlite_channel_id_store.cc:231: return;
On 2015/04/09 22:40:09, Ryan Sleevi wrote:
> Doesn't this cause things to explode if no channel_ids exist (e.g. a new
> profile)?

I didn't see any explosions.

I deleted my chromium profile and started up a build of this CL and there were
no explosions. I also navigated to a google site and checked the table in the
sqlite database to verify that the table was created, and that it had entries
(since going to google.com ends up creating channel ids for 7 different google
domains).

Maybe I should try putting the db in a bad state (where it still exists) so that
it makes EnsureDatabaseVersion fail, and see what happens?

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

https://codereview.chromium.org/1076063002/diff/1/net/socket/ssl_client_socke...
net/socket/ssl_client_socket_nss.cc:2324: if (channel_id_key_.get() == nullptr)
On 2015/04/09 22:40:09, Ryan Sleevi wrote:
> if (!channel_id_key_)

Done.

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

https://codereview.chromium.org/1076063002/diff/1/net/ssl/channel_id_service....
net/ssl/channel_id_service.cc:98: if (!key.get()) {
On 2015/04/09 22:40:09, Ryan Sleevi wrote:
> if (!key)

Done.

https://codereview.chromium.org/1076063002/diff/1/net/ssl/channel_id_service....
net/ssl/channel_id_service.cc:123: int CreateECPrivateKeyFromSerializedKey(
On 2015/04/09 22:40:09, Ryan Sleevi wrote:
> return net::Error instead?

The callsites of CreateECPrivateKey expect an int error code, which is why I did
it this way. I'm not very familiar with error codes or net::Error.

https://codereview.chromium.org/1076063002/diff/1/net/ssl/channel_id_service....
net/ssl/channel_id_service.cc:129: }
On 2015/04/09 22:40:09, Ryan Sleevi wrote:
> no braces

Done.

https://codereview.chromium.org/1076063002/diff/1/net/ssl/channel_id_service....
net/ssl/channel_id_service.cc:130: // crypto::ECPrivateKey expects
std::vector<uint8> instead of std::string.
On 2015/04/09 22:40:09, Ryan Sleevi wrote:
> newline + delete the comment

Done.

https://codereview.chromium.org/1076063002/diff/1/net/ssl/channel_id_service....
net/ssl/channel_id_service.cc:151: if (!key.get())
I also changed this to if (!key)

https://codereview.chromium.org/1076063002/diff/1/net/ssl/channel_id_service....
net/ssl/channel_id_service.cc:222: }
On 2015/04/09 22:40:10, Ryan Sleevi wrote:
> no braces

Done.

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

https://codereview.chromium.org/1076063002/diff/1/net/ssl/channel_id_service....
net/ssl/channel_id_service.h:216: std::string* private_key);
On 2015/04/09 22:40:10, Ryan Sleevi wrote:
> This is a little weird. Why isn't this part of the interface of
> crypto::ECPrivateKey?

crypto::ECPrivateKey deals in std::vector<uint8> instead of std::string. It also
separates this out into 2 methods (ExportEncryptedPrivateKey and
ExportPublicKey). I tried to minimize the number of times we translate between
std::string keypairs and crypto::ECPrivateKey, but it ended up being done in
multiple places (depending on whether the key is created or provided, and
whether we get the result synchronously or asynchronously), so I factored it out
into its own function (and it's also used in unittests). The code for either
this function has a TODO about possibly changing crypto::ECPrivateKey to output
a std::string* instead of a vector.

https://codereview.chromium.org/1076063002/diff/1/net/ssl/channel_id_service....
net/ssl/channel_id_service.h:227: scoped_ptr<crypto::ECPrivateKey>* key_out);
On 2015/04/09 22:40:10, Ryan Sleevi wrote:
> Why does the API require the public key to create the private key?

My opinion is that it shouldn't be needed, but the NSS code requires it
(boringssl ignores the public key).

When the NSS code creates the SECKEYPrivateKey, it uses PK11_UnwrapPrivKey,
which takes a SECItem* publicValue (which is generated from the public key
passed in to crypto::ECPrivateKey::CreateFromEncryptedPrivateKeyInfo). I haven't
read through very carefully the implemenation of PK11_UnwrapPrivKey to see what
exactly it's doing with the SECItem, but it does use it. It might be possible to
generate some other SECItem to pass in (I don't know if it's actually possible),
but that seems like a lot of work and setup for a situation where we already
have access to the public key.

[Ryan Sleevi, 2015-04-10 00:42:14]

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

https://codereview.chromium.org/1076063002/diff/1/net/ssl/channel_id_service....
net/ssl/channel_id_service.cc:123: int CreateECPrivateKeyFromSerializedKey(
On 2015/04/10 00:32:09, nharper wrote:
> On 2015/04/09 22:40:09, Ryan Sleevi wrote:
> > return net::Error instead?
> 
> The callsites of CreateECPrivateKey expect an int error code, which is why I
did
> it this way. I'm not very familiar with error codes or net::Error.

net::Error is just an enum that promotes to int. It just is better that if
you're never going to return a "byte size" to actually return a net::Error than
a "net::Error as an int". The compiler will convert for you, but it makes the
API contract cleaner.

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

https://codereview.chromium.org/1076063002/diff/1/net/ssl/channel_id_service....
net/ssl/channel_id_service.h:214: NET_EXPORT_PRIVATE int
ExportKeypair(scoped_ptr<crypto::ECPrivateKey>& key,
naming: KeyPair

https://codereview.chromium.org/1076063002/diff/1/net/ssl/channel_id_service....
net/ssl/channel_id_service.h:214: NET_EXPORT_PRIVATE int
ExportKeypair(scoped_ptr<crypto::ECPrivateKey>& key,
style: const-ref or pass as raw pointer

https://codereview.chromium.org/1076063002/diff/1/net/ssl/channel_id_service....
net/ssl/channel_id_service.h:216: std::string* private_key);
On 2015/04/10 00:32:09, nharper wrote:
> crypto::ECPrivateKey deals in std::vector<uint8> instead of std::string. It
also
> separates this out into 2 methods (ExportEncryptedPrivateKey and
> ExportPublicKey). I tried to minimize the number of times we translate between
> std::string keypairs and crypto::ECPrivateKey, but it ended up being done in
> multiple places (depending on whether the key is created or provided, and
> whether we get the result synchronously or asynchronously), so I factored it
out
> into its own function (and it's also used in unittests). The code for either
> this function has a TODO about possibly changing crypto::ECPrivateKey to
output
> a std::string* instead of a vector.

We only need the two calls because of NSS, right? That is, in a 'sane' API, we'd
just be exporting the private key, and so the only distinction between this and
the function on crypto::ECPrivateKey is... std vs vector?

https://codereview.chromium.org/1076063002/diff/1/net/ssl/channel_id_service....
net/ssl/channel_id_service.h:227: scoped_ptr<crypto::ECPrivateKey>* key_out);
On 2015/04/10 00:32:09, nharper wrote:
> (I don't know if it's actually possible),
> but that seems like a lot of work and setup for a situation where we already
> have access to the public key.

On the other hand, it encourages readers/API designers to think the public key
is required, when it's "not" (for all values of not not equal to iOS,
post-Chimera)

I'd be inclined to recover the public key from the private key. However, your
further explanation makes me think that the format of |private_key| supplied to
this call varies depending on whether you're using NSS vs BoringSSL - is that
correct?

If so, that's sort of a dangerous API to leave lying around...

[mattm, 2015-04-10 01:00:28]

https://codereview.chromium.org/1076063002/diff/1/net/extras/sqlite/sqlite_ch...
File net/extras/sqlite/sqlite_channel_id_store.cc (right):

https://codereview.chromium.org/1076063002/diff/1/net/extras/sqlite/sqlite_ch...
net/extras/sqlite/sqlite_channel_id_store.cc:34: const int
kCompatibleVersionNumber = 1;
Should be 5 also?

https://codereview.chromium.org/1076063002/diff/1/net/extras/sqlite/sqlite_ch...
net/extras/sqlite/sqlite_channel_id_store.cc:348:
meta_table_.SetCompatibleVersionNumber(kCurrentVersionNumber);
kCompatibleVersionNumber

https://codereview.chromium.org/1076063002/diff/1/net/extras/sqlite/sqlite_ch...
File net/extras/sqlite/sqlite_channel_id_store_unittest.cc (right):

https://codereview.chromium.org/1076063002/diff/1/net/extras/sqlite/sqlite_ch...
net/extras/sqlite/sqlite_channel_id_store_unittest.cc:444: // Create a version 3
database.
4

https://codereview.chromium.org/1076063002/diff/1/net/quic/crypto/channel_id_...
File net/quic/crypto/channel_id_chromium.cc (right):

https://codereview.chromium.org/1076063002/diff/1/net/quic/crypto/channel_id_...
net/quic/crypto/channel_id_chromium.cc:199: channel_id_key_.reset(new
ChannelIDKeyChromium(ec_private_key));
Doesn't this mean channel_id_crypto_key_ and channel_id_key_ both now own the
same ECPrivateKey?

(I'd also change ChannelIDKeyChromium to take a scoped_ptr as input so that the
ownership transfer is clear.)

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

https://codereview.chromium.org/1076063002/diff/1/net/socket/ssl_client_socke...
net/socket/ssl_client_socket_nss.cc:2328: *key = channel_id_key_->key();
Need to SECKEY_CopyPrivateKey and SECKEY_CopyPublicKey here.

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

https://codereview.chromium.org/1076063002/diff/1/net/ssl/channel_id_service....
net/ssl/channel_id_service.cc:135: crypto::ECPrivateKey* key =
make this a scoped_ptr or stick directly in key_out

https://codereview.chromium.org/1076063002/diff/1/net/ssl/channel_id_service....
net/ssl/channel_id_service.cc:139: if (key == nullptr)
if (!key)

https://codereview.chromium.org/1076063002/diff/1/net/ssl/channel_id_service....
net/ssl/channel_id_service.cc:204: case ERR_ORIGIN_BOUND_CERT_GENERATION_FAILED:
Can remove this case.

Also "remove" ERR_ORIGIN_BOUND_CERT_GENERATION_FAILED from net_error_list.h
(replace the line with a comment that it was removed, search for "was removed"
for examples). Same with  ASYNC_FAILURE_CREATE_CERT.

https://codereview.chromium.org/1076063002/diff/1/net/ssl/channel_id_service....
net/ssl/channel_id_service.cc:219:
CreateECPrivateKeyFromSerializedKey(public_key, private_key, key_);
Is there a reason to try this if error is not OK?

https://codereview.chromium.org/1076063002/diff/1/net/ssl/channel_id_service....
net/ssl/channel_id_service.cc:220: if (error == OK && err != OK) {
Having such similarly named variables is confusing.

https://codereview.chromium.org/1076063002/diff/1/net/ssl/channel_id_service....
net/ssl/channel_id_service.cc:440: std::string private_key;
these are unused

https://codereview.chromium.org/1076063002/diff/1/net/ssl/channel_id_service....
net/ssl/channel_id_service.cc:639: // |expiration_time| is ignored, and expired
certs are considered valid.
update comment

https://codereview.chromium.org/1076063002/diff/1/net/ssl/channel_id_service....
net/ssl/channel_id_service.cc:651: }
no braces

https://codereview.chromium.org/1076063002/diff/1/net/ssl/channel_id_service_...
File net/ssl/channel_id_service_unittest.cc (right):

https://codereview.chromium.org/1076063002/diff/1/net/ssl/channel_id_service_...
net/ssl/channel_id_service_unittest.cc:124: EXPECT_FALSE(key1.get() == nullptr
|| key2.get() == nullptr);
I think it's easier to read (and provides better debugging if it does fail) to
split these into separate statements.
Also they should be ASSERTs, not EXPECTs (since continuing would cause the
unittest to segfault).

https://codereview.chromium.org/1076063002/diff/1/net/ssl/channel_id_service_...
net/ssl/channel_id_service_unittest.cc:126:
ASSERT_TRUE(key1->ExportRawPublicKey(&public_key1) &&
use EXPECT here

https://codereview.chromium.org/1076063002/diff/1/net/ssl/channel_id_service_...
net/ssl/channel_id_service_unittest.cc:127:
key2->ExportRawPublicKey(&public_key2));
split into separate expects

https://codereview.chromium.org/1076063002/diff/1/net/ssl/channel_id_service_...
net/ssl/channel_id_service_unittest.cc:249: EXPECT_NE(key2.get(), key3.get());
actually want ExpectKeysNotEqual functions, since the ECPrivateKey objects could
have different pointers while still containing the same key data

https://codereview.chromium.org/1076063002/diff/1/net/ssl/channel_id_service_...
net/ssl/channel_id_service_unittest.cc:321: TEST_F(ChannelIDServiceTest,
ExtractValuesFromBytesEC) {
Test doesn't do what its name says anymore (probably just remove this test)

https://codereview.chromium.org/1076063002/diff/1/net/ssl/channel_id_service_...
net/ssl/channel_id_service_unittest.cc:476: EXPECT_NE(key2.get(), key3.get());
Same as above (ExpectKeysNotEqual)

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

https://codereview.chromium.org/1076063002/diff/1/net/ssl/channel_id_store.h#...
net/ssl/channel_id_store.h:41: // The time the certificate was created.
key

[Ryan Sleevi, 2015-04-15 22:50:03]

A lot more comments.

The biggest overall comment is why does ChannelIDStore need to know about
public/private keypairs? It seems refactoring that to strictly
crypto::ECPrivateKey<> buys us wins across the board (e.g. WebView support for
opaque keys), and gives us a transition path for when crypto::ECPrivateKey<>
becomes the opaque-ish handle that we were talking about with David

https://codereview.chromium.org/1076063002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/api/messaging/message_property_provider.cc
(right):

https://codereview.chromium.org/1076063002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/message_property_provider.cc:93:
base::StringPiece spki(reinterpret_cast<char*>(&spki_vector[0]),
vector_as_array<> is far safer for this (see base/stl_util)

https://codereview.chromium.org/1076063002/diff/20001/net/cert/x509_util_nss.cc
File net/cert/x509_util_nss.cc (right):

https://codereview.chromium.org/1076063002/diff/20001/net/cert/x509_util_nss....
net/cert/x509_util_nss.cc:79: }
This is all now dead code

https://codereview.chromium.org/1076063002/diff/20001/net/cert/x509_util_nss....
net/cert/x509_util_nss.cc:262: CERTCertificate* cert =
CreateCertificate(key->public_key(),
Should |CreateCertificate| be folded into this? Does it make sense to have a
distinct function?

https://codereview.chromium.org/1076063002/diff/20001/net/cert/x509_util_open...
File net/cert/x509_util_openssl.cc (right):

https://codereview.chromium.org/1076063002/diff/20001/net/cert/x509_util_open...
net/cert/x509_util_openssl.cc:239: ScopedX509 cert(CreateCertificate(key->key(),
Ditto here - do we still need a distinct function?

https://codereview.chromium.org/1076063002/diff/20001/net/extras/sqlite/sqlit...
File net/extras/sqlite/sqlite_channel_id_store.cc (left):

https://codereview.chromium.org/1076063002/diff/20001/net/extras/sqlite/sqlit...
net/extras/sqlite/sqlite_channel_id_store.cc:386: LOG_IF(WARNING, cur_version <
kCurrentVersionNumber)
Why is this deleted?

https://codereview.chromium.org/1076063002/diff/20001/net/extras/sqlite/sqlit...
File net/extras/sqlite/sqlite_channel_id_store.cc (right):

https://codereview.chromium.org/1076063002/diff/20001/net/extras/sqlite/sqlit...
net/extras/sqlite/sqlite_channel_id_store.cc:273:
db_->GetUniqueStatement("INSERT INTO channel_id VALUES (?, ?, ?, ?)"));
Recommendation: Make the column names explicit

INSERT INTO channel_id (host, private_key, public_key, creation_time) VALUES (?,
?, ?, ?)

https://codereview.chromium.org/1076063002/diff/20001/net/extras/sqlite/sqlit...
net/extras/sqlite/sqlite_channel_id_store.cc:288: }
No braces for one-line conditionals, right?

https://codereview.chromium.org/1076063002/diff/20001/net/extras/sqlite/sqlit...
net/extras/sqlite/sqlite_channel_id_store.cc:301: return false;
Is this correct? Should you skip that channel ID instead? Why or why not?

https://codereview.chromium.org/1076063002/diff/20001/net/extras/sqlite/sqlit...
net/extras/sqlite/sqlite_channel_id_store.cc:303: insert_statement.BindBlob(2,
spki.data(), spki.size());
In the other cases, you cast to an int. Why do you not need to? the size_type
for BasicStringPiece (what influences base::StringPiece) is size_t

https://codereview.chromium.org/1076063002/diff/20001/net/extras/sqlite/sqlit...
net/extras/sqlite/sqlite_channel_id_store.cc:327:
meta_table_.SetCompatibleVersionNumber(kCurrentVersionNumber);
Just confirming - these are part of part of the transaction as well, right?

https://codereview.chromium.org/1076063002/diff/20001/net/extras/sqlite/sqlit...
File net/extras/sqlite/sqlite_channel_id_store_unittest.cc (right):

https://codereview.chromium.org/1076063002/diff/20001/net/extras/sqlite/sqlit...
net/extras/sqlite/sqlite_channel_id_store_unittest.cc:444: // Create a version 3
database.
typo? You say UpgradeV4, but then say v3 here

https://codereview.chromium.org/1076063002/diff/20001/net/extras/sqlite/sqlit...
net/extras/sqlite/sqlite_channel_id_store_unittest.cc:468: add_smt.BindInt64(4,
1000);
Does this mean we migrate expired channel IDs? Is that expected?

(I don't know - probably; Matt would probably know)

https://codereview.chromium.org/1076063002/diff/20001/net/quic/crypto/channel...
File net/quic/crypto/channel_id_chromium.cc (right):

https://codereview.chromium.org/1076063002/diff/20001/net/quic/crypto/channel...
net/quic/crypto/channel_id_chromium.cc:90: scoped_ptr<crypto::ECPrivateKey>
channel_id_crypto_key_;
Should add a comment to note that this must be created before / destroyed after
|channel_id_key_|, since channel_id_key contains a non-owning pointer to this
object.

https://codereview.chromium.org/1076063002/diff/20001/net/quic/crypto/channel...
net/quic/crypto/channel_id_chromium.cc:199: channel_id_key_.reset(new
ChannelIDKeyChromium(ec_private_key));
Does it make sense to |.Pass()| ownership of |channel_id_crypto_key_| when doing
this? Why or why not?

https://codereview.chromium.org/1076063002/diff/20001/net/ssl/channel_id_serv...
File net/ssl/channel_id_service.cc (right):

https://codereview.chromium.org/1076063002/diff/20001/net/ssl/channel_id_serv...
net/ssl/channel_id_service.cc:131: memcpy(&public_key_vector[0],
public_key.data(), public_key.size());
DANGER: What if |public_key| is .empty()? This would be a fatal thing
(&public_key_vector[0] is undefined then)

It does seem like simply "fixing" ECPrivateKey to use base::StringPiece resolves
this, right? Do you want to go do that before this complexity?

https://codereview.chromium.org/1076063002/diff/20001/net/ssl/channel_id_serv...
net/ssl/channel_id_service.cc:144: int
ExportKeypair(scoped_ptr<crypto::ECPrivateKey>& key,
pass as const-ref

But really, you should be sending in a naked key, right?

crypto::ECPrivateKey* key

https://codereview.chromium.org/1076063002/diff/20001/net/ssl/channel_id_serv...
net/ssl/channel_id_service.cc:162: *public_key =
std::string(public_key_vector.begin(), public_key_vector.end());
Does this work across platforms without throwing conversion errors about
signed/unsigned conversions?

https://codereview.chromium.org/1076063002/diff/20001/net/ssl/channel_id_serv...
net/ssl/channel_id_service.cc:166: }
Should match order in .h

https://codereview.chromium.org/1076063002/diff/20001/net/ssl/channel_id_serv...
net/ssl/channel_id_service.cc:398: is_system_time_valid_ =
x509_util::IsSupportedValidityRange(start, end);
None of this is needed, is it, now that we're no longer bound by the
X509Certificate type. This also applies to the Service (and propogates through
the interface)

https://codereview.chromium.org/1076063002/diff/20001/net/ssl/channel_id_serv...
net/ssl/channel_id_service.cc:438: std::string private_key;
These two are unused, right?

https://codereview.chromium.org/1076063002/diff/20001/net/ssl/channel_id_serv...
File net/ssl/channel_id_service.h (right):

https://codereview.chromium.org/1076063002/diff/20001/net/ssl/channel_id_serv...
net/ssl/channel_id_service.h:17: #include "crypto/ec_private_key.h"
You can fully forward-declare this object in this header; no need to include the
header.

https://codereview.chromium.org/1076063002/diff/20001/net/ssl/channel_id_serv...
net/ssl/channel_id_service.h:224: NET_EXPORT_PRIVATE int
CreateECPrivateKeyFromSerializedKey(
If these are only for testing, does it make more sense to make them private and
exclusively friend the unit tests?

https://codereview.chromium.org/1076063002/diff/20001/net/ssl/channel_id_serv...
File net/ssl/channel_id_service_unittest.cc (right):

https://codereview.chromium.org/1076063002/diff/20001/net/ssl/channel_id_serv...
net/ssl/channel_id_service_unittest.cc:123: scoped_ptr<crypto::ECPrivateKey>&
key2) {
If you're going to pass-by-ref, pass by const-ref.

But it seems here you can/should pass by naked pointer

https://codereview.chromium.org/1076063002/diff/20001/net/ssl/channel_id_serv...
net/ssl/channel_id_service_unittest.cc:124: EXPECT_FALSE(key1.get() == nullptr
|| key2.get() == nullptr);
Break these into separate bits

EXPECT_TRUE(key1)
EXPECT_TRUE(key2)

https://codereview.chromium.org/1076063002/diff/20001/net/ssl/channel_id_serv...
net/ssl/channel_id_service_unittest.cc:127:
key2->ExportRawPublicKey(&public_key2));
This can just be EXPECT, right?

https://codereview.chromium.org/1076063002/diff/20001/net/ssl/channel_id_serv...
net/ssl/channel_id_service_unittest.cc:165: EXPECT_EQ(nullptr, key.get());
EXPECT_FALSE(key)

https://codereview.chromium.org/1076063002/diff/20001/net/ssl/channel_id_serv...
net/ssl/channel_id_service_unittest.cc:185: EXPECT_NE(nullptr, key1.get());
EXPECT_TRUE(key1)

https://codereview.chromium.org/1076063002/diff/20001/net/ssl/channel_id_serv...
net/ssl/channel_id_service_unittest.cc:335: EXPECT_TRUE(key != nullptr);
EXPECT_TRUE(key)

https://codereview.chromium.org/1076063002/diff/20001/net/ssl/channel_id_store.h
File net/ssl/channel_id_store.h (right):

https://codereview.chromium.org/1076063002/diff/20001/net/ssl/channel_id_stor...
net/ssl/channel_id_store.h:45: // ecdsa_sign: DER-encoded
EncryptedPrivateKeyInfo struct.
This comment no longer applies, does it?

If not, should this (and the rest of the ChannelID store) be refactored in terms
of simply crypto::ECPrivateKey<>, which lets you push the encoding/decoding down
into the store?

That is, no consumer of the ChannelIDStore ever has to know of the
public/private key split.

https://codereview.chromium.org/1076063002/diff/20001/net/ssl/channel_id_stor...
net/ssl/channel_id_store.h:77: std::string* cert_result,
*cough*

https://codereview.chromium.org/1076063002/diff/20001/net/ssl/channel_id_stor...
net/ssl/channel_id_store.h:85: const std::string& cert) = 0;
*cough*

[mattm, 2015-04-15 23:11:46]

https://codereview.chromium.org/1076063002/diff/20001/net/extras/sqlite/sqlit...
File net/extras/sqlite/sqlite_channel_id_store_unittest.cc (right):

https://codereview.chromium.org/1076063002/diff/20001/net/extras/sqlite/sqlit...
net/extras/sqlite/sqlite_channel_id_store_unittest.cc:468: add_smt.BindInt64(4,
1000);
On 2015/04/15 22:50:02, Ryan Sleevi wrote:
> Does this mean we migrate expired channel IDs? Is that expected?
> 
> (I don't know - probably; Matt would probably know)

Seems right to me, since we've been ignoring the expiration for a long time.

[nharper, 2015-04-25 02:59:20]

https://codereview.chromium.org/1076063002/diff/1/net/extras/sqlite/sqlite_ch...
File net/extras/sqlite/sqlite_channel_id_store.cc (right):

https://codereview.chromium.org/1076063002/diff/1/net/extras/sqlite/sqlite_ch...
net/extras/sqlite/sqlite_channel_id_store.cc:34: const int
kCompatibleVersionNumber = 1;
On 2015/04/10 01:00:27, mattm wrote:
> Should be 5 also?

I need it to be both 1 and 5, depending on the context. In EnsureDatabaseVersion
(when migrating from a previous version to the current version), the compatible
version number is 1 - we can migrate from version 1. However, when updating the
current and compatible versions once the schema is updated, they are both 5.

https://codereview.chromium.org/1076063002/diff/1/net/extras/sqlite/sqlite_ch...
net/extras/sqlite/sqlite_channel_id_store.cc:348:
meta_table_.SetCompatibleVersionNumber(kCurrentVersionNumber);
On 2015/04/10 01:00:27, mattm wrote:
> kCompatibleVersionNumber

The compatible version number here needs to be 5, not 1. Maybe I should change
kCompatibleVersionNumber to 2 constants?

const int kOldCompatibleVersionNumber = 1;
const int kNewCompatibleVersionNumber = 5;

https://codereview.chromium.org/1076063002/diff/1/net/extras/sqlite/sqlite_ch...
File net/extras/sqlite/sqlite_channel_id_store_unittest.cc (right):

https://codereview.chromium.org/1076063002/diff/1/net/extras/sqlite/sqlite_ch...
net/extras/sqlite/sqlite_channel_id_store_unittest.cc:444: // Create a version 3
database.
On 2015/04/10 01:00:27, mattm wrote:
> 4

Done.

https://codereview.chromium.org/1076063002/diff/1/net/quic/crypto/channel_id_...
File net/quic/crypto/channel_id_chromium.cc (right):

https://codereview.chromium.org/1076063002/diff/1/net/quic/crypto/channel_id_...
net/quic/crypto/channel_id_chromium.cc:199: channel_id_key_.reset(new
ChannelIDKeyChromium(ec_private_key));
On 2015/04/10 01:00:27, mattm wrote:
> Doesn't this mean channel_id_crypto_key_ and channel_id_key_ both now own the
> same ECPrivateKey?
> 
> (I'd also change ChannelIDKeyChromium to take a scoped_ptr as input so that
the
> ownership transfer is clear.)

I've fixed the ownership issues here so that channel_id_crypto_key_ relinquishes
ownership to channel_id_key_. I also changed ChannelIDKeyChromium to take a
scoped_ptr.

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

https://codereview.chromium.org/1076063002/diff/1/net/socket/ssl_client_socke...
net/socket/ssl_client_socket_nss.cc:2328: *key = channel_id_key_->key();
On 2015/04/10 01:00:27, mattm wrote:
> Need to SECKEY_CopyPrivateKey and SECKEY_CopyPublicKey here.

Done.

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

https://codereview.chromium.org/1076063002/diff/1/net/ssl/channel_id_service....
net/ssl/channel_id_service.cc:123: int CreateECPrivateKeyFromSerializedKey(
On 2015/04/10 00:42:14, Ryan Sleevi wrote:
> On 2015/04/10 00:32:09, nharper wrote:
> > On 2015/04/09 22:40:09, Ryan Sleevi wrote:
> > > return net::Error instead?
> > 
> > The callsites of CreateECPrivateKey expect an int error code, which is why I
> did
> > it this way. I'm not very familiar with error codes or net::Error.
> 
> net::Error is just an enum that promotes to int. It just is better that if
> you're never going to return a "byte size" to actually return a net::Error
than
> a "net::Error as an int". The compiler will convert for you, but it makes the
> API contract cleaner.

Done.

https://codereview.chromium.org/1076063002/diff/1/net/ssl/channel_id_service....
net/ssl/channel_id_service.cc:135: crypto::ECPrivateKey* key =
On 2015/04/10 01:00:27, mattm wrote:
> make this a scoped_ptr or stick directly in key_out

Done.

https://codereview.chromium.org/1076063002/diff/1/net/ssl/channel_id_service....
net/ssl/channel_id_service.cc:139: if (key == nullptr)
On 2015/04/10 01:00:27, mattm wrote:
> if (!key)

Done.

https://codereview.chromium.org/1076063002/diff/1/net/ssl/channel_id_service....
net/ssl/channel_id_service.cc:204: case ERR_ORIGIN_BOUND_CERT_GENERATION_FAILED:
On 2015/04/10 01:00:27, mattm wrote:
> Can remove this case.
> 
> Also "remove" ERR_ORIGIN_BOUND_CERT_GENERATION_FAILED from net_error_list.h
> (replace the line with a comment that it was removed, search for "was removed"
> for examples). Same with  ASYNC_FAILURE_CREATE_CERT.

Done.

https://codereview.chromium.org/1076063002/diff/1/net/ssl/channel_id_service....
net/ssl/channel_id_service.cc:219:
CreateECPrivateKeyFromSerializedKey(public_key, private_key, key_);
On 2015/04/10 01:00:27, mattm wrote:
> Is there a reason to try this if error is not OK?

If I don't call it when error is not OK, the unit tests still pass, so I guess
it isn't needed in all cases. (I really should understand what's actually going
on and come up with a better answer here.)

https://codereview.chromium.org/1076063002/diff/1/net/ssl/channel_id_service....
net/ssl/channel_id_service.cc:220: if (error == OK && err != OK) {
On 2015/04/10 01:00:27, mattm wrote:
> Having such similarly named variables is confusing.

Done.

https://codereview.chromium.org/1076063002/diff/1/net/ssl/channel_id_service....
net/ssl/channel_id_service.cc:440: std::string private_key;
On 2015/04/10 01:00:27, mattm wrote:
> these are unused

Done.

https://codereview.chromium.org/1076063002/diff/1/net/ssl/channel_id_service....
net/ssl/channel_id_service.cc:639: // |expiration_time| is ignored, and expired
certs are considered valid.
On 2015/04/10 01:00:27, mattm wrote:
> update comment

Done.

https://codereview.chromium.org/1076063002/diff/1/net/ssl/channel_id_service....
net/ssl/channel_id_service.cc:651: }
On 2015/04/10 01:00:27, mattm wrote:
> no braces

Done.

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

https://codereview.chromium.org/1076063002/diff/1/net/ssl/channel_id_service....
net/ssl/channel_id_service.h:214: NET_EXPORT_PRIVATE int
ExportKeypair(scoped_ptr<crypto::ECPrivateKey>& key,
On 2015/04/10 00:42:14, Ryan Sleevi wrote:
> style: const-ref or pass as raw pointer

Done.

https://codereview.chromium.org/1076063002/diff/1/net/ssl/channel_id_service....
net/ssl/channel_id_service.h:214: NET_EXPORT_PRIVATE int
ExportKeypair(scoped_ptr<crypto::ECPrivateKey>& key,
On 2015/04/10 00:42:14, Ryan Sleevi wrote:
> naming: KeyPair

Done.

https://codereview.chromium.org/1076063002/diff/1/net/ssl/channel_id_service....
net/ssl/channel_id_service.h:216: std::string* private_key);
On 2015/04/10 00:42:14, Ryan Sleevi wrote:
> On 2015/04/10 00:32:09, nharper wrote:
> > crypto::ECPrivateKey deals in std::vector<uint8> instead of std::string. It
> also
> > separates this out into 2 methods (ExportEncryptedPrivateKey and
> > ExportPublicKey). I tried to minimize the number of times we translate
between
> > std::string keypairs and crypto::ECPrivateKey, but it ended up being done in
> > multiple places (depending on whether the key is created or provided, and
> > whether we get the result synchronously or asynchronously), so I factored it
> out
> > into its own function (and it's also used in unittests). The code for either
> > this function has a TODO about possibly changing crypto::ECPrivateKey to
> output
> > a std::string* instead of a vector.
> 
> We only need the two calls because of NSS, right? That is, in a 'sane' API,
we'd
> just be exporting the private key, and so the only distinction between this
and
> the function on crypto::ECPrivateKey is... std vs vector?

Yes, in a sane API (like openssl's) we'd only need the private key. If we assume
a sane API, then the difference between this and ExportPrivateKey (the Encrypted
part is a restriction imposed by NSS - in an openssl-only world that could also
be removed) is std::string vs std::vector<uint8> - i.e. the type of container
and whether the underlying type is signed or unsigned.

https://codereview.chromium.org/1076063002/diff/1/net/ssl/channel_id_service....
net/ssl/channel_id_service.h:227: scoped_ptr<crypto::ECPrivateKey>* key_out);
On 2015/04/10 00:42:14, Ryan Sleevi wrote:
> On 2015/04/10 00:32:09, nharper wrote:
> > (I don't know if it's actually possible),
> > but that seems like a lot of work and setup for a situation where we already
> > have access to the public key.
> 
> On the other hand, it encourages readers/API designers to think the public key
> is required, when it's "not" (for all values of not not equal to iOS,
> post-Chimera)
> 
> I'd be inclined to recover the public key from the private key. However, your
> further explanation makes me think that the format of |private_key| supplied
to
> this call varies depending on whether you're using NSS vs BoringSSL - is that
> correct?
> 
> If so, that's sort of a dangerous API to leave lying around...

The format of the encrypted private_key is the same for both NSS and BoringSSL -
it's a pkcs8 EncryptedPrivateKeyInfo with an encryptionAlgorithm of
pbeWithSHAAnd3-KeyTripleDES-CBC (1.2.840.113549.1.12.1.3).

I'm not sure it's worth it for me to figure out the NSS code to recover the
public key from the private key, especially given the direction we're headed in.
Also, if we push the transition from crypto::ECPrivateKey to std::string down
from here (ChannelIDService) to ChannelIDStore, it should be less visible to
readers.

https://codereview.chromium.org/1076063002/diff/1/net/ssl/channel_id_service_...
File net/ssl/channel_id_service_unittest.cc (right):

https://codereview.chromium.org/1076063002/diff/1/net/ssl/channel_id_service_...
net/ssl/channel_id_service_unittest.cc:124: EXPECT_FALSE(key1.get() == nullptr
|| key2.get() == nullptr);
On 2015/04/10 01:00:27, mattm wrote:
> I think it's easier to read (and provides better debugging if it does fail) to
> split these into separate statements.
> Also they should be ASSERTs, not EXPECTs (since continuing would cause the
> unittest to segfault).

Done.

https://codereview.chromium.org/1076063002/diff/1/net/ssl/channel_id_service_...
net/ssl/channel_id_service_unittest.cc:126:
ASSERT_TRUE(key1->ExportRawPublicKey(&public_key1) &&
On 2015/04/10 01:00:27, mattm wrote:
> use EXPECT here

Done.

https://codereview.chromium.org/1076063002/diff/1/net/ssl/channel_id_service_...
net/ssl/channel_id_service_unittest.cc:127:
key2->ExportRawPublicKey(&public_key2));
On 2015/04/10 01:00:28, mattm wrote:
> split into separate expects

Done.

https://codereview.chromium.org/1076063002/diff/1/net/ssl/channel_id_service_...
net/ssl/channel_id_service_unittest.cc:249: EXPECT_NE(key2.get(), key3.get());
On 2015/04/10 01:00:27, mattm wrote:
> actually want ExpectKeysNotEqual functions, since the ECPrivateKey objects
could
> have different pointers while still containing the same key data

Done.

https://codereview.chromium.org/1076063002/diff/1/net/ssl/channel_id_service_...
net/ssl/channel_id_service_unittest.cc:321: TEST_F(ChannelIDServiceTest,
ExtractValuesFromBytesEC) {
On 2015/04/10 01:00:27, mattm wrote:
> Test doesn't do what its name says anymore (probably just remove this test)

Test removed.

https://codereview.chromium.org/1076063002/diff/1/net/ssl/channel_id_service_...
net/ssl/channel_id_service_unittest.cc:476: EXPECT_NE(key2.get(), key3.get());
On 2015/04/10 01:00:27, mattm wrote:
> Same as above (ExpectKeysNotEqual)

Done.

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

https://codereview.chromium.org/1076063002/diff/1/net/ssl/channel_id_store.h#...
net/ssl/channel_id_store.h:41: // The time the certificate was created.
On 2015/04/10 01:00:28, mattm wrote:
> key

Done.

https://codereview.chromium.org/1076063002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/api/messaging/message_property_provider.cc
(right):

https://codereview.chromium.org/1076063002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/messaging/message_property_provider.cc:93:
base::StringPiece spki(reinterpret_cast<char*>(&spki_vector[0]),
On 2015/04/15 22:50:02, Ryan Sleevi wrote:
> vector_as_array<> is far safer for this (see base/stl_util)

Done.

https://codereview.chromium.org/1076063002/diff/20001/net/cert/x509_util_nss.cc
File net/cert/x509_util_nss.cc (right):

https://codereview.chromium.org/1076063002/diff/20001/net/cert/x509_util_nss....
net/cert/x509_util_nss.cc:79: }
On 2015/04/15 22:50:02, Ryan Sleevi wrote:
> This is all now dead code

Done.

https://codereview.chromium.org/1076063002/diff/20001/net/cert/x509_util_nss....
net/cert/x509_util_nss.cc:262: CERTCertificate* cert =
CreateCertificate(key->public_key(),
On 2015/04/15 22:50:02, Ryan Sleevi wrote:
> Should |CreateCertificate| be folded into this? Does it make sense to have a
> distinct function?

I like keeping it the way it is. CreateCertificate is a rather lengthy function,
and keeping it as 2 functions separates out the creating a certificate part with
signing the certificate. If CreateCertificate were to be folded in, then it
would also make sense to fold in SignCertificate (since this is the only place
that uses SignCertificate). I think it's cleaner to keep the functions smaller
and more focused, even if it's only being called from one place.

https://codereview.chromium.org/1076063002/diff/20001/net/cert/x509_util_open...
File net/cert/x509_util_openssl.cc (right):

https://codereview.chromium.org/1076063002/diff/20001/net/cert/x509_util_open...
net/cert/x509_util_openssl.cc:239: ScopedX509 cert(CreateCertificate(key->key(),
On 2015/04/15 22:50:02, Ryan Sleevi wrote:
> Ditto here - do we still need a distinct function?

See my argument on the other thread for why it's cleaner code to keep
CreateCertificate and SignAndDerEncodeCert around instead of rolling it all up.

https://codereview.chromium.org/1076063002/diff/20001/net/extras/sqlite/sqlit...
File net/extras/sqlite/sqlite_channel_id_store.cc (left):

https://codereview.chromium.org/1076063002/diff/20001/net/extras/sqlite/sqlit...
net/extras/sqlite/sqlite_channel_id_store.cc:386: LOG_IF(WARNING, cur_version <
kCurrentVersionNumber)
On 2015/04/15 22:50:02, Ryan Sleevi wrote:
> Why is this deleted?

This check never would have fired earlier - it was added to be defensive of
someone adding a new version and breaking things (like what I'm doing). The old
migration code here upgraded one version at a time, and updated cur_version to
match what it had just been upgraded to. Since there is upgrade logic for every
version number 1 to 4, this check never would have tripped.

In my new code, cur_version has different semantics. Instead of upgrading one
version at a time, I upgrade from each supported previous version (1 thru 4) to
the new version (5) in a single pass, and I also don't update cur_version -
cur_version keeps the version number that the db had before the migration
started. With this new meaning, I have an if for this conditional (see line 319
on the right) that finishes handling part of the migration. This log statement
doesn't make any sense in the new code, so it was removed.

https://codereview.chromium.org/1076063002/diff/20001/net/extras/sqlite/sqlit...
File net/extras/sqlite/sqlite_channel_id_store.cc (right):

https://codereview.chromium.org/1076063002/diff/20001/net/extras/sqlite/sqlit...
net/extras/sqlite/sqlite_channel_id_store.cc:273:
db_->GetUniqueStatement("INSERT INTO channel_id VALUES (?, ?, ?, ?)"));
On 2015/04/15 22:50:02, Ryan Sleevi wrote:
> Recommendation: Make the column names explicit
> 
> INSERT INTO channel_id (host, private_key, public_key, creation_time) VALUES
(?,
> ?, ?, ?)

Done.

https://codereview.chromium.org/1076063002/diff/20001/net/extras/sqlite/sqlit...
net/extras/sqlite/sqlite_channel_id_store.cc:288: }
On 2015/04/15 22:50:02, Ryan Sleevi wrote:
> No braces for one-line conditionals, right?

Done.

https://codereview.chromium.org/1076063002/diff/20001/net/extras/sqlite/sqlit...
net/extras/sqlite/sqlite_channel_id_store.cc:301: return false;
On 2015/04/15 22:50:02, Ryan Sleevi wrote:
> Is this correct? Should you skip that channel ID instead? Why or why not?

The thought here is that the db should only contain valid certs - if we have a
cert that we can't extract a spki from, then something has probably gone wrong
with the db and we should blow it away (that's what this currently does). Before
we try to extract the spki from the der cert, we've already constructed an
X500Certificate from the der cert, so this call should succeed.

On the other hand, when we check the cert key type and verify that it's ecdsa,
if that check fails we just skip it, even though there should never be a
non-ecdsa key/cert in the db at v>=2.

I'd argue that if we can't extract the spki (yet were able to parse the cert),
then something has gone wrong and we should fail harder than if we have a
cert/key of the wrong type.

https://codereview.chromium.org/1076063002/diff/20001/net/extras/sqlite/sqlit...
net/extras/sqlite/sqlite_channel_id_store.cc:303: insert_statement.BindBlob(2,
spki.data(), spki.size());
On 2015/04/15 22:50:02, Ryan Sleevi wrote:
> In the other cases, you cast to an int. Why do you not need to? the size_type
> for BasicStringPiece (what influences base::StringPiece) is size_t

I don't know why other places cast to an int - presumably for some platform
other than linux. It works fine without the cast on linux. It should be
consistent - I can either remove all the casts (and find out why they were there
with the trybots if they were actually needed), or add the cast here and go with
what's in the rest of the file (and assume that whoever added the casts had a
good reason).

https://codereview.chromium.org/1076063002/diff/20001/net/extras/sqlite/sqlit...
net/extras/sqlite/sqlite_channel_id_store.cc:327:
meta_table_.SetCompatibleVersionNumber(kCurrentVersionNumber);
On 2015/04/15 22:50:02, Ryan Sleevi wrote:
> Just confirming - these are part of part of the transaction as well, right?

That's my intention and that's my understanding of how the sql code works -
meta_table_ is operating on the same sql::Connection as the rest of the
statements here.

https://codereview.chromium.org/1076063002/diff/20001/net/extras/sqlite/sqlit...
File net/extras/sqlite/sqlite_channel_id_store_unittest.cc (right):

https://codereview.chromium.org/1076063002/diff/20001/net/extras/sqlite/sqlit...
net/extras/sqlite/sqlite_channel_id_store_unittest.cc:444: // Create a version 3
database.
On 2015/04/15 22:50:02, Ryan Sleevi wrote:
> typo? You say UpgradeV4, but then say v3 here

Typo. Fixed.

https://codereview.chromium.org/1076063002/diff/20001/net/extras/sqlite/sqlit...
net/extras/sqlite/sqlite_channel_id_store_unittest.cc:468: add_smt.BindInt64(4,
1000);
On 2015/04/15 22:50:02, Ryan Sleevi wrote:
> Does this mean we migrate expired channel IDs? Is that expected?
> 
> (I don't know - probably; Matt would probably know)

Yes, it means we migrate keys that have expired certs. This is fine (and
intended), because we never checked the expiration time.

https://codereview.chromium.org/1076063002/diff/20001/net/quic/crypto/channel...
File net/quic/crypto/channel_id_chromium.cc (right):

https://codereview.chromium.org/1076063002/diff/20001/net/quic/crypto/channel...
net/quic/crypto/channel_id_chromium.cc:90: scoped_ptr<crypto::ECPrivateKey>
channel_id_crypto_key_;
On 2015/04/15 22:50:02, Ryan Sleevi wrote:
> Should add a comment to note that this must be created before / destroyed
after
> |channel_id_key_|, since channel_id_key contains a non-owning pointer to this
> object.

channel_id_key_ contains an owning pointer to the object. Now that I'm
.release()ing channel_id_crypto_key_ when creating channel_id_key_, custody of
the object should be kept at exactly one owner, there shouldn't be any concern
about it having 2 owners?

https://codereview.chromium.org/1076063002/diff/20001/net/quic/crypto/channel...
net/quic/crypto/channel_id_chromium.cc:199: channel_id_key_.reset(new
ChannelIDKeyChromium(ec_private_key));
On 2015/04/15 22:50:02, Ryan Sleevi wrote:
> Does it make sense to |.Pass()| ownership of |channel_id_crypto_key_| when
doing
> this? Why or why not?

It wouldn't make sense to .Pass() channel_id_crypto_key_, since nothing else is
expecting a scoped_ptr<crypto::ECPrivateKey> - ChannelIDKeyChromium wants a raw
pointer. However, it would make sense to .release() the channel_id_crypto_key_
so that the scoped_ptr no longer owns it (since the ChannelIDKeyChromium wants
to own it). That would also solve the issue of requiring channel_id_key_ to be
destroyed before channel_id_crypto_key_.

https://codereview.chromium.org/1076063002/diff/20001/net/ssl/channel_id_serv...
File net/ssl/channel_id_service.cc (right):

https://codereview.chromium.org/1076063002/diff/20001/net/ssl/channel_id_serv...
net/ssl/channel_id_service.cc:131: memcpy(&public_key_vector[0],
public_key.data(), public_key.size());
On 2015/04/15 22:50:02, Ryan Sleevi wrote:
> DANGER: What if |public_key| is .empty()? This would be a fatal thing
> (&public_key_vector[0] is undefined then)
> 
> It does seem like simply "fixing" ECPrivateKey to use base::StringPiece
resolves
> this, right? Do you want to go do that before this complexity?

If public_key or private_key is .empty(), then this function doesn't do anything
to key_out and returns ERR_UNEXPECTED (line 128).

My main concern with the complexity here is the memcpy - the rest is just ugly
conversions.

https://codereview.chromium.org/1076063002/diff/20001/net/ssl/channel_id_serv...
net/ssl/channel_id_service.cc:144: int
ExportKeypair(scoped_ptr<crypto::ECPrivateKey>& key,
On 2015/04/15 22:50:02, Ryan Sleevi wrote:
> pass as const-ref
> 
> But really, you should be sending in a naked key, right?
> 
> crypto::ECPrivateKey* key

Changed to const-ref. Does the style guide have input as to whether I should use
const scoped_ptr<T>& vs T*? In the only callsite of this function (outside of
unittests), the key is a scoped_ptr<crypto::ECPrivateKey> instead of a raw
pointer (to facilitate deleting the object pointed to appropriately), and I
don't think there's any disagreement about using a scoped_ptr there. In the
unittests, using a scoped_ptr is handy because it takes care of deleting the
object when it goes out of scope at the end of the test case.

My reasoning for using scoped_ptr<crypto::ECPrivateKey>& here instead of
crypto::ECPrivateKey* is because it makes it easier on the callsite (since all
callers are using a scoped_ptr) - this way the caller doesn't have to call
.get() on the scoped_ptr.

I'm not that familiar with working with scoped_ptrs, so if convention suggests
doing things differently (or if I'm missing something), I'd be glad to change
it.

https://codereview.chromium.org/1076063002/diff/20001/net/ssl/channel_id_serv...
net/ssl/channel_id_service.cc:162: *public_key =
std::string(public_key_vector.begin(), public_key_vector.end());
On 2015/04/15 22:50:02, Ryan Sleevi wrote:
> Does this work across platforms without throwing conversion errors about
> signed/unsigned conversions?

This code was copied from the lhs of this diff (see line 120), so it should be
fine.

https://codereview.chromium.org/1076063002/diff/20001/net/ssl/channel_id_serv...
net/ssl/channel_id_service.cc:166: }
On 2015/04/15 22:50:02, Ryan Sleevi wrote:
> Should match order in .h

Done.

https://codereview.chromium.org/1076063002/diff/20001/net/ssl/channel_id_serv...
net/ssl/channel_id_service.cc:398: is_system_time_valid_ =
x509_util::IsSupportedValidityRange(start, end);
On 2015/04/15 22:50:02, Ryan Sleevi wrote:
> None of this is needed, is it, now that we're no longer bound by the
> X509Certificate type. This also applies to the Service (and propogates through
> the interface)

Done.

https://codereview.chromium.org/1076063002/diff/20001/net/ssl/channel_id_serv...
net/ssl/channel_id_service.cc:438: std::string private_key;
On 2015/04/15 22:50:02, Ryan Sleevi wrote:
> These two are unused, right?

Correct. Removed.

https://codereview.chromium.org/1076063002/diff/20001/net/ssl/channel_id_serv...
File net/ssl/channel_id_service.h (right):

https://codereview.chromium.org/1076063002/diff/20001/net/ssl/channel_id_serv...
net/ssl/channel_id_service.h:17: #include "crypto/ec_private_key.h"
On 2015/04/15 22:50:02, Ryan Sleevi wrote:
> You can fully forward-declare this object in this header; no need to include
the
> header.

Done.

https://codereview.chromium.org/1076063002/diff/20001/net/ssl/channel_id_serv...
net/ssl/channel_id_service.h:224: NET_EXPORT_PRIVATE int
CreateECPrivateKeyFromSerializedKey(
On 2015/04/15 22:50:02, Ryan Sleevi wrote:
> If these are only for testing, does it make more sense to make them private
and
> exclusively friend the unit tests?

If these functions stay as they are, then yes it makes sense to make them
private and friend the unit tests. There was some discussion on the .cc file
about moving this functionality into ECPrivateKey, so I'll hold off on changes
here until that's figured out.

https://codereview.chromium.org/1076063002/diff/20001/net/ssl/channel_id_serv...
File net/ssl/channel_id_service_unittest.cc (right):

https://codereview.chromium.org/1076063002/diff/20001/net/ssl/channel_id_serv...
net/ssl/channel_id_service_unittest.cc:123: scoped_ptr<crypto::ECPrivateKey>&
key2) {
On 2015/04/15 22:50:03, Ryan Sleevi wrote:
> If you're going to pass-by-ref, pass by const-ref.
> 
> But it seems here you can/should pass by naked pointer

I'm sticking with const-ref here because otherwise I have to change all of the
callsites from ExpectKeysEqual(key1, key2) to ExpectKeysEqual(key1.get(),
key2.get()) - I don't see any benefit to changing this from const-ref to naked
pointer.

https://codereview.chromium.org/1076063002/diff/20001/net/ssl/channel_id_serv...
net/ssl/channel_id_service_unittest.cc:124: EXPECT_FALSE(key1.get() == nullptr
|| key2.get() == nullptr);
On 2015/04/15 22:50:03, Ryan Sleevi wrote:
> Break these into separate bits
> 
> EXPECT_TRUE(key1)
> EXPECT_TRUE(key2)

Done.

https://codereview.chromium.org/1076063002/diff/20001/net/ssl/channel_id_serv...
net/ssl/channel_id_service_unittest.cc:127:
key2->ExportRawPublicKey(&public_key2));
On 2015/04/15 22:50:03, Ryan Sleevi wrote:
> This can just be EXPECT, right?

This was ASSERT because the EXPECT below uses the result of the computation in
the ASSERT, and it doesn't make sense to do the following EXPECT if the ASSERT
fails. There won't be any problems with the logic or execution of this function
if it's changed to EXPECT.

https://codereview.chromium.org/1076063002/diff/20001/net/ssl/channel_id_serv...
net/ssl/channel_id_service_unittest.cc:165: EXPECT_EQ(nullptr, key.get());
On 2015/04/15 22:50:03, Ryan Sleevi wrote:
> EXPECT_FALSE(key)

Done.

https://codereview.chromium.org/1076063002/diff/20001/net/ssl/channel_id_serv...
net/ssl/channel_id_service_unittest.cc:185: EXPECT_NE(nullptr, key1.get());
On 2015/04/15 22:50:03, Ryan Sleevi wrote:
> EXPECT_TRUE(key1)

Done.

https://codereview.chromium.org/1076063002/diff/20001/net/ssl/channel_id_serv...
net/ssl/channel_id_service_unittest.cc:335: EXPECT_TRUE(key != nullptr);
On 2015/04/15 22:50:03, Ryan Sleevi wrote:
> EXPECT_TRUE(key)

Done.

https://codereview.chromium.org/1076063002/diff/20001/net/ssl/channel_id_store.h
File net/ssl/channel_id_store.h (right):

https://codereview.chromium.org/1076063002/diff/20001/net/ssl/channel_id_stor...
net/ssl/channel_id_store.h:45: // ecdsa_sign: DER-encoded
EncryptedPrivateKeyInfo struct.
On 2015/04/15 22:50:03, Ryan Sleevi wrote:
> This comment no longer applies, does it?
Correct - it no longer applies. It's reworded.
> 
> If not, should this (and the rest of the ChannelID store) be refactored in
terms
> of simply crypto::ECPrivateKey<>, which lets you push the encoding/decoding
down
> into the store?
> 
> That is, no consumer of the ChannelIDStore ever has to know of the
> public/private key split.

My impression from reading the code is that ChannelIDStore is only used by
ChannelIDService, and any consumers we care about would be working with that
interface. That's why I decided to make ChannelIDService do the
encoding/decoding, and let the store just handle storing the keypair (and no
encoding/decoding). I guess I wasn't very clear on this part of the design in my
doc
https://docs.google.com/document/d/1lNTkg-1iSMkeqgIvFoekz7rBUfIHaaDF3v2XSPyPw....

I think I'm ok with pushing it down from ChannelIDService to ChannelIDStore (it
might also clean up some of the
ExportKeypair/CreateECPrivateKeyFromSerializedKey helper method nonsense in
ChannelIDService), but I want to check that that's the correct design choice
before jumping into it.

https://codereview.chromium.org/1076063002/diff/20001/net/ssl/channel_id_stor...
net/ssl/channel_id_store.h:77: std::string* cert_result,
On 2015/04/15 22:50:03, Ryan Sleevi wrote:
> *cough*

Done.

https://codereview.chromium.org/1076063002/diff/20001/net/ssl/channel_id_stor...
net/ssl/channel_id_store.h:85: const std::string& cert) = 0;
On 2015/04/15 22:50:03, Ryan Sleevi wrote:
> *cough*

Done.

[mattm, 2015-04-27 20:55:53]

https://codereview.chromium.org/1076063002/diff/1/net/extras/sqlite/sqlite_ch...
File net/extras/sqlite/sqlite_channel_id_store.cc (right):

https://codereview.chromium.org/1076063002/diff/1/net/extras/sqlite/sqlite_ch...
net/extras/sqlite/sqlite_channel_id_store.cc:34: const int
kCompatibleVersionNumber = 1;
On 2015/04/25 02:59:18, nharper wrote:
> On 2015/04/10 01:00:27, mattm wrote:
> > Should be 5 also?
> 
> I need it to be both 1 and 5, depending on the context. In
EnsureDatabaseVersion
> (when migrating from a previous version to the current version), the
compatible
> version number is 1 - we can migrate from version 1. However, when updating
the
> current and compatible versions once the schema is updated, they are both 5.

Hm, it looks to me that in this CL kCompatibleVersionNumber is only currently
used in the meta_table_.Init statement, where does it need to be 1?

https://codereview.chromium.org/1076063002/diff/1/net/extras/sqlite/sqlite_ch...
net/extras/sqlite/sqlite_channel_id_store.cc:348:
meta_table_.SetCompatibleVersionNumber(kCurrentVersionNumber);
On 2015/04/25 02:59:18, nharper wrote:
> On 2015/04/10 01:00:27, mattm wrote:
> > kCompatibleVersionNumber
> 
> The compatible version number here needs to be 5, not 1. Maybe I should change
> kCompatibleVersionNumber to 2 constants?
> 
> const int kOldCompatibleVersionNumber = 1;
> const int kNewCompatibleVersionNumber = 5;

Yeah, I think it should be 5 here also. I don't see where
kOldCompatibleVersionNumber is needed though.

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

https://codereview.chromium.org/1076063002/diff/1/net/ssl/channel_id_service....
net/ssl/channel_id_service.cc:123: int CreateECPrivateKeyFromSerializedKey(
On 2015/04/25 02:59:18, nharper wrote:
> On 2015/04/10 00:42:14, Ryan Sleevi wrote:
> > On 2015/04/10 00:32:09, nharper wrote:
> > > On 2015/04/09 22:40:09, Ryan Sleevi wrote:
> > > > return net::Error instead?
> > > 
> > > The callsites of CreateECPrivateKey expect an int error code, which is why
I
> > did
> > > it this way. I'm not very familiar with error codes or net::Error.
> > 
> > net::Error is just an enum that promotes to int. It just is better that if
> > you're never going to return a "byte size" to actually return a net::Error
> than
> > a "net::Error as an int". The compiler will convert for you, but it makes
the
> > API contract cleaner.
> 
> Done.

Still seems to be int?

https://codereview.chromium.org/1076063002/diff/1/net/ssl/channel_id_service....
net/ssl/channel_id_service.cc:135: crypto::ECPrivateKey* key =
On 2015/04/25 02:59:18, nharper wrote:
> On 2015/04/10 01:00:27, mattm wrote:
> > make this a scoped_ptr or stick directly in key_out
> 
> Done.

Doesn't seem to be done either. Did some changes get dropped?

https://codereview.chromium.org/1076063002/diff/1/net/ssl/channel_id_service....
net/ssl/channel_id_service.cc:139: if (key == nullptr)
On 2015/04/25 02:59:18, nharper wrote:
> On 2015/04/10 01:00:27, mattm wrote:
> > if (!key)
> 
> Done.

same

https://codereview.chromium.org/1076063002/diff/1/net/ssl/channel_id_service....
net/ssl/channel_id_service.cc:204: case ERR_ORIGIN_BOUND_CERT_GENERATION_FAILED:
On 2015/04/25 02:59:18, nharper wrote:
> On 2015/04/10 01:00:27, mattm wrote:
> > Can remove this case.
> > 
> > Also "remove" ERR_ORIGIN_BOUND_CERT_GENERATION_FAILED from net_error_list.h
> > (replace the line with a comment that it was removed, search for "was
removed"
> > for examples). Same with  ASYNC_FAILURE_CREATE_CERT.
> 
> Done.

same

https://codereview.chromium.org/1076063002/diff/1/net/ssl/channel_id_service....
net/ssl/channel_id_service.cc:639: // |expiration_time| is ignored, and expired
certs are considered valid.
On 2015/04/25 02:59:18, nharper wrote:
> On 2015/04/10 01:00:27, mattm wrote:
> > update comment
> 
> Done.

same

https://codereview.chromium.org/1076063002/diff/1/net/ssl/channel_id_service....
net/ssl/channel_id_service.cc:651: }
On 2015/04/25 02:59:18, nharper wrote:
> On 2015/04/10 01:00:27, mattm wrote:
> > no braces
> 
> Done.

same

https://codereview.chromium.org/1076063002/diff/1/net/ssl/channel_id_service_...
File net/ssl/channel_id_service_unittest.cc (right):

https://codereview.chromium.org/1076063002/diff/1/net/ssl/channel_id_service_...
net/ssl/channel_id_service_unittest.cc:127:
key2->ExportRawPublicKey(&public_key2));
On 2015/04/25 02:59:18, nharper wrote:
> On 2015/04/10 01:00:28, mattm wrote:
> > split into separate expects
> 
> Done.

still should be split

https://codereview.chromium.org/1076063002/diff/1/net/ssl/channel_id_service_...
net/ssl/channel_id_service_unittest.cc:249: EXPECT_NE(key2.get(), key3.get());
On 2015/04/25 02:59:18, nharper wrote:
> On 2015/04/10 01:00:27, mattm wrote:
> > actually want ExpectKeysNotEqual functions, since the ECPrivateKey objects
> could
> > have different pointers while still containing the same key data
> 
> Done.

not done?

https://codereview.chromium.org/1076063002/diff/1/net/ssl/channel_id_service_...
net/ssl/channel_id_service_unittest.cc:321: TEST_F(ChannelIDServiceTest,
ExtractValuesFromBytesEC) {
On 2015/04/25 02:59:19, nharper wrote:
> On 2015/04/10 01:00:27, mattm wrote:
> > Test doesn't do what its name says anymore (probably just remove this test)
> 
> Test removed.

?

https://codereview.chromium.org/1076063002/diff/1/net/ssl/channel_id_service_...
net/ssl/channel_id_service_unittest.cc:476: EXPECT_NE(key2.get(), key3.get());
On 2015/04/25 02:59:19, nharper wrote:
> On 2015/04/10 01:00:27, mattm wrote:
> > Same as above (ExpectKeysNotEqual)
> 
> Done.

?

https://codereview.chromium.org/1076063002/diff/40001/net/quic/test_tools/cry...
File net/quic/test_tools/crypto_test_utils_nss.cc (right):

https://codereview.chromium.org/1076063002/diff/40001/net/quic/test_tools/cry...
net/quic/test_tools/crypto_test_utils_nss.cc:41: return
scoped_ptr<crypto::ECPrivateKey>(it->second->Copy());
make_scoped_ptr

https://codereview.chromium.org/1076063002/diff/40001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_nss.cc (right):

https://codereview.chromium.org/1076063002/diff/40001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_nss.cc:113: #if defined(USE_NSS_CERTS)
When updating, it's generally appreciated to upload one patchset with just the
rebase, and another with the changes to the CL.

https://codereview.chromium.org/1076063002/diff/40001/net/ssl/channel_id_serv...
File net/ssl/channel_id_service_unittest.cc (right):

https://codereview.chromium.org/1076063002/diff/40001/net/ssl/channel_id_serv...
net/ssl/channel_id_service_unittest.cc:125: EXPECT_TRUE(key2);
Still should be ASSERTs

[nharper, 2015-04-29 22:07:15]

https://codereview.chromium.org/1076063002/diff/1/net/extras/sqlite/sqlite_ch...
File net/extras/sqlite/sqlite_channel_id_store.cc (right):

https://codereview.chromium.org/1076063002/diff/1/net/extras/sqlite/sqlite_ch...
net/extras/sqlite/sqlite_channel_id_store.cc:34: const int
kCompatibleVersionNumber = 1;
On 2015/04/27 20:55:53, mattm wrote:
> On 2015/04/25 02:59:18, nharper wrote:
> > On 2015/04/10 01:00:27, mattm wrote:
> > > Should be 5 also?
> > 
> > I need it to be both 1 and 5, depending on the context. In
> EnsureDatabaseVersion
> > (when migrating from a previous version to the current version), the
> compatible
> > version number is 1 - we can migrate from version 1. However, when updating
> the
> > current and compatible versions once the schema is updated, they are both 5.
> 
> Hm, it looks to me that in this CL kCompatibleVersionNumber is only currently
> used in the meta_table_.Init statement, where does it need to be 1?

Now that I re-read the code and what I wrote, I realized my mistake: The second
check in EnsureDatabaseVersion (meta_table_.GetCompatibleVersionNumber() >
kCurrentVersionNumber) is checking that the table (on disk) isn't newer than the
code - I thought it was doing kCompatibleVersionNumber >
meta_table_.GetVersionNumber() to check that the table (on disk) isn't too old
(which is where I'd want to check against 1).

I've updated kCompatibleVersionNumber to be 5.

https://codereview.chromium.org/1076063002/diff/1/net/extras/sqlite/sqlite_ch...
net/extras/sqlite/sqlite_channel_id_store.cc:348:
meta_table_.SetCompatibleVersionNumber(kCurrentVersionNumber);
On 2015/04/27 20:55:53, mattm wrote:
> On 2015/04/25 02:59:18, nharper wrote:
> > On 2015/04/10 01:00:27, mattm wrote:
> > > kCompatibleVersionNumber
> > 
> > The compatible version number here needs to be 5, not 1. Maybe I should
change
> > kCompatibleVersionNumber to 2 constants?
> > 
> > const int kOldCompatibleVersionNumber = 1;
> > const int kNewCompatibleVersionNumber = 5;
> 
> Yeah, I think it should be 5 here also. I don't see where
> kOldCompatibleVersionNumber is needed though.

I figured out my mistake and set it to 5.

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

https://codereview.chromium.org/1076063002/diff/1/net/ssl/channel_id_service....
net/ssl/channel_id_service.cc:135: crypto::ECPrivateKey* key =
On 2015/04/27 20:55:53, mattm wrote:
> On 2015/04/25 02:59:18, nharper wrote:
> > On 2015/04/10 01:00:27, mattm wrote:
> > > make this a scoped_ptr or stick directly in key_out
> > 
> > Done.
> 
> Doesn't seem to be done either. Did some changes get dropped?

Some changes got dropped (I found them in my git stash - apparently I stashed
some changes instead of committing them). I popped the changes out of the git
stash, so these things should be done now.

https://codereview.chromium.org/1076063002/diff/40001/net/quic/test_tools/cry...
File net/quic/test_tools/crypto_test_utils_nss.cc (right):

https://codereview.chromium.org/1076063002/diff/40001/net/quic/test_tools/cry...
net/quic/test_tools/crypto_test_utils_nss.cc:41: return
scoped_ptr<crypto::ECPrivateKey>(it->second->Copy());
On 2015/04/27 20:55:53, mattm wrote:
> make_scoped_ptr

Done.

https://codereview.chromium.org/1076063002/diff/40001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_nss.cc (right):

https://codereview.chromium.org/1076063002/diff/40001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_nss.cc:113: #if defined(USE_NSS_CERTS)
On 2015/04/27 20:55:53, mattm wrote:
> When updating, it's generally appreciated to upload one patchset with just the
> rebase, and another with the changes to the CL.

I'll keep that in mind for future updates.

https://codereview.chromium.org/1076063002/diff/40001/net/ssl/channel_id_serv...
File net/ssl/channel_id_service_unittest.cc (right):

https://codereview.chromium.org/1076063002/diff/40001/net/ssl/channel_id_serv...
net/ssl/channel_id_service_unittest.cc:125: EXPECT_TRUE(key2);
On 2015/04/27 20:55:53, mattm wrote:
> Still should be ASSERTs

Done.

[mattm, 2015-04-30 00:30:11]

https://codereview.chromium.org/1076063002/diff/60001/net/cert/x509_util_nss.cc
File net/cert/x509_util_nss.cc (right):

https://codereview.chromium.org/1076063002/diff/60001/net/cert/x509_util_nss....
net/cert/x509_util_nss.cc:198: void ParsePrincipal(CERTName* name,
CertPrincipal* principal) {
Looks like a bad merge here actually.

https://codereview.chromium.org/1076063002/diff/60001/net/ssl/channel_id_serv...
File net/ssl/channel_id_service_unittest.cc (right):

https://codereview.chromium.org/1076063002/diff/60001/net/ssl/channel_id_serv...
net/ssl/channel_id_service_unittest.cc:134: ASSERT_NE(nullptr, key1.get());
ASSERT_TRUE(key)

[nharper, 2015-04-30 20:31:56]

There are still some larger design questions that need to be addressed/agreed
upon:
- Is it feasible to store only the private key?
- Where should the transition from DER-encoded keypair to crypto::ECPrivateKey
object happen?
- What is the correct place for helper functions
CreateECPrivateKeyFromSerializedKey and ExportKeyPair?

For the first question, if we used boringssl for crypto everywhere (instead of
nss), the answer would be yes. However, even with the chimera build, we still
have nss in use on ios, and my view is that the way the crypto operations are
set up in nss it is unfeasible (but not impossible) to generate a
crypto::ECPrivateKey from only the encrypted private key.

Here's a thought that might resolve both the 2nd and 3rd points nicely: Move the
transition (crypto::ECPrivateKey <=> std::string pair) from ChannelIDService to
ChannelIDStore (and that way if a ChannelIDStore doesn't care about
serializing/deserializing a keypair it doesn't have to, e.g. an in-memory mock
store). Internal to SQLiteChannelIDStore, use std::vector<unsigned char> instead
of std::string for the encoded keypair objects. This change will allow using the
existing crypto::ECPrivateKey methods that work with vectors (instead of strings
or StringPieces), and the sql::Statement code for selecting/inserting the
keypair will work just as well with vectors as with strings. Doing so will allow
removing the helper functions. The only thing this doesn't address is the
broader question of string vs vector<uint> vs StringPiece throughout net and
crypto code, but I don't think that needs to be solved in this CL (so long as
this CL doesn't contribute to the problem).

https://codereview.chromium.org/1076063002/diff/60001/net/cert/x509_util_nss.cc
File net/cert/x509_util_nss.cc (right):

https://codereview.chromium.org/1076063002/diff/60001/net/cert/x509_util_nss....
net/cert/x509_util_nss.cc:198: void ParsePrincipal(CERTName* name,
CertPrincipal* principal) {
On 2015/04/30 00:30:11, mattm wrote:
> Looks like a bad merge here actually.

Done.

https://codereview.chromium.org/1076063002/diff/60001/net/ssl/channel_id_serv...
File net/ssl/channel_id_service_unittest.cc (right):

https://codereview.chromium.org/1076063002/diff/60001/net/ssl/channel_id_serv...
net/ssl/channel_id_service_unittest.cc:134: ASSERT_NE(nullptr, key1.get());
On 2015/04/30 00:30:11, mattm wrote:
> ASSERT_TRUE(key)

Done.

[davidben, 2015-04-30 20:45:14]

On 2015/04/30 20:31:56, nharper wrote:
> There are still some larger design questions that need to be addressed/agreed
> upon:
> - Is it feasible to store only the private key?

I haven't been paying attention to or looking at the CL, so apologies if I'm
saying something silly missing context, but an ECPrivateKey structure already
includes the public key. It's technically OPTIONAL, but the spec says you SHOULD
always include it. In BoringSSL's case, if the public key isn't serialized,
it'll compute it. You can compute the public key from the private key with a
multiplication. For NSS, I thought there was SECKEY_ConvertToPublicKey which
would let you get the public key out of the private half, so I presume they're
fungible in NSS too.

(An RSAPrivateKey serialization also includes the public half.)

[nharper, 2015-04-30 21:19:11]

On 2015/04/30 20:45:14, David Benjamin wrote:
> On 2015/04/30 20:31:56, nharper wrote:
> > There are still some larger design questions that need to be
addressed/agreed
> > upon:
> > - Is it feasible to store only the private key?
> 
> I haven't been paying attention to or looking at the CL, so apologies if I'm
> saying something silly missing context, but an ECPrivateKey structure already
> includes the public key. It's technically OPTIONAL, but the spec says you
SHOULD
> always include it. In BoringSSL's case, if the public key isn't serialized,
> it'll compute it. You can compute the public key from the private key with a
> multiplication. For NSS, I thought there was SECKEY_ConvertToPublicKey which
> would let you get the public key out of the private half, so I presume they're
> fungible in NSS too.
> 
> (An RSAPrivateKey serialization also includes the public half.)

The issue is that nss only allows us to extract an encrypted private key from an
ECPrivateKey, and to recreate an ECPrivateKey (or SECKEYPrivateKey) from the
encrypted private key we use PK11_UnwrapPrivKey, which takes a SECItem
*publicValue which comes from the public key (specifically the SECKEYPublicKey
that was created using SECKEY_ExtractPublicKey). That is, I need the public key
to create the SECKEYPrivateKey, so I can't use SECKEY_ConvertToPublicKey to get
the public key. If there's some way to pass in a different publicValue to
PK11_UnwrapPrivKey that isn't dependent on the public key, that would solve this
problem.

[nharper, 2015-05-08 03:43:53]

Refactored handling crypto::ECPrivateKeys. This should address the second and
third points in my previous comment.

[Ryan Sleevi, 2015-05-08 23:43:14]

Mostly LG!

https://codereview.chromium.org/1076063002/diff/140001/net/extras/sqlite/sqli...
File net/extras/sqlite/sqlite_channel_id_store.cc (right):

https://codereview.chromium.org/1076063002/diff/140001/net/extras/sqlite/sqli...
net/extras/sqlite/sqlite_channel_id_store.cc:299: continue;
Should this be on line 293, to avoid the other copying of bits?

https://codereview.chromium.org/1076063002/diff/140001/net/extras/sqlite/sqli...
File net/extras/sqlite/sqlite_channel_id_store_unittest.cc (right):

https://codereview.chromium.org/1076063002/diff/140001/net/extras/sqlite/sqli...
net/extras/sqlite/sqlite_channel_id_store_unittest.cc:126: }
So this is what I was talking about w/r/t tests

https://code.google.com/p/googletest/wiki/V1_7_AdvancedGuide#Predicate_Assert...

See also
https://code.google.com/p/googletest/wiki/V1_7_AdvancedGuide#Using_Assertions...

Either approach would work. Probably the latter is the easiest?

https://codereview.chromium.org/1076063002/diff/140001/net/extras/sqlite/sqli...
net/extras/sqlite/sqlite_channel_id_store_unittest.cc:238:
ReadTestKeyAndCert(&key_data, &cert_data, &key);
BUG: If your ASSERT_TRUE (line 60) failed, this will *NOT* abort the test with a
failure. Instead, it will continue - potentially crashing the test!

You need to wrap these calls with an EXPECT_/ASSERT_NO_FATAL_FAILURE (see above
references)

https://codereview.chromium.org/1076063002/diff/140001/net/quic/crypto/channe...
File net/quic/crypto/channel_id_chromium.cc (right):

https://codereview.chromium.org/1076063002/diff/140001/net/quic/crypto/channe...
net/quic/crypto/channel_id_chromium.cc:195: if (!ec_private_key) {
Can this be just

if (!channel_id_crypto_key_) {
  // blah
}
channel_id_key_.reset(new ChannelIDKeyChromium(channel_id_crypto_key_.Pass()));

(deleting line 194 entirely)

https://codereview.chromium.org/1076063002/diff/140001/net/ssl/channel_id_ser...
File net/ssl/channel_id_service.cc (right):

https://codereview.chromium.org/1076063002/diff/140001/net/ssl/channel_id_ser...
net/ssl/channel_id_service.cc:99: not_valid_before, key.get()));
Should this be renamed to |creation_date|?

https://codereview.chromium.org/1076063002/diff/140001/net/ssl/channel_id_ser...
net/ssl/channel_id_service.cc:123: key_->reset();
Why is this explicitly necessary? Shouldn't the dtor handle this? Are there
mutability concerns? What breaks if this isn't set - and why?

https://codereview.chromium.org/1076063002/diff/140001/net/ssl/channel_id_ser...
net/ssl/channel_id_service.cc:203: scoped_ptr<ChannelIDStore::ChannelID> cert =
This variable name is no longer correct, is it?

https://codereview.chromium.org/1076063002/diff/140001/net/ssl/channel_id_ser...
net/ssl/channel_id_service.cc:205: DVLOG(1) << "GenerateCert " <<
server_identifier_ << " returned " << error;
That looks wonky :)

Just delete the DVLOG IMO

https://codereview.chromium.org/1076063002/diff/140001/net/ssl/channel_id_ser...
net/ssl/channel_id_service.cc:451: DVLOG(1) << "Cert store had valid key for "
<< server_identifier;
Can nuke this as well

https://codereview.chromium.org/1076063002/diff/140001/net/ssl/channel_id_ser...
net/ssl/channel_id_service.cc:504: crypto::ECPrivateKey* key) {
Is there any risk of |key| being invalidated? Would it make more sense to force
the callers of HandleResult() to .Pass() the scoped_ptr<crypto::ECPrivateKey>?

My fear would be that |key| refers to some member variable of some job that ends
up deleted. I suppose this is technically a risk for |server_identifier| as well
(because it's a const-ref and thus may be a pointer to a member), but I'm just
curious if forcing an ownership transfer makes for a cleaner experience?

(It may not, just curious your judgement now that you know this code)

https://codereview.chromium.org/1076063002/diff/140001/net/ssl/channel_id_ser...
File net/ssl/channel_id_service.h (right):

https://codereview.chromium.org/1076063002/diff/140001/net/ssl/channel_id_ser...
net/ssl/channel_id_service.h:18: #include "net/base/net_errors.h"
unused?

https://codereview.chromium.org/1076063002/diff/140001/net/ssl/channel_id_ser...
net/ssl/channel_id_service.h:28: }
}  // crypto

(and also line 24)

Style pedantry at its finest

https://codereview.chromium.org/1076063002/diff/140001/net/ssl/channel_id_ser...
net/ssl/channel_id_service.h:152: const scoped_ptr<crypto::ECPrivateKey> key);
weird to pass this as const - bug?

https://codereview.chromium.org/1076063002/diff/140001/net/ssl/channel_id_ser...
File net/ssl/channel_id_service_unittest.cc (right):

https://codereview.chromium.org/1076063002/diff/140001/net/ssl/channel_id_ser...
net/ssl/channel_id_service_unittest.cc:97:
base::MessageLoop::current()->PostTask(
Future cleanup (with low hanging fruit)
base::ThreadTaskRunnerHandle::Get()->PostTask [...]

(https://crbug.com/391045 )

https://codereview.chromium.org/1076063002/diff/140001/net/ssl/channel_id_ser...
net/ssl/channel_id_service_unittest.cc:132: EXPECT_NE(public_key1, public_key2);
same comments re expect/assert in subroutines

https://codereview.chromium.org/1076063002/diff/140001/net/ssl/default_channe...
File net/ssl/default_channel_id_store.h (right):

https://codereview.chromium.org/1076063002/diff/140001/net/ssl/default_channe...
net/ssl/default_channel_id_store.h:23: }
}  // namespace crypto

[mattm, 2015-05-09 00:01:01]

https://codereview.chromium.org/1076063002/diff/120001/net/extras/sqlite/sqli...
File net/extras/sqlite/sqlite_channel_id_store.cc (right):

https://codereview.chromium.org/1076063002/diff/120001/net/extras/sqlite/sqli...
net/extras/sqlite/sqlite_channel_id_store.cc:228:
base::Time::FromInternalValue(smt.ColumnInt64(3)), key.release()));
key is leaked

https://codereview.chromium.org/1076063002/diff/120001/net/extras/sqlite/sqli...
File net/extras/sqlite/sqlite_channel_id_store_unittest.cc (right):

https://codereview.chromium.org/1076063002/diff/120001/net/extras/sqlite/sqli...
net/extras/sqlite/sqlite_channel_id_store_unittest.cc:58:
memcpy(&private_key[0], key_data->data(), key_data->size());
vector_as_array?

https://codereview.chromium.org/1076063002/diff/120001/net/ssl/channel_id_ser...
File net/ssl/channel_id_service.h (right):

https://codereview.chromium.org/1076063002/diff/120001/net/ssl/channel_id_ser...
net/ssl/channel_id_service.h:159: crypto::ECPrivateKey* key);
const?

https://codereview.chromium.org/1076063002/diff/120001/net/ssl/channel_id_sto...
File net/ssl/channel_id_store.h (right):

https://codereview.chromium.org/1076063002/diff/120001/net/ssl/channel_id_sto...
net/ssl/channel_id_store.h:38: crypto::ECPrivateKey* key);
add comment about key ownership, make const.

Or make this scoped_ptr and have the caller make a copy if they want to keep
one.

[nharper, 2015-05-11 21:26:44]

https://codereview.chromium.org/1076063002/diff/120001/net/extras/sqlite/sqli...
File net/extras/sqlite/sqlite_channel_id_store.cc (right):

https://codereview.chromium.org/1076063002/diff/120001/net/extras/sqlite/sqli...
net/extras/sqlite/sqlite_channel_id_store.cc:228:
base::Time::FromInternalValue(smt.ColumnInt64(3)), key.release()));
On 2015/05/09 00:01:00, mattm wrote:
> key is leaked

Done.

https://codereview.chromium.org/1076063002/diff/120001/net/extras/sqlite/sqli...
File net/extras/sqlite/sqlite_channel_id_store_unittest.cc (right):

https://codereview.chromium.org/1076063002/diff/120001/net/extras/sqlite/sqli...
net/extras/sqlite/sqlite_channel_id_store_unittest.cc:58:
memcpy(&private_key[0], key_data->data(), key_data->size());
On 2015/05/09 00:01:00, mattm wrote:
> vector_as_array?

Done.

https://codereview.chromium.org/1076063002/diff/120001/net/ssl/channel_id_ser...
File net/ssl/channel_id_service.h (right):

https://codereview.chromium.org/1076063002/diff/120001/net/ssl/channel_id_ser...
net/ssl/channel_id_service.h:159: crypto::ECPrivateKey* key);
On 2015/05/09 00:01:00, mattm wrote:
> const?

Changed to scoped_ptr.

https://codereview.chromium.org/1076063002/diff/120001/net/ssl/channel_id_sto...
File net/ssl/channel_id_store.h (right):

https://codereview.chromium.org/1076063002/diff/120001/net/ssl/channel_id_sto...
net/ssl/channel_id_store.h:38: crypto::ECPrivateKey* key);
On 2015/05/09 00:01:01, mattm wrote:
> add comment about key ownership, make const.
> 
> Or make this scoped_ptr and have the caller make a copy if they want to keep
> one.

I changed it to take a scoped_ptr. I looked back at why I had done it this way
(instead of initially taking a scoped_ptr), and found that taking a
crypto::ECPrivateKey* (and making a copy in the constructor) instead of a
scoped_ptr made things easier in the unittests, but resulted in doing an extra
copy. Since the unittests are the only place where making a copy is needed, I
moved the copying to be the caller's responsibility (which is what the unittests
are now doing).

https://codereview.chromium.org/1076063002/diff/140001/net/extras/sqlite/sqli...
File net/extras/sqlite/sqlite_channel_id_store.cc (right):

https://codereview.chromium.org/1076063002/diff/140001/net/extras/sqlite/sqli...
net/extras/sqlite/sqlite_channel_id_store.cc:299: continue;
On 2015/05/08 23:43:13, Ryan Sleevi wrote:
> Should this be on line 293, to avoid the other copying of bits?

Yes, there's no reason not to do that check first.

https://codereview.chromium.org/1076063002/diff/140001/net/extras/sqlite/sqli...
File net/extras/sqlite/sqlite_channel_id_store_unittest.cc (right):

https://codereview.chromium.org/1076063002/diff/140001/net/extras/sqlite/sqli...
net/extras/sqlite/sqlite_channel_id_store_unittest.cc:126: }
On 2015/05/08 23:43:13, Ryan Sleevi wrote:
> So this is what I was talking about w/r/t tests
> 
>
https://code.google.com/p/googletest/wiki/V1_7_AdvancedGuide#Predicate_Assert...
> 
> See also
>
https://code.google.com/p/googletest/wiki/V1_7_AdvancedGuide#Using_Assertions...
> 
> Either approach would work. Probably the latter is the easiest?

I'm going with the latter - they both seem about the same in terms of
ease-of-use, but I don't see the former anywhere in net code.

https://codereview.chromium.org/1076063002/diff/140001/net/extras/sqlite/sqli...
net/extras/sqlite/sqlite_channel_id_store_unittest.cc:238:
ReadTestKeyAndCert(&key_data, &cert_data, &key);
On 2015/05/08 23:43:13, Ryan Sleevi wrote:
> BUG: If your ASSERT_TRUE (line 60) failed, this will *NOT* abort the test with
a
> failure. Instead, it will continue - potentially crashing the test!
> 
> You need to wrap these calls with an EXPECT_/ASSERT_NO_FATAL_FAILURE (see
above
> references)

Done.

https://codereview.chromium.org/1076063002/diff/140001/net/quic/crypto/channe...
File net/quic/crypto/channel_id_chromium.cc (right):

https://codereview.chromium.org/1076063002/diff/140001/net/quic/crypto/channe...
net/quic/crypto/channel_id_chromium.cc:195: if (!ec_private_key) {
On 2015/05/08 23:43:13, Ryan Sleevi wrote:
> Can this be just
> 
> if (!channel_id_crypto_key_) {
>   // blah
> }
> channel_id_key_.reset(new
ChannelIDKeyChromium(channel_id_crypto_key_.Pass()));
> 
> (deleting line 194 entirely)

Done.

https://codereview.chromium.org/1076063002/diff/140001/net/ssl/channel_id_ser...
File net/ssl/channel_id_service.cc (right):

https://codereview.chromium.org/1076063002/diff/140001/net/ssl/channel_id_ser...
net/ssl/channel_id_service.cc:99: not_valid_before, key.get()));
On 2015/05/08 23:43:13, Ryan Sleevi wrote:
> Should this be renamed to |creation_date|?

Renamed to creation_time.

https://codereview.chromium.org/1076063002/diff/140001/net/ssl/channel_id_ser...
net/ssl/channel_id_service.cc:123: key_->reset();
On 2015/05/08 23:43:13, Ryan Sleevi wrote:
> Why is this explicitly necessary? Shouldn't the dtor handle this? Are there
> mutability concerns? What breaks if this isn't set - and why?

I don't know why it would be necessary - I only put it in to match the previous
style of setting private_key_ and cert_ to NULL. (Perhaps it was to keep the
object in a consistent state if it was Cancel()ed without being destroyed?)
Removing it seems to be fine.

https://codereview.chromium.org/1076063002/diff/140001/net/ssl/channel_id_ser...
net/ssl/channel_id_service.cc:203: scoped_ptr<ChannelIDStore::ChannelID> cert =
On 2015/05/08 23:43:13, Ryan Sleevi wrote:
> This variable name is no longer correct, is it?

Done.

https://codereview.chromium.org/1076063002/diff/140001/net/ssl/channel_id_ser...
net/ssl/channel_id_service.cc:205: DVLOG(1) << "GenerateCert " <<
server_identifier_ << " returned " << error;
On 2015/05/08 23:43:13, Ryan Sleevi wrote:
> That looks wonky :)
> 
> Just delete the DVLOG IMO

Done.

https://codereview.chromium.org/1076063002/diff/140001/net/ssl/channel_id_ser...
net/ssl/channel_id_service.cc:451: DVLOG(1) << "Cert store had valid key for "
<< server_identifier;
On 2015/05/08 23:43:14, Ryan Sleevi wrote:
> Can nuke this as well

Done.

https://codereview.chromium.org/1076063002/diff/140001/net/ssl/channel_id_ser...
net/ssl/channel_id_service.cc:504: crypto::ECPrivateKey* key) {
On 2015/05/08 23:43:13, Ryan Sleevi wrote:
> Is there any risk of |key| being invalidated? Would it make more sense to
force
> the callers of HandleResult() to .Pass() the scoped_ptr<crypto::ECPrivateKey>?
> 
> My fear would be that |key| refers to some member variable of some job that
ends
> up deleted. I suppose this is technically a risk for |server_identifier| as
well
> (because it's a const-ref and thus may be a pointer to a member), but I'm just
> curious if forcing an ownership transfer makes for a cleaner experience?
> 
> (It may not, just curious your judgement now that you know this code)

I'm not concerned about |key| being invalidated. HandleResult only gets called
from ChannelIDService::GotChannelID and ChannelIDService::GeneratedChannelID. In
both of those cases, the underlying crypto::ECPrivateKey object is owned by the
calling method - in GotChannelID it's owned by the scoped_ptr that's passed in
as a param, and in GeneratedChannelID it's owned by a scoped_ptr that's created
in the method.

Ultimately, I decided to refactor ChannelIDService::HandleResult,
ChannelIDServiceJob::HandleResult, ChannelIDServiceJob::PostAll, and
ChannelIDServiceRequest::Post to take scoped_ptrs instead of raw pointers, to
make ownership of objects more clear. The reason why I originally used a raw
pointer instead of scoped_ptr has to do with how ChannelIDServiceJob::PostAll()
and ChannelIDServiceRequest::Post() work. PostAll() gets called by HandleResult,
and then calls Post() on all of the Job's Requests. This makes it necessary to
either use a raw pointer here or to copy the object for all of the calls to
Post. The other complication is that it is perfectly valid to call HandleResult
with nullptr, so when the object is copied, it needs to check for null.

tl;dr: there was a reason for using a raw pointer, but it's cleaner if I use a
scoped_ptr, so I'm changing it to use a scoped_ptr.

https://codereview.chromium.org/1076063002/diff/140001/net/ssl/channel_id_ser...
File net/ssl/channel_id_service.h (right):

https://codereview.chromium.org/1076063002/diff/140001/net/ssl/channel_id_ser...
net/ssl/channel_id_service.h:18: #include "net/base/net_errors.h"
On 2015/05/08 23:43:14, Ryan Sleevi wrote:
> unused?

Done.

https://codereview.chromium.org/1076063002/diff/140001/net/ssl/channel_id_ser...
net/ssl/channel_id_service.h:28: }
On 2015/05/08 23:43:14, Ryan Sleevi wrote:
> }  // crypto
> 
> (and also line 24)
> 
> Style pedantry at its finest

Actually, it's

}  // namespace crypto

https://codereview.chromium.org/1076063002/diff/140001/net/ssl/channel_id_ser...
net/ssl/channel_id_service.h:152: const scoped_ptr<crypto::ECPrivateKey> key);
On 2015/05/08 23:43:14, Ryan Sleevi wrote:
> weird to pass this as const - bug?

Refactoring mistake - I think it was a const-ref at some point. Removed the
const.

https://codereview.chromium.org/1076063002/diff/140001/net/ssl/channel_id_ser...
File net/ssl/channel_id_service_unittest.cc (right):

https://codereview.chromium.org/1076063002/diff/140001/net/ssl/channel_id_ser...
net/ssl/channel_id_service_unittest.cc:132: EXPECT_NE(public_key1, public_key2);
On 2015/05/08 23:43:14, Ryan Sleevi wrote:
> same comments re expect/assert in subroutines

Done.

https://codereview.chromium.org/1076063002/diff/140001/net/ssl/default_channe...
File net/ssl/default_channel_id_store.h (right):

https://codereview.chromium.org/1076063002/diff/140001/net/ssl/default_channe...
net/ssl/default_channel_id_store.h:23: }
On 2015/05/08 23:43:14, Ryan Sleevi wrote:
> }  // namespace crypto

Done.

[Ryan Sleevi, 2015-05-12 01:11:37]

LGTM. Will leave Matt to take a final pass.

https://codereview.chromium.org/1076063002/diff/180001/net/ssl/channel_id_ser...
File net/ssl/channel_id_service.cc (right):

https://codereview.chromium.org/1076063002/diff/180001/net/ssl/channel_id_ser...
net/ssl/channel_id_service.cc:155: key_->reset(key.release());
key_ = key.Pass()?

https://codereview.chromium.org/1076063002/diff/180001/net/ssl/channel_id_ser...
net/ssl/channel_id_service.cc:474: scoped_ptr<crypto::ECPrivateKey>());
You can just pass nullptr here, right? implicit construction should work for
that.

[mattm, 2015-05-12 07:47:39]

https://codereview.chromium.org/1076063002/diff/180001/net/ssl/default_channe...
File net/ssl/default_channel_id_store_unittest.cc (right):

https://codereview.chromium.org/1076063002/diff/180001/net/ssl/default_channe...
net/ssl/default_channel_id_store_unittest.cc:129:
scoped_ptr<crypto::ECPrivateKey>(crypto::ECPrivateKey::Create())));
nit: there are a number of places make_scoped_ptr could be used (not just this
file)

https://codereview.chromium.org/1076063002/diff/180001/net/ssl/default_channe...
net/ssl/default_channel_id_store_unittest.cc:381: crypto::ECPrivateKey*
copied_key = crypto::ECPrivateKey::Create();
these should be scoped_ptrs?

https://codereview.chromium.org/1076063002/diff/180001/net/ssl/default_channe...
net/ssl/default_channel_id_store_unittest.cc:425: crypto::ECPrivateKey*
copied_key = crypto::ECPrivateKey::Create();
these too

[nharper, 2015-05-12 18:57:27]

https://codereview.chromium.org/1076063002/diff/180001/net/ssl/channel_id_ser...
File net/ssl/channel_id_service.cc (right):

https://codereview.chromium.org/1076063002/diff/180001/net/ssl/channel_id_ser...
net/ssl/channel_id_service.cc:155: key_->reset(key.release());
On 2015/05/12 01:11:37, Ryan Sleevi wrote:
> key_ = key.Pass()?

Done. I didn't realize scoped_ptr overloaded operator= to do something sane.

https://codereview.chromium.org/1076063002/diff/180001/net/ssl/channel_id_ser...
net/ssl/channel_id_service.cc:474: scoped_ptr<crypto::ECPrivateKey>());
On 2015/05/12 01:11:37, Ryan Sleevi wrote:
> You can just pass nullptr here, right? implicit construction should work for
> that.

Yes, nullptr works here.

https://codereview.chromium.org/1076063002/diff/180001/net/ssl/default_channe...
File net/ssl/default_channel_id_store_unittest.cc (right):

https://codereview.chromium.org/1076063002/diff/180001/net/ssl/default_channe...
net/ssl/default_channel_id_store_unittest.cc:129:
scoped_ptr<crypto::ECPrivateKey>(crypto::ECPrivateKey::Create())));
On 2015/05/12 07:47:39, mattm wrote:
> nit: there are a number of places make_scoped_ptr could be used (not just this
> file)

I searched my diff for "scoped_ptr<crypto::ECPrivateKey>(" and replaced with
make_scoped_ptr where appropriate.

https://codereview.chromium.org/1076063002/diff/180001/net/ssl/default_channe...
net/ssl/default_channel_id_store_unittest.cc:381: crypto::ECPrivateKey*
copied_key = crypto::ECPrivateKey::Create();
On 2015/05/12 07:47:39, mattm wrote:
> these should be scoped_ptrs?

Yes - fixed here and elsewhere. I also did an asan/lsan run and found and fixed
some leaks caused by this anti-pattern in other tests as well.

https://codereview.chromium.org/1076063002/diff/180001/net/ssl/default_channe...
net/ssl/default_channel_id_store_unittest.cc:425: crypto::ECPrivateKey*
copied_key = crypto::ECPrivateKey::Create();
On 2015/05/12 07:47:39, mattm wrote:
> these too

Done.

[mattm, 2015-05-12 20:04:28]

lgtm

[nharper, 2015-05-12 20:06:41]

The CQ bit was checked by nharper@chromium.org

[nharper, 2015-05-12 20:06:41]

The patchset sent to the CQ was uploaded after l-g-t-m from rsleevi@chromium.org
Link to the patchset: https://codereview.chromium.org/1076063002/#ps200001
(title: "minor nits: use make_scoped_ptr, clean asan/lsan test run")

[commit-bot: I haz the power, 2015-05-12 20:07:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1076063002/200001

[commit-bot: I haz the power, 2015-05-12 20:17:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-05-12 20:17:16]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[nharper, 2015-05-12 20:23:48]

nharper@chromium.org changed reviewers:
+ kalman@chromium.org

[nharper, 2015-05-12 20:23:49]

nharper@chromium.org changed required reviewers:
+ kalman@chromium.org

[nharper, 2015-05-12 20:23:50]

+kalman for chrome/browser/extensions/api/messaging/message_property_provider.cc

[not at google - send to devlin, 2015-05-12 20:59:52]

kalman@chromium.org changed reviewers:
+ juanlang@chromium.org

[not at google - send to devlin, 2015-05-12 20:59:52]

lgtm, +juanlang as FYI since he wrote the code.

[nharper, 2015-05-13 23:39:00]

Add missing parts of patch sets 14 and 15; fix an msvc compiler error

[nharper, 2015-05-15 21:03:14]

testing -- ignore

[nharper, 2015-05-15 21:05:54]

try using ternary operator

[nharper, 2015-05-15 21:37:59]

testing - ignore

[nharper, 2015-05-15 22:26:16]

Check that key is not null before copying it

[nharper, 2015-05-15 22:29:07]

Patchset #17 (id:310001) has been deleted

[nharper, 2015-05-15 22:29:21]

Patchset #17 (id:330001) has been deleted

[nharper, 2015-05-15 22:29:33]

Patchset #17 (id:350001) has been deleted

[nharper, 2015-05-18 20:55:34]

rebase

[nharper, 2015-05-18 20:57:00]

Fix mixplaced close paren

[nharper, 2015-05-18 22:32:38]

nharper@chromium.org changed reviewers:
+ markusheintz@chromium.org, nkostylev@chromium.org

[nharper, 2015-05-18 22:32:40]

ptal. The largest change is creating net/test/channel_id_test_util.cc|h to hold
helper methods for the unittests instead of copy-pasting them across unittest
files.

Also adding markusheintz for chrome/browser/browsing_data and nkostylev for
chrome/browser/chromeos/login.

[Ryan Sleevi, 2015-05-18 23:54:35]

https://codereview.chromium.org/1076063002/diff/410001/net/test/channel_id_te...
File net/test/channel_id_test_util.h (right):

https://codereview.chromium.org/1076063002/diff/410001/net/test/channel_id_te...
net/test/channel_id_test_util.h:17: void
ExpectKeysNotEqual(crypto::ECPrivateKey* key1, crypto::ECPrivateKey* key2);
1) I think if we're going to go this route, it probably makes more sense to
return a ::testing::AssertionResult

2) Having this return void, instead of a bool, doesn't really engender good
usage. That is, callers have to know about GTest's rules about testing
subroutines.

3) Not terribly keen on a single-file test helper.

For example, why are these two functions and not just a single bool function?
Because failures could happen even when the keys are equal? How is that best
expressed.

[nharper, 2015-05-19 07:18:50]

https://codereview.chromium.org/1076063002/diff/410001/net/test/channel_id_te...
File net/test/channel_id_test_util.h (right):

https://codereview.chromium.org/1076063002/diff/410001/net/test/channel_id_te...
net/test/channel_id_test_util.h:17: void
ExpectKeysNotEqual(crypto::ECPrivateKey* key1, crypto::ECPrivateKey* key2);
On 2015/05/18 23:54:35, Ryan Sleevi wrote:
> 1) I think if we're going to go this route, it probably makes more sense to
> return a ::testing::AssertionResult
> 
> 2) Having this return void, instead of a bool, doesn't really engender good
> usage. That is, callers have to know about GTest's rules about testing
> subroutines.
> 
> 3) Not terribly keen on a single-file test helper.
> 
> For example, why are these two functions and not just a single bool function?
> Because failures could happen even when the keys are equal? How is that best
> expressed.

I've updated this to combine these 2 methods into one method that returns a
::testing::AssertionResult. Given how testing::AssertionResult works in the
EXPECT_TRUE/FALSE macros, it makes more sense to return that instead of a bool
(and returning either a testing::AssertionResult or a bool makes more sense than
returning void).

It makes sense to move this into some sort of helper from individual
*_unittest.cc files, since it's now used by 5 separate test files. I put it in a
new file because I didn't see a file where it looked like it belonged, but if
there's a better place for it, I can move it elsewhere.

[Nikita (slow), 2015-05-19 12:02:23]

On 2015/05/18 22:32:40, nharper wrote:
> Also adding markusheintz for chrome/browser/browsing_data and nkostylev for
> chrome/browser/chromeos/login.

lgtm

[mattm, 2015-05-20 01:43:49]

https://codereview.chromium.org/1076063002/diff/430001/net/test/channel_id_te...
File net/test/channel_id_test_util.cc (right):

https://codereview.chromium.org/1076063002/diff/430001/net/test/channel_id_te...
net/test/channel_id_test_util.cc:17: return ::testing::AssertionFailure() <<
"key1 is null";
Doesn't this have the problem that EXPECT_FALSE(KeysEqual(...)) would succeed if
one of these preconditions fails?

https://codereview.chromium.org/1076063002/diff/430001/net/test/channel_id_te...
File net/test/channel_id_test_util.h (right):

https://codereview.chromium.org/1076063002/diff/430001/net/test/channel_id_te...
net/test/channel_id_test_util.h:19: crypto::ECPrivateKey* key2);
WARN_UNUSED_RESULT ?

[nharper, 2015-05-20 18:54:49]

https://codereview.chromium.org/1076063002/diff/430001/net/test/channel_id_te...
File net/test/channel_id_test_util.cc (right):

https://codereview.chromium.org/1076063002/diff/430001/net/test/channel_id_te...
net/test/channel_id_test_util.cc:17: return ::testing::AssertionFailure() <<
"key1 is null";
On 2015/05/20 01:43:49, mattm wrote:
> Doesn't this have the problem that EXPECT_FALSE(KeysEqual(...)) would succeed
if
> one of these preconditions fails?

Yes, it has that problem. I've fixed it by checking the preconditions in an
EXPECT_TRUE() instead of returning an AssertionFailure, so if a precondition
fails, the test case will fail. (I can't use ASSERT_TRUE in a function that
returns non-void; also, by EXPECT_TRUE the caller doesn't need to use
ASSERT_NO_FATAL_FAILURE.)

There are a few problems with this approach. 1) If a precondition fails, the
test output won't be very helpful in identifying the problem, since it will just
point to the EXPECT_TRUE lines here. 2) If both preconditions fail, then this
function will return an AssertionSuccess (because the public_key strings will
both be empty, thus equal).

https://codereview.chromium.org/1076063002/diff/430001/net/test/channel_id_te...
File net/test/channel_id_test_util.h (right):

https://codereview.chromium.org/1076063002/diff/430001/net/test/channel_id_te...
net/test/channel_id_test_util.h:19: crypto::ECPrivateKey* key2);
On 2015/05/20 01:43:49, mattm wrote:
> WARN_UNUSED_RESULT ?

Done.

[mattm, 2015-05-22 20:45:47]

https://codereview.chromium.org/1076063002/diff/430001/net/test/channel_id_te...
File net/test/channel_id_test_util.cc (right):

https://codereview.chromium.org/1076063002/diff/430001/net/test/channel_id_te...
net/test/channel_id_test_util.cc:17: return ::testing::AssertionFailure() <<
"key1 is null";
On 2015/05/20 18:54:49, nharper wrote:
> On 2015/05/20 01:43:49, mattm wrote:
> > Doesn't this have the problem that EXPECT_FALSE(KeysEqual(...)) would
succeed
> if
> > one of these preconditions fails?
> 
> Yes, it has that problem. I've fixed it by checking the preconditions in an
> EXPECT_TRUE() instead of returning an AssertionFailure, so if a precondition
> fails, the test case will fail. (I can't use ASSERT_TRUE in a function that
> returns non-void; also, by EXPECT_TRUE the caller doesn't need to use
> ASSERT_NO_FATAL_FAILURE.)
> 
> There are a few problems with this approach. 1) If a precondition fails, the
> test output won't be very helpful in identifying the problem, since it will
just
> point to the EXPECT_TRUE lines here. 2) If both preconditions fail, then this
> function will return an AssertionSuccess (because the public_key strings will
> both be empty, thus equal).

This seems like an acceptable compromise if we want to have only a single
function instead of separate ones for equal/notequal, so I'm okay with this.

[nharper, 2015-05-26 15:02:42]

markusheintz: Can you look at the files in chrome/browser/browsing_data for
OWNERS approval (or suggest someone else for it)?

[nharper, 2015-05-27 17:13:15]

nharper@chromium.org changed reviewers:
+ mkwst@chromium.org

[nharper, 2015-05-27 17:13:16]

+mkwst for OWNERS approval on chrome/browser/browsing_data

[Mike West, 2015-05-28 06:16:53]

chrome/browser/browsing_data LGTM.

[markusheintz_, 2015-05-29 15:15:09]

On 2015/05/28 06:16:53, Mike West wrote:
> chrome/browser/browsing_data LGTM.

LGTM

[markusheintz_, 2015-05-29 15:15:14]

On 2015/05/28 06:16:53, Mike West wrote:
> chrome/browser/browsing_data LGTM.

LGTM

[markusheintz_, 2015-05-29 15:15:19]

On 2015/05/28 06:16:53, Mike West wrote:
> chrome/browser/browsing_data LGTM.

LGTM

[Mike West, 2015-05-29 15:23:47]

On 2015/05/29 at 15:15:19, markusheintz wrote:
> On 2015/05/28 06:16:53, Mike West wrote:
> > chrome/browser/browsing_data LGTM.
> 
> LGTM

*ahem* You _really_ liked this patch, Markus. :)

[nharper, 2015-06-01 17:29:16]

The CQ bit was checked by nharper@chromium.org

[nharper, 2015-06-01 17:29:17]

The patchset sent to the CQ was uploaded after l-g-t-m from mattm@chromium.org,
rsleevi@chromium.org, kalman@chromium.org, nkostylev@chromium.org
Link to the patchset: https://codereview.chromium.org/1076063002/#ps450001
(title: "Udpate KeysEqual to fail if preconditions fail")

[commit-bot: I haz the power, 2015-06-01 17:31:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1076063002/450001

[commit-bot: I haz the power, 2015-06-01 20:29:42]

Committed patchset #21 (id:450001)

[commit-bot: I haz the power, 2015-06-01 20:30:26]

Patchset 21 (id:??) landed as
https://crrev.com/2e171cf4d960a6c723a529b2a4cbceb075bcd306
Cr-Commit-Position: refs/heads/master@{#332260}