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

[Nalin Dahyabhai]

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.

> * 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.

> * 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.

> * 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.

> * 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.

> I will give this a closer review for obvious logic errors and will also
> build the NSS trunk and run this through our test suite (which minimally
> exercises PKINIT).  I probably won't be able to give it a terribly
> thorough functional review since there are still a lot of gaps in my
> understanding of the primitives used by PKINIT.  But it sounds like
> you've put this through some pretty careful testing, and it's opt-in
> code, so I don't consider that an impediment to committing it.

Thanks, I'm glad for any feedback you can provide.

Nalin