Last Comment Bug 500439 - Make certutil to be able to read and display a der cert from a file
: Make certutil to be able to read and display a der cert from a file
Status: RESOLVED FIXED
:
Product: NSS
Classification: Components
Component: Tools (show other bugs)
: 3.12.1
: All All
: P3 enhancement (vote)
: 3.12.8
Assigned To: Alexei Volkov
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-06-25 10:21 PDT by Alexei Volkov
Modified: 2010-09-15 14:44 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
read cert from a file if failed to read it from a db. (3.08 KB, patch)
2009-06-25 10:21 PDT, Alexei Volkov
nelson: review-
Details | Diff | Splinter Review
Patch v2: read cert from a file if failed to read it from a db. (8.96 KB, patch)
2009-08-03 16:17 PDT, Alexei Volkov
nelson: review+
Details | Diff | Splinter Review
Remove fallback on NSS_NoDB_Init. Remove SECU_GetCert. (checked in) (3.54 KB, patch)
2010-08-28 17:50 PDT, Wan-Teh Chang
alvolkov.bgs: review+
nelson: superreview+
Details | Diff | Splinter Review

Description Alexei Volkov 2009-06-25 10:21:05 PDT
Created attachment 385146 [details] [diff] [review]
read cert from a file if failed to read it from a db.
Comment 1 Nelson Bolyard (seldom reads bugmail) 2009-06-25 12:51:29 PDT
Is this necessary? 
We have another program for doing this.  It is named "pp" (pretty print).
Comment 2 Nelson Bolyard (seldom reads bugmail) 2009-07-23 12:23:03 PDT
Comment on attachment 385146 [details] [diff] [review]
read cert from a file if failed to read it from a db.

I think there are numerous certutil commands that might also wish
to be able to load a cert from a file name if the -n option value
is not that of an existing nickname.  Rather than repeating this
code in each and every command that might want to do that, it would
be much better to write a new function that is similar in signature
to PK11_FindCertFromNickname, but which looks first for a cert by 
nickname and then by file name, and then call that function in 
any certutil command that wants to use it. You might call it 
CERTU_FindCertByNicknameOrFilename or something like that. 
This new function should go into nss/cmd/lib and be declared in 
nss/cmd/something.h.  Any other NSS cmd programs that already 
duplicate this functionality should call the lib function instead.
Comment 3 Nelson Bolyard (seldom reads bugmail) 2009-07-23 12:52:32 PDT
I want to correct two things I wrote:

> I think there are numerous certutil commands that might also wish
I meant: numerous NSS cmd programs.

> You might call it CERTU_FindCertByNicknameOrFilename or something like that. 
I meant: SECU_FindCertByNicknameOrFilename since SECU is the common prefix 
for code in nss/cmd/lib
Comment 4 Alexei Volkov 2009-08-03 16:17:59 PDT
Created attachment 392362 [details] [diff] [review]
Patch v2: read cert from a file if failed to read it from a db.

Create SECU_FindCertByNicknameOrFilename function and use it in certutil.c.
Patch to tools that read cert from a file and will make a use of this function is coming.
Comment 5 Nelson Bolyard (seldom reads bugmail) 2009-10-24 20:06:42 PDT
I confirm this is an enhancement request.
The patch is incomplete.  It should modify more utility programs 
than merely certutil.
Comment 6 Nelson Bolyard (seldom reads bugmail) 2010-01-11 07:57:28 PST
Comment on attachment 392362 [details] [diff] [review]
Patch v2: read cert from a file if failed to read it from a db.

r=nelson
Comment 7 Nelson Bolyard (seldom reads bugmail) 2010-08-28 10:38:45 PDT
Committed on trunk.
Checking in certutil/certutil.c; new revision: 1.155; previous revision: 1.154
Checking in lib/secutil.c;       new revision: 1.101; previous revision: 1.100
Checking in lib/secutil.h;       new revision: 1.33; previous revision: 1.32
Comment 8 Nelson Bolyard (seldom reads bugmail) 2010-08-28 12:47:49 PDT
Committed on 3.12 branch
Checking in cmd/lib/secutil.h; new revision: 1.32.2.1; previous revision: 1.32
Checking in cmd/lib/secutil.c; new revision: 1.99.2.1; previous revision: 1.99
Checking in cmd/certutil/certutil.c; new r: 1.149.2.1; previous revision: 1.149
Comment 9 Wan-Teh Chang 2010-08-28 14:48:40 PDT
Comment on attachment 392362 [details] [diff] [review]
Patch v2: read cert from a file if failed to read it from a db.

In certutil/certutil.c:

>@@ -2369,6 +2373,9 @@ certutil_main(int argc, char **argv, PRB
>             rv = NSS_Initialize(SECU_ConfigDirectory(NULL), 
> 			    certPrefix, certPrefix,
>                             "secmod.db", readOnly ? NSS_INIT_READONLY: 0);
>+            if (rv == SECFailure) {
>+                rv = NSS_NoDB_Init(NULL);
>+            }
> 	} else {
>             rv = NSS_InitWithMerge(SECU_ConfigDirectory(NULL), 
> 			    certPrefix, certPrefix, "secmod.db",

This change may have broken two tests that expect certutil
to fail when opening the db read/write in a nonexisting
directory.  For some reason the tests that open the db
readonly are not affected.

See http://tinderbox.mozilla.org/showlog.cgi?log=NSS/1283017696.1283025497.21640.gz&fulltext=1

Running tests for dbtests
TIMESTAMP dbtests BEGIN: Sat Aug 28 10:52:55 PDT 2010
dbtests.sh: CERT and Key DB Tests ===============================

---------------------------------------------------------------
| test opening the database read/write in a nonexisting directory
---------------------------------------------------------------

Certificate Nickname                                         Trust Attributes
                                                             SSL,S/MIME,JAR/XPI

dbtests.sh: #619: Certutil succeeded in a nonexisting directory 0 - FAILED
dbdir selected is ./non_existent_dir

ERROR: Directory "./non_existent_dir" does not exist.
dbtest: function failed: security library: bad database.
dbtests.sh: #620: Dbtest readonly didn't work in a nonexisting dir 46 - PASSED
Comment 10 Wan-Teh Chang 2010-08-28 14:57:48 PDT
Comment on attachment 392362 [details] [diff] [review]
Patch v2: read cert from a file if failed to read it from a db.

The new SECU_GetCert function in mozilla/security/nss/cmd/lib/secutil.c
is dead code.  Please remove it.
Comment 11 Wan-Teh Chang 2010-08-28 17:50:30 PDT
Created attachment 470228 [details] [diff] [review]
Remove fallback on NSS_NoDB_Init.  Remove SECU_GetCert. (checked in)

Checked in on the NSS trunk (NSS 3.13) and NSS_3_12_BRANCH (NSS 3.12.8).

Checking in certutil.c;
/cvsroot/mozilla/security/nss/cmd/certutil/certutil.c,v  <--  certutil.c
new revision: 1.156; previous revision: 1.155
done
Checking in secutil.c;
/cvsroot/mozilla/security/nss/cmd/lib/secutil.c,v  <--  secutil.c
new revision: 1.102; previous revision: 1.101
done

Checking in certutil.c;
/cvsroot/mozilla/security/nss/cmd/certutil/certutil.c,v  <--  certutil.c
new revision: 1.149.2.2; previous revision: 1.149.2.1
done
Checking in secutil.c;
/cvsroot/mozilla/security/nss/cmd/lib/secutil.c,v  <--  secutil.c
new revision: 1.99.2.2; previous revision: 1.99.2.1
done
Comment 12 Nelson Bolyard (seldom reads bugmail) 2010-08-29 13:25:22 PDT
Wan-Teh, I think you were too quick  to remove the new function. :-(

The point of the new function was to eliminate much duplicated code by 
putting it all in one callable function.  Sounds like the new function
was created but the other places that duplicated that code were never 
changed to call it, and continue to duplicate it.  

Removing the new function will not help to reduce the duplication.
Comment 13 Wan-Teh Chang 2010-08-30 17:06:53 PDT
Comment on attachment 392362 [details] [diff] [review]
Patch v2: read cert from a file if failed to read it from a db.

Nelson: this patch added two new functions: SECU_GetCert and
SECU_FindCertByNicknameOrFilename.  SECU_GetCert is not being
used, and looks like an earlier version of
SECU_FindCertByNicknameOrFilename.  Are you sure you want to
keep SECU_GetCert?

Re: too quick to remove: the NSS tinderboxes have very long
cycles.  So it's necessary to act before 7 or 8 pm if I plan
to go to bed that night.  I waited almost 3 hours for a reply
from either you or Alexei and studied the code carefully
before I removed it.
Comment 14 Wan-Teh Chang 2010-08-30 17:16:01 PDT
Comment on attachment 470228 [details] [diff] [review]
Remove fallback on NSS_NoDB_Init.  Remove SECU_GetCert. (checked in)

Alexei, I don't know what the fallback on NSS_NoDB_Init is for.
Is it required for the new feature?  It seems useful to be able
to use certutil to read and display a DER cert from a file.  But
as Nelson noted, you can use pp to do that.  Please advise.

Nelson, I wonder if you thought I removed SECU_FindCertByNicknameOrFilename.
Comment 15 Alexei Volkov 2010-09-15 14:44:49 PDT
Comment on attachment 470228 [details] [diff] [review]
Remove fallback on NSS_NoDB_Init.  Remove SECU_GetCert. (checked in)

SECU_GetCert has code that prints error messages that I intended to use to printout errors when obtaining cert object, but since it didn't get used it is ok to remove it.

As for NSS_NoDB_Init, I needed to init nss even in cases, when db does not exist to printout a cert from a file. This feature is needed by customers. They intend to use openssl instead to printout nss cearted certs since we do not ship pp utility as a part of NSS package.

I'd still like to call NSS_NoDB_Init, but may be when -d is not specified. I think this will make tests to pass.

Note You need to log in before you can comment on or make changes to this bug.