The default bug view has changed. See this FAQ.

Make certutil to be able to read and display a der cert from a file

RESOLVED FIXED in 3.12.8

Status

NSS
Tools
P3
enhancement
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: Alexei Volkov, Assigned: Alexei Volkov)

Tracking

3.12.1
3.12.8

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

8 years ago
Created attachment 385146 [details] [diff] [review]
read cert from a file if failed to read it from a db.
Attachment #385146 - Flags: review?(nelson)
Is this necessary? 
We have another program for doing this.  It is named "pp" (pretty print).
Severity: normal → enhancement
Target Milestone: 3.12.1 → 3.12.4
Version: 3.12.4 → 3.12.1
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.
Attachment #385146 - Flags: review?(nelson) → review-
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
(Assignee)

Comment 4

8 years ago
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.
Attachment #385146 - Attachment is obsolete: true
Attachment #392362 - Flags: review?

Updated

8 years ago
Attachment #392362 - Flags: review? → review?(nelson)
I confirm this is an enhancement request.
The patch is incomplete.  It should modify more utility programs 
than merely certutil.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment on attachment 392362 [details] [diff] [review]
Patch v2: read cert from a file if failed to read it from a db.

r=nelson
Attachment #392362 - Flags: review?(nelson) → review+
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
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: 3.12.4 → 3.13
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
Priority: -- → P3
Target Milestone: 3.13 → 3.12.8

Comment 9

7 years ago
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

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

Updated

7 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 11

7 years ago
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
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

7 years ago
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

7 years ago
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.
Attachment #470228 - Flags: superreview?(nelson)
Attachment #470228 - Flags: review?(alexei.volkov.bugs)

Updated

7 years ago
Status: REOPENED → RESOLVED
Last Resolved: 7 years ago7 years ago
Resolution: --- → FIXED
Attachment #470228 - Flags: superreview?(nelson) → superreview+
(Assignee)

Comment 15

7 years ago
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.
Attachment #470228 - Flags: review?(alexei.volkov.bugs) → review+
You need to log in before you can comment on or make changes to this bug.