Bring X.509 cert handling (at least preliminarily) to the Mac.

Bring X.509 cert handling (at least preliminarily) to the Mac.


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

[Avi (use Gerrit), 2008-09-18 15:25:57]

For your review.

BTW, since the mac code is a variant of the win code, I svn cp-ed it. Also, the
move from x509_certificate.cc to x509_certificate_win.cc is in svn, but the
uploader doesn't seem to want to upload it.

[Amanda Walker, 2008-09-18 16:14:43]

General comment: since we're moving files around, can you update the SCons build
as well?  We should get into that habit...

http://codereview.chromium.org/2963/diff/206/11
File net/base/x509_certificate.h (right):

http://codereview.chromium.org/2963/diff/206/11#newcode136
Line 136: #if defined(OS_WIN)
Since you wrote code to extract individual fields from the cert, we could
implement these methods directly.  If they'd be unused, add a comment here
explaining why they're only necessary on Windows

http://codereview.chromium.org/2963/diff/206/11#newcode155
Line 155: #if defined(OS_WIN)
Same comment as above: we should either implement or explain why it's
windows-only here.

http://codereview.chromium.org/2963/diff/206/11#newcode186
Line 186: #endif
Add a declaration for the Mac version, mostly to keep it scoped to this class

http://codereview.chromium.org/2963/diff/206/12
File net/base/x509_certificate_mac.cc (right):

http://codereview.chromium.org/2963/diff/206/12#newcode75
Line 75: const CSSM_OID* kOIDs[8] = { &CSSMOID_CommonName,
suggest using [] instead of [8] so that we don't need to update the bounds if we
add additional elements later

http://codereview.chromium.org/2963/diff/206/12#newcode85
Line 85: // OID.
Add TODO

http://codereview.chromium.org/2963/diff/206/12#newcode87
Line 87: std::vector<std::string>* values[8] = {
suggest using [] instead of [8] so that we don't need to update the bounds if we
add additional elements later

http://codereview.chromium.org/2963/diff/206/12#newcode182
Line 182: // A thread-safe cache for X509Certificate objects.
Can we move this into a header file, since it has no platform dependencies?

http://codereview.chromium.org/2963/diff/206/12#newcode356
Line 356: X509Certificate::Policy::Judgment X509Certificate::Policy::Check(
If X509Certificate::Policy has no platform dependencies, can we break it out
into its own .cc file?

[Avi (use Gerrit), 2008-09-18 18:37:40]

All fixed except for the refactoring. I want to land this first so that I can
build some unit tests.

[Amanda Walker, 2008-09-18 18:43:25]

LGTM

[wtc, 2008-09-18 20:37:55]

I have some comments and suggested changes below.
I'll be happy to help add unit tests and implement
some of the missing code.

The amount of CSSM code is less than I feared.  Hope
it'll stay this way.

http://codereview.chromium.org/2963/diff/206/11
File net/base/x509_certificate.h (right):

http://codereview.chromium.org/2963/diff/206/11#newcode136
Line 136: #if defined(OS_WIN)
I believe the valid_start() and valid_expiry() methods
are being used.

http://codereview.chromium.org/2963/diff/206/12
File net/base/x509_certificate_mac.cc (right):

http://codereview.chromium.org/2963/diff/206/12#newcode43
Line 43: // The Windows code uses CryptHashCertificate, a function specially
designed
I believe CryptHashCertificate is just a convenience
function like Mac's CC_SHA1, which hashes something in
one shot.  To use Windows CryptoAPI directly, we'd need
several function calls to hash something.

So you can remove this block comment.

http://codereview.chromium.org/2963/diff/206/12#newcode64
Line 64: inline bool CSSMOIDEqual(const CSSM_OID* oid1, const CSSM_OID* oid2) {
I'm a little surprised that the Security Framework doesn't
provide this function.  Perhaps because it's so trivial.

http://codereview.chromium.org/2963/diff/206/12#newcode82
Line 82: &CSSMOID_DNQualifier };  // This should be "DC"
"DN" most likely stands for Distinguished Name, so this
is probably the wrong OID.  We may not need to parse the
DC attribute.

http://codereview.chromium.org/2963/diff/206/12#newcode127
Line 127: void GetCertFieldsForOID(X509Certificate::OSCertHandle cert_handle,
Use "Field", singular.  You're returning only one
field because of the 'break' statement in the for
loop.

Since only one value is returned, can we return the
results in a std::string instead of a vector?

http://codereview.chromium.org/2963/diff/206/12#newcode135
Line 135: CSSM_CL_HANDLE clHandle;
clHandle doesn't follow the naming convention in the
Coding Style.

http://codereview.chromium.org/2963/diff/206/12#newcode140
Line 140: uint32 numOfFields;
numOfFields doesn't follow the naming convention in the
Coding Style.

http://codereview.chromium.org/2963/diff/206/12#newcode336
Line 336: GetCertFieldsForOID(cert_handle_, CSSMOID_SubjectAltName, dns_names);
Add a TODO comment to note that we still need to parse
the subjectAltName field to get the DNS names.

[Avi (use Gerrit), 2008-09-18 20:48:17]

comments; code in a different CL.

http://codereview.chromium.org/2963/diff/206/11
File net/base/x509_certificate.h (right):

http://codereview.chromium.org/2963/diff/206/11#newcode136
Line 136: #if defined(OS_WIN)
On 2008/09/18 20:37:55, wtc wrote:
> I believe the valid_start() and valid_expiry() methods
> are being used.

No, they are not. valid_start() isn't used anywhere, and valid_expiry() is only
used in HasExpired(), which is only used to display an error message.

http://codereview.chromium.org/2963/diff/206/12
File net/base/x509_certificate_mac.cc (right):

http://codereview.chromium.org/2963/diff/206/12#newcode43
Line 43: // The Windows code uses CryptHashCertificate, a function specially
designed
On 2008/09/18 20:37:55, wtc wrote:
> To use Windows CryptoAPI directly, we'd need
> several function calls to hash something.
> 
> So you can remove this block comment.

Nice, thanks.

http://codereview.chromium.org/2963/diff/206/12#newcode64
Line 64: inline bool CSSMOIDEqual(const CSSM_OID* oid1, const CSSM_OID* oid2) {
On 2008/09/18 20:37:55, wtc wrote:
> I'm a little surprised that the Security Framework doesn't
> provide this function.  Perhaps because it's so trivial.

Me too, but I looked...

http://codereview.chromium.org/2963/diff/206/12#newcode127
Line 127: void GetCertFieldsForOID(X509Certificate::OSCertHandle cert_handle,
The break is a mistake on my part, copied from the Windows where the loop is
different. Oops.

http://codereview.chromium.org/2963/diff/206/12#newcode336
Line 336: GetCertFieldsForOID(cert_handle_, CSSMOID_SubjectAltName, dns_names);
On 2008/09/18 20:37:55, wtc wrote:
> Add a TODO comment to note that we still need to parse
> the subjectAltName field to get the DNS names.

OK; this is me coding not quite sure what needs to go here, so this looked good.
When I make the unit test, can you make sure this function is tested so I can
fix the Mac version?

[wtc, 2008-09-18 21:08:14]

On 2008/09/18 20:48:17, Avi wrote:
>
> http://codereview.chromium.org/2963/diff/206/12#newcode336
> Line 336: GetCertFieldsForOID(cert_handle_, CSSMOID_SubjectAltName,
dns_names);
> On 2008/09/18 20:37:55, wtc wrote:
> > Add a TODO comment to note that we still need to parse
> > the subjectAltName field to get the DNS names.
> 
> OK; this is me coding not quite sure what needs to go here, so this looked
good.
> When I make the unit test, can you make sure this function is tested so I can
> fix the Mac version?

Definitely!