I need an NSS tool that can dump the DER encoded issuer name as base64 and the serial number as base64, suitable as input for CERT_FindCertByIssuerAndSN.
The intention is to:
- embed the unique identifiers of root certs into PSM source code
- have PSM call FindCertByIssuerAndSN at runtime to find the full cert
- use found root(s) as parameters for libpkix chain building
The tool should also print the fingerprints for embedding in the PSM source code, so the search result can be tested for matching the expected cert.
At the same time, the tool should print some human readable ID strings that can be added as identifying comments to the PSM sources.

Comment on attachment 308528[details][diff][review]
Patch v1
I have a bunch of small issues with this patch. The comments are
large, probably larger than the required changes. :)
When you proposed to hack pp for this, I assumed you meant to produce a
program for your own private use, so I didn't disucss numerous other
subjects that I would have if I knew you wanted to actually extend the
program we ship.
1. This patch creates some new functions whose names differ from existing
function names only by appending a single 'W'. The use of a 'W' suffix
is part of a coding ocnvention that very well known to many programmers.
It signifies that the strings involved are "Wide" character strings
(Unicode UTF16 characters). Since that's not what the 'W' suffix means
here, please find a different suffix to mean "with line wrapping explicitly
specified". I do appreciate that 'W' was terse, and hope you can find
another suffix that's not much longer.
2. This patch wants to offer a set of new functions that are minor variants
on existing functions, omitting some of the functionality of the existing
functions. For some functions that previously always output data that was
line-wrapped at ~80 columns, this patch wishes to offer a form in which
line wrapping is optional and explicitly controlled through an additional
argument. For some other functions that output collections of things (such
as signed data AND the signature on that data), this function wants to
offer a variant that can suppress the output of the signature.
The functions to be extended in that manner include:
SECU_PrintAsHex
SECU_PrintInteger
SECU_PrintName
SECU_PrintSignedData
secu_PrintRawString (a static function)
Rather than extending the function signatures of each of those functions in
about the same way, this patch extends them in several different ways.
The number of new functions seems to have grown unnecessarily large.
For SECU_PrintAsHex, this patch adds SECU_PrintAsHexW, which has one extra
argument, and reimplements SECU_PrintAsHex as a call to that new function.
Only one new function is exported, the one with the additional argument.
SECU_PrintInteger and SECU_PrintName were extended in the same way, with
one new function with one additional argument. I like that.
SECU_PrintSignedData was extended by adding two new functions.
a) SECU_PrintSignedDataSigOpt adds one new argument, must like the way in
which the 3 functions named above were extended.
b) SECU_PrintSignedContent has the same signature (except for the name)
as SECU_PrintSignedData, and it invokes SECU_PrintSignedDataSigOpt with
the opposite value of the one used by the new implementation of
SECU_PrintSignedData.
- I would say that it is not necessary to expose both of those two new
APIs. I would prefer that only one new variant of SECU_PrintSignedData
be offered by this library. I think SECU_PrintSignedDataSigOpt suffices,
and most closely resembles the other additions described above.
Three new alternatives have been added to secu_PrintRawString. All 4 of
them are static. secu_PrintRawStringWithWrap is just a synonym for
secu_PrintRawString, identical in every way except the name.
secu_PrintRawStringNoWrap has the same signature as the first two, but
does something very different.
secu_PrintRawStringW has one more argument than secu_PrintRawString, and
calls either secu_PrintRawStringWithWrap or secu_PrintRawStringNoWrap
according to that additional argument.
- I'd prefer not to see the number of functions for this purpose explode.
- Given that these are all static functions, unknown outside of this
source file, there's really no need for ANY new functions. There's no
need to preserve the existing old function name, since all the references
to it are in this same source file. You can just rename the function and
drop the old name after changing all the other references in the file.
- You really could get by with just one function, the one with the extra
argument, and combine the other functions into it. Or you could just
have two functions, "WithWrap" and "NoWrap", and skip the separate
function that chooses one or the other.
Point 3. SECU_PrintDumpDerIssuerAndSerial causes two new strings to
be created and allocated, derIssuerB64 and derSerialB64.
- It appears to leak them.
- It also declares them in the middle of a basic block, rather than at
the beginning of the block. This c++ style is allowed by the latest c
language standard, but is not accepted by c compilers that conform to the
older c language standard. We've tried to keep this out of NSS so that
NSS will remain compatible with as many c compilers as possible. It
still builds with MSVC 5. :-)
Point 4. Let's not complicate the pp syntax with any new option letters.
This new code is yet another alternative to all the existing types --
such as "certificate", "certificate-request", "pkcs7", and "crl", and the
undocumented "private-key" and "public-key" -- and is mutually exclusive
with all of them. So, let's add it as a new "type" string, perhaps
"cert-id", or "issuer-serial".

Sorry, I have not yet had the time to address the review comments.
Besides all the stylistic changes to requested, I happen to find a real bug in the patch:
+ SECU_PrintNameW(out, &c->subject, "Subject", 0, 0);
+ SECU_PrintNameW(out, &c->subject, "Issuer", 0, 0);
The latter must be changed to c->issuer.
I will attach an updated patch, where the fix for this bug is the only change.
I'll work on the other requests later...

Russel, the attached patch is a work in progress. It has not yet been
submitted for review, and hence has not yet received a review. You should
not assume that any patch lacking a positive review is ready to be pushed.

Dear Nelson, thanks a lot for your thorough review in comment 3.
To be honest, I was surprised when I saw your big amount of change requests,
given that this isn't about APIs, but simply about a little helper tool.
2.5 years ago I didn't have the energy to discuss your point (2).
Now I'd like to make another attempt.
Let me respond to your easy comments first:
(1) the "W" suffix
I fully agree with your change request. I will use a "WrapOpt" suffix.
(3)
I fully agree, I will fix the leak and move the variable declaration up to where it belongs.
(4) avoiding new parameter -x
I fully agree with your change request.
I will change the patch, drop parameter -x,
and introduce a new type for parameter "-t" named "certificate-identity".
This is patch v2 which addresses the above points
(but not yet your point (2)).

(In reply to comment #3)
>
> 2. This patch wants to offer a set of new functions that are minor variants
> on existing functions, omitting some of the functionality of the existing
> functions. For some functions that previously always output data that was
> line-wrapped at ~80 columns, this patch wishes to offer a form in which
> line wrapping is optional and explicitly controlled through an additional
> argument. For some other functions that output collections of things (such
> as signed data AND the signature on that data), this function wants to
> offer a variant that can suppress the output of the signature.
>
> The functions to be extended in that manner include:
>
> SECU_PrintAsHex
> SECU_PrintInteger
> SECU_PrintName
> SECU_PrintSignedData
> secu_PrintRawString (a static function)
>
> Rather than extending the function signatures of each of those functions in
> about the same way, this patch extends them in several different ways.
> The number of new functions seems to have grown unnecessarily large.
I guess that's debatable.
> For SECU_PrintAsHex, this patch adds SECU_PrintAsHexW, which has one extra
> argument, and reimplements SECU_PrintAsHex as a call to that new function.
> Only one new function is exported,
I don't understand why you say "is exported".
The new function is not declared in the header file.
But you probably see "missing static" in the function definition equivalent
with "exported".
Maybe you made that assumption, because the function name starts with
an uppercase SECU_ prefix?
I'm fine to declare all such functions with a lowercase secu_ prefix,
to make it clear they are supposed to be static functions.
> the one with the additional argument.
> SECU_PrintInteger and SECU_PrintName were extended in the same way, with
> one new function with one additional argument. I like that.
OK.
> SECU_PrintSignedData was extended by adding two new functions.
> a) SECU_PrintSignedDataSigOpt adds one new argument, must like the way in
> which the 3 functions named above were extended.
> b) SECU_PrintSignedContent has the same signature (except for the name)
> as SECU_PrintSignedData, and it invokes SECU_PrintSignedDataSigOpt with
> the opposite value of the one used by the new implementation of
> SECU_PrintSignedData.
> - I would say that it is not necessary to expose both of those two new
> APIs.
I didn't intend to do expose both.
I added only one of the new functions to the header file.
I didn't expect that it would be worrysome to have that additional wraper function.
I believe your worry is based on the fact that it's not a static function.
That's easily solved.
SECU_PrintSignedDataSigOpt is supposed to be a static helper function,
I will make it static and change the prefix to lowercase secu_.
> I would prefer that only one new variant of SECU_PrintSignedData
> be offered by this library. I think SECU_PrintSignedDataSigOpt suffices,
> and most closely resembles the other additions described above.
The other additions are not "API" additions, as explained above,
for clarification I've changed them to be static.
SECU_PrintSignedData is an existing API and supposed to be kept.
SECU_PrintSignedContent is supposed to print less information.
> Three new alternatives have been added to secu_PrintRawString. All 4 of
> them are static. secu_PrintRawStringWithWrap is just a synonym for
> secu_PrintRawString, identical in every way except the name.
> secu_PrintRawStringNoWrap has the same signature as the first two, but
> does something very different.
> secu_PrintRawStringW has one more argument than secu_PrintRawString, and
> calls either secu_PrintRawStringWithWrap or secu_PrintRawStringNoWrap
> according to that additional argument.
> - I'd prefer not to see the number of functions for this purpose explode.
> - Given that these are all static functions, unknown outside of this
> source file, there's really no need for ANY new functions. There's no
> need to preserve the existing old function name, since all the references
> to it are in this same source file. You can just rename the function and
> drop the old name after changing all the other references in the file.
> - You really could get by with just one function, the one with the extra
> argument, and combine the other functions into it. Or you could just
> have two functions, "WithWrap" and "NoWrap", and skip the separate
> function that chooses one or the other.
I was striving for minimal modifications, avoiding changes to unrelated code.
Your optimization path is different, you're striving for less functions,
accepting larger changes.
I'm fine with your request, but please be warned that your request will mean a patch
that is more difficult to review.
Given that reviewer time is so precious, I'm always striving to produce the
smallest possible patches.

This patch addresses most of your requests from point (2), according to my responses in comment 12.
However, for clarity, it does NOT YET include the changes to the
various variants of secu_PrintRawString.
I believe your request to reduce the code to only one variant will produce a much larger and less readable patch.
In particular, my worry is, by using only one function that has the wrap/nowrap option as an integer parameter, the code will be less easy to understand.
In my opinion, the simplification should introduce a new enum value, that makes allows readers to easily understand whether there will be wrapping or no wrapping, without indirect lookup of a numeric integer.

Comment on attachment 494006[details][diff][review]
Patch v4
Nelson, when you review this patch, when reviewing function secu_PrintRawString, please ease your life and review it using the "diff -w" version of this patch (see other attachment).

Comment on attachment 494013[details][diff][review]
Patch v5 - "diff -w" version (ignores whitespace changes for easier reviewing)
This patch is a big improvement over the previous one. Thanks.
I request a few additional changes.
1. Please remove the new enumerated type "WrapOptionType", and either
use a simple int as a boolean (as you did for new function secu_PrintSignedDataSigOpt) or use PRBool instead.
2. Either way, please change any/all tests for the value of the argument
wantWrap to be of the form "if (wantWrap)" or "if (!wantWrap)" rather than
testing against each of the two enumerated values. This is perfectly valid c.
Alternatively, you could test "if (wantWrap != PR_FALSE)" and
"if (wantWrap == PR_FALSE)", but please don't test for
"if (wantWrap == PR_TRUE)" and "if (wantWrap == PR_FALSE)", because such
testing produces unexpected results if the variable has some other binary
value (e.g. 2), being neither PR_TRUE nor PR_FALSE.
3. Your patch has an effect on SECU_PrintSignedData that may or may not have
been intentional. It reduces the level of indentation used with the "inner"
function by one level. e.g.
- rv = (*inner)(out, &sd->data, "Data", level+1);
+ rv = (*inner)(out, &sd->data, "Data", level);
This will change the appearance of the output for all existing callers of this function. I'm pretty sure that change is undesirable, because (sadly) there are programs that parse the output of pp and certutil.

Comment on attachment 494013[details][diff][review]
Patch v5 - "diff -w" version (ignores whitespace changes for easier reviewing)
Kai, I have two reasons for my request.
1. Inconsistency of style on changes introduced in the same patch.
2. Code that correctly handles only 2 out of the ~4 billion possible values
that could be passed for a given argument.
This patch introduces a similar change, adding a 32-bit wide "boolean" argument to two different internal functions, and to the numerous callers of those functions.
To one of those functions it adds the boolean argument as a new enumerated type
WrapOptionType wantWrap, the callers pass noWrap or doWrap and the function
tests for equality to noWrap or equality to doWrap.
To the other function, it adds a simple int argument named withSignature,
The callers pass either 0 or 1. The function tests the variable for being
not equal to zero.
OK, so first there is the inconsistency. The solution to that is to either
use enumerated types in both places, or in neither place. I asked for neither, but I could live with both. However, see below.
Second, there is the logical correctness, the issue of whether the function's behavior is well defined for all inputs.
In this regard, the simple "int" approach that tests the variable for being zero or non-zero produces well defined behavior for all 4 billion possible "int" input values. There is no input value for this argument that causes
the function to behave in an unexpected way (given that c programmers all expect any non-zero values to be treated as "true", and only zero to be
treated as "false").
In the c language, an enumerated type is also an "int", by definition. Any
32-bit int value may be passed to a function expecting an enumerated argument.
Some compilers will emit a warning unless a cast is provided, but that's it.
So, imagine that someone later, at some time, adds code that calls your new
secu_PrintAsHexWrapOpt with the value 2 for wantWrap. 2 is neither noWrap
nor doWrap, yet the variable named wantWrap can surely hold that value.
In this case, the function behavior is not consistent with wantWrap==noWrap
and neither is it consistent with wantWrap==doWrap. The function NEVER puts
a newline into the output.
This sort of mistake is analogous to lacking a default label in a switch.
c programming text books teach about this very issue.
Some ways to solve it include:
a) At the beginning of the function, test that wantWrap is either noWrap or doWrap, and print an error message if not.
b) At the beginning of the function force wantWrap to be one of those two values with a line such as wantWrap &= 1;
c) (My preferred solution) Simply always test for equality or inequality to
zero. Testing an enumerated boolean variable with code such as "if (wantWrap)" and "if (!wantWrap)" avoids the "value is not one of the two enumerations" problem just as well as the approach that abandons the enum type.
d) if you happen to have created an enumerated boolean in which false is not
zero, reconsider, and if you cannot change it, then make all your tests be
for equality or inequality to the symbol with the "false" value.

Nelson, can we please simplify this discussion?
This is a simple patch.
It's not highly optimized code, it's an utility function to print some information.
If you can't accept it with your consistency understanding that I use an "int" parameter in one place, and an "enum" parameter in another, I'm happy to use the same type in both places.
I don't understand why it's necessary to be as much concerned as you are.
I don't buy your argument that a programmer would ever call the function with an integer parameter of "2", when the interface clearly defines what the programmer is supposed to do. Also, the program will not crash, but might produce the wrong wrapping behaviour.
I propose that I use an enum type in both places.
As a minor improvement, I propose that I always test for equality or inequality against the enum value that is zero. That should address your concerns.

Why do I bother to introduce enum into NSS at all?
Because C code is difficult to read, and I'm attempting to improve readability.
I would have hoped that you support such initiatives, instead of asking me to go back to less readable code.

The patch appears to work fine and everything builds ok, but when I attempt to run the built pp.exe against a certificate, it does not give me ther der base64 encoded issuer and serial number???...It seems to just print out the normal output one would expect from pp. What could I be doing wrong?

> I presume you wanted:
> rv =(*inner)(out, &sd->data, "Data", level+1);
I agree this is what we need.
With your correction, the output of "certificate" and "crl" and "certificate-request" the output of pp will be unchanged.
> r- though my only gripe is the following:
Because you said this was your only gripe, and since I'm changing it in the way you requested, I'm concluding r=rrelyea