421989 - Enhance pp to dump DER issuer and serial (-t certificate-identity)

Status: RESOLVED FIXED

(NSS :: Tools, enhancement)

Description

[Kai Engert [:KaiE:]]

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 1

[Kai Engert [:KaiE:]]

Attachment: Patch v1

Comment 2

[Kai Engert [:KaiE:]]

Sample output:

$ pp -t certificate -x -a -i 10_entrust.pem
Subject:
    CN=Entrust.net Secure Server Certification Authority,OU=(c) 1999 Entrust.net Limited,OU=www.entrust.net/CPS incorp. by ref. (limits liab.),O=Entrust.net,C=US
Issuer:
    CN=Entrust.net Secure Server Certification Authority,OU=(c) 1999 Entrust.net Limited,OU=www.entrust.net/CPS incorp. by ref. (limits liab.),O=Entrust.net,C=US
Serial Number: 927650371 (0x374ad243)
Issuer DER Base64:
MIHDMQswCQYDVQQGEwJVUzEUMBIGA1UEChMLRW50cnVzdC5uZXQxOzA5BgNVBAsT
Mnd3dy5lbnRydXN0Lm5ldC9DUFMgaW5jb3JwLiBieSByZWYuIChsaW1pdHMgbGlh
Yi4pMSUwIwYDVQQLExwoYykgMTk5OSBFbnRydXN0Lm5ldCBMaW1pdGVkMTowOAYD
VQQDEzFFbnRydXN0Lm5ldCBTZWN1cmUgU2VydmVyIENlcnRpZmljYXRpb24gQXV0
aG9yaXR5
Serial DER Base64:
N0rSQw==
Fingerprint (MD5):
    DF:F2:80:73:CC:F1:E6:61:73:FC:F5:42:E9:C5:7C:EE
Fingerprint (SHA1):
    99:A6:9B:E6:1A:FE:88:6B:4D:2B:82:00:7C:B8:54:FC:31:7E:15:39

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

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

Comment 4

[Kai Engert [:KaiE:]]

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

Comment 5

[Kai Engert [:KaiE:]]

Attachment: Patch v1 with bug fixed

Comment 6

[Russell Dyas]

Having problems using the latest firefox 3.5 source (and mozilla-central) when adding the patch using qpush (as detailed here: http://blog.sidkalra.com/2008/10/applying-a-patch-to-a-repo-using-mercurial/) gives build errors. (Windows XP SP3 w/ VS9)

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Russel, Is your comment relevant to this bug?  
If so, how does it relate to this bug?

Comment 8

[Russell Dyas]

This bug is now broken (applying the patch causes compile errors) and has not been committed yet.

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

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.

Comment 10

[Kai Engert [:KaiE:]]

Attachment: patch v1c

merged previous patch to current NSS head

Comment 11

[Kai Engert [:KaiE:]]

Attachment: Patch v2

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

Comment 12

[Kai Engert [:KaiE:]]

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

Comment 13

[Kai Engert [:KaiE:]]

Attachment: Patch v3

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 14

[Kai Engert [:KaiE:]]

Attachment: Patch v4

This version uses a single variant of secu_PrintRawString*

Function secu_PrintRawString is difficult to review in this patch, because of indenting changes.

Comment 15

[Kai Engert [:KaiE:]]

Attachment: Patch v4 - "diff -w" version (ignores whitespace changes for easier reviewing)

Comment 16

[Kai Engert [:KaiE:]]

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 17

[Kai Engert [:KaiE:]]

Attachment: Patch v5

Minor improvement over v4, combine consecutive if statements into a single one.

Comment 18

[Kai Engert [:KaiE:]]

Attachment: Patch v5 - "diff -w" version (ignores whitespace changes for easier reviewing)

Comment 19

[Nelson Bolyard (seldom reads bugmail)]

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 20

[Kai Engert [:KaiE:]]

Nelson. in my opinion the use of an enum and obvious identifiers produces code that is easier to understand.

What is your reasoning for asking me to write less readable code?

Comment 21

[Nelson Bolyard (seldom reads bugmail)]

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.

Comment 22

[Kai Engert [:KaiE:]]

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.

Comment 23

[Kai Engert [:KaiE:]]

In my opinion, 

  print(data, wrap)

is MUCH more readable than

  print(data, TRUE);

Comment 24

[Kai Engert [:KaiE:]]

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.

Comment 25

[Kai Engert [:KaiE:]]

Attachment: Patch v6

Comment 26

[Kai Engert [:KaiE:]]

Attachment: Patch v6 - "diff -w" version (ignores whitespace changes for easier reviewing)

Comment 27

[Kai Engert [:KaiE:]]

Attachment: Patch v13

Comment 28

[Steve Perryman]

This patch doesn't seem to work for the release-source branch of mozilla.  What version does this patch work with and still compile and build properly?

Comment 29

[Steve Perryman]

I sure would like to get this patch working.  I have a need for the base64 encoded der of the issuer and serial number.  Help is appreciated.

Comment 30

[Kai Engert [:KaiE:]]

If you need the output for just one or two certs, I'm happy to help, if you point me to the certs.

If you'd like to build yourself:
Instead of using the Mozilla release branch, please get the NSS trunk.

The NSS build instructions at http://www.mozilla.org/projects/security/pki/nss/nss-3.11.4/nss-3.11.4-build.html should still work.

cvs -z3 -q co mozilla/nsprpub
cvs -z3 -q co mozilla/security/nss mozilla/security/coreconf mozilla/security/dbm mozilla/dbm

I just attempted to apply this patch to the NSS trunk (cvs), which works fine.

$ cat p13-421989 | patch --dry-run -p0
patching file mozilla/security/nss/cmd/lib/secutil.c
Hunk #1 succeeded at 773 (offset -6 lines).
Hunk #2 succeeded at 827 (offset -6 lines).
Hunk #3 succeeded at 851 (offset -6 lines).
Hunk #4 succeeded at 947 (offset -6 lines).
Hunk #5 succeeded at 986 (offset -6 lines).
Hunk #6 succeeded at 1345 (offset -6 lines).
Hunk #7 succeeded at 1380 (offset -6 lines).
Hunk #8 succeeded at 2077 (offset -6 lines).
Hunk #9 succeeded at 2445 (offset -6 lines).
Hunk #10 succeeded at 2468 with fuzz 2 (offset -6 lines).
Hunk #11 succeeded at 2707 (offset -6 lines).
Hunk #12 succeeded at 3434 (offset -6 lines).
Hunk #13 succeeded at 3461 (offset -6 lines).
patching file mozilla/security/nss/cmd/lib/secutil.h
Hunk #1 succeeded at 46 (offset 1 line).
patching file mozilla/security/nss/cmd/pp/pp.c

Comment 31

[Steve Perryman]

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?

Comment 32

[Steve Perryman]

Nevermind!  I got it!  Thanks so much for your help!!

Comment 33

[Kai Engert [:KaiE:]]

Attachment: Patch v14

Comment 34

[Kai Engert [:KaiE:]]

I frequently use this tool, it would help me to get this in.

Merging was necessary today because of bitrot.

Comment 35

[Robert Relyea]

Comment on attachment 643344 [details] [diff] [review]
Patch v14

r- though my only gripe is the following:

-    SECU_Indent(out, level); fprintf(out, "%s:\n", m);
-    rv = (*inner)(out, &sd->data, "Data", level+1);
+    if (m) {
+        SECU_Indent(out, level); fprintf(out, "%s:\n", m);
+    } else {
+        level -= 1;
+    }
+    rv = (*inner)(out, &sd->data, "Data", level);

It appears the you no longer increment level on the inner parsing for any of the cases, and in one case it appears you decrement level. I presume you wanted:

rv =(*inner)(out, &sd->data, "Data", level+1);

as the last line (then the decrement makes sense. Otherwise there is a case where inner is outdented here.

bob

Comment 36

[Kai Engert [:KaiE:]]

Attachment: patch v15

> 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

Comment 37

[Kai Engert [:KaiE:]]

Checking in lib/secutil.c;
/cvsroot/mozilla/security/nss/cmd/lib/secutil.c,v  <--  secutil.c
new revision: 1.122; previous revision: 1.121
done
Checking in lib/secutil.h;
/cvsroot/mozilla/security/nss/cmd/lib/secutil.h,v  <--  secutil.h
new revision: 1.47; previous revision: 1.46
done
Checking in pp/pp.c;
/cvsroot/mozilla/security/nss/cmd/pp/pp.c,v  <--  pp.c
new revision: 1.12; previous revision: 1.11
done