Cleanup X509CertificateMac style nits

Cleanup X509CertificateMac style nits and introduce a crypto library agnostic means of parsing certificate dates.

In the process of addressing style issues, a bug was found in the date/time parsing routines used on Mac. Though not currently hit, it could lead to wild misrepresentation of the validity period for a certificate. Switch to using the routine used when using OpenSSL, which does things correctly.

BUG=none
TEST=existing

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

[Ryan Sleevi, 2010-11-08 01:50:12]

joth/bulach: Would you be willing to take a look at this? I've repurposed the
OpenSSL date/time parsing into a generic method in x509_cert_types, as the
X509CertificateMac also had need for string parsing, and was doing it wrong.

This is primarily a style-nit cleanup, and given that wtc is swamped, I was
hoping one of you might be willing to check for Style Guide compliance.

wtc: fyi

[bulach, 2010-11-08 14:20:07]

LGTM

nice, great to see code being reused and fixing bugs at the same time (not to
mention the style!)

a few nits below:

http://codereview.chromium.org/4653002/diff/1/2
File net/base/x509_cert_types.cc (right):

http://codereview.chromium.org/4653002/diff/1/2#newcode180
net/base/x509_cert_types.cc:180: NOTREACHED() << "Can't parse X.509 date " <<
raw_date;
since this method is now part of an API, you may want to remove the "NOTREACHED"
and just keep:
if (!valid)
  return false;

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

http://codereview.chromium.org/4653002/diff/1/3#newcode154
net/base/x509_cert_types.h:154: CERT_DATE_FORMAT_GENERALIZED_TIME
append a ,

http://codereview.chromium.org/4653002/diff/1/5
File net/base/x509_certificate_mac.cc (left):

http://codereview.chromium.org/4653002/diff/1/5#oldcode995
net/base/x509_certificate_mac.cc:995: goto exit;
nice! :)

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

http://codereview.chromium.org/4653002/diff/1/5#newcode273
net/base/x509_certificate_mac.cc:273: return;
previously, it'd break on the first oid field it found. I don't think there'd be
more than one, but it may be nice to just:

ParseCertificateDate(time_string, format, result);
break;

http://codereview.chromium.org/4653002/diff/1/5#newcode325
net/base/x509_certificate_mac.cc:325: NULL, const_cast<const
void**>(reinterpret_cast<void**>(&cert_handle)),
does reinterpret_cast<const void**> work?

http://codereview.chromium.org/4653002/diff/1/5#newcode844
net/base/x509_certificate_mac.cc:844: if (result)
maybe keep as != noError since it's sort of opaque datatype.

http://codereview.chromium.org/4653002/diff/1/5#newcode890
net/base/x509_certificate_mac.cc:890: // validIssuers is not unfortunately
not-yet implemented on OS X.
:)
s/not unfortunately/unfortunately/

but tbh, I think the previous style is clearer as it relates directly to the
specific param..

[Ryan Sleevi, 2010-11-09 00:34:20]

Thanks for looking at this bulach. I've uploaded a new set addressing the things
you pointed out.

One thing I wasn't sure of was whether it was necessary/preferred from an
API-design to expose CertificateDateFormat. For X.509, only the two date formats
have ever been supported. From an ASN.1 layer, there are only two universal
types for dates.

Given that X.509 has always been explicit about the exact encoding form of these
dates, ergo has always been explicit about their length, I'm wondering whether
or not it's acceptable to drop CertificateDateFormat entirely, and just look at
the length of the string: 13 chars vs 15.

On the Pro side, it's one less parameter/symbol, and in the history of X.509,
hasn't changed.

On the Con side, it's not directly obvious at the callsite (though the
documentation of the function could make this fairly clear)

Thoughts?

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

http://codereview.chromium.org/4653002/diff/1/5#newcode844
net/base/x509_certificate_mac.cc:844: if (result)
On 2010/11/08 14:20:07, bulach wrote:
> maybe keep as != noError since it's sort of opaque datatype.

That's what I thought at first, but in previous reviews wtc raised it as a style
issue for Chromium-code, with this style preferred over checking != noErr.

http://codereview.chromium.org/4653002/diff/1/5#newcode890
net/base/x509_certificate_mac.cc:890: // validIssuers is not unfortunately
not-yet implemented on OS X.
On 2010/11/08 14:20:07, bulach wrote:
> :)
> s/not unfortunately/unfortunately/
> 
> but tbh, I think the previous style is clearer as it relates directly to the
> specific param..

Good catch. I've expanded the comment more, trying to match my original intent
of providing a bit more context to anyone else reading the code about why this
was a pain to track down originally :)

[bulach, 2010-11-09 14:08:30]

thanks Ryan! feel free to land once the trybots are green, reply inline:

On 2010/11/09 00:34:20, rsleevi wrote:
> Thanks for looking at this bulach. I've uploaded a new set addressing the
things
> you pointed out.
> 
> One thing I wasn't sure of was whether it was necessary/preferred from an
> API-design to expose CertificateDateFormat. For X.509, only the two date
formats
> have ever been supported. From an ASN.1 layer, there are only two universal
> types for dates.
> 
> Given that X.509 has always been explicit about the exact encoding form of
these
> dates, ergo has always been explicit about their length, I'm wondering whether
> or not it's acceptable to drop CertificateDateFormat entirely, and just look
at
> the length of the string: 13 chars vs 15.
> 
> On the Pro side, it's one less parameter/symbol, and in the history of X.509,
> hasn't changed.
> 
> On the Con side, it's not directly obvious at the callsite (though the
> documentation of the function could make this fairly clear)
> 
> Thoughts?

hmm...
if we remove the param, there's a (hypothetical?) scenario that we might
successfully parse something that we shouldn't, i.e., the certificate says is
type UTC but in fact it contains generalized (or vice-versa), and once we pass
the string (caller wouldn't check the length...), the parse routine would
happily parse.

I think this is not a real concern, but I guess keeping it strict / explicit
with the param doesn't hurt? it feels more correct, but I'm happy either way.


> 
> http://codereview.chromium.org/4653002/diff/1/5
> File net/base/x509_certificate_mac.cc (right):
> 
> http://codereview.chromium.org/4653002/diff/1/5#newcode844
> net/base/x509_certificate_mac.cc:844: if (result)
> On 2010/11/08 14:20:07, bulach wrote:
> > maybe keep as != noError since it's sort of opaque datatype.
> 
> That's what I thought at first, but in previous reviews wtc raised it as a
style
> issue for Chromium-code, with this style preferred over checking != noErr.
> 
> http://codereview.chromium.org/4653002/diff/1/5#newcode890
> net/base/x509_certificate_mac.cc:890: // validIssuers is not unfortunately
> not-yet implemented on OS X.
> On 2010/11/08 14:20:07, bulach wrote:
> > :)
> > s/not unfortunately/unfortunately/
> > 
> > but tbh, I think the previous style is clearer as it relates directly to the
> > specific param..
> 
> Good catch. I've expanded the comment more, trying to match my original intent
> of providing a bit more context to anyone else reading the code about why this
> was a pain to track down originally :)

[wtc, 2010-11-11 01:33:52]

rsleevi: as requested, I did NOT review this CL.  Just
two drive-by nit-picking comments.

http://codereview.chromium.org/4653002/diff/7001/net/base/x509_cert_types.h
File net/base/x509_cert_types.h (right):

http://codereview.chromium.org/4653002/diff/7001/net/base/x509_cert_types.h#n...
net/base/x509_cert_types.h:149: enum CertificateDateFormat {
Nit: probably should rename this enum CertDataFormat for
consistency with CERT_DATE_FORMAT_xxx.  Not sure what our
convention is.

http://codereview.chromium.org/4653002/diff/7001/net/base/x509_openssl_util.cc
File net/base/x509_openssl_util.cc (right):

http://codereview.chromium.org/4653002/diff/7001/net/base/x509_openssl_util.c...
net/base/x509_openssl_util.cc:67: : CERT_DATE_FORMAT_GENERALIZED_TIME;
Nit: we have a convention to break an expression at an operator.
So it would be better to format this statement as:
  CertificateDateFormat format = x509_time->type == V_ASN1_UTCTIME ?
      CERT_DATE_FORMAT_UTC_TIME : CERT_DATE_FORMAT_GENERALIZED_TIME;