*WIP* Store NSS slots per profile. Move keygen to chrome.

*WIP* Store NSS slots per profile. Move keygen to chrome.

BUG=218653

[mattm, 2013-06-28 02:06:33]

Hey Ryan, just wanted a quick opinion on this.  Obviously still needs some
cleanups and such, but wanted to get your opinion on the overall architecture
first.

[Ryan Sleevi, 2013-06-28 17:01:59]

I think it's trending in the right direction.

I agree that storing the public/private slot for persistent data (as opposed to
a keygen handler, foo handler, bar handler) is most likely going to be the right
approach, especially given ChromeOS and its need to know the actual slots.

A few comments below.

https://codereview.chromium.org/18121007/diff/1/net/cert/cert_verify_proc_nss.cc
File net/cert/cert_verify_proc_nss.cc (right):

https://codereview.chromium.org/18121007/diff/1/net/cert/cert_verify_proc_nss...
net/cert/cert_verify_proc_nss.cc:462: // XXX can't specify slots here
Curious what/why you mentioned this?

https://codereview.chromium.org/18121007/diff/1/net/ssl/client_cert_store_imp...
File net/ssl/client_cert_store_impl_nss.cc (right):

https://codereview.chromium.org/18121007/diff/1/net/ssl/client_cert_store_imp...
net/ssl/client_cert_store_impl_nss.cc:82: // XXX
I'm guessing this is where you want to filter by slot?

[Ryan Sleevi, 2013-09-12 21:41:08]

Is this dead, given priorities?

[mattm, 2013-09-12 21:46:32]

On 2013/09/12 21:41:08, Ryan Sleevi wrote:
> Is this dead, given priorities?

It's all very confusing, but apparently it's not actually dead. I was just
working on updating this actually.

https://codereview.chromium.org/18121007/diff/1/net/cert/cert_verify_proc_nss.cc
File net/cert/cert_verify_proc_nss.cc (right):

https://codereview.chromium.org/18121007/diff/1/net/cert/cert_verify_proc_nss...
net/cert/cert_verify_proc_nss.cc:462: // XXX can't specify slots here
On 2013/06/28 17:01:59, Ryan Sleevi wrote:
> Curious what/why you mentioned this?
Was just looking at places that would care about using the correct slot. For
Issue 218627 (Honor trust roots per-profile) this cert verification stuff seemed
relevant, but I'm not familiar with the API mentioned in #0 of that bug. Doing
that would be a different CL anyway though.

https://codereview.chromium.org/18121007/diff/1/net/ssl/client_cert_store_imp...
File net/ssl/client_cert_store_impl_nss.cc (right):

https://codereview.chromium.org/18121007/diff/1/net/ssl/client_cert_store_imp...
net/ssl/client_cert_store_impl_nss.cc:82: // XXX
On 2013/06/28 17:01:59, Ryan Sleevi wrote:
> I'm guessing this is where you want to filter by slot?

yeah, probably something for a separate CL though.

[mattm, 2013-09-12 22:11:40]

I just uploaded my latest changes.  Main thing I'm wondering about now is how to
pipe through chrome from content/browser/loader/resource_loader.cc (see comments
I added there)

[Ryan Sleevi, 2013-10-09 19:18:00]

I have no objections to having chrome/ pass the ClientCertStore through to
content/. That was certainly the end-game with the content/ refactor. 

https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/lo...
was just the stub to minimize API churn.

https://codereview.chromium.org/18121007/diff/43001/chrome/browser/chrome_con...
File chrome/browser/chrome_content_browser_client.h (right):

https://codereview.chromium.org/18121007/diff/43001/chrome/browser/chrome_con...
chrome/browser/chrome_content_browser_client.h:178: virtual void Keygen(
Keygen in this context sounds like a noun. Should we "verbify" it by
"GenerateKey" (or "GenerateKeygenKey", if necessary)

https://codereview.chromium.org/18121007/diff/43001/content/browser/renderer_...
File content/browser/renderer_host/render_message_filter.cc (right):

https://codereview.chromium.org/18121007/diff/43001/content/browser/renderer_...
content/browser/renderer_host/render_message_filter.cc:964:
ViewHostMsg_Keygen::WriteReplyParams(reply_msg, result ? *result : "");
s/""/std::string()

https://codereview.chromium.org/18121007/diff/43001/net/cert/cert_verify_proc...
File net/cert/cert_verify_proc_nss.cc (right):

https://codereview.chromium.org/18121007/diff/43001/net/cert/cert_verify_proc...
net/cert/cert_verify_proc_nss.cc:454: // XXX can't specify slots here
Why would you need to? What's the thought?

[mattm, 2013-10-09 23:52:37]

I had another question about initialization async.  For example,
nss_util::GetPrivateNSSKeySlot will currently return null until the TPM is
ready. I can make the per-profile ones do the same, but I was also wondering
about the software slot, as doing file IO on the UI thread to load the nss db
while initializing the profile seems bad.

So I was wondering if the public key slot methods should also return NULL while
it loads on a background thread, or if the functions should just be async with a
callback, or if I should try to find a way to make creating the profile wait
until the nss db is loaded on some background thread..

https://codereview.chromium.org/18121007/diff/43001/chrome/browser/chrome_con...
File chrome/browser/chrome_content_browser_client.h (right):

https://codereview.chromium.org/18121007/diff/43001/chrome/browser/chrome_con...
chrome/browser/chrome_content_browser_client.h:178: virtual void Keygen(
On 2013/10/09 19:18:00, Ryan Sleevi wrote:
> Keygen in this context sounds like a noun. Should we "verbify" it by
> "GenerateKey" (or "GenerateKeygenKey", if necessary)

will do.

https://codereview.chromium.org/18121007/diff/43001/content/browser/renderer_...
File content/browser/renderer_host/render_message_filter.cc (right):

https://codereview.chromium.org/18121007/diff/43001/content/browser/renderer_...
content/browser/renderer_host/render_message_filter.cc:964:
ViewHostMsg_Keygen::WriteReplyParams(reply_msg, result ? *result : "");
On 2013/10/09 19:18:00, Ryan Sleevi wrote:
> s/""/std::string()

will do.

https://codereview.chromium.org/18121007/diff/43001/net/cert/cert_verify_proc...
File net/cert/cert_verify_proc_nss.cc (right):

https://codereview.chromium.org/18121007/diff/43001/net/cert/cert_verify_proc...
net/cert/cert_verify_proc_nss.cc:454: // XXX can't specify slots here
On 2013/10/09 19:18:00, Ryan Sleevi wrote:
> Why would you need to? What's the thought?

Mentioned in an earlier reply, but was just thinking about the "Honor trust
roots per-profile" bug.  Cert verification needs to be profile aware in some
manner, right? I guess the comment is just saying I'm not sure yet how that will
work.

[mattm, 2013-10-15 05:18:23]

new patches uploaded.

In patchset 5 I was aiming to implement the meat of the stuff in ProfileIOData,
but then I realized that regular and incognito profiles have separate
ProfileIOData objects and it would end up reopening the user db if you created
an incognito window.

In patchset 6 I went back to having a NSSSlotFactory (but not a
BrowserContextKeyedFactory this time, just a leaky LazyInstance). 

I also implemented the slot ready callbacks, though I made it just run the
callback immediately if the slot is already ready, which seems a bit simpler
than having the caller have to implement separate branches for whether the slot
is already loaded or not.

The code still needs some cleanup, but it's working, so I thought I'd send it
out again to see what you think of this approach.

[mattm, 2013-10-16 07:03:26]

I uploaded another patchset.  Client auth is basically working except that
CERT_FindUserCertsByUsage doesn't return any certs for the 2nd tpm slot.

I dug a little bit into it, the CERT_GetCertNicknames in
CERT_FindUserCertsByUsage finds the cert from the 2nd slot, but the
CERT_FindCertByNickname doesn't find the matching cert.

Other things I checked:
If I use the cert manager, both show up (it doesn't do any filtering yet).

PK11_NeedLogin returns false on both tpm slots.

If I log in to the 2nd profile as the primary profile, CERT_FindUserCertsByUsage
finds the cert.


I'll try to dig more on this tomorrow, but any ideas that jump out?

[Ryan Sleevi, 2013-11-21 23:49:45]

On 2013/10/16 07:03:26, mattm wrote:
> I uploaded another patchset.  Client auth is basically working except that
> CERT_FindUserCertsByUsage doesn't return any certs for the 2nd tpm slot.
> 
> I dug a little bit into it, the CERT_GetCertNicknames in
> CERT_FindUserCertsByUsage finds the cert from the 2nd slot, but the
> CERT_FindCertByNickname doesn't find the matching cert.
> 
> Other things I checked:
> If I use the cert manager, both show up (it doesn't do any filtering yet).
> 
> PK11_NeedLogin returns false on both tpm slots.
> 
> If I log in to the 2nd profile as the primary profile,
CERT_FindUserCertsByUsage
> finds the cert.
> 
> 
> I'll try to dig more on this tomorrow, but any ideas that jump out?

I seem to recall you dug in and filed a Mozilla bug on this, right?

Is this something I should be working on for the Mozilla 3.15.5 release?

[mattm, 2013-11-21 23:55:03]

On 2013/11/21 23:49:45, Ryan Sleevi wrote:
> On 2013/10/16 07:03:26, mattm wrote:
> > I uploaded another patchset.  Client auth is basically working except that
> > CERT_FindUserCertsByUsage doesn't return any certs for the 2nd tpm slot.
> > 
> > I dug a little bit into it, the CERT_GetCertNicknames in
> > CERT_FindUserCertsByUsage finds the cert from the 2nd slot, but the
> > CERT_FindCertByNickname doesn't find the matching cert.
> > 
> > Other things I checked:
> > If I use the cert manager, both show up (it doesn't do any filtering yet).
> > 
> > PK11_NeedLogin returns false on both tpm slots.
> > 
> > If I log in to the 2nd profile as the primary profile,
> CERT_FindUserCertsByUsage
> > finds the cert.
> > 
> > 
> > I'll try to dig more on this tomorrow, but any ideas that jump out?
> 
> I seem to recall you dug in and filed a Mozilla bug on this, right?
> 
> Is this something I should be working on for the Mozilla 3.15.5 release?

I think I only filed it in crbug/315285.  I will file an upstream one too
though.  I don't think it needs to make a specific NSS release since we could
easily include a patch file in chromeos's ebuild, but it would be good to have a
fix for m33.