Mac client-side SSL cert improvements.

Mac client-side SSL cert improvements.
Allow Netscape-style client certs.
Remember which identity the user chooses for a domain, and put it at the top of the list next time.
BUG=36316, 36446
TEST=none

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=39904

[wtc, 2010-02-24 01:44:50]

LGTM.  I have some questions and suggested changes below.

Great idea to take advantage of Mac's identity preference.

None of the client certs I got from real CAs uses only
the SSL Client flag in the (obsolete) NetscapeCertType
extension.  So the OpenSSL program should also be updated
to follow the current practice.

http://codereview.chromium.org/651090/diff/1/2
File chrome/browser/ssl/ssl_client_auth_handler_mac.mm (right):

http://codereview.chromium.org/651090/diff/1/2#newcode44
chrome/browser/ssl/ssl_client_auth_handler_mac.mm:44: [panel setDomain:domain];
Does this call SecIdentitySetPreference under the cover?

http://codereview.chromium.org/651090/diff/1/3
File net/base/x509_certificate.h (right):

http://codereview.chromium.org/651090/diff/1/3#newcode237
net/base/x509_certificate.h:237: std::string domain,
Nit: this input parameter should be const std::string&

It's not obvious why the domain matters to SSL client auth.
The name of that argument should contain "server", and
you should explain that it is for looking up the preferred
identity for that server.

http://codereview.chromium.org/651090/diff/1/4
File net/base/x509_certificate_mac.cc (right):

http://codereview.chromium.org/651090/diff/1/4#newcode12
net/base/x509_certificate_mac.cc:12: #include "base/sys_string_conversions.h"
Nit: List "base/sys_string_conversions.h" after
"base/pickle.h"

http://codereview.chromium.org/651090/diff/1/4#newcode720
net/base/x509_certificate_mac.cc:720: const CSSM_FIELD &field =
fields.fields[f];
Nit: put & next to the type, CSSM_FIELD.

http://codereview.chromium.org/651090/diff/1/4#newcode759
net/base/x509_certificate_mac.cc:759: if (domain != "") {
Is it better to say
  !domain.empty()
?

http://codereview.chromium.org/651090/diff/1/4#newcode764
net/base/x509_certificate_mac.cc:764: if (SecIdentityCopyPreference(domain_str,
The first argument should also contain the port, if it's not
443.

Where do we call SecIdentitySetPreference?

Is the identity preference stored persistently?

http://codereview.chromium.org/651090/diff/1/4#newcode805
net/base/x509_certificate_mac.cc:805: // selected by default in the UI.)
Nit: the period (.) should be after the closing parenthesis.

[Jens Alfke, 2010-02-24 18:09:29]

On Feb 23, 2010, at 5:44 PM, wtc@chromium.org wrote:

> So the OpenSSL program should also be updated
> to follow the current practice.

I'll file a bug report with them. (It may also have been fixed in a  
more recent version; Mac OS X 10.5 ships with whatever version was  
current in mid-2007.)

> http://codereview.chromium.org/651090/diff/1/2#newcode44
> chrome/browser/ssl/ssl_client_auth_handler_mac.mm:44: [panel
> setDomain:domain];
> Does this call SecIdentitySetPreference under the cover?

Yup.

> http://codereview.chromium.org/651090/diff/1/3#newcode237
> net/base/x509_certificate.h:237: std::string domain,
> Nit: this input parameter should be const std::string&

Done.

> It's not obvious why the domain matters to SSL client auth.
> The name of that argument should contain "server", and
> you should explain that it is for looking up the preferred
> identity for that server.

Done.

> net/base/x509_certificate_mac.cc:12: #include
> "base/sys_string_conversions.h"
> Nit: List "base/sys_string_conversions.h" after
> "base/pickle.h"

Done.

> net/base/x509_certificate_mac.cc:720: const CSSM_FIELD &field =
> fields.fields[f];
> Nit: put & next to the type, CSSM_FIELD.

Done.

> http://codereview.chromium.org/651090/diff/1/4#newcode759
> net/base/x509_certificate_mac.cc:759: if (domain != "") {
> Is it better to say
>  !domain.empty()
> ?

Done.

> net/base/x509_certificate_mac.cc:764: if
> (SecIdentityCopyPreference(domain_str,
> The first argument should also contain the port, if it's not
> 443.

Do you think that different SSL ports on the same host would want  
different client certs? If I implement this, it looks like the only  
way for the caller (SSLClientSocketMac) to determine the port number  
is to grope it out of the underlying socket (transport_). I can write  
this up as a bug and fix it in a follow-up patch.

> Where do we call SecIdentitySetPreference?

The SFChooseIdentityPanel does it for us, when the user presses OK.

> Is the identity preference stored persistently?

Yes, it's stored in the keychain. It even shows up in Keychain Access  
as an item of type "identity preference" so the user can manually  
change or remove it if necessary. These are also shared by Safari.

> net/base/x509_certificate_mac.cc:805: // selected by default in the  
> UI.)
> Nit: the period (.) should be after the closing parenthesis.

Not if the parenthesized text is an entire clause; then the period  
goes inside.

—Jens

[wtc, 2010-02-24 20:13:51]

http://codereview.chromium.org/651090/diff/1/4
File net/base/x509_certificate_mac.cc (right):

http://codereview.chromium.org/651090/diff/1/4#newcode764
net/base/x509_certificate_mac.cc:764: if (SecIdentityCopyPreference(domain_str,
On 2010/02/24 01:44:51, wtc wrote:
> The first argument should also contain the port, if it's not
> 443.

I see the difficulty of getting the port.  I forgot that we
pass only the hostname but not the port to the
SSLClientSocket constructor.  Perhaps we should fix that.
The port number is useful for SSL session reuse as well.

Whether different ports on the same post are considered as
the same server depends on the browser.  IE considers
them as the same server, whereas Firefox considers them
as different server instances.  If we want to treat
different ports as different server instances, than the
port needs to be included.