Closed
Bug 421989
Opened 15 years ago
Closed 11 years ago
Enhance pp to dump DER issuer and serial (-t certificate-identity)
Categories
(NSS :: Tools, enhancement)
NSS
Tools
Tracking
(Not tracked)
RESOLVED
FIXED
3.14
People
(Reporter: KaiE, Assigned: KaiE)
Details
Attachments
(1 file, 13 obsolete files)
10.77 KB,
patch
|
KaiE
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•15 years ago
|
||
Attachment #308528 -
Flags: review?(nelson)
Assignee | ||
Comment 2•15 years ago
|
||
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•15 years ago
|
||
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".
Attachment #308528 -
Flags: review?(nelson) → review-
Assignee | ||
Comment 4•15 years ago
|
||
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...
Assignee | ||
Comment 5•15 years ago
|
||
Attachment #308528 -
Attachment is obsolete: true
Comment 6•14 years ago
|
||
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•14 years ago
|
||
Russel, Is your comment relevant to this bug? If so, how does it relate to this bug?
Assignee: nobody → kaie
Comment 8•14 years ago
|
||
This bug is now broken (applying the patch causes compile errors) and has not been committed yet.
Comment 9•14 years ago
|
||
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.
Severity: normal → enhancement
Assignee | ||
Comment 10•12 years ago
|
||
merged previous patch to current NSS head
Attachment #312369 -
Attachment is obsolete: true
Assignee | ||
Comment 11•12 years ago
|
||
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)).
Assignee | ||
Comment 12•12 years ago
|
||
(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.
Assignee | ||
Comment 13•12 years ago
|
||
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.
Assignee | ||
Comment 14•12 years ago
|
||
This version uses a single variant of secu_PrintRawString* Function secu_PrintRawString is difficult to review in this patch, because of indenting changes.
Assignee | ||
Comment 15•12 years ago
|
||
Assignee | ||
Comment 16•12 years ago
|
||
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).
Assignee | ||
Comment 17•12 years ago
|
||
Minor improvement over v4, combine consecutive if statements into a single one.
Attachment #493972 -
Attachment is obsolete: true
Attachment #493982 -
Attachment is obsolete: true
Attachment #493998 -
Attachment is obsolete: true
Attachment #494006 -
Attachment is obsolete: true
Attachment #494012 -
Flags: review?(nelson)
Assignee | ||
Comment 18•12 years ago
|
||
Attachment #494007 -
Attachment is obsolete: true
Comment 19•12 years ago
|
||
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.
Attachment #494013 -
Flags: review-
Assignee | ||
Comment 20•12 years ago
|
||
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•12 years ago
|
||
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.
Assignee | ||
Comment 22•12 years ago
|
||
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.
Assignee | ||
Comment 23•12 years ago
|
||
In my opinion, print(data, wrap) is MUCH more readable than print(data, TRUE);
Assignee | ||
Comment 24•12 years ago
|
||
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.
Assignee | ||
Comment 25•12 years ago
|
||
Attachment #494012 -
Attachment is obsolete: true
Attachment #494012 -
Flags: review?(nelson)
Assignee | ||
Comment 26•12 years ago
|
||
Assignee | ||
Comment 27•12 years ago
|
||
Comment 28•12 years ago
|
||
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•12 years ago
|
||
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.
Assignee | ||
Comment 30•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
||
Nevermind! I got it! Thanks so much for your help!!
Assignee | ||
Comment 33•11 years ago
|
||
Attachment #494013 -
Attachment is obsolete: true
Attachment #509759 -
Attachment is obsolete: true
Attachment #509761 -
Attachment is obsolete: true
Attachment #519988 -
Attachment is obsolete: true
Attachment #643344 -
Flags: review?(rrelyea)
Assignee | ||
Comment 34•11 years ago
|
||
I frequently use this tool, it would help me to get this in. Merging was necessary today because of bitrot.
Comment 35•11 years ago
|
||
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
Attachment #643344 -
Flags: review?(rrelyea) → review-
Assignee | ||
Comment 36•11 years ago
|
||
> 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
Attachment #643344 -
Attachment is obsolete: true
Attachment #652339 -
Flags: review+
Assignee | ||
Updated•11 years ago
|
Summary: Enhance pp to dump DER issuer and serial → Enhance pp to dump DER issuer and serial (-t certificate-identity)
Assignee | ||
Comment 37•11 years ago
|
||
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
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → 3.14
You need to log in
before you can comment on or make changes to this bug.
Description
•