Mac: Make client-cert picker only show certs the server will accept.

Mac: Make client-cert picker only show certs the server will accept.
BUG=38691
TEST=manual testing with various sites

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

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

[Jens Alfke, 2010-03-19 20:42:06]

This adds code to
- Parse BER DistinguishedNames into Principal objects
- Test whether a cert has an issuer whose name comes from a vector of Principals
- Filter the set of available client certs by the valid-issuers list sent by the
server.

The support classes in x509_certificate and x509_certificate_mac were getting
pretty large, so I factored them out into a new file x509_types.

[rsleevi-old, 2010-03-20 01:26:37]

For what it's worth on platform compatibility/behaviour, this will result in a
different chain building behaviour from CryptoAPI and NSS.

Microsoft documentation -
http://technet.microsoft.com/en-us/library/bb457027.aspx (Appendix G and H are
the concern)

NSS documentation - CERT_FilterCertListByCANames, which ultimately ends up in
nssCertificateArray_FindBestCertificate, which enumerates all certificates found
for a specific issuer and runs additional policy and time validity checks to
determine the one that 'best' satisfies the criteria (the newest that passes all
of those checks). This is also part of NSS_CmpCertChainWCANames

As discussed on apple-cdsa, the chain building logic of Apple's TP chain builder
simply returns the first certificate from the supplied CSSM DBs that match on
names - it doesn't perform the signature / policy / date validity checks until
after the chain has already been built.

This may all be acceptable, but I thought it might be worth mentioning. You can
find an in-depth discussion of the various subtleties in RFC 4158, which is
applicable since it's the client's responsibility to suggest/supply a
certificate that is likely to be accepted by the server.

http://codereview.chromium.org/1128008/diff/4001/5006
File net/base/x509_types_mac.cc (right):

http://codereview.chromium.org/1128008/diff/4001/5006#newcode91
net/base/x509_types_mac.cc:91: { SEC_ASN1_PRINTABLE_STRING,
offsetof(KeyValuePair, value), },
Per http://www.ietf.org/rfc/rfc5280.txt 4.1.2.4, shouldn't this be some form of
ANY, since it's a CHOICE based on key?

See the notes on DirectoryString (in that section) and Section 7.1 -
"Internationalized Names in Distinguished Names" - UTF8String is
allowed/encouraged in 4.1.2.4 and MUST be supported according to 7.1.

According to NSS (which is, after all, Apple's decoder - see the docs from
Security), the docs at
http://www.mozilla.org/projects/security/pki/nss/tech-notes/tn1.html suggest
that SEC_ASN1_CHOICE may be a better call here. I've certainly encountered
UTF8Strings and BMPStrings in certs from some of the main commercial Trust
Anchors.

One option is - 
http://old.nabble.com/Which-way-to-decode-DER-ASN1-CHOICE---td25747634.html

Another option is
http://www.mozilla.org/projects/security/pki/nss/tech-notes/tn1.html#decoder_...
- SEC_ASN1_DYNAMIC with SEC_ASN1TemplateChooser callback. Probably also need to
specify SEC_ASN1_INLINE so that the decoder will actually recurse into it.

[Jens Alfke, 2010-03-22 18:23:40]

http://codereview.chromium.org/1128008/diff/4001/5006
File net/base/x509_types_mac.cc (right):

http://codereview.chromium.org/1128008/diff/4001/5006#newcode91
net/base/x509_types_mac.cc:91: { SEC_ASN1_PRINTABLE_STRING,
offsetof(KeyValuePair, value), },
On 2010/03/20 01:26:38, rsleevi wrote:
> Per http://www.ietf.org/rfc/rfc5280.txt 4.1.2.4, shouldn't this be some form
of
> ANY, since it's a CHOICE based on key?

I think you're right-- this does need to support multiple data types. This is my
first attempt at using the SecAsn1Coder and I'm kind of struggling. Thanks for
pointing out that this is the same as NSS's API! I hadn't been able to find any
real documentation on it, but now the NSS docs will be a big help.

[Jens Alfke, 2010-03-22 22:06:49]

OK, I've added support for IA5STRING, UTF8STRING and BMPSTRING in addition to
PRINTABLESTRING. I made sure to add test cases that exercise BMPSTRING (since
it's 16-bit) and non-ascii strings.

There's also T61STRING, but it seems to be a bizarro obsolete encoding. Does it
even occur in the wild anymore?

[rsleevi-old, 2010-03-22 23:25:11]

http://codereview.chromium.org/1128008/diff/13001/14006
File net/base/x509_types_mac.cc (right):

http://codereview.chromium.org/1128008/diff/13001/14006#newcode127
net/base/x509_types_mac.cc:127: { 0, }
You seem to be missing both UniversalString (UTF-32 encoding, per X.690-0207
8.21.7, and, IIRC, a big endian form, per BER/DER encoding rules) and (as
identified in your comments), TeletexString/T61String.

Understandably, the challenge of converting T61String back to its UTF-8
counterpart is no small task (see
http://www.mail-archive.com/asn1@asn1.org/msg00460.html ). However, such
encodings are permissible for legacy certificates, even though current standards
direct against their use for new certificates.

It depends on the level of support you're intending with regards to the name
values on Mac. If you're intending for both comparison and display, your best
bet is likely to approach with the recommendations at
http://www.mail-archive.com/openca-users@lists.sourceforge.net/msg04365.html -
essentially treating the name as Latin-1 (ISO-8859-1). This appears to be the
same degree of support that Microsoft, NSS, and OpenSSL offer, so this is likely
acceptable.

If just for comparison, you've got much more flexibility as to what your target
name matching algorithm is. You could adopt RFCs 2459/3280, which states binary
comparison for everything but PrintableString, or you can go with RFC 5280,
which specifies TeletexString as optional to support (see 7.1.). Presumably, if
you do support it, because of the confusion related to T.61 and it's canonical
form, presumably one can simply 'fall back' to the binary comparison for such
fields, and it would yield an appropriate path-building equivalent.

You can find the Mozilla equivalent code at
http://hg.mozilla.org/mozilla-central/file/b4d603fb501d/security/nss/lib/cert...
, which indicates that T.61 strings are not treated as T.61, but instead as
ISO-8859-1, per common practice.

[rsleevi-old, 2010-03-23 01:23:30]

http://codereview.chromium.org/1128008/diff/13001/14004
File net/base/x509_types.cc (right):

http://codereview.chromium.org/1128008/diff/13001/14004#newcode15
net/base/x509_types.cc:15: return against.empty() || against == str;
Shouldn't this be 

return (str.empty() && against.empty()) || against == str

The current behaviour allows a common_name of "foo" to succesfully match against
an empty common_name, which would be incorrect behaviour.

http://codereview.chromium.org/1128008/diff/13001/14006
File net/base/x509_types_mac.cc (right):

http://codereview.chromium.org/1128008/diff/13001/14006#newcode206
net/base/x509_types_mac.cc:206: DCHECK_EQ(pair->value_type,
KeyValuePair::kTypeOther);
As implemented, you end up dropping the unsupported values from comparison,
which can lead to improper matches between Principals. This may not represent a
significant issue for the issue at hand (Mac client-cert picker), but could
result in security bugs if the comparison ended up being used for some form of
security assertion (although hopefully, any such assertion would also consider
the certificate public key).

Would it be better to simply AddPair of the raw string data, so that they end up
in a bit-for-bit comparison during X509Principal::Match ?

[wtc, 2010-03-23 01:55:49]

Jens,

Thanks a lot for this changelist.

I'm very nervous about us trying to implement distinguished
name comparison because the rules are very complicated.
That belongs in a crypto library such as NSS or Mac OS X's
Certificate, Key, and Trust Services.

I suggest that we first check in a changelist that does binary
comparison (which seems to be allowed by RFC 3280; search
for "binary comparison" in that RFC).  Then, when we switch
to using NSS for SSL on the Mac, we can call NSS functions
to do distinguished name comparison.

On Linux, Chrome calls the NSS_CmpCertChainWCANames function,
which does binary comparison:
http://mxr.mozilla.org/security/ident?i=NSS_CmpCertChainWCANames

FYI, here is the equivalent Firefox code:
http://mxr.mozilla.org/mozilla-central/ident?i=nsNSS_SSLGetClientAuthData

It calls nsConvertCANamesToStrings to convert the issuer
names from distinguished names to strings:
http://mxr.mozilla.org/mozilla-central/ident?i=nsConvertCANamesToStrings

The it calls CERT_FilterCertListByCANames to look for
client certs issued by an acceptable issuer:
http://mxr.mozilla.org/mozilla-central/ident?i=CERT_FilterCertListByCANames

The issuerName field of NSS's CERTCertificate structure
is also converted from distinguished name to string:
http://mxr.mozilla.org/mozilla-central/source/security/nss/lib/certdb/certdb....

The CERT_NameToAscii function that does the DN to string
conversion is complicated:
http://mxr.mozilla.org/mozilla-central/ident?i=CERT_NameToAscii

http://codereview.chromium.org/1128008/diff/13001/14004
File net/base/x509_types.cc (right):

http://codereview.chromium.org/1128008/diff/13001/14004#newcode28
net/base/x509_types.cc:28: // Matching rules:
<http://www.ietf.org/rfc/rfc2459.txt>
We should implement RFC 5280.  RFC 2459 is two versions old.

The condition expression that enumerates the names to match
is bound to be incomplete.  We define those members of
Principal for UI display purposes, not for equality comparison
purposes.

http://codereview.chromium.org/1128008/diff/13001/14005
File net/base/x509_types.h (right):

http://codereview.chromium.org/1128008/diff/13001/14005#newcode51
net/base/x509_types.h:51: struct X509Principal {
This header should be renamed x509_cert_types.h or cert_types.h.  Or create one
header per type.  Or
still define these X509Certificate-nested types in
x509_certificate.h but outside the X509Certificate class.

Rename X509Principal to CertPrincipal.
Rename X509Policy to CertPolicy.

[rsleevi-old, 2010-03-23 04:25:21]

On 2010/03/23 01:55:49, wtc wrote:
> Jens,
> 
> Thanks a lot for this changelist.
> 
> I'm very nervous about us trying to implement distinguished
> name comparison because the rules are very complicated.
> That belongs in a crypto library such as NSS or Mac OS X's
> Certificate, Key, and Trust Services.
> 
> I suggest that we first check in a changelist that does binary
> comparison (which seems to be allowed by RFC 3280; search
> for "binary comparison" in that RFC).  Then, when we switch
> to using NSS for SSL on the Mac, we can call NSS functions
> to do distinguished name comparison.

For what it's worth, on OS X, the name rules match RFC 3280: PrintableString is
normalized, while all other string types have binary comparisons performed
against them. 

Furthermore, the Apple-canonicalized DER form of an existing certificate's
subject/issuer can be obtained via the CDSA call CL_CertGetFirstFieldValue. This
normalized form is the same form returned by the Keychain call
SecKeychainItemCopyAttributesAndData with kSecSubjectItemAttr /
kSecIssuerItemAttr, or by querying the Keychain DL_DB for the "Subject" or
"Issuer" BLOB attributes.

The non-canoncalized CSSM value can be obtained by SecCertificateGetSubject.
This value has a 1:1 mapping with the NSS/SecASN1 templates Jens has developed.

Thus, for purposes of OS X Keychain searching/ca_names evaluation, the following
steps are necessary:
  1) DER-decode the ca_name into it's component AVA parts (the templates Jens
developed implement this)
  2) For all PrintableString types (and PrintableString only), normalize
according to RFC 3280. This involves making the search case-insensitive
(specifically, Apple normalizes to upper case), stripping leading/trailing
whitespace, and collapsing all runs of whitespace to a singular value.
  3) DER encoding the new structure, which is now the normalized form
  4) You can lookup all certificates matching this issuer (and potentially
public key) using SecKeychainSearchCreateWithAttributes (using
kSecIssuerItemAttr) or, alternatively, compare two names of two existing,
normalized using a straight binary comparison (or against the results of a
hashing function)

  These steps are unfortunately necessary because at no point does Apple expose
their CSSM_X509_NAME normalization routines to outside callers. The closest that
Apple comes is the private export
SecKeychainSearchCreateForCertificateByIssuerAndSN, which normalizes the issuer
on input, but unfortunately requires a serial number (and an exact match, at
that) for searching. As Jens has noted, SecIdentitySearchCreateWithAttributes
makes allusions to this support, but fails to implement it.

An algorithm that is capable of evaluating whether a given certificate is
chainable to a list of issuers, in a manner compliant with RFC3280, may thus be
described:

For each Issuer in ca_names
  Issuer = RFC3280Normalize(Issuer)

For each Certificate in Candidates
  If Certificate->Issuer in Issuers:
    it is a viable candidate chain (self-signed)
  If Certificate->Subject == Certificate->Issuer
    Not a viable candidate, remove
  Chains = BuildChains(Certificate->Issuer)
  For each Chain in Chains
    For each Certificate2 in Chain
      If Certificate2->Subject || Certificate2->Issuer in Issuers
        it is a viable candidate chain

BuildChains(IssuerDN):
  For each IssuerCertificate in
SecKeychainSearchCreateWithAttributes(kSecSubjectItemAttr, IssuerDN)
   Chains += IssuerCertificate + BuildChains(IssuerCertificate->Issuer)

  Return Chains

The distinction of chains and certificates is to facilitate communication with
remote servers which may not have the necessary intermediates, nor support
resolution protocols like SCVP.

Further, care must be taken about evaluating certificate policies, as it may be
possible that a viable certificate fails evaluation on the client, but that
would otherwise succeed on the server.

The client, on supplying a certificate, is allowed to supply one or more
additional, ordered CAs, that the server may use to create a chain back to a
supported Trust Anchor. SecureTransport supports this by means of
SSLSetCertificate taking a CFArrayRef of certificates.

Such a scenario reflecting the two situations above can be described as such:
Server 1 supports ca_name D, and has the following certificates:
  D: Issued by D
Client 1 has the following certificates:
  A: Their identity cert, Issued by B
  B(I): CA I, Issued by C
  B(II): Bridge CA II, Issued by D

In such a scenario, the acceptable certificate is A, but the server needs to
receive the intermediate B(II) to be able to complete the chain of A->B(II)->D.
If the client supplies A alone, the server will fail the authentication, unless
it's capable of receiving out-of-band (SCVP, LDAP, TAMP) the B(II) certificate.

Likewise, the Client lacks Certificate D, so they are unable to compare against
the necessary policy/policy constraints, but the chain itself would succeed if
presented to the server (in the form the client has).

Installing B(I) or B(II) on Server 1 may not be viable, as it may be operating
in a constrained memory environment (eg: embedded server).

Currently, support for this functionality is provided by both the CryptoAPI and
the NSS implementations, ergo IE and Firefox. Given the API limitations, I doubt
Safari is as robust, so this may be acceptable, but it certainly would be
desirable that both the support and behaviour is consistent across all platforms
(thus it is understandable by a native NSS implementation is being examined for
all platforms). Assuming the Keychain will still be supported in an OS X
implementation, and assuming Apple does not modify their CL/TP, something
similar to the above will likely be necessary at some point.

[Jens Alfke, 2010-03-23 20:19:57]

http://codereview.chromium.org/1128008/diff/13001/14004
File net/base/x509_types.cc (right):

http://codereview.chromium.org/1128008/diff/13001/14004#newcode15
net/base/x509_types.cc:15: return against.empty() || against == str;
On 2010/03/23 01:23:30, rsleevi wrote:
> The current behaviour allows a common_name of "foo" to succesfully match
against
> an empty common_name, which would be incorrect behaviour.

I was assuming that if the server didn't specify a particular field (like
common_name) that it didn't care what the value was; but on looking at RFC 5280,
I think you're correct. I'll change it to a simple == comparison.

http://codereview.chromium.org/1128008/diff/13001/14004#newcode28
net/base/x509_types.cc:28: // Matching rules:
<http://www.ietf.org/rfc/rfc2459.txt>
On 2010/03/23 01:55:50, wtc wrote:
> We should implement RFC 5280.  RFC 2459 is two versions old.

OK, I've updated the references.

http://codereview.chromium.org/1128008/diff/13001/14005
File net/base/x509_types.h (right):

http://codereview.chromium.org/1128008/diff/13001/14005#newcode51
net/base/x509_types.h:51: struct X509Principal {
On 2010/03/23 01:55:50, wtc wrote:
> This header should be renamed x509_cert_types.h or cert_types.h.  Or create
one
> header per type.  Or
> still define these X509Certificate-nested types in
> x509_certificate.h but outside the X509Certificate class.
> 
> Rename X509Principal to CertPrincipal.
> Rename X509Policy to CertPolicy.

OK. Renamed to x509_cert_types.

http://codereview.chromium.org/1128008/diff/13001/14006
File net/base/x509_types_mac.cc (right):

http://codereview.chromium.org/1128008/diff/13001/14006#newcode127
net/base/x509_types_mac.cc:127: { 0, }
On 2010/03/22 23:25:11, rsleevi wrote:
> You seem to be missing both UniversalString (UTF-32 encoding, per X.690-0207
> 8.21.7, and, IIRC, a big endian form, per BER/DER encoding rules) and (as
> identified in your comments), TeletexString/T61String.

OK, I've added both of those. The problem with doing the comparisons is that the
Principal class in its current form doesn't remember what data type a string
originally came from; it just stores std::strings, in UTF-8 by convention. This
would require structural changes to the Principal class, perhaps by changing its
string type to a custom class that contains both a std::string and a tag
indicating the ASN.1 type.

http://codereview.chromium.org/1128008/diff/13001/14006#newcode206
net/base/x509_types_mac.cc:206: DCHECK_EQ(pair->value_type,
KeyValuePair::kTypeOther);
On 2010/03/23 01:23:30, rsleevi wrote:
>  Would it be better to simply AddPair of the raw string data, so that they end
up
> in a bit-for-bit comparison during X509Principal::Match ?

Good point. I'll change it to do that.

[wtc, 2010-03-24 23:52:05]

LGTM.  Sorry about the delay in reviewing this CL.

Two of my comments below are marked with "IMPORTANT".  Please
make sure you address them.

I will try to find the source code of SecIdentityCopyPreference
to see how it uses its validIssuers argument.

http://codereview.chromium.org/1128008/diff/29001/30001
File net/base/x509_cert_types.cc (right):

http://codereview.chromium.org/1128008/diff/29001/30001#newcode27
net/base/x509_cert_types.cc:27: for (i2 = 0; i2 < rdn2.size(); ++i2)
Nit: add curly braces around the for loop's body because
it's too lines.

http://codereview.chromium.org/1128008/diff/29001/30001#newcode49
net/base/x509_cert_types.cc:49: std::ostream& operator<<(std::ostream& s, const
CertPrincipal& p) {
I seem to recall domainComponent is printed as "dc=".

It would be nice to use standard abbreviations of the
attribute types, but I don't know where they're defined.

http://codereview.chromium.org/1128008/diff/29001/30001#newcode70
net/base/x509_cert_types.cc:70: X509Certificate::Policy::Judgment
X509Certificate::Policy::Check(
In the rest of the file, X509Certificate::Policy should
be changed to CertPolicy, right?

http://codereview.chromium.org/1128008/diff/29001/30002
File net/base/x509_cert_types.h (right):

http://codereview.chromium.org/1128008/diff/29001/30002#newcode1
net/base/x509_cert_types.h:1: // Copyright (c) 2006-2010 The Chromium Authors.
All rights reserved.
Nit: the copyright notice should just say 2010.  If you like
a range of years better, it should start in 2007 because none
of the code in this file existed before the summer of 2007.

http://codereview.chromium.org/1128008/diff/29001/30002#newcode33
net/base/x509_cert_types.h:33: class X509Certificate;
Nit: don't indent the contents of a namespace.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Namespace_Form...

http://codereview.chromium.org/1128008/diff/29001/30002#newcode50
net/base/x509_cert_types.h:50: // Principal represent an X.509 principal.
Nit: let's change this comment to:

  // CertPrincipal represents the issuer or subject field of an X.509
certificate.

http://codereview.chromium.org/1128008/diff/29001/30002#newcode55
net/base/x509_cert_types.h:55: // Parses a BER-format X.509 DistinguishedName.
Nit: remove "X.509" because DistinguishedName comes from X.500.

Similarly remove "x509_" from the input parameter's name.

http://codereview.chromium.org/1128008/diff/29001/30002#newcode64
net/base/x509_cert_types.h:64: // corresponding components of the receiver,
where "match" is defined
Nit: receiver => object?

I still think we should just do a binary comparison, because
our CertPrincipal type does not have all the possible members.
RFC 5280 page 21 lists the following attribute types:

      * country,
      * organization,
      * organizational unit,
      * distinguished name qualifier,
      * state or province name,
      * common name (e.g., "Susan Housley"), and
      * serial number.
and
      * locality,
      * title,
      * surname,
      * given name,
      * initials,
      * pseudonym, and
      * generation qualifier (e.g., "Jr.", "3rd", or "IV").

Some of them don't apply to CA certificates, but you can
see that our CertPrincipal type is missing distinguished
name qualifier and serial number.

http://codereview.chromium.org/1128008/diff/29001/30002#newcode82
net/base/x509_cert_types.h:82: // Writes a human-readable description of a
Principal, for debugging.
Nit: Principal => CertPrincipal

http://codereview.chromium.org/1128008/diff/29001/30003
File net/base/x509_cert_types_mac.cc (right):

http://codereview.chromium.org/1128008/diff/29001/30003#newcode25
net/base/x509_cert_types_mac.cc:25: &CSSMOID_DNQualifier };   // This should be
"DC" but is undoubtedly wrong.
Nit: put the closing curly brace and semicolon on the next line.

Make the same change to line 65 and line 178 below.

http://codereview.chromium.org/1128008/diff/29001/30003#newcode29
net/base/x509_cert_types_mac.cc:29: static std::string DataToString(CSSM_DATA
data);
Nit: CSSM_DATA probably should be passed as a const reference.
Same for the next function.

http://codereview.chromium.org/1128008/diff/29001/30003#newcode46
net/base/x509_cert_types_mac.cc:46: static void AddPair(const CSSM_OID type,
Nit: should CSSM_OID be passed as a const reference?

It's not clear what "Pair" means.  AttributeTypeAndValue?
I suggest you rename this function "AddTypeValuePair".

http://codereview.chromium.org/1128008/diff/29001/30003#newcode142
net/base/x509_cert_types_mac.cc:142: KeyValuePairs** pairsList;
Nit: pairsList => pairs_list

http://codereview.chromium.org/1128008/diff/29001/30003#newcode198
net/base/x509_cert_types_mac.cc:198: pair->value.Length / sizeof(char16),
Nit: indentation

The next case has the same indentation problem.

http://codereview.chromium.org/1128008/diff/29001/30003#newcode214
net/base/x509_cert_types_mac.cc:214: // Displaying the string may not work, but
at least it can be compared
This comment is inaccurate because if the attribute type is
unknown to us, the AddPair call won't add the pair to |values|,
and this blob won't be used by Matches().

Actually, all the cases of this switch statement have this
potential problem if the attribute type is unknown to us.

http://codereview.chromium.org/1128008/diff/29001/30004
File net/base/x509_cert_types_unittest.cc (right):

http://codereview.chromium.org/1128008/diff/29001/30004#newcode120
net/base/x509_cert_types_unittest.cc:120: //107:d=3  hl=2 l=  84 prim:   
UTF8STRING        :TÜRKTRUST Bilgi İletişim ve Bilişim Güvenliği Hizmetleri A.Ş.
(c) Kasım 2005
IMPORTANT: we may need to avoid non-ASCII characters in our
source code.  Could you please ask jshin if this is an issue?

http://codereview.chromium.org/1128008/diff/29001/30007
File net/base/x509_certificate_mac.cc (right):

http://codereview.chromium.org/1128008/diff/29001/30007#newcode331
net/base/x509_certificate_mac.cc:331: OSStatus CopyCertChain(SecCertificateRef
cert_handle,
Nit: CopyCertChain also copies the entire cert chain,
including cert_handle itself, but this comment says only the
intermediate and root certs are copied.

Typo: out_trust_chain => out_cert_chain

http://codereview.chromium.org/1128008/diff/29001/30007#newcode719
net/base/x509_certificate_mac.cc:719: CFArrayRef trust_chain = NULL;
Nit: trust_chain => cert_chain
     scoped_trust_chain => scoped_cert_chain

http://codereview.chromium.org/1128008/diff/29001/30007#newcode729
net/base/x509_certificate_mac.cc:729: SecCertificateRef intermediate_handle =
reinterpret_cast<SecCertificateRef>(
Nit: "intermediate" is misleading, because this can also be
an end-entity client cert or a root cert.

We can probably start with i = 1 because I think i = 0 is the
end-entity client cert.

http://codereview.chromium.org/1128008/diff/29001/30007#newcode732
net/base/x509_certificate_mac.cc:732: X509Certificate* intermediate =
X509Certificate::CreateFromHandle(
X509Certificate is reference counted, so always store it in
a scoped_refptr.  Otherwise the reference is leaked.

http://codereview.chromium.org/1128008/diff/29001/30007#newcode736
net/base/x509_certificate_mac.cc:736: for (unsigned j = 0; j <
valid_issuers.size(); j++)
Nit: use curly braces around the for loop's body, but not
around the if statement's one-liner body.

http://codereview.chromium.org/1128008/diff/29001/30007#newcode771
net/base/x509_certificate_mac.cc:771: NULL,
IMPORTANT: we can finally pass a real validIssuers
argument to this SecIdentityCopyPreference call now.

http://codereview.chromium.org/1128008/diff/29001/30007#newcode848
net/base/x509_certificate_mac.cc:848: CFArrayRef trust_chain = NULL;
Nit: trust_chain => cert_chain

http://codereview.chromium.org/1128008/diff/29001/30009
File net/socket/ssl_client_socket_mac.cc (right):

http://codereview.chromium.org/1128008/diff/29001/30009#newcode665
net/socket/ssl_client_socket_mac.cc:665: int n =
CFArrayGetCount(valid_issuer_names);
Nit: move this line up so that in the SSL_LOG statement you
can just use |n|.

http://codereview.chromium.org/1128008/diff/29001/30009#newcode667
net/socket/ssl_client_socket_mac.cc:667: // Parse each name into a Principal
object.
Nit: Principal => CertPrincipal.

http://codereview.chromium.org/1128008/diff/29001/30009#newcode679
net/socket/ssl_client_socket_mac.cc:679: // Now get the available client certs
that match.
Nit: match => are issued by the issuers allowed by the server

[wtc, 2010-03-25 00:07:59]

I just glanced over rsleevi's earlier comments on this CL.
rsleevi, your understanding of PKI and its implementations
in CryptoAPI and Mac seems to exceed mine significantly :-)

Re: the switchover to NSS: I'm planning to use NSS for SSL
only, and continue to use the native crypto library for
certificate verification.  If you know of serious limitations
of Apple's certificate library that's difficult for us to
work around, that may change my plans.

http://codereview.chromium.org/1128008/diff/13001/14006
File net/base/x509_types_mac.cc (right):

http://codereview.chromium.org/1128008/diff/13001/14006#newcode206
net/base/x509_types_mac.cc:206: DCHECK_EQ(pair->value_type,
KeyValuePair::kTypeOther);
On 2010/03/23 20:19:57, Jens Alfke wrote:
> On 2010/03/23 01:23:30, rsleevi wrote:
> >  Would it be better to simply AddPair of the raw string data, so that they
end
> up
> > in a bit-for-bit comparison during X509Principal::Match ?
> 
> Good point. I'll change it to do that.

I'm glad that rsleevi also pointed out this issue.

As I noted in my code review, I'm afraid that the AddPair
call is a no-op in this case.

http://codereview.chromium.org/1128008/diff/29001/30003
File net/base/x509_cert_types_mac.cc (right):

http://codereview.chromium.org/1128008/diff/29001/30003#newcode272
net/base/x509_cert_types_mac.cc:272: if (CSSMOIDEqual(&type, kOIDs[oid])) {
This check means any pair whose type is not in our kOIDs
array will be dropped by the AddPair function.

[wtc, 2010-03-25 01:00:35]

http://codereview.chromium.org/1128008/diff/29001/30007
File net/base/x509_certificate_mac.cc (right):

http://codereview.chromium.org/1128008/diff/29001/30007#newcode771
net/base/x509_certificate_mac.cc:771: NULL,
On 2010/03/24 23:52:05, wtc wrote:
> IMPORTANT: we can finally pass a real validIssuers
> argument to this SecIdentityCopyPreference call now.

I looked at the source code of SecIdentityCopyPreference in
Mac OS X 10.6.2 (libsecurity_keychain-37184/lib/SecIdentity.cpp).
It ignores the validIssuers argument.  Sigh.

    // filter on valid issuers, if provided
    if (validIssuers) {
        //%%%TBI
    }

Could you add a comment to note this?  Thanks.

http://codereview.chromium.org/1128008/diff/29001/30009
File net/socket/ssl_client_socket_mac.cc (right):

http://codereview.chromium.org/1128008/diff/29001/30009#newcode661
net/socket/ssl_client_socket_mac.cc:661: if
(SSLCopyDistinguishedNames(ssl_context_, &valid_issuer_names) == noErr &&
Note: the server may also tell us what kind of client
certificate they accept in the certificate_types field of
the CertificateRequest message.

In Mac OS X 10.6, we can use the
SSLGetNumberOfClientAuthTypes and SSLGetClientAuthTypes
functions to get certificate_types, which can also be
used to filter the client certs.  Unfortunately these
two functions are declared in SecureTransportPriv.h.

In Mac OS X 10.5, Secure Transport sort of handles that
for us.  It supports only one type of client cert (rsa_sign),
so it will only do client auth if the server's certificate_types
includes rsa_sign.

[rsleevi-old, 2010-03-25 05:34:10]

http://codereview.chromium.org/1128008/diff/29001/30001
File net/base/x509_cert_types.cc (right):

http://codereview.chromium.org/1128008/diff/29001/30001#newcode49
net/base/x509_cert_types.cc:49: std::ostream& operator<<(std::ostream& s, const
CertPrincipal& p) {
On 2010/03/24 23:52:05, wtc wrote:
> I seem to recall domainComponent is printed as "dc=".
> 
> It would be nice to use standard abbreviations of the
> attribute types, but I don't know where they're defined.

Historically, RFC1485 [1] established IANA as the register. That role was
deprecated by RFC4520 [2], by means of referencing the procedures/values
established in RFC 4514 [3] . 

By reference in RFC4514, additional name abbreviations can be found in RFC4519.

The use of the LDAPv3 RFCs as the source for generating a displayable string
from a given distinguishedName is not specified in 5280, but the inclusion of
RFC4518 in Section 7.1. does incorporate some of the string processing rules, as
a means of addressing the internationalization requirements.

To be fair, ITU-R X.520 [5] may also be considered as the authoritative source
of names, by means of the fact that RFC5280 builds on ITU-R X.509. However, as
noted in RFC5280 Section 7.1, the rules for string preparation provided by X.520
are insufficient for the needs of internationalization.

The NSS implementation can be found at [6]

[1] http://tools.ietf.org/html/rfc1485
[2] http://tools.ietf.org/html/rfc4520
[3] http://tools.ietf.org/html/rfc4514
[4] http://tools.ietf.org/html/rfc4519
[5] http://www.itu.int/rec/recommendation.asp?lang=en&parent=T-REC-X.520
[6] http://mxr.mozilla.org/mozilla/source/security/nss/lib/certdb/alg1485.c#57

[wtc, 2010-03-25 07:07:45]

rsleevi: thanks for the info.

I copied this table from RFC 4514:

      String  X.500 AttributeType
      ------  --------------------------------------------
      CN      commonName (2.5.4.3)
      L       localityName (2.5.4.7)
      ST      stateOrProvinceName (2.5.4.8)
      O       organizationName (2.5.4.10)
      OU      organizationalUnitName (2.5.4.11)
      C       countryName (2.5.4.6)
      STREET  streetAddress (2.5.4.9)
      DC      domainComponent (0.9.2342.19200300.100.1.25)
      UID     userId (0.9.2342.19200300.100.1.1)

I should add "STREET" to NSS's table.

[Jens Alfke, 2010-03-26 17:15:04]

Comments below. Except as noted, I've made all suggested changes. I'll be
checking in shortly.

http://codereview.chromium.org/1128008/diff/29001/30003
File net/base/x509_cert_types_mac.cc (right):

http://codereview.chromium.org/1128008/diff/29001/30003#newcode29
net/base/x509_cert_types_mac.cc:29: static std::string DataToString(CSSM_DATA
data);
On 2010/03/24 23:52:05, wtc wrote:
> Nit: CSSM_DATA probably should be passed as a const reference.
> Same for the next function.

My opinion (and Apple's, in APIs like AppKit and CoreGraphics) is that it's more
efficient to pass small structs by value. In this case it's just like passing
one extra parameter, and saves dereferencing of the struct members every time
they're used in the callee.

http://codereview.chromium.org/1128008/diff/29001/30003#newcode214
net/base/x509_cert_types_mac.cc:214: // Displaying the string may not work, but
at least it can be compared
On 2010/03/24 23:52:05, wtc wrote:
> This comment is inaccurate because if the attribute type is
> unknown to us, the AddPair call won't add the pair to |values|

This comment is about the _data_ type, not the attribute type. Two different
things. I'll change the comment to explicitly say "data type".

http://codereview.chromium.org/1128008/diff/29001/30003#newcode272
net/base/x509_cert_types_mac.cc:272: if (CSSMOIDEqual(&type, kOIDs[oid])) {
On 2010/03/25 00:07:59, wtc wrote:
> This check means any pair whose type is not in our kOIDs
> array will be dropped by the AddPair function.

Yup. That's because Principal only stores a specific set of attributes that it
has instance variables for. Changing that would require rewriting Principal as a
general-purpose dictionary/map structure, or at least adding an
"other_attributes" map to it.

http://codereview.chromium.org/1128008/diff/29001/30004
File net/base/x509_cert_types_unittest.cc (right):

http://codereview.chromium.org/1128008/diff/29001/30004#newcode120
net/base/x509_cert_types_unittest.cc:120: //107:d=3  hl=2 l=  84 prim:   
UTF8STRING        :TÜRKTRUST Bilgi İletişim ve Bilişim Güvenliği Hizmetleri A.Ş.
(c) Kasım 2005
On 2010/03/24 23:52:05, wtc wrote:
> IMPORTANT: we may need to avoid non-ASCII characters in our
> source code.  Could you please ask jshin if this is an issue?

OK. Sent an email at 5PM yesterday. Waiting to hear back.

http://codereview.chromium.org/1128008/diff/29001/30007
File net/base/x509_certificate_mac.cc (right):

http://codereview.chromium.org/1128008/diff/29001/30007#newcode729
net/base/x509_certificate_mac.cc:729: SecCertificateRef intermediate_handle =
reinterpret_cast<SecCertificateRef>(
On 2010/03/24 23:52:05, wtc wrote:
> We can probably start with i = 1 because I think i = 0 is the
> end-entity client cert.

It is, but we have to check the issuer of that cert.

[wtc, 2010-03-26 21:03:22]

snej: Thanks for the reply!

http://codereview.chromium.org/1128008/diff/29001/30003
File net/base/x509_cert_types_mac.cc (right):

http://codereview.chromium.org/1128008/diff/29001/30003#newcode214
net/base/x509_cert_types_mac.cc:214: // Displaying the string may not work, but
at least it can be compared
On 2010/03/26 17:15:04, Jens Alfke wrote:
>
> This comment is about the _data_ type, not the attribute type. Two different
> things. I'll change the comment to explicitly say "data type".

You're right.  Sorry about the confusion!

http://codereview.chromium.org/1128008/diff/29001/30003#newcode272
net/base/x509_cert_types_mac.cc:272: if (CSSMOIDEqual(&type, kOIDs[oid])) {
On 2010/03/26 17:15:04, Jens Alfke wrote:
>
> Yup. That's because Principal only stores a specific set of attributes that it
> has instance variables for. Changing that would require rewriting Principal as
a
> general-purpose dictionary/map structure, or at least adding an
> "other_attributes" map to it.

Yes.  My point is that our Matches() method is using
CertPrincipal for proper DistinguishedName comparison,
so attribute types not in the kOIDs array are ignored.
Given this limitation of our Matches() method, we might as
well just do a binary comparison of DistinguishedNames,
which works in practice.

But I do want to note that our Matches() method is fine for
what we use it for (to look for a client cert issued by an
issuer on the server's valie issuers list), so you can check
this in.

http://codereview.chromium.org/1128008/diff/29001/30007
File net/base/x509_certificate_mac.cc (right):

http://codereview.chromium.org/1128008/diff/29001/30007#newcode729
net/base/x509_certificate_mac.cc:729: SecCertificateRef intermediate_handle =
reinterpret_cast<SecCertificateRef>(
On 2010/03/26 17:15:04, Jens Alfke wrote:
>
> It is, but we have to check the issuer of that cert.

In each iteration, you are checking intermediate->subject()
(at line 737 below).  Did you mean to check
intermediate->issuer()?

Nit: since the cert at index i=0 is not an intermediate
cert, the variable names |intermediate_handle| and
|intermediate| are inaccurate.  Perhaps rename them
|cert_handle| and |cert|?