NSS for PKINIT, in-progress patches available, feedback sought

[Nalin Dahyabhai]

On Thu, Sep 08, 2011 at 06:02:36PM -0400, Nalin Dahyabhai wrote:
> On Thu, Sep 08, 2011 at 02:16:41PM -0400, Greg Hudson wrote:
> > Some quick reactions:
> > 
> > * The configure.in error message says 3.12.11 is required, but the code
> > only checks for 3.12.10 or later.
> 
> I was dithering on what version number to use there because 3.12.11
> isn't released yet -- it should consistently ask for 3.12.11, for now at
> least.

This should be fixed now.

> > * In Makefile.in, avoid using direct @ substitutions inside variable
> > expansions; define intermediate variables like PKINIT_CRYPTO_IMPL_LIBS.
> > This allows more flexibility when building, and also means fewer changes
> > if the code ever gets to be built on Windows.
> 
> Ok, fixing this.  The variables will go into pre.in, with others.

This should be fixed now.

> > * If pkinit_crypto_nss.c doesn't need anything from k5-int.h, use
> > k5-platform.h instead.
> 
> Yup, looks like you're right there, too.  Fixing.

Fixed now.

> > * Our language platform assumption is C89, not C99.  C99 features found
> > in MSVC (like variadic macros) are probably okay, although we haven't
> > formally decided that.  Other C99 features (like named structure field
> > initializers) will be an impediment to using this code on Windows--which
> > seems a bit of a shame as I think NSS has better Windows support than
> > OpenSSL.
> 
> That's a shame, I really like the field initializers.  Removing them.

Done.

> > * Style conformance generally looks good, with a few exceptions:
> > 
> >   - There are some >79-character lines.  Some are unavoidable because
> > NSS uses absurdly long names; others can be reasonably broken up.
> > 
> >   - (*fptr) (args) should be (*fptr)(args).
> >
> >   - We normally put a blank line after variable declarations.  This
> > doesn't seem to be a written rule, though, so I suppose addressing this
> > is optional.
> > 
> >   - We often omit braces around single-line flow control bodies.  Also
> > not a written rule, and I know opinions vary on this, so addressing this
> > is even more optional.
> 
> I've taken a pass through it and tried to clean these up some.

If there are any deviations in there still, I can take another pass, but
I think it's sorted.

NSS uses OCSP when available, so in order to honor the
require_crl_checking option, when the option is enabled, the routines
which verify CMS SignedData items now check certificates in the
certifying chain directly for either a CRL issued by an issuer or an
AuthInfoAccess extension which specifies an OCSP responder.

Patches are rebased against trunk (as of yesterday), and the agility KDF
is now implemented.  The bits that construct the OtherInfo blob are
mostly culled from pkinit_crypto_openssl.c.

I'd appreciate any additional feedback.

Thanks,

Nalin