NSS: Handle unfriendly tokens in client auth.

NSS: Handle unfriendly tokens in client auth.

BUG=42073, 113434
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=237487

[mattm, 2013-11-23 00:56:45]

joi: chrome/, content/
wtc: net/

rsleevi: fyi

[Ryan Sleevi, 2013-11-23 01:10:13]

Does this not require a change to the SSLClientSocketNSS? I seem to recall
that some of the tokens wouldn't require auth until you tried the C_Sign
operation, rather than when you acquired the CKO_PRIVATE object.

Have you tested this? Should we create a test module based on the chapsd
replay code?
On Nov 22, 2013 4:56 PM, <mattm@chromium.org> wrote:

> Reviewers: Ryan Sleevi, wtc, Jói,
>
> Message:
> joi: chrome/, content/
> wtc: net/
>
> rsleevi: fyi
>
> Description:
> NSS: Handle unfriendly tokens in client auth.
>
> BUG=42073
>
> Please review this at https://codereview.chromium.org/83793006/
>
> SVN Base: svn://svn.chromium.org/chrome/trunk/src
>
> Affected files (+89, -19 lines):
>   M chrome/browser/profiles/profile_io_data.h
>   M chrome/browser/profiles/profile_io_data.cc
>   M content/browser/loader/resource_loader.cc
>   M content/browser/loader/resource_loader_unittest.cc
>   M content/browser/resource_context_impl.cc
>   M content/public/browser/resource_context.h
>   M net/ssl/client_cert_store_impl.h
>   M net/ssl/client_cert_store_impl_mac.cc
>   M net/ssl/client_cert_store_impl_nss.cc
>   M net/ssl/client_cert_store_impl_win.cc
>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[mattm, 2013-11-23 01:13:58]

On 2013/11/23 01:10:13, Ryan Sleevi wrote:
> Does this not require a change to the SSLClientSocketNSS? I seem to recall
> that some of the tokens wouldn't require auth until you tried the C_Sign
> operation, rather than when you acquired the CKO_PRIVATE object.

That would be the "friendly" token case right? Those should be handled by the
"UnlockCertSlotIfNecessary" stuff in ssl_client_certificate_selector.cc, but it
would be interesting to see if we can use the password delegate for that case
too.

> Have you tested this?

Yeah, I tested with a "goldkey" token.

> Should we create a test module based on the chapsd replay code?

Haven't heard of this. Pointers?

> On Nov 22, 2013 4:56 PM, <mailto:mattm@chromium.org> wrote:
> 
> > Reviewers: Ryan Sleevi, wtc, Jói,
> >
> > Message:
> > joi: chrome/, content/
> > wtc: net/
> >
> > rsleevi: fyi
> >
> > Description:
> > NSS: Handle unfriendly tokens in client auth.
> >
> > BUG=42073
> >
> > Please review this at https://codereview.chromium.org/83793006/
> >
> > SVN Base: svn://svn.chromium.org/chrome/trunk/src
> >
> > Affected files (+89, -19 lines):
> >   M chrome/browser/profiles/profile_io_data.h
> >   M chrome/browser/profiles/profile_io_data.cc
> >   M content/browser/loader/resource_loader.cc
> >   M content/browser/loader/resource_loader_unittest.cc
> >   M content/browser/resource_context_impl.cc
> >   M content/public/browser/resource_context.h
> >   M net/ssl/client_cert_store_impl.h
> >   M net/ssl/client_cert_store_impl_mac.cc
> >   M net/ssl/client_cert_store_impl_nss.cc
> >   M net/ssl/client_cert_store_impl_win.cc
> >
> >
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[Ryan Sleevi, 2013-11-25 06:16:19]

On 2013/11/23 01:13:58, mattm wrote:
> On 2013/11/23 01:10:13, Ryan Sleevi wrote:
> > Does this not require a change to the SSLClientSocketNSS? I seem to recall
> > that some of the tokens wouldn't require auth until you tried the C_Sign
> > operation, rather than when you acquired the CKO_PRIVATE object.
> 
> That would be the "friendly" token case right? Those should be handled by the
> "UnlockCertSlotIfNecessary" stuff in ssl_client_certificate_selector.cc, but
it
> would be interesting to see if we can use the password delegate for that case
> too.

It's friendly in that they can be enumerated, but we'd still need to wire an
auth delegate through wincx, which I'm not aware of us doing in
SSLClientSocketNSS. Still, it's unrelated to this CL, so happy to punt.

> 
> > Have you tested this?
> 
> Yeah, I tested with a "goldkey" token.
> 
> > Should we create a test module based on the chapsd replay code?

https://chromium.googlesource.com/chromiumos/platform/chaps/+/master/p11_repl...
is the replay code I mentioned, but that replays as a client against a
known-good module.

What we'd want is the mock slot stuff from
https://chromium.googlesource.com/chromiumos/platform/chaps/+/master/session_...
and friends. But that's also something we can investigate later.

> 
> Haven't heard of this. Pointers?
> 
> > On Nov 22, 2013 4:56 PM, <mailto:mattm@chromium.org> wrote:
> > 
> > > Reviewers: Ryan Sleevi, wtc, Jói,
> > >
> > > Message:
> > > joi: chrome/, content/
> > > wtc: net/
> > >
> > > rsleevi: fyi
> > >
> > > Description:
> > > NSS: Handle unfriendly tokens in client auth.
> > >
> > > BUG=42073
> > >
> > > Please review this at https://codereview.chromium.org/83793006/
> > >
> > > SVN Base: svn://svn.chromium.org/chrome/trunk/src
> > >
> > > Affected files (+89, -19 lines):
> > >   M chrome/browser/profiles/profile_io_data.h
> > >   M chrome/browser/profiles/profile_io_data.cc
> > >   M content/browser/loader/resource_loader.cc
> > >   M content/browser/loader/resource_loader_unittest.cc
> > >   M content/browser/resource_context_impl.cc
> > >   M content/public/browser/resource_context.h
> > >   M net/ssl/client_cert_store_impl.h
> > >   M net/ssl/client_cert_store_impl_mac.cc
> > >   M net/ssl/client_cert_store_impl_nss.cc
> > >   M net/ssl/client_cert_store_impl_win.cc
> > >
> > >
> > >
> > 
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.

[Ryan Sleevi, 2013-11-25 06:24:34]

https://codereview.chromium.org/83793006/diff/70001/content/public/browser/re...
File content/public/browser/resource_context.h (right):

https://codereview.chromium.org/83793006/diff/70001/content/public/browser/re...
content/public/browser/resource_context.h:48: const std::string& host_and_port);
I'm really not a fan of threading the host name up to this layer. I think the
goal should be that there's a single ClientCertStore, so that we can centrally
leverage things like platform notifications.

I admit, this is somewhat of a design nit; we could conceivably have a
ClientCertStore creation that, under the hood, used a shared object to
collaborate on things like notifications and caching, but that seems like a
pretty ugly layer of abstraction.

Would it be possible to thread the details of needing the hostname into the
ClientCertStoreNSS, so that we don't have to expose this detail to all CCS
implementations?

I'm not sure whether that would be a bound function / factory object (eg:
instead of SetPasswordDelegate, SetPasswordDelegateFactory) or simply an
interface in the implementation, but that'd avoid introducing the concept of a
"per-host" client cert store.

WDYT?

[Jói, 2013-11-25 13:02:30]

I think you should get one of the listed OWNERS for ProfileIOData to look at the
Chrome bits (willchan, mmenke or ajwong).

I'm not a reviewer for all of content. For content/public the change looks OK if
this is a design other reviewers are OK with.

You could ask avi@ or jam@ for the content review once other reviewers are
happy.

Cheers,
Jói

[mattm, 2013-11-26 01:15:20]

https://codereview.chromium.org/83793006/diff/70001/content/public/browser/re...
File content/public/browser/resource_context.h (right):

https://codereview.chromium.org/83793006/diff/70001/content/public/browser/re...
content/public/browser/resource_context.h:48: const std::string& host_and_port);
On 2013/11/25 06:24:34, Ryan Sleevi wrote:
> I'm really not a fan of threading the host name up to this layer. I think the
> goal should be that there's a single ClientCertStore, so that we can centrally
> leverage things like platform notifications.
> 
> I admit, this is somewhat of a design nit; we could conceivably have a
> ClientCertStore creation that, under the hood, used a shared object to
> collaborate on things like notifications and caching, but that seems like a
> pretty ugly layer of abstraction.
> 
> Would it be possible to thread the details of needing the hostname into the
> ClientCertStoreNSS, so that we don't have to expose this detail to all CCS
> implementations?
> 
> I'm not sure whether that would be a bound function / factory object (eg:
> instead of SetPasswordDelegate, SetPasswordDelegateFactory) or simply an
> interface in the implementation, but that'd avoid introducing the concept of a
> "per-host" client cert store.
> 
> WDYT?

Yeah, I agree that it wasn't the cleanest approach. I've changed to the factory
method as you suggested, PTAL.

[Ryan Sleevi, 2013-11-26 19:59:02]

This LGTM, although I feel like we should be considering moving from
"ClientCertStoreImpl" (with the ifdefs) into a proper
ClientCertStoreMac/Win/NSS. The only place this object is created is
chrome/browser/profiles/profile_io_data.cc, and it has the 'benefit' of making
the password delegate a constructor parameter for the *NSS case.

Just $.02

https://codereview.chromium.org/83793006/diff/290001/net/ssl/client_cert_stor...
File net/ssl/client_cert_store_impl.h (right):

https://codereview.chromium.org/83793006/diff/290001/net/ssl/client_cert_stor...
net/ssl/client_cert_store_impl.h:33: const std::string&)>
PasswordDelegateFactory;
Can you include a documentation note about what this callback (and the
associated parameter)

https://codereview.chromium.org/83793006/diff/290001/net/ssl/client_cert_stor...
net/ssl/client_cert_store_impl.h:34: void set_password_delegate_factory(
style: Shouldn't this be SetPasswordDelegateFactory

https://codereview.chromium.org/83793006/diff/290001/net/ssl/client_cert_stor...
File net/ssl/client_cert_store_impl_nss.cc (right):

https://codereview.chromium.org/83793006/diff/290001/net/ssl/client_cert_stor...
net/ssl/client_cert_store_impl_nss.cc:86: DVLOG(1) << "Finding client certs on
worker thread.";
Seems very spammy. Nuke?

[mattm, 2013-11-26 21:49:14]

On 2013/11/26 19:59:02, Ryan Sleevi wrote:
> This LGTM, although I feel like we should be considering moving from
> "ClientCertStoreImpl" (with the ifdefs) into a proper
> ClientCertStoreMac/Win/NSS. The only place this object is created is
> chrome/browser/profiles/profile_io_data.cc, and it has the 'benefit' of making
> the password delegate a constructor parameter for the *NSS case.
> 
> Just $.02

Hm, would also required some work on the unittest too (though could clean up
some ifdefs there too possibly).  I'll consider that separately.

https://codereview.chromium.org/83793006/diff/290001/net/ssl/client_cert_stor...
File net/ssl/client_cert_store_impl.h (right):

https://codereview.chromium.org/83793006/diff/290001/net/ssl/client_cert_stor...
net/ssl/client_cert_store_impl.h:33: const std::string&)>
PasswordDelegateFactory;
On 2013/11/26 19:59:03, Ryan Sleevi wrote:
> Can you include a documentation note about what this callback (and the
> associated parameter)

Done.

https://codereview.chromium.org/83793006/diff/290001/net/ssl/client_cert_stor...
net/ssl/client_cert_store_impl.h:34: void set_password_delegate_factory(
On 2013/11/26 19:59:03, Ryan Sleevi wrote:
> style: Shouldn't this be SetPasswordDelegateFactory

Style guide says that simple setters & getters use the lower_underscore style:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Names

https://codereview.chromium.org/83793006/diff/290001/net/ssl/client_cert_stor...
File net/ssl/client_cert_store_impl_nss.cc (right):

https://codereview.chromium.org/83793006/diff/290001/net/ssl/client_cert_stor...
net/ssl/client_cert_store_impl_nss.cc:86: DVLOG(1) << "Finding client certs on
worker thread.";
On 2013/11/26 19:59:03, Ryan Sleevi wrote:
> Seems very spammy. Nuke?

Done.

[mattm, 2013-11-26 21:49:35]

+mmenke for profile_io_data.cc

[mmenke, 2013-11-26 22:01:31]

On 2013/11/26 21:49:35, mattm wrote:
> +mmenke for profile_io_data.cc

Rubberstamp LGTM for profile_io_data.cc.

[commit-bot: I haz the power, 2013-11-26 22:39:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mattm@chromium.org/83793006/300001

[wtc, 2013-11-26 23:28:16]

Patch set 3 LGTM.

https://codereview.chromium.org/83793006/diff/300001/net/ssl/client_cert_stor...
File net/ssl/client_cert_store_impl_mac.cc (right):

https://codereview.chromium.org/83793006/diff/300001/net/ssl/client_cert_stor...
net/ssl/client_cert_store_impl_mac.cc:177:
ClientCertStoreImpl::~ClientCertStoreImpl() {}

Nit: It is a shame that we need to define the constructor and destructor in
three files.

If we don't declare the constructor and destructor in the header file, will the
compiler do the right thing?

https://codereview.chromium.org/83793006/diff/300001/net/ssl/client_cert_stor...
File net/ssl/client_cert_store_impl_nss.cc (right):

https://codereview.chromium.org/83793006/diff/300001/net/ssl/client_cert_stor...
net/ssl/client_cert_store_impl_nss.cc:114:
password_delegate_factory_.Run(request.host_and_port));

Nit: curly braces should be used because the statement spans multiple lines.

[Ryan Sleevi, 2013-11-26 23:36:26]

https://codereview.chromium.org/83793006/diff/300001/net/ssl/client_cert_stor...
File net/ssl/client_cert_store_impl_mac.cc (right):

https://codereview.chromium.org/83793006/diff/300001/net/ssl/client_cert_stor...
net/ssl/client_cert_store_impl_mac.cc:177:
ClientCertStoreImpl::~ClientCertStoreImpl() {}
On 2013/11/26 23:28:17, wtc wrote:
> 
> Nit: It is a shame that we need to define the constructor and destructor in
> three files.
> 
> If we don't declare the constructor and destructor in the header file, will
the
> compiler do the right thing?

The clang style checker will warn about a non-trivial ctor/dtor being inlined
(either the compiler generated or inlined)

However, the (maybe) splitting of this into separate classes will solve this.

We could alternatively add a ssl_client_cert_store_impl.cc with just the
ctor/dtor, but that seemed overkill, so I didn't suggest it.

[mattm, 2013-11-26 23:47:55]

https://codereview.chromium.org/83793006/diff/300001/net/ssl/client_cert_stor...
File net/ssl/client_cert_store_impl_nss.cc (right):

https://codereview.chromium.org/83793006/diff/300001/net/ssl/client_cert_stor...
net/ssl/client_cert_store_impl_nss.cc:114:
password_delegate_factory_.Run(request.host_and_port));
On 2013/11/26 23:28:17, wtc wrote:
> 
> Nit: curly braces should be used because the statement spans multiple lines.

Done.

[commit-bot: I haz the power, 2013-11-26 23:54:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mattm@chromium.org/83793006/320001

[commit-bot: I haz the power, 2013-11-27 00:36:05]

Retried try job too often on mac_rel for step(s) telemetry_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-11-27 00:39:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mattm@chromium.org/83793006/320001

[commit-bot: I haz the power, 2013-11-27 02:18:08]

Change committed as 237487