Add new certificateProvider extension API.

Add new certificateProvider extension API.

BUG=514575
Committed: https://crrev.com/9cac4acf0e0a0898e14ac0c187a4872eef41b820
Cr-Commit-Position: refs/heads/master@{#347916}

[pneubeck (no reviews), 2015-07-10 11:44:31]

Patchset #1 (id:1) has been deleted

[Ryan Sleevi, 2015-07-10 12:07:55]

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

[Ryan Sleevi, 2015-07-10 12:07:56]

https://codereview.chromium.org/1232553003/diff/20001/chrome/common/extension...
File chrome/common/extensions/api/certificate_provider.idl (right):

https://codereview.chromium.org/1232553003/diff/20001/chrome/common/extension...
chrome/common/extensions/api/certificate_provider.idl:54: // which the extension
can sign data with the according private key.
// Notifies Chrome that this extension is capable of responding to signing
requests for the certificates
// listed in |certificates|. The list must only contain certificates for which
the extension can sign data
// using the associated private key.


I'm trying to eliminate the "client certificate" portion, since it's conceivable
in some future world (like WebRTC) that we may wish to use 'server
certificates', and the API would be the same. We don't support that, certainly,
but there's no *strict* reason that a card implementing this API is limited to
client vs server. Further, the filtering (e.g. for TLS Client Auth in the EKUs)
will be done by Chrome, so you wouldn't want smart cards reimplementing that and
botching it all.

https://codereview.chromium.org/1232553003/diff/20001/chrome/common/extension...
chrome/common/extensions/api/certificate_provider.idl:67: //
|publicClientCertificates|. To use a client certificate to authenticate
Comment-wise, I think you want to reword this to indicate it's not at all
related to navigation

// This event fires every time the browser needs to sign a message using a
certificate provided by this
// extension using |publishClientCertificates|. The extension must sign the data
in |request| using
// the appropriate algorithm and return it using |replyToSignRequest|.

https://codereview.chromium.org/1232553003/diff/20001/chrome/common/extension...
chrome/common/extensions/api/certificate_provider.idl:72: // navigation is
blocked. Long delays reduce the user's experience.
I'd delete these two sentences entirely - we don't want to promise this
behaviour forever (e.g. we may want to have a "30 second extension timeout" in
the future, for example)

[pneubeck (no reviews), 2015-07-10 12:23:52]

https://codereview.chromium.org/1232553003/diff/20001/chrome/common/extension...
File chrome/common/extensions/api/certificate_provider.idl (right):

https://codereview.chromium.org/1232553003/diff/20001/chrome/common/extension...
chrome/common/extensions/api/certificate_provider.idl:54: // which the extension
can sign data with the according private key.
On 2015/07/10 12:07:56, Ryan Sleevi (slow through 7-15 wrote:
> // Notifies Chrome that this extension is capable of responding to signing
> requests for the certificates
> // listed in |certificates|. The list must only contain certificates for which
> the extension can sign data
> // using the associated private key.
> 
> 
> I'm trying to eliminate the "client certificate" portion, since it's
conceivable
> in some future world (like WebRTC) that we may wish to use 'server
> certificates', and the API would be the same. We don't support that,
certainly,
> but there's no *strict* reason that a card implementing this API is limited to
> client vs server. Further, the filtering (e.g. for TLS Client Auth in the
EKUs)
> will be done by Chrome, so you wouldn't want smart cards reimplementing that
and
> botching it all.

Done.

https://codereview.chromium.org/1232553003/diff/20001/chrome/common/extension...
chrome/common/extensions/api/certificate_provider.idl:67: //
|publicClientCertificates|. To use a client certificate to authenticate
On 2015/07/10 12:07:56, Ryan Sleevi (slow through 7-15 wrote:
> Comment-wise, I think you want to reword this to indicate it's not at all
> related to navigation
> 
> // This event fires every time the browser needs to sign a message using a
> certificate provided by this
> // extension using |publishClientCertificates|. The extension must sign the
data
> in |request| using
> // the appropriate algorithm and return it using |replyToSignRequest|.

only added to your proposal:
 s/algorithm/algorithm and private key/
Done.

https://codereview.chromium.org/1232553003/diff/20001/chrome/common/extension...
chrome/common/extensions/api/certificate_provider.idl:72: // navigation is
blocked. Long delays reduce the user's experience.
On 2015/07/10 12:07:56, Ryan Sleevi (slow through 7-15 wrote:
> I'd delete these two sentences entirely - we don't want to promise this
> behaviour forever (e.g. we may want to have a "30 second extension timeout" in
> the future, for example)

Done.

[davidben, 2015-07-10 14:03:31]

davidben@chromium.org changed reviewers:
+ davidben@chromium.org

[davidben, 2015-07-10 14:03:32]

I only looked at the extension API. Looks fairly reasonable. Some comments.

https://codereview.chromium.org/1232553003/diff/30001/chrome/common/extension...
File chrome/common/extensions/api/certificate_provider.idl (right):

https://codereview.chromium.org/1232553003/diff/30001/chrome/common/extension...
chrome/common/extensions/api/certificate_provider.idl:19: ArrayBuffer
certificate;
Is there a story for the smartcard sending a certificate chain / do we need one?
(Or is that to be built using certificates in NSS? I've always found the client
certificate chain configuration story bizarre.)

https://codereview.chromium.org/1232553003/diff/30001/chrome/common/extension...
chrome/common/extensions/api/certificate_provider.idl:27: dictionary SignRequest
{
Shouldn't this include the certificate we're trying to sign with or some other
handle to the private key? Otherwise you don't know which is in use.

https://codereview.chromium.org/1232553003/diff/30001/chrome/common/extension...
chrome/common/extensions/api/certificate_provider.idl:45: // padding.
Nit: Dunno if it's worth explicitly mentioning here or at the top that MD5_SHA1
is weird and doesn't have DigestInfo padding? Pre-TLS-1.2 had a somewhat custom
construct.

https://codereview.chromium.org/1232553003/diff/30001/chrome/common/extension...
chrome/common/extensions/api/certificate_provider.idl:57: DoneCallback
callback);
Any reason why it's publicClientCertificates and not an async
getClientCeritifcates API? It seems the latter would be more natural and save us
maintaining a list in-memory on behalf of the xtn.

[pneubeck (no reviews), 2015-07-10 15:17:25]

https://codereview.chromium.org/1232553003/diff/30001/chrome/common/extension...
File chrome/common/extensions/api/certificate_provider.idl (right):

https://codereview.chromium.org/1232553003/diff/30001/chrome/common/extension...
chrome/common/extensions/api/certificate_provider.idl:27: dictionary SignRequest
{
On 2015/07/10 14:03:32, David Benjamin wrote:
> Shouldn't this include the certificate we're trying to sign with or some other
> handle to the private key? Otherwise you don't know which is in use.

ops. of course. I'll probably add
 // The certificate of the key that must be used for signing the digest.
 ArrayBuffer certificate

(ArrayBuffer publicKey
 would be another option)

https://codereview.chromium.org/1232553003/diff/30001/chrome/common/extension...
chrome/common/extensions/api/certificate_provider.idl:45: // padding.
On 2015/07/10 14:03:32, David Benjamin wrote:
> Nit: Dunno if it's worth explicitly mentioning here or at the top that
MD5_SHA1
> is weird and doesn't have DigestInfo padding? Pre-TLS-1.2 had a somewhat
custom
> construct.

Yes, we should probably explain such in more detail.

[pneubeck (no reviews), 2015-07-22 18:59:11]

Patchset #4 (id:70001) has been deleted

[pneubeck (no reviews), 2015-07-22 18:59:17]

Patchset #4 (id:90001) has been deleted

[pneubeck (no reviews), 2015-07-22 20:30:54]

Patchset #4 (id:110001) has been deleted

[pneubeck (no reviews), 2015-07-24 14:24:22]

Patchset #5 (id:150001) has been deleted

[pneubeck (no reviews), 2015-07-24 14:26:50]

Hi there,

so this implementation should be in a rather good shape now, except the hack in
net/ssl/ssl_platform_key* .
If you already have an idea how we should inject keys int net/, please let me
know.
I can probably also implement the required pieces as well.

[pneubeck (no reviews), 2015-07-28 09:41:40]

@David, @Ryan, please provide input on net/ssl/ssl_platform_key* . Either we add
a singleton for NSS similar to OpenSSLClientKeyStore instead of that
fetch_private_key_func global in the current patchset, or we do the refactoring
as explained in crbug.com/394131 . Not sure whether the latter is feasible until
M46.

[davidben, 2015-07-28 15:15:46]

On 2015/07/28 09:41:40, pneubeck wrote:
> @David, @Ryan, please provide input on net/ssl/ssl_platform_key* . Either we
add
> a singleton for NSS similar to OpenSSLClientKeyStore instead of that
> fetch_private_key_func global in the current patchset, or we do the
refactoring
> as explained in crbug.com/394131 . Not sure whether the latter is feasible
until
> M46.

Something akin to OpenSSLClientKeyStore is probably reasonable for now. It's
kinda ugly, but it'll be short-lived. We're getting a new person fairly soon and
we'll probably be tasking him to finish up the refactor as his first task.

One thing to note is that, since it'll be a singleton, this needs to be
thread-safe. A single URLRequestContext may only live on one thread, but it
should be safe to instantiate multiple instances of the stack on different
threads.

(All this also depends on the chimera sticking for M46, and we still haven't
shipped a single Dev channel release with it, and there's no canary. Although
the breakage would have to be pretty dire by now to revert. We've been running
without bot coverage for any of the NSS code for two weeks now.)

[pneubeck (no reviews), 2015-07-29 09:57:51]

On 2015/07/28 15:15:46, David Benjamin wrote:
> On 2015/07/28 09:41:40, pneubeck wrote:
> > @David, @Ryan, please provide input on net/ssl/ssl_platform_key* . Either we
> add
> > a singleton for NSS similar to OpenSSLClientKeyStore instead of that
> > fetch_private_key_func global in the current patchset, or we do the
> refactoring
> > as explained in crbug.com/394131 . Not sure whether the latter is feasible
> until
> > M46.
> 
> Something akin to OpenSSLClientKeyStore is probably reasonable for now. It's
> kinda ugly, but it'll be short-lived. We're getting a new person fairly soon
and
> we'll probably be tasking him to finish up the refactor as his first task.
> 
> One thing to note is that, since it'll be a singleton, this needs to be
> thread-safe. A single URLRequestContext may only live on one thread, but it
> should be safe to instantiate multiple instances of the stack on different
> threads.
I hope you meant what you said :-)
Since the FetchClientCertPrivateKey is returning synchronously, this implies
that we have to block internally using a base::Lock if there is a concurrent
modification of the list of certificates. Since there isn't any
Read/ReadWriteLock implementation in chrome AFAICT, I'd use a single lock for
both readers and writers, meaning even readers block each other. Which is... not
great... but might not be a problem in practice.

> 
> (All this also depends on the chimera sticking for M46, and we still haven't
> shipped a single Dev channel release with it, and there's no canary. Although
> the breakage would have to be pretty dire by now to revert. We've been running
> without bot coverage for any of the NSS code for two weeks now.)

[davidben, 2015-07-29 16:39:37]

On 2015/07/29 09:57:51, pneubeck wrote:
> On 2015/07/28 15:15:46, David Benjamin wrote:
> > On 2015/07/28 09:41:40, pneubeck wrote:
> > > @David, @Ryan, please provide input on net/ssl/ssl_platform_key* . Either
we
> > add
> > > a singleton for NSS similar to OpenSSLClientKeyStore instead of that
> > > fetch_private_key_func global in the current patchset, or we do the
> > refactoring
> > > as explained in crbug.com/394131 . Not sure whether the latter is feasible
> > until
> > > M46.
> > 
> > Something akin to OpenSSLClientKeyStore is probably reasonable for now. It's
> > kinda ugly, but it'll be short-lived. We're getting a new person fairly soon
> and
> > we'll probably be tasking him to finish up the refactor as his first task.
> > 
> > One thing to note is that, since it'll be a singleton, this needs to be
> > thread-safe. A single URLRequestContext may only live on one thread, but it
> > should be safe to instantiate multiple instances of the stack on different
> > threads.
> I hope you meant what you said :-)
> Since the FetchClientCertPrivateKey is returning synchronously, this implies
> that we have to block internally using a base::Lock if there is a concurrent
> modification of the list of certificates. Since there isn't any
> Read/ReadWriteLock implementation in chrome AFAICT, I'd use a single lock for
> both readers and writers, meaning even readers block each other. Which is...
not
> great... but might not be a problem in practice.

Certainly don't take a lock around a callback or anything like that, but taking
a lock internally to access some map keyed by certificate or something should be
fine. In practice, net stack consumers all live on the IO thread anyway, so you
effectively take a lock.

(Hrm, that OpenSSLClientKeyStore doesn't take a lock is probably a bug...)

> > 
> > (All this also depends on the chimera sticking for M46, and we still haven't
> > shipped a single Dev channel release with it, and there's no canary.
Although
> > the breakage would have to be pretty dire by now to revert. We've been
running
> > without bot coverage for any of the NSS code for two weeks now.)

[pneubeck (no reviews), 2015-08-03 19:45:34]

Hi there,

this CL is ready for review. I will need your review in particular for net/,
crypto-related code and the API itself (.idl file).

A colleague in Munich will review most of the CL.

Note that we planned for M46 dev-channel.

Thanks

[pneubeck (no reviews), 2015-08-06 14:15:17]

Patchset #12 (id:310001) has been deleted

[pneubeck (no reviews), 2015-08-06 14:45:04]

Patchset #13 (id:350001) has been deleted

[pneubeck (no reviews), 2015-08-06 14:48:47]

to make this change more manageable, I broke out the net/ changes into
  https://codereview.chromium.org/1278763002/
  https://codereview.chromium.org/1274143002/
  https://codereview.chromium.org/1275853003/

[pneubeck (no reviews), 2015-08-17 12:13:03]

I incorporated the remaining API suggestions in
https://codereview.chromium.org/1289963003/ .

https://codereview.chromium.org/1232553003/diff/30001/chrome/common/extension...
File chrome/common/extensions/api/certificate_provider.idl (right):

https://codereview.chromium.org/1232553003/diff/30001/chrome/common/extension...
chrome/common/extensions/api/certificate_provider.idl:19: ArrayBuffer
certificate;
On 2015/07/10 14:03:32, David Benjamin wrote:
> Is there a story for the smartcard sending a certificate chain / do we need
one?
> (Or is that to be built using certificates in NSS? I've always found the
client
> certificate chain configuration story bizarre.)

We can add this if it ever becomes an important requirement.

https://codereview.chromium.org/1232553003/diff/30001/chrome/common/extension...
chrome/common/extensions/api/certificate_provider.idl:45: // padding.
On 2015/07/10 15:17:25, pneubeck wrote:
> On 2015/07/10 14:03:32, David Benjamin wrote:
> > Nit: Dunno if it's worth explicitly mentioning here or at the top that
> MD5_SHA1
> > is weird and doesn't have DigestInfo padding? Pre-TLS-1.2 had a somewhat
> custom
> > construct.
> 
> Yes, we should probably explain such in more detail.

I'll remove MD5_SHA1 for now. Doesn't seem to be required by the VPN providers
that we know of.

https://codereview.chromium.org/1232553003/diff/30001/chrome/common/extension...
chrome/common/extensions/api/certificate_provider.idl:57: DoneCallback
callback);
On 2015/07/10 14:03:32, David Benjamin wrote:
> Any reason why it's publicClientCertificates and not an async
> getClientCeritifcates API? It seems the latter would be more natural and save
us
> maintaining a list in-memory on behalf of the xtn.
(thoroughly discussed in the implementation in another CL, just a few points for
other readers)

At least with the currently synchronous FetchClientCertPrivateKey I don't see
how we can avoid caching the certificate list in Chrome.
However, we agreed that notifying the extension, allows the extension to delay
loading certificates if it wants to.

My main criticism of the notification is that we leak information about when the
user is using client authentication.

[davidben, 2015-08-18 21:17:52]

https://codereview.chromium.org/1232553003/diff/30001/chrome/common/extension...
File chrome/common/extensions/api/certificate_provider.idl (right):

https://codereview.chromium.org/1232553003/diff/30001/chrome/common/extension...
chrome/common/extensions/api/certificate_provider.idl:45: // padding.
On 2015/08/17 12:13:02, pneubeck wrote:
> On 2015/07/10 15:17:25, pneubeck wrote:
> > On 2015/07/10 14:03:32, David Benjamin wrote:
> > > Nit: Dunno if it's worth explicitly mentioning here or at the top that
> > MD5_SHA1
> > > is weird and doesn't have DigestInfo padding? Pre-TLS-1.2 had a somewhat
> > custom
> > > construct.
> > 
> > Yes, we should probably explain such in more detail.
> 
> I'll remove MD5_SHA1 for now. Doesn't seem to be required by the VPN providers
> that we know of.

[Commented in other CL, but MD5_SHA1 is the only hash that can be signed before
TLS 1.2, so it almost certainly needs to be there. VPN providers might have
confused it with something else.]

https://codereview.chromium.org/1232553003/diff/30001/chrome/common/extension...
chrome/common/extensions/api/certificate_provider.idl:57: DoneCallback
callback);
On 2015/08/17 12:13:02, pneubeck wrote:
> On 2015/07/10 14:03:32, David Benjamin wrote:
> > Any reason why it's publicClientCertificates and not an async
> > getClientCeritifcates API? It seems the latter would be more natural and
save
> us
> > maintaining a list in-memory on behalf of the xtn.
> (thoroughly discussed in the implementation in another CL, just a few points
for
> other readers)
> 
> At least with the currently synchronous FetchClientCertPrivateKey I don't see
> how we can avoid caching the certificate list in Chrome.
> However, we agreed that notifying the extension, allows the extension to delay
> loading certificates if it wants to.

It seems it's a getClientCertificates API now but, for posterity,
FetchClientCertPrivateKey doesn't constrain this at all. That's done once we've
selected a single cert. Querying the certificate list needn't be synchronous.

> My main criticism of the notification is that we leak information about when
the
> user is using client authentication.

I don't think this is much of a concern. You don't actually get much information
about it. Also, realistically, people aren't likely to have client certs from
multiple deployments at once so the only certificates available are the ones
from this provider anyway.

Plus, for privacy, client certs are already horrible. They send the certificate
in the clear. :-)

[davidben, 2015-08-20 02:00:44]

davidben@chromium.org changed reviewers:
+ mattm@chromium.org

[davidben, 2015-08-20 02:00:45]

+mattm, do you mind looking over this for load-balancing purposes? This one's
kinda large, so I guess we probably want more than one set of eyes anyway? I
dunno.

This is the main context for the other two.

[pneubeck (no reviews), 2015-08-20 14:55:48]

note that I'm still updating this CL to the changed design of the hooks to net//
.

[pneubeck (no reviews), 2015-08-26 13:27:56]

Patchset #15 (id:400001) has been deleted

[pneubeck (no reviews), 2015-09-01 12:00:54]

Patchset #15 (id:420001) has been deleted

[pneubeck (no reviews), 2015-09-01 12:14:30]

Patchset #15 (id:440001) has been deleted

[pneubeck (no reviews), 2015-09-01 14:11:08]

Patchset #15 (id:460001) has been deleted

[pneubeck (no reviews), 2015-09-01 14:20:26]

pneubeck@chromium.org changed reviewers:
+ emaxx@chromium.org, kalman@chromium.org, mmenke@chromium.org

[pneubeck (no reviews), 2015-09-01 14:20:27]

@Maksim, ptal at
 chrome/common/extensions/api/*
 chrome/browser/extensions/api/certificate_provider/*
 chrome/browser/chromeos/certificate_provider/*
 chrome/test/data/extensions/api_test/certificate_provider/*
 chrome/renderer/resources/extensions/certificate_provider_custom_bindings.js

@Benjamin, ptal at the whether the updated public and internal idl +
custom_bindings is idiomatic and OWNER review at least:
 chrome/common/extensions/*
 chrome/renderer/extensions/*
 chrome/renderer/resources/*
 extensions/common/permissions/api_permission.h

@mmenke, ptal at profile_io_data.*

[pneubeck (no reviews), 2015-09-01 14:34:35]

pneubeck@chromium.org changed reviewers:
+ asvitkine@chromium.org

[pneubeck (no reviews), 2015-09-01 14:34:36]

asvitkine@chromium.org: Please review changes in 
  extensions/browser/*
  tools/metrics/histograms/histograms.xml

Thanks

[mmenke, 2015-09-01 15:28:52]

On 2015/09/01 14:20:27, pneubeck wrote:
> @Maksim, ptal at
>  chrome/common/extensions/api/*
>  chrome/browser/extensions/api/certificate_provider/*
>  chrome/browser/chromeos/certificate_provider/*
>  chrome/test/data/extensions/api_test/certificate_provider/*
>  chrome/renderer/resources/extensions/certificate_provider_custom_bindings.js
> 
> @Benjamin, ptal at the whether the updated public and internal idl +
> custom_bindings is idiomatic and OWNER review at least:
>  chrome/common/extensions/*
>  chrome/renderer/extensions/*
>  chrome/renderer/resources/*
>  extensions/common/permissions/api_permission.h
> 
> @mmenke, ptal at profile_io_data.*

I'll defer to davidben on this one - once he signs off, I will.

[davidben, 2015-09-01 15:32:22]

On 2015/09/01 15:28:52, mmenke wrote:
> On 2015/09/01 14:20:27, pneubeck wrote:
> > @Maksim, ptal at
> >  chrome/common/extensions/api/*
> >  chrome/browser/extensions/api/certificate_provider/*
> >  chrome/browser/chromeos/certificate_provider/*
> >  chrome/test/data/extensions/api_test/certificate_provider/*
> > 
chrome/renderer/resources/extensions/certificate_provider_custom_bindings.js
> > 
> > @Benjamin, ptal at the whether the updated public and internal idl +
> > custom_bindings is idiomatic and OWNER review at least:
> >  chrome/common/extensions/*
> >  chrome/renderer/extensions/*
> >  chrome/renderer/resources/*
> >  extensions/common/permissions/api_permission.h
> > 
> > @mmenke, ptal at profile_io_data.*
> 
> I'll defer to davidben on this one - once he signs off, I will.

(Will try to get to this today.)

[davidben, 2015-09-01 18:45:49]

On 2015/09/01 15:32:22, David Benjamin wrote:
> On 2015/09/01 15:28:52, mmenke wrote:
> > On 2015/09/01 14:20:27, pneubeck wrote:
> > > @Maksim, ptal at
> > >  chrome/common/extensions/api/*
> > >  chrome/browser/extensions/api/certificate_provider/*
> > >  chrome/browser/chromeos/certificate_provider/*
> > >  chrome/test/data/extensions/api_test/certificate_provider/*
> > > 
> chrome/renderer/resources/extensions/certificate_provider_custom_bindings.js
> > > 
> > > @Benjamin, ptal at the whether the updated public and internal idl +
> > > custom_bindings is idiomatic and OWNER review at least:
> > >  chrome/common/extensions/*
> > >  chrome/renderer/extensions/*
> > >  chrome/renderer/resources/*
> > >  extensions/common/permissions/api_permission.h
> > > 
> > > @mmenke, ptal at profile_io_data.*
> > 
> > I'll defer to davidben on this one - once he signs off, I will.
> 
> (Will try to get to this today.)

Which bits would you like me to review? This seems to be almost entirely
extensions glue, which I'm not particularly well-suited to review.

[Alexei Svitkine (slow), 2015-09-01 21:16:40]

histograms lgtm, didn't look at the rest

[not at google - send to devlin, 2015-09-01 21:39:11]

lgtm for extensions

[not at google - send to devlin, 2015-09-01 21:39:32]

On 2015/09/01 21:39:11, kalman wrote:
> lgtm for extensions

extensions idioms, structure, etc. not the correctness of the API.

[pneubeck (no reviews), 2015-09-02 06:47:00]

@mmenke / davidben, I need the sign-off of profile_io_data.* .

[mmenke, 2015-09-02 17:54:57]

On 2015/09/02 06:47:00, pneubeck wrote:
> @mmenke / davidben, I need the sign-off of profile_io_data.* .

David:  Are the ProfileIOData changes sane?  I don't know what a cert provider
service is, what copying one does, or the effect of passing one / not passing
one to ClientCertStoreChromeOS (If you're not familiar with the class, we need a
reviewer who is).  Also, are the uses of net or cert APIs sane?

[pneubeck (no reviews), 2015-09-02 19:43:39]

mmenke: Note, that it was myself who changed ClientCertStoreChromeOS and added
ClientKeyStore in the first place in https://codereview.chromium.org/1274143002/
and https://codereview.chromium.org/1278763002/ . They were reviewed by mattm@
and David.
This new API allows extensions to provide client certificates that will be used
for TLS client authentications driven by the browser.
Therefore, I added these two entry points to the net/ layer in the previous two
CLs.
In this CL, I finally hook up the API to the net/ layer. The rest is primarily
extension system related.

On 2015/09/02 17:54:57, mmenke wrote:
> On 2015/09/02 06:47:00, pneubeck wrote:
> > @mmenke / davidben, I need the sign-off of profile_io_data.* .
> 
> David:  Are the ProfileIOData changes sane?  I don't know what a cert provider
> service is, what copying one does, or the effect of passing one / not passing
> one to ClientCertStoreChromeOS (If you're not familiar with the class, we need
a
> reviewer who is).
Extension API functions start on the UI thread and so does the KeyedService
backing the API (CertificateProviderService).
ClientCertStoreChromeOS is created and lives on the IO thread. 
When ClientCertStoreChromeOS has to retrieve and match all known certificates
against a TLS certificate request, it somehow has to pull the current list of
certs provided by extensions from CertificateProviderService.
The way I solved this in this CL is:
 1) Obtain a CertificateProvider object from CertificateProviderService in
ProfileIOData::InitializeOnUIThread .
 2) Copy that CertificateProvider object for each ClientCertStoreChromeOS
creation in ResourceContext::CreateClientCertStore .
 3) ClientCertStoreChromeOS gets the list of certs from the owned
CertificateProvider object, which internally interacts with the Service .
This ensures that we access CertificateProviderService only on the UI thread and
that no WeakPtr or similar is exposed in any of the new interfaces.

> Also, are the uses of net or cert APIs sane?

[pneubeck (no reviews), 2015-09-03 09:00:33]

pneubeck@chromium.org changed reviewers:
+ bartfab@chromium.org

[pneubeck (no reviews), 2015-09-03 09:00:34]

@Bartosz, Maskim, one of you should take a look at
 chrome/common/extensions/api/*
 chrome/browser/extensions/api/certificate_provider/*
 chrome/browser/chromeos/certificate_provider/*
 chrome/test/data/extensions/api_test/certificate_provider/*
 chrome/renderer/resources/extensions/certificate_provider_custom_bindings.js

It's not necessary that you both spend time on this.

[bartfab (slow), 2015-09-03 17:30:54]

The C++ is reviewed. I will do the JS tomorrow.

https://codereview.chromium.org/1232553003/diff/530001/chrome/browser/chromeo...
File
chrome/browser/chromeos/certificate_provider/certificate_provider_service_factory.cc
(right):

https://codereview.chromium.org/1232553003/diff/530001/chrome/browser/chromeo...
chrome/browser/chromeos/certificate_provider/certificate_provider_service_factory.cc:7:
#include "base/debug/stack_trace.h"
Nit: Not used.

https://codereview.chromium.org/1232553003/diff/530001/chrome/browser/chromeo...
chrome/browser/chromeos/certificate_provider/certificate_provider_service_factory.cc:28:
public extensions::ExtensionRegistryObserver {
Nit: #include "extensions/browser/extension_registry_observer.h"

https://codereview.chromium.org/1232553003/diff/530001/chrome/browser/chromeo...
chrome/browser/chromeos/certificate_provider/certificate_provider_service_factory.cc:35:
std::vector<std::string> CertificateProviderExtensions() override;
Nit 1: // CertificateProviderService::Delegate:
Nit 2: #include <vector>
Nit 3: #include <string>

https://codereview.chromium.org/1232553003/diff/530001/chrome/browser/chromeo...
chrome/browser/chromeos/certificate_provider/certificate_provider_service_factory.cc:73:
event_router_->listeners().GetEventListenersByName(event_name)) {
Nit: #include "extensions/browser/event_listener_map.h"

https://codereview.chromium.org/1232553003/diff/530001/chrome/browser/chromeo...
chrome/browser/chromeos/certificate_provider/certificate_provider_service_factory.cc:81:
scoped_ptr<base::ListValue> internal_args(new base::ListValue);
Nit: #include "base/values.h"

https://codereview.chromium.org/1232553003/diff/530001/chrome/browser/chromeo...
chrome/browser/chromeos/certificate_provider/certificate_provider_service_factory.cc:83:
scoped_ptr<extensions::Event> event(new extensions::Event(
Nit: #include "base/memory/scoped_ptr.h"

https://codereview.chromium.org/1232553003/diff/530001/chrome/browser/chromeo...
chrome/browser/chromeos/certificate_provider/certificate_provider_service_factory.cc:84:
extensions::events::CERTIFICATEPROVIDER_ON_CERTIFICATES_REQUESTED,
Nit: #include "extensions/browser/extension_event_histogram_value.h"

https://codereview.chromium.org/1232553003/diff/530001/chrome/browser/chromeo...
chrome/browser/chromeos/certificate_provider/certificate_provider_service_factory.cc:99:
switch (hash) {
Nit: #include "net/ssl/ssl_private_key.h"

https://codereview.chromium.org/1232553003/diff/530001/chrome/browser/chromeo...
chrome/browser/chromeos/certificate_provider/certificate_provider_service_factory.cc:134:
service_->OnExtensionUnloaded(extension->id());
Nit: #include "extensions/common/extension.h"

https://codereview.chromium.org/1232553003/diff/530001/chrome/browser/chromeo...
File
chrome/browser/chromeos/certificate_provider/certificate_provider_service_factory.h
(right):

https://codereview.chromium.org/1232553003/diff/530001/chrome/browser/chromeo...
chrome/browser/chromeos/certificate_provider/certificate_provider_service_factory.h:23:
content::BrowserContext* context);
Nit: Forward-declare |content::BrowserContext|.

https://codereview.chromium.org/1232553003/diff/530001/chrome/browser/chromeo...
chrome/browser/chromeos/certificate_provider/certificate_provider_service_factory.h:31:
~CertificateProviderServiceFactory() override;
Nit: Why do you need to override the destructor?

https://codereview.chromium.org/1232553003/diff/530001/chrome/browser/extensi...
File
chrome/browser/extensions/api/certificate_provider/certificate_provider_api.cc
(right):

https://codereview.chromium.org/1232553003/diff/530001/chrome/browser/extensi...
chrome/browser/extensions/api/certificate_provider/certificate_provider_api.cc:35:
scoped_ptr<api_cpi::ReportCertificates::Params> params(
Nit: #include "base/memory/scoped_ptr.h"

https://codereview.chromium.org/1232553003/diff/530001/chrome/browser/extensi...
chrome/browser/extensions/api/certificate_provider/certificate_provider_api.cc:39:
chromeos::CertificateProviderService* service =
Nit: const pointer.

https://codereview.chromium.org/1232553003/diff/530001/chrome/browser/extensi...
chrome/browser/extensions/api/certificate_provider/certificate_provider_api.cc:48:
std::vector<std::vector<char>> rejected_certificates;
Nit: #include <vector>

https://codereview.chromium.org/1232553003/diff/530001/chrome/browser/extensi...
chrome/browser/extensions/api/certificate_provider/certificate_provider_api.cc:74:
WriteToConsole(content::CONSOLE_MESSAGE_LEVEL_ERROR, kErrorInvalidX509Cert);
Nit: #include "content/public/common/console_message_level.h"

https://codereview.chromium.org/1232553003/diff/530001/chrome/browser/extensi...
chrome/browser/extensions/api/certificate_provider/certificate_provider_api.cc:85:
size_t public_key_length_in_bits = 0;
Nit: #include <stddef.h>

https://codereview.chromium.org/1232553003/diff/530001/chrome/browser/extensi...
chrome/browser/extensions/api/certificate_provider/certificate_provider_api.cc:94:
DCHECK(public_key_length_in_bits != 0);
Nit: DCHECK(public_key_length_in_bits) is sufficient

https://codereview.chromium.org/1232553003/diff/530001/chrome/browser/extensi...
chrome/browser/extensions/api/certificate_provider/certificate_provider_api.cc:99:
out_info->type = net::SSLPrivateKey::Type::RSA;
Nit: #include "net/ssl/ssl_private_key.h"

https://codereview.chromium.org/1232553003/diff/530001/chrome/browser/extensi...
chrome/browser/extensions/api/certificate_provider/certificate_provider_api.cc:146:
chromeos::CertificateProviderService* service =
Nit: const pointer.

https://codereview.chromium.org/1232553003/diff/530001/chrome/browser/extensi...
chrome/browser/extensions/api/certificate_provider/certificate_provider_api.cc:151:
std::vector<uint8_t> signature;
Nit: #include <stdint.h>

https://codereview.chromium.org/1232553003/diff/530001/chrome/browser/extensi...
File
chrome/browser/extensions/api/certificate_provider/certificate_provider_api.h
(right):

https://codereview.chromium.org/1232553003/diff/530001/chrome/browser/extensi...
chrome/browser/extensions/api/certificate_provider/certificate_provider_api.h:27:
~CertificateProviderInternalReportCertificatesFunction() override;
Nit: Why do you need to override the destructor?

https://codereview.chromium.org/1232553003/diff/530001/chrome/browser/extensi...
chrome/browser/extensions/api/certificate_provider/certificate_provider_api.h:41:
~CertificateProviderInternalReportSignatureFunction() override;
Nit: Why do you need to override the destructor?

https://codereview.chromium.org/1232553003/diff/530001/chrome/browser/extensi...
File
chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc
(right):

https://codereview.chromium.org/1232553003/diff/530001/chrome/browser/extensi...
chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc:9:
#include "base/macros.h"
Nit: Not used.

https://codereview.chromium.org/1232553003/diff/530001/chrome/browser/extensi...
chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc:16:
#include "chromeos/chromeos_switches.h"
Nit: Not used.

https://codereview.chromium.org/1232553003/diff/530001/chrome/browser/extensi...
chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc:28:
#include "net/base/escape.h"
Nit: Not used.

https://codereview.chromium.org/1232553003/diff/530001/chrome/browser/extensi...
chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc:29:
#include "net/base/test_data_directory.h"
Nit: Not used.

https://codereview.chromium.org/1232553003/diff/530001/chrome/browser/extensi...
chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc:30:
#include "net/cert/test_root_certs.h"
Nit: Not used.

https://codereview.chromium.org/1232553003/diff/530001/chrome/browser/extensi...
chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc:40:
void IgnoreResult(base::Closure callback, const base::Value* value) {
Nit 1: Pass |callback| by const reference.
Nit 2: #include "base/callback.h"
Nit 3: #include "base/values.h"

https://codereview.chromium.org/1232553003/diff/530001/chrome/browser/extensi...
chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc:44:
void StoreBool(bool* result, base::Closure callback, const base::Value* value) {
Nit: Pass |callback| by const reference.

https://codereview.chromium.org/1232553003/diff/530001/chrome/browser/extensi...
chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc:49:
void StoreString(std::string* result,
Nit: #include <string>

https://codereview.chromium.org/1232553003/diff/530001/chrome/browser/extensi...
chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc:50:
base::Closure callback,
Nit: Pass |callback| by const reference.

https://codereview.chromium.org/1232553003/diff/530001/chrome/browser/extensi...
chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc:56:
void StoreDigest(base::Closure callback,
Nit 1: Pass |callback| by const reference.
Nit 2: Reorder arguments to match the other Store*() methods.

https://codereview.chromium.org/1232553003/diff/530001/chrome/browser/extensi...
chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc:57:
std::vector<uint8>* digest,
Nit 1: Here and throughout the file: s/uint8/uint8_t/, then #include <stdint.h>
Nit 2: #include <vector>

https://codereview.chromium.org/1232553003/diff/530001/chrome/browser/extensi...
chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc:61:
const uint8* const binary_begin =
Nit: const pointer to const.

https://codereview.chromium.org/1232553003/diff/530001/chrome/browser/extensi...
chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc:78:
size_t prefixed_digest_len = 0;
Nit: #include <stddef.h>

https://codereview.chromium.org/1232553003/diff/530001/chrome/browser/extensi...
chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc:81:
NID_sha1, vector_as_array(&digest),
Nit: #include "base/stl_util.h"

https://codereview.chromium.org/1232553003/diff/530001/chrome/browser/extensi...
chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc:91:
free(prefixed_digest);
Nit: #include <stdlib.h>

https://codereview.chromium.org/1232553003/diff/530001/chrome/browser/extensi...
chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc:102:
// Create a string that if evaluated in Javascript returns a Uint8Array with
Nit: s/Javascript/JavaScript/

https://codereview.chromium.org/1232553003/diff/530001/chrome/browser/extensi...
chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc:106:
for (size_t i = 0; i < bytes.size(); i++) {
Nit: Why not use an iterator?

https://codereview.chromium.org/1232553003/diff/530001/chrome/browser/extensi...
chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc:108:
if (i < bytes.size() - 1) {
JavaScript actually allows trailing commas, so you can get away with just always
adding one.

https://codereview.chromium.org/1232553003/diff/530001/chrome/browser/extensi...
chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc:129:
// Setup the AutoSelectCertificateForUrls policy to avoid the client
Nit: s/Setup/Set up/

https://codereview.chromium.org/1232553003/diff/530001/chrome/browser/extensi...
chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc:153:
// Start an HTTPS test server that requests a client certificates.
Nit: s/certificates/certificate/

https://codereview.chromium.org/1232553003/diff/530001/chrome/browser/extensi...
chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc:158:
base::FilePath(FILE_PATH_LITERAL("chrome/test/data")));
Nit: Use |PathService::Get(chrome::DIR_TEST_DATA, &test_data_dir);| instead.

https://codereview.chromium.org/1232553003/diff/530001/chrome/browser/extensi...
chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc:165:
const extensions::Extension* extension = LoadExtension(extension_path);
Nit 1: const pointer to const.
Nit 2: #include "extensions/common/extension.h"

https://codereview.chromium.org/1232553003/diff/530001/chrome/browser/extensi...
chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc:191:
base::RunLoop run_loop;
Nit: #include "base/run_loop.h"

https://codereview.chromium.org/1232553003/diff/530001/chrome/browser/extensi...
chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc:194:
"(function() {return signDigestRequest.digest;})();"),
Nit: Does this really need to be wrapped into a function?

https://codereview.chromium.org/1232553003/diff/530001/chrome/browser/extensi...
chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc:195:
base::Bind(&StoreDigest, run_loop.QuitClosure(), &request_digest));
Nit: #include "base/bind.h"

https://codereview.chromium.org/1232553003/diff/530001/chrome/browser/extensi...
chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc:201:
base::ReadFileToString(extension_path.AppendASCII("l1_leaf.pk8"), &key_pk8);
Nit: #include "base/files/file_util.h"

https://codereview.chromium.org/1232553003/diff/530001/chrome/browser/extensi...
chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc:211:
ASSERT_TRUE(RsaSign(request_digest, key.get(), &signature));
Nit: EXPECT_TRUE() should be sufficient here.

https://codereview.chromium.org/1232553003/diff/530001/chrome/browser/extensi...
chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc:216:
const std::string code = "(function() {replyWithSignature(" +
Nit: As above, does this really need to be wrapped into a function?

https://codereview.chromium.org/1232553003/diff/530001/chrome/browser/extensi...
chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc:234:
"(function() {return document.body.textContent;})();"),
Nit: As above, does this really need to be wrapped into a function?

https://codereview.chromium.org/1232553003/diff/530001/chrome/common/extensio...
File chrome/common/extensions/api/certificate_provider.idl (right):

https://codereview.chromium.org/1232553003/diff/530001/chrome/common/extensio...
chrome/common/extensions/api/certificate_provider.idl:40: callback
ResultCallback = void (ArrayBuffer[] rejectedCertificates);
Nit: Document who will call these two callbacks and when.

https://codereview.chromium.org/1232553003/diff/530001/chrome/common/extensio...
chrome/common/extensions/api/certificate_provider.idl:43: // the digest using
the private of the requested certificate.
Nit: s/private/private key/

https://codereview.chromium.org/1232553003/diff/530001/chrome/common/extensio...
chrome/common/extensions/api/certificate_provider.idl:50: void(optional
ArrayBuffer signature, DoneCallback callback);
Nit 1: Document |callback|. When will it be called, when *what* is done?
Nit 2: It seems that for callbacks, you put a space between return type and
brackets?

https://codereview.chromium.org/1232553003/diff/530001/chrome/common/extensio...
chrome/common/extensions/api/certificate_provider.idl:58: // the extension.
Will |callback| also be invoked if there are no invalid certificates?

https://codereview.chromium.org/1232553003/diff/530001/chrome/common/extensio...
File chrome/common/extensions/api/certificate_provider_internal.idl (right):

https://codereview.chromium.org/1232553003/diff/530001/chrome/common/extensio...
chrome/common/extensions/api/certificate_provider_internal.idl:5: // Internal
API used to run callbacks passed to chrome.certificateProvider API
I somehow find this entire comment block incredibly hard to parse. It is
difficult to understand who adds and removes the requestIds, whether the are
attached to events or callbacks or both. I know this wrappper foo to hide the
requestIDs is difficult to grasp, but would you mind taking a stab at clarifying
it a bit more? I am happy to discuss this offline if it helps.

https://codereview.chromium.org/1232553003/diff/530001/chrome/common/extensio...
chrome/common/extensions/api/certificate_provider_internal.idl:29: // the
certificates argument to report an error.
It is not documented in certificate_provider.idl that you can report an error by
leaving out the certificates argument.

[bartfab (slow), 2015-09-04 09:21:58]

I reviewed the JS you asked me to take a look at as well now.

https://codereview.chromium.org/1232553003/diff/530001/chrome/test/data/exten...
File chrome/test/data/extensions/api_test/certificate_provider/basic.js (right):

https://codereview.chromium.org/1232553003/diff/530001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/certificate_provider/basic.js:37: function
readData(dictionary, callback) {
Nit: Do we really need all this if in the end, we just read a single file?

https://codereview.chromium.org/1232553003/diff/530001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/certificate_provider/basic.js:40: if (index
>= keys.length) {
Nit: How is ">" possible?

https://codereview.chromium.org/1232553003/diff/530001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/certificate_provider/basic.js:53:
recurse(0);
Whatever happened to good old for loops?

https://codereview.chromium.org/1232553003/diff/530001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/certificate_provider/basic.js:76: }
Should this not verify which cert was rejected?

https://codereview.chromium.org/1232553003/diff/530001/chrome/test/data/exten...
File
chrome/test/data/extensions/api_test/certificate_provider/create_test_certs.sh
(right):

https://codereview.chromium.org/1232553003/diff/530001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/certificate_provider/create_test_certs.sh:9:
#         \
Nit: Un-indent so "\" aligns with the start of the word "root."

[davidben, 2015-09-04 16:00:10]

chrome/browser/profiles/ lgtm

[mmenke, 2015-09-04 16:01:07]

On 2015/09/04 16:00:10, David Benjamin wrote:
> chrome/browser/profiles/ lgtm

LGTM, deferring to david.

[pneubeck (no reviews), 2015-09-07 17:19:23]

Patchset #19 (id:550001) has been deleted

[pneubeck (no reviews), 2015-09-07 17:21:33]

Addressed feedback.

https://codereview.chromium.org/1232553003/diff/530001/chrome/browser/chromeo...
File
chrome/browser/chromeos/certificate_provider/certificate_provider_service_factory.cc
(right):

https://codereview.chromium.org/1232553003/diff/530001/chrome/browser/chromeo...
chrome/browser/chromeos/certificate_provider/certificate_provider_service_factory.cc:7:
#include "base/debug/stack_trace.h"
On 2015/09/03 17:30:51, bartfab wrote:
> Nit: Not used.

Done.

https://codereview.chromium.org/1232553003/diff/530001/chrome/browser/chromeo...
chrome/browser/chromeos/certificate_provider/certificate_provider_service_factory.cc:28:
public extensions::ExtensionRegistryObserver {
On 2015/09/03 17:30:52, bartfab wrote:
> Nit: #include "extensions/browser/extension_registry_observer.h"

Done.

https://codereview.chromium.org/1232553003/diff/530001/chrome/browser/chromeo...
chrome/browser/chromeos/certificate_provider/certificate_provider_service_factory.cc:35:
std::vector<std::string> CertificateProviderExtensions() override;
On 2015/09/03 17:30:51, bartfab wrote:
> Nit 1: // CertificateProviderService::Delegate:
> Nit 2: #include <vector>
> Nit 3: #include <string>

Done.

https://codereview.chromium.org/1232553003/diff/530001/chrome/browser/chromeo...
chrome/browser/chromeos/certificate_provider/certificate_provider_service_factory.cc:73:
event_router_->listeners().GetEventListenersByName(event_name)) {
On 2015/09/03 17:30:51, bartfab wrote:
> Nit: #include "extensions/browser/event_listener_map.h"

Done.

https://codereview.chromium.org/1232553003/diff/530001/chrome/browser/chromeo...
chrome/browser/chromeos/certificate_provider/certificate_provider_service_factory.cc:81:
scoped_ptr<base::ListValue> internal_args(new base::ListValue);
On 2015/09/03 17:30:52, bartfab wrote:
> Nit: #include "base/values.h"

Done.

https://codereview.chromium.org/1232553003/diff/530001/chrome/browser/chromeo...
chrome/browser/chromeos/certificate_provider/certificate_provider_service_factory.cc:83:
scoped_ptr<extensions::Event> event(new extensions::Event(
On 2015/09/03 17:30:51, bartfab wrote:
> Nit: #include "base/memory/scoped_ptr.h"

Done.

https://codereview.chromium.org/1232553003/diff/530001/chrome/browser/chromeo...
chrome/browser/chromeos/certificate_provider/certificate_provider_service_factory.cc:84:
extensions::events::CERTIFICATEPROVIDER_ON_CERTIFICATES_REQUESTED,
On 2015/09/03 17:30:52, bartfab wrote:
> Nit: #include "extensions/browser/extension_event_histogram_value.h"

Done.

https://codereview.chromium.org/1232553003/diff/530001/chrome/browser/chromeo...
chrome/browser/chromeos/certificate_provider/certificate_provider_service_factory.cc:99:
switch (hash) {
On 2015/09/03 17:30:51, bartfab wrote:
> Nit: #include "net/ssl/ssl_private_key.h"

Done.

https://codereview.chromium.org/1232553003/diff/530001/chrome/browser/chromeo...
chrome/browser/chromeos/certificate_provider/certificate_provider_service_factory.cc:134:
service_->OnExtensionUnloaded(extension->id());
On 2015/09/03 17:30:51, bartfab wrote:
> Nit: #include "extensions/common/extension.h"

Done.

https://codereview.chromium.org/1232553003/diff/530001/chrome/browser/chromeo...
File
chrome/browser/chromeos/certificate_provider/certificate_provider_service_factory.h
(right):

https://codereview.chromium.org/1232553003/diff/530001/chrome/browser/chromeo...
chrome/browser/chromeos/certificate_provider/certificate_provider_service_factory.h:23:
content::BrowserContext* context);
On 2015/09/03 17:30:52, bartfab wrote:
> Nit: Forward-declare |content::BrowserContext|.

Done.

https://codereview.chromium.org/1232553003/diff/530001/chrome/browser/chromeo...
chrome/browser/chromeos/certificate_provider/certificate_provider_service_factory.h:31:
~CertificateProviderServiceFactory() override;
On 2015/09/03 17:30:52, bartfab wrote:
> Nit: Why do you need to override the destructor?

Done.

https://codereview.chromium.org/1232553003/diff/530001/chrome/browser/extensi...
File
chrome/browser/extensions/api/certificate_provider/certificate_provider_api.cc
(right):

https://codereview.chromium.org/1232553003/diff/530001/chrome/browser/extensi...
chrome/browser/extensions/api/certificate_provider/certificate_provider_api.cc:35:
scoped_ptr<api_cpi::ReportCertificates::Params> params(
On 2015/09/03 17:30:52, bartfab wrote:
> Nit: #include "base/memory/scoped_ptr.h"

Done.

https://codereview.chromium.org/1232553003/diff/530001/chrome/browser/extensi...
chrome/browser/extensions/api/certificate_provider/certificate_provider_api.cc:39:
chromeos::CertificateProviderService* service =
On 2015/09/03 17:30:52, bartfab wrote:
> Nit: const pointer.

Done.

https://codereview.chromium.org/1232553003/diff/530001/chrome/browser/extensi...
chrome/browser/extensions/api/certificate_provider/certificate_provider_api.cc:48:
std::vector<std::vector<char>> rejected_certificates;
On 2015/09/03 17:30:52, bartfab wrote:
> Nit: #include <vector>

Done.

https://codereview.chromium.org/1232553003/diff/530001/chrome/browser/extensi...
chrome/browser/extensions/api/certificate_provider/certificate_provider_api.cc:74:
WriteToConsole(content::CONSOLE_MESSAGE_LEVEL_ERROR, kErrorInvalidX509Cert);
On 2015/09/03 17:30:52, bartfab wrote:
> Nit: #include "content/public/common/console_message_level.h"

Done.

https://codereview.chromium.org/1232553003/diff/530001/chrome/browser/extensi...
chrome/browser/extensions/api/certificate_provider/certificate_provider_api.cc:85:
size_t public_key_length_in_bits = 0;
On 2015/09/03 17:30:52, bartfab wrote:
> Nit: #include <stddef.h>

Done.

https://codereview.chromium.org/1232553003/diff/530001/chrome/browser/extensi...
chrome/browser/extensions/api/certificate_provider/certificate_provider_api.cc:94:
DCHECK(public_key_length_in_bits != 0);
On 2015/09/03 17:30:52, bartfab wrote:
> Nit: DCHECK(public_key_length_in_bits) is sufficient

Done.

https://codereview.chromium.org/1232553003/diff/530001/chrome/browser/extensi...
chrome/browser/extensions/api/certificate_provider/certificate_provider_api.cc:99:
out_info->type = net::SSLPrivateKey::Type::RSA;
On 2015/09/03 17:30:52, bartfab wrote:
> Nit: #include "net/ssl/ssl_private_key.h"

Done.

https://codereview.chromium.org/1232553003/diff/530001/chrome/browser/extensi...
chrome/browser/extensions/api/certificate_provider/certificate_provider_api.cc:146:
chromeos::CertificateProviderService* service =
On 2015/09/03 17:30:52, bartfab wrote:
> Nit: const pointer.

Done.

https://codereview.chromium.org/1232553003/diff/530001/chrome/browser/extensi...
chrome/browser/extensions/api/certificate_provider/certificate_provider_api.cc:151:
std::vector<uint8_t> signature;
On 2015/09/03 17:30:52, bartfab wrote:
> Nit: #include <stdint.h>

Done.

https://codereview.chromium.org/1232553003/diff/530001/chrome/browser/extensi...
File
chrome/browser/extensions/api/certificate_provider/certificate_provider_api.h
(right):

https://codereview.chromium.org/1232553003/diff/530001/chrome/browser/extensi...
chrome/browser/extensions/api/certificate_provider/certificate_provider_api.h:27:
~CertificateProviderInternalReportCertificatesFunction() override;
On 2015/09/03 17:30:52, bartfab wrote:
> Nit: Why do you need to override the destructor?
Would fail with
 [chromium-style] Classes that are ref-counted should have explicit destructors
that are declared protected or private.

https://codereview.chromium.org/1232553003/diff/530001/chrome/browser/extensi...
chrome/browser/extensions/api/certificate_provider/certificate_provider_api.h:41:
~CertificateProviderInternalReportSignatureFunction() override;
On 2015/09/03 17:30:52, bartfab wrote:
> Nit: Why do you need to override the destructor?

Would fail with
 [chromium-style] Classes that are ref-counted should have explicit destructors
that are declared protected or private.

https://codereview.chromium.org/1232553003/diff/530001/chrome/browser/extensi...
File
chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc
(right):

https://codereview.chromium.org/1232553003/diff/530001/chrome/browser/extensi...
chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc:9:
#include "base/macros.h"
On 2015/09/03 17:30:53, bartfab wrote:
> Nit: Not used.

Done.

https://codereview.chromium.org/1232553003/diff/530001/chrome/browser/extensi...
chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc:16:
#include "chromeos/chromeos_switches.h"
On 2015/09/03 17:30:53, bartfab wrote:
> Nit: Not used.

Done.

https://codereview.chromium.org/1232553003/diff/530001/chrome/browser/extensi...
chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc:16:
#include "chromeos/chromeos_switches.h"
On 2015/09/03 17:30:53, bartfab wrote:
> Nit: Not used.

Done.

https://codereview.chromium.org/1232553003/diff/530001/chrome/browser/extensi...
chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc:28:
#include "net/base/escape.h"
On 2015/09/03 17:30:53, bartfab wrote:
> Nit: Not used.

Done.

https://codereview.chromium.org/1232553003/diff/530001/chrome/browser/extensi...
chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc:29:
#include "net/base/test_data_directory.h"
On 2015/09/03 17:30:53, bartfab wrote:
> Nit: Not used.

Done.

https://codereview.chromium.org/1232553003/diff/530001/chrome/browser/extensi...
chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc:30:
#include "net/cert/test_root_certs.h"
On 2015/09/03 17:30:52, bartfab wrote:
> Nit: Not used.

Done.

https://codereview.chromium.org/1232553003/diff/530001/chrome/browser/extensi...
chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc:40:
void IgnoreResult(base::Closure callback, const base::Value* value) {
On 2015/09/03 17:30:53, bartfab wrote:
> Nit 1: Pass |callback| by const reference.
> Nit 2: #include "base/callback.h"
> Nit 3: #include "base/values.h"

Done.

https://codereview.chromium.org/1232553003/diff/530001/chrome/browser/extensi...
chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc:44:
void StoreBool(bool* result, base::Closure callback, const base::Value* value) {
On 2015/09/03 17:30:53, bartfab wrote:
> Nit: Pass |callback| by const reference.

Done.

https://codereview.chromium.org/1232553003/diff/530001/chrome/browser/extensi...
chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc:49:
void StoreString(std::string* result,
On 2015/09/03 17:30:52, bartfab wrote:
> Nit: #include <string>

Done.

https://codereview.chromium.org/1232553003/diff/530001/chrome/browser/extensi...
chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc:50:
base::Closure callback,
On 2015/09/03 17:30:53, bartfab wrote:
> Nit: Pass |callback| by const reference.

Done.

https://codereview.chromium.org/1232553003/diff/530001/chrome/browser/extensi...
chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc:56:
void StoreDigest(base::Closure callback,
On 2015/09/03 17:30:53, bartfab wrote:
> Nit 1: Pass |callback| by const reference.
> Nit 2: Reorder arguments to match the other Store*() methods.

Done.

https://codereview.chromium.org/1232553003/diff/530001/chrome/browser/extensi...
chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc:57:
std::vector<uint8>* digest,
On 2015/09/03 17:30:53, bartfab wrote:
> Nit 1: Here and throughout the file: s/uint8/uint8_t/, then #include
<stdint.h>
> Nit 2: #include <vector>

Done.

https://codereview.chromium.org/1232553003/diff/530001/chrome/browser/extensi...
chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc:61:
const uint8* const binary_begin =
On 2015/09/03 17:30:53, bartfab wrote:
> Nit: const pointer to const.

It is already. Isn't it?

https://codereview.chromium.org/1232553003/diff/530001/chrome/browser/extensi...
chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc:78:
size_t prefixed_digest_len = 0;
On 2015/09/03 17:30:53, bartfab wrote:
> Nit: #include <stddef.h>

Done.

https://codereview.chromium.org/1232553003/diff/530001/chrome/browser/extensi...
chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc:81:
NID_sha1, vector_as_array(&digest),
On 2015/09/03 17:30:52, bartfab wrote:
> Nit: #include "base/stl_util.h"

Done.

https://codereview.chromium.org/1232553003/diff/530001/chrome/browser/extensi...
chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc:91:
free(prefixed_digest);
On 2015/09/03 17:30:52, bartfab wrote:
> Nit: #include <stdlib.h>

Done.

https://codereview.chromium.org/1232553003/diff/530001/chrome/browser/extensi...
chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc:102:
// Create a string that if evaluated in Javascript returns a Uint8Array with
On 2015/09/03 17:30:53, bartfab wrote:
> Nit: s/Javascript/JavaScript/

Done.

https://codereview.chromium.org/1232553003/diff/530001/chrome/browser/extensi...
chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc:106:
for (size_t i = 0; i < bytes.size(); i++) {
On 2015/09/03 17:30:53, bartfab wrote:
> Nit: Why not use an iterator?

Done.

https://codereview.chromium.org/1232553003/diff/530001/chrome/browser/extensi...
chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc:108:
if (i < bytes.size() - 1) {
On 2015/09/03 17:30:53, bartfab wrote:
> JavaScript actually allows trailing commas, so you can get away with just
always
> adding one.

Done.

https://codereview.chromium.org/1232553003/diff/530001/chrome/browser/extensi...
chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc:129:
// Setup the AutoSelectCertificateForUrls policy to avoid the client
On 2015/09/03 17:30:52, bartfab wrote:
> Nit: s/Setup/Set up/

Done.

https://codereview.chromium.org/1232553003/diff/530001/chrome/browser/extensi...
chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc:153:
// Start an HTTPS test server that requests a client certificates.
On 2015/09/03 17:30:52, bartfab wrote:
> Nit: s/certificates/certificate/

Done.

https://codereview.chromium.org/1232553003/diff/530001/chrome/browser/extensi...
chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc:165:
const extensions::Extension* extension = LoadExtension(extension_path);
On 2015/09/03 17:30:52, bartfab wrote:
> Nit 1: const pointer to const.
> Nit 2: #include "extensions/common/extension.h"

Done.

https://codereview.chromium.org/1232553003/diff/530001/chrome/browser/extensi...
chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc:191:
base::RunLoop run_loop;
On 2015/09/03 17:30:52, bartfab wrote:
> Nit: #include "base/run_loop.h"

Done.

https://codereview.chromium.org/1232553003/diff/530001/chrome/browser/extensi...
chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc:194:
"(function() {return signDigestRequest.digest;})();"),
On 2015/09/03 17:30:53, bartfab wrote:
> Nit: Does this really need to be wrapped into a function?

Done.

https://codereview.chromium.org/1232553003/diff/530001/chrome/browser/extensi...
chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc:195:
base::Bind(&StoreDigest, run_loop.QuitClosure(), &request_digest));
On 2015/09/03 17:30:53, bartfab wrote:
> Nit: #include "base/bind.h"

Done.

https://codereview.chromium.org/1232553003/diff/530001/chrome/browser/extensi...
chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc:201:
base::ReadFileToString(extension_path.AppendASCII("l1_leaf.pk8"), &key_pk8);
On 2015/09/03 17:30:53, bartfab wrote:
> Nit: #include "base/files/file_util.h"

Done.

https://codereview.chromium.org/1232553003/diff/530001/chrome/browser/extensi...
chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc:211:
ASSERT_TRUE(RsaSign(request_digest, key.get(), &signature));
On 2015/09/03 17:30:53, bartfab wrote:
> Nit: EXPECT_TRUE() should be sufficient here.

Done.

https://codereview.chromium.org/1232553003/diff/530001/chrome/browser/extensi...
chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc:216:
const std::string code = "(function() {replyWithSignature(" +
On 2015/09/03 17:30:53, bartfab wrote:
> Nit: As above, does this really need to be wrapped into a function?

Done.

https://codereview.chromium.org/1232553003/diff/530001/chrome/browser/extensi...
chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc:234:
"(function() {return document.body.textContent;})();"),
On 2015/09/03 17:30:53, bartfab wrote:
> Nit: As above, does this really need to be wrapped into a function?

Done.

https://codereview.chromium.org/1232553003/diff/530001/chrome/common/extensio...
File chrome/common/extensions/api/certificate_provider.idl (right):

https://codereview.chromium.org/1232553003/diff/530001/chrome/common/extensio...
chrome/common/extensions/api/certificate_provider.idl:40: callback
ResultCallback = void (ArrayBuffer[] rejectedCertificates);
On 2015/09/03 17:30:53, bartfab wrote:
> Nit: Document who will call these two callbacks and when.

Done.

https://codereview.chromium.org/1232553003/diff/530001/chrome/common/extensio...
chrome/common/extensions/api/certificate_provider.idl:43: // the digest using
the private of the requested certificate.
On 2015/09/03 17:30:53, bartfab wrote:
> Nit: s/private/private key/

Done.

https://codereview.chromium.org/1232553003/diff/530001/chrome/common/extensio...
chrome/common/extensions/api/certificate_provider.idl:50: void(optional
ArrayBuffer signature, DoneCallback callback);
On 2015/09/03 17:30:54, bartfab wrote:
> Nit 1: Document |callback|. When will it be called, when *what* is done?
> Nit 2: It seems that for callbacks, you put a space between return type and
> brackets?

Nit 1: Removed.
Nit 2: Done.

https://codereview.chromium.org/1232553003/diff/530001/chrome/common/extensio...
chrome/common/extensions/api/certificate_provider.idl:58: // the extension.
On 2015/09/03 17:30:54, bartfab wrote:
> Will |callback| also be invoked if there are no invalid certificates?

Done.

https://codereview.chromium.org/1232553003/diff/530001/chrome/common/extensio...
File chrome/common/extensions/api/certificate_provider_internal.idl (right):

https://codereview.chromium.org/1232553003/diff/530001/chrome/common/extensio...
chrome/common/extensions/api/certificate_provider_internal.idl:5: // Internal
API used to run callbacks passed to chrome.certificateProvider API
On 2015/09/03 17:30:54, bartfab wrote:
> I somehow find this entire comment block incredibly hard to parse. It is
> difficult to understand who adds and removes the requestIds, whether the are
> attached to events or callbacks or both. I know this wrappper foo to hide the
> requestIDs is difficult to grasp, but would you mind taking a stab at
clarifying
> it a bit more? I am happy to discuss this offline if it helps.

Done.

https://codereview.chromium.org/1232553003/diff/530001/chrome/common/extensio...
chrome/common/extensions/api/certificate_provider_internal.idl:29: // the
certificates argument to report an error.
On 2015/09/03 17:30:54, bartfab wrote:
> It is not documented in certificate_provider.idl that you can report an error
by
> leaving out the certificates argument.

That's because you can't. Here certificates is optional, in the public facing
API it's mandatory.

https://codereview.chromium.org/1232553003/diff/530001/chrome/test/data/exten...
File chrome/test/data/extensions/api_test/certificate_provider/basic.js (right):

https://codereview.chromium.org/1232553003/diff/530001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/certificate_provider/basic.js:37: function
readData(dictionary, callback) {
On 2015/09/04 09:21:57, bartfab wrote:
> Nit: Do we really need all this if in the end, we just read a single file?

If possible I'd like to share this with the same function in
api_test/platform_keys .
But let me remove it, as long as we only have a single cert.

https://codereview.chromium.org/1232553003/diff/530001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/certificate_provider/basic.js:40: if (index
>= keys.length) {
On 2015/09/04 09:21:57, bartfab wrote:
> Nit: How is ">" possible?

Done.

https://codereview.chromium.org/1232553003/diff/530001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/certificate_provider/basic.js:53:
recurse(0);
On 2015/09/04 09:21:57, bartfab wrote:
> Whatever happened to good old for loops?

If we had an asynchronous for loop, I'd be happy to use it.
The recursive call happens in the callback of readFile.

https://codereview.chromium.org/1232553003/diff/530001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/certificate_provider/basic.js:76: }
On 2015/09/04 09:21:57, bartfab wrote:
> Should this not verify which cert was rejected?

Requires comparing arrays, which the standard javascript library doesn't
provide.
I instead compare the length now, here and in the sign request.

https://codereview.chromium.org/1232553003/diff/530001/chrome/test/data/exten...
File
chrome/test/data/extensions/api_test/certificate_provider/create_test_certs.sh
(right):

https://codereview.chromium.org/1232553003/diff/530001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/certificate_provider/create_test_certs.sh:9:
#         \
On 2015/09/04 09:21:58, bartfab wrote:
> Nit: Un-indent so "\" aligns with the start of the word "root."

Done.

[pneubeck (no reviews), 2015-09-07 17:24:25]

@Bartosz, ptal also at the chrome/*.gypi files.

[pneubeck (no reviews), 2015-09-07 17:25:55]

pneubeck@chromium.org changed reviewers:
+ jhawkins@chromium.org

[pneubeck (no reviews), 2015-09-07 17:25:56]

jhawkins@chromium.org: Please review changes in 
  chrome/renderer/resources/renderer_resources.grd

[emaxx, 2015-09-07 17:35:14]

lgtm

https://codereview.chromium.org/1232553003/diff/570001/chrome/browser/chromeo...
File
chrome/browser/chromeos/certificate_provider/certificate_provider_service_factory.cc
(right):

https://codereview.chromium.org/1232553003/diff/570001/chrome/browser/chromeo...
chrome/browser/chromeos/certificate_provider/certificate_provider_service_factory.cc:128:
}
Maybe worth adding "default: NOTREACHED();".

https://codereview.chromium.org/1232553003/diff/570001/chrome/browser/extensi...
File
chrome/browser/extensions/api/certificate_provider/certificate_provider_api.cc
(right):

https://codereview.chromium.org/1232553003/diff/570001/chrome/browser/extensi...
chrome/browser/extensions/api/certificate_provider/certificate_provider_api.cc:53:
return RespondNow(Error("Failure"));
Is it really OK to provide no error details here?

https://codereview.chromium.org/1232553003/diff/570001/chrome/common/extensio...
File chrome/common/extensions/api/certificate_provider.idl (right):

https://codereview.chromium.org/1232553003/diff/570001/chrome/common/extensio...
chrome/common/extensions/api/certificate_provider.idl:40: // rejected
certificates. See <code>CertficiatesCallbac</code>.
Typos in "CertificatesCallback".

[pneubeck (no reviews), 2015-09-08 07:58:33]

pneubeck@chromium.org changed reviewers:
+ jochen@chromium.org
- jhawkins@chromium.org

[pneubeck (no reviews), 2015-09-08 07:58:34]

-James

jochen@chromium.org: Please review changes in 
  chrome/renderer/resources/renderer_resources.grd

[jochen (gone - plz use gerrit), 2015-09-08 12:30:33]

lgtm

[bartfab (slow), 2015-09-08 14:55:17]

lgtm

https://codereview.chromium.org/1232553003/diff/530001/chrome/browser/extensi...
File
chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc
(right):

https://codereview.chromium.org/1232553003/diff/530001/chrome/browser/extensi...
chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc:61:
const uint8* const binary_begin =
On 2015/09/07 17:21:32, pneubeck wrote:
> On 2015/09/03 17:30:53, bartfab wrote:
> > Nit: const pointer to const.
> 
> It is already. Isn't it?

Sorry :).

https://codereview.chromium.org/1232553003/diff/570001/chrome/browser/chromeo...
File
chrome/browser/chromeos/certificate_provider/certificate_provider_service_factory.cc
(right):

https://codereview.chromium.org/1232553003/diff/570001/chrome/browser/chromeo...
chrome/browser/chromeos/certificate_provider/certificate_provider_service_factory.cc:131:
if (!net::X509Certificate::GetDEREncoded(certificate->os_cert_handle(),
Nit 1: #include "src/net/cert/x509_certificate.h"
Nit 2: #include "base/memory/ref_counted.h"

https://codereview.chromium.org/1232553003/diff/570001/chrome/browser/extensi...
File
chrome/browser/extensions/api/certificate_provider/certificate_provider_api.h
(right):

https://codereview.chromium.org/1232553003/diff/570001/chrome/browser/extensi...
chrome/browser/extensions/api/certificate_provider/certificate_provider_api.h:8:
#include "chrome/browser/extensions/chrome_extension_function.h"
Nit: Not used.

https://codereview.chromium.org/1232553003/diff/570001/chrome/browser/extensi...
chrome/browser/extensions/api/certificate_provider/certificate_provider_api.h:25:
: public UIThreadExtensionFunction {
Nit: #include "src/extensions/browser/extension_function.h"

https://codereview.chromium.org/1232553003/diff/570001/chrome/browser/extensi...
File
chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc
(right):

https://codereview.chromium.org/1232553003/diff/570001/chrome/browser/extensi...
chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc:166:
base::FilePath(FILE_PATH_LITERAL("chrome/test/data")));
You missed this comment: Nit: Use |PathService::Get(chrome::DIR_TEST_DATA,
&test_data_dir);| instead.

https://codereview.chromium.org/1232553003/diff/570001/chrome/common/extensio...
File chrome/common/extensions/api/certificate_provider.idl (right):

https://codereview.chromium.org/1232553003/diff/570001/chrome/common/extensio...
chrome/common/extensions/api/certificate_provider.idl:40: // rejected
certificates. See <code>CertficiatesCallbac</code>.
Can you use a $(ref:) here instead of <code></code>?

https://codereview.chromium.org/1232553003/diff/570001/chrome/common/extensio...
chrome/common/extensions/api/certificate_provider.idl:47: // padding. If an
<code>MD5_SHA1</code> hash must be signed, the extension
Nit: s/must/is to/ (to avoid double "must" in one sentence)

https://codereview.chromium.org/1232553003/diff/570001/chrome/common/extensio...
chrome/common/extensions/api/certificate_provider.idl:75: // |request|: Contains
the details about the sign request.
Nit: s:|:<code>: s:|:</code>:

https://codereview.chromium.org/1232553003/diff/570001/chrome/test/data/exten...
File chrome/test/data/extensions/api_test/certificate_provider/basic.js (right):

https://codereview.chromium.org/1232553003/diff/570001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/certificate_provider/basic.js:79:
assertEq(validCertInfo.supportedHashes[0], request.hash);
Nit: Assert first that |supportedHashes| has length 1.

[pneubeck (no reviews), 2015-09-08 15:30:51]

https://codereview.chromium.org/1232553003/diff/570001/chrome/browser/chromeo...
File
chrome/browser/chromeos/certificate_provider/certificate_provider_service_factory.cc
(right):

https://codereview.chromium.org/1232553003/diff/570001/chrome/browser/chromeo...
chrome/browser/chromeos/certificate_provider/certificate_provider_service_factory.cc:128:
}
On 2015/09/07 17:35:14, emaxx wrote:
> Maybe worth adding "default: NOTREACHED();".

As it is, the compiler complains about a missing case. Not so, if I add a
default case.

https://codereview.chromium.org/1232553003/diff/570001/chrome/browser/chromeo...
chrome/browser/chromeos/certificate_provider/certificate_provider_service_factory.cc:131:
if (!net::X509Certificate::GetDEREncoded(certificate->os_cert_handle(),
On 2015/09/08 14:55:17, bartfab wrote:
> Nit 1: #include "src/net/cert/x509_certificate.h"
> Nit 2: #include "base/memory/ref_counted.h"

Done.

https://codereview.chromium.org/1232553003/diff/570001/chrome/browser/extensi...
File
chrome/browser/extensions/api/certificate_provider/certificate_provider_api.cc
(right):

https://codereview.chromium.org/1232553003/diff/570001/chrome/browser/extensi...
chrome/browser/extensions/api/certificate_provider/certificate_provider_api.cc:53:
return RespondNow(Error("Failure"));
On 2015/09/07 17:35:14, emaxx wrote:
> Is it really OK to provide no error details here?

Done.

https://codereview.chromium.org/1232553003/diff/570001/chrome/browser/extensi...
File
chrome/browser/extensions/api/certificate_provider/certificate_provider_api.h
(right):

https://codereview.chromium.org/1232553003/diff/570001/chrome/browser/extensi...
chrome/browser/extensions/api/certificate_provider/certificate_provider_api.h:8:
#include "chrome/browser/extensions/chrome_extension_function.h"
On 2015/09/08 14:55:17, bartfab wrote:
> Nit: Not used.

Done.

https://codereview.chromium.org/1232553003/diff/570001/chrome/browser/extensi...
chrome/browser/extensions/api/certificate_provider/certificate_provider_api.h:25:
: public UIThreadExtensionFunction {
On 2015/09/08 14:55:17, bartfab wrote:
> Nit: #include "src/extensions/browser/extension_function.h"

Done.

https://codereview.chromium.org/1232553003/diff/570001/chrome/browser/extensi...
File
chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc
(right):

https://codereview.chromium.org/1232553003/diff/570001/chrome/browser/extensi...
chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc:166:
base::FilePath(FILE_PATH_LITERAL("chrome/test/data")));
On 2015/09/08 14:55:17, bartfab wrote:
> You missed this comment: Nit: Use |PathService::Get(chrome::DIR_TEST_DATA,
> &test_data_dir);| instead.

Done for real.

https://codereview.chromium.org/1232553003/diff/570001/chrome/common/extensio...
File chrome/common/extensions/api/certificate_provider.idl (right):

https://codereview.chromium.org/1232553003/diff/570001/chrome/common/extensio...
chrome/common/extensions/api/certificate_provider.idl:40: // rejected
certificates. See <code>CertficiatesCallbac</code>.
On 2015/09/07 17:35:14, emaxx wrote:
> Typos in "CertificatesCallback".

Done.

https://codereview.chromium.org/1232553003/diff/570001/chrome/common/extensio...
chrome/common/extensions/api/certificate_provider.idl:40: // rejected
certificates. See <code>CertficiatesCallbac</code>.
On 2015/09/08 14:55:17, bartfab wrote:
> Can you use a $(ref:) here instead of <code></code>?

I don't know, as this comment currently does not appear in the generated
documentation.

https://codereview.chromium.org/1232553003/diff/570001/chrome/common/extensio...
chrome/common/extensions/api/certificate_provider.idl:47: // padding. If an
<code>MD5_SHA1</code> hash must be signed, the extension
On 2015/09/08 14:55:17, bartfab wrote:
> Nit: s/must/is to/ (to avoid double "must" in one sentence)

Done.

https://codereview.chromium.org/1232553003/diff/570001/chrome/common/extensio...
chrome/common/extensions/api/certificate_provider.idl:75: // |request|: Contains
the details about the sign request.
On 2015/09/08 14:55:17, bartfab wrote:
> Nit: s:|:<code>: s:|:</code>:

Nope. the |argument_name| syntax is expected by the documentation generator.

https://codereview.chromium.org/1232553003/diff/570001/chrome/test/data/exten...
File chrome/test/data/extensions/api_test/certificate_provider/basic.js (right):

https://codereview.chromium.org/1232553003/diff/570001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/certificate_provider/basic.js:79:
assertEq(validCertInfo.supportedHashes[0], request.hash);
On 2015/09/08 14:55:17, bartfab wrote:
> Nit: Assert first that |supportedHashes| has length 1.

Done.

[pneubeck (no reviews), 2015-09-08 15:31:01]

The CQ bit was checked by pneubeck@chromium.org

[pneubeck (no reviews), 2015-09-08 15:31:01]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org, kalman@chromium.org, davidben@chromium.org,
mmenke@chromium.org, jochen@chromium.org, emaxx@chromium.org,
bartfab@chromium.org
Link to the patchset: https://codereview.chromium.org/1232553003/#ps610001
(title: "Addressed last feedback.")

[commit-bot: I haz the power, 2015-09-08 15:31:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1232553003/610001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1232553003/610001

[commit-bot: I haz the power, 2015-09-08 15:53:14]

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

[commit-bot: I haz the power, 2015-09-08 15:53:15]

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

[bartfab (slow), 2015-09-08 16:44:48]

Still LGTM.

[pneubeck (no reviews), 2015-09-08 17:21:45]

The CQ bit was checked by pneubeck@chromium.org

[pneubeck (no reviews), 2015-09-08 17:21:46]

The patchset sent to the CQ was uploaded after l-g-t-m from
bartfab@chromium.org, jochen@chromium.org, emaxx@chromium.org,
davidben@chromium.org, asvitkine@chromium.org, mmenke@chromium.org,
kalman@chromium.org
Link to the patchset: https://codereview.chromium.org/1232553003/#ps630001
(title: "Fix unittest.")

[commit-bot: I haz the power, 2015-09-08 17:22:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1232553003/630001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1232553003/630001

[commit-bot: I haz the power, 2015-09-08 17:39:14]

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

[commit-bot: I haz the power, 2015-09-08 17:39:15]

Try jobs failed on following builders:
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[pneubeck (no reviews), 2015-09-08 18:04:09]

The CQ bit was checked by pneubeck@chromium.org

[pneubeck (no reviews), 2015-09-08 18:04:10]

The patchset sent to the CQ was uploaded after l-g-t-m from
bartfab@chromium.org, jochen@chromium.org, emaxx@chromium.org,
davidben@chromium.org, asvitkine@chromium.org, mmenke@chromium.org,
kalman@chromium.org
Link to the patchset: https://codereview.chromium.org/1232553003/#ps650001
(title: "Fix non-chromeos.")

[commit-bot: I haz the power, 2015-09-08 18:04:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1232553003/650001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1232553003/650001

[Andrew T Wilson (Slow), 2015-09-08 18:24:00]

On 2015/09/08 18:04:57, commit-bot: I haz the power wrote:
> CQ is trying da patch. Follow status at
>  https://chromium-cq-status.appspot.com/patch-status/1232553003/650001
> View timeline at
>  https://chromium-cq-status.appspot.com/patch-timeline/1232553003/650001

Lgtm

[commit-bot: I haz the power, 2015-09-08 18:38:16]

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

[commit-bot: I haz the power, 2015-09-08 18:38:18]

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

[bartfab (slow), 2015-09-09 09:08:29]

lgtm

[pneubeck (no reviews), 2015-09-09 09:09:48]

The CQ bit was checked by pneubeck@chromium.org

[pneubeck (no reviews), 2015-09-09 09:09:48]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
emaxx@chromium.org, atwilson@chromium.org, davidben@chromium.org,
asvitkine@chromium.org, mmenke@chromium.org, kalman@chromium.org
Link to the patchset: https://codereview.chromium.org/1232553003/#ps670001
(title: "Fixing gn and adding a permission message.")

[commit-bot: I haz the power, 2015-09-09 09:10:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1232553003/670001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1232553003/670001

[commit-bot: I haz the power, 2015-09-09 09:16:52]

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

[commit-bot: I haz the power, 2015-09-09 09:16:54]

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...)

[pneubeck (no reviews), 2015-09-09 09:26:08]

The CQ bit was checked by pneubeck@chromium.org

[pneubeck (no reviews), 2015-09-09 09:26:08]

The patchset sent to the CQ was uploaded after l-g-t-m from
bartfab@chromium.org, jochen@chromium.org, emaxx@chromium.org,
atwilson@chromium.org, davidben@chromium.org, asvitkine@chromium.org,
mmenke@chromium.org, kalman@chromium.org
Link to the patchset: https://codereview.chromium.org/1232553003/#ps690001
(title: " Rebased.")

[commit-bot: I haz the power, 2015-09-09 09:26:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1232553003/690001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1232553003/690001

[commit-bot: I haz the power, 2015-09-09 11:37:48]

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

[commit-bot: I haz the power, 2015-09-09 11:37:50]

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

[pneubeck (no reviews), 2015-09-09 11:38:37]

The CQ bit was checked by pneubeck@chromium.org

[commit-bot: I haz the power, 2015-09-09 11:38:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1232553003/690001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1232553003/690001

[commit-bot: I haz the power, 2015-09-09 12:53:33]

Committed patchset #25 (id:690001)

[commit-bot: I haz the power, 2015-09-09 12:54:22]

Patchset 25 (id:??) landed as
https://crrev.com/9cac4acf0e0a0898e14ac0c187a4872eef41b820
Cr-Commit-Position: refs/heads/master@{#347916}

[yosin_UTC9, 2015-09-09 14:18:36]

On 2015/09/09 12:54:22, commit-bot: I haz the power wrote:
> Patchset 25 (id:??) landed as
> https://crrev.com/9cac4acf0e0a0898e14ac0c187a4872eef41b820
> Cr-Commit-Position: refs/heads/master@{#347916}

Causes compilation error on chromeos_daisy_chromium_compile_only_ng
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...

FAILED: /b/build/goma/gomacc armv7a-cros-linux-gnueabi-g++
-B/b/build/slave/chromeos_daisy/build/.cros_cache/chrome-sdk/tarballs/daisy+7424.0.0+target_toolchain/usr/x86_64-pc-linux-gnu/armv7a-cros-linux-gnueabi/binutils-bin/2.25.51-gold
-MMD -MF
obj/chrome/browser/extensions/api/certificate_provider/browser_tests.certificate_provider_apitest.o.d
 ... snip ... -c
../../chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc
-o
obj/chrome/browser/extensions/api/certificate_provider/browser_tests.certificate_provider_apitest.o
../../chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc:246:5:
error: multi-line comment [-Werror=comment]
     // openssl x509 -inform DER -noout -fingerprint -in \
     ^

[engedy, 2015-09-09 16:04:08]

A follow-up CL has been landed to fix compiler errors mentioned by Yosi:
https://codereview.chromium.org/1309363008.