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

Greg Hudson ghudson at MIT.EDU
Thu Sep 8 14:16:41 EDT 2011


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.

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

* If pkinit_crypto_nss.c doesn't need anything from k5-int.h, use
k5-platform.h instead.

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

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





More information about the krbdev mailing list