Closed Bug 382775 Opened 13 years ago Closed 13 years ago

There should be a test for certutil -D (certificate deletion)


(NSS :: Test, defect, P2)



(Not tracked)



(Reporter: julien.pierre, Assigned: slavomir.katuscak+mozilla)



(1 file, 1 obsolete file)

5.65 KB, patch
: review+
: superreview+
Details | Diff | Splinter Review
Currently, there is a test that says it checks certutil -D in, but in fact it calls certutil -F . The name of that test should be fixed, and there should be a new test that really calls certutil -D .
Currents steps related to certificates export/import in

1. Export certificate and key as PKCS#12 file (pk12util -o).
2. Export certificate as DER-encoded file (certutil -L -r). 
3. Delete certificate and key file (certutil -F).
4. Import certificate and key from PKCS#12 file (pk12util -i).
5. Continue testing with certificate and key.

I suggested following change:
1. Export certificate and key as PKCS#12 file (pk12util -o).
2. Export certificate as DER-encoded file (certutil -L -r). 
3. Delete certificate and key (certutil -F).
4. Import certificate from DER-encoded file (certutil -A).
5. Delete certificate (certutil -D).
6. Import certificate and key from PKCS#12 file (pk12util -i).
7. Continue testing with certificate and key.

If this was OK, it will add not only test for certificate deletion, but also test for importing DER-encoded certificate. But this failed on 4th step, so I reported it as bug 385946.

Because of problems with importing DER-encoded certificate (certutil -A) I tried to import certificate + key (pk12util -A) as 4th step and them delete certificate (certutil -D) as 5th step. 

Now there is only key in DB, no certificate. Is this status correct ?
In 6th step I imported certificate + key again (pk12util -i). This passed. I'm not sure if this is OK, because key was already there before. I played with this more and reported bug 386057. But if rewriting of existing certificate is OK, I suggest this solution for now until bug 385946 is fixed. 

Attached patch Patch v1. (obsolete) — Splinter Review
Implemented suggestions from previous comment. When bug 385946 is fixed, I suggest to use use certutil -A in 4th step instead of pk12util -i, but for certutil -D is this solution OK.

I found out that certutil -L tests checked only return value, but not content of database (returned 0 also when DB is empty), so I modified certutil -L tests to check also content (otherwise we don't know if certificate was really deleted).
Attachment #271658 - Flags: review?(julien.pierre.boogz)
Attachment #271658 - Flags: review?(julien.pierre.boogz) → review+
Checking in;
/cvsroot/mozilla/security/nss/tests/fips/,v  <--
new revision: 1.28; previous revision: 1.27
Attachment #271658 - Flags: superreview?(nelson)
Backed out last patch:
Checking in;
/cvsroot/mozilla/security/nss/tests/fips/,v  <--
new revision: 1.29; previous revision: 1.28
Attached patch Patch v2.Splinter Review
After discussion with Nelson added some improvements.

1. Output from certutil -L logged to output log.
2. DB is checked for certificate name (before there was only check if there is any content).
Attachment #271658 - Attachment is obsolete: true
Attachment #272664 - Flags: review?(nelson)
Attachment #271658 - Flags: superreview?(nelson)
Comment on attachment 272664 [details] [diff] [review]
Patch v2.

Attachment #272664 - Flags: review?(nelson) → review+
Priority: -- → P2
Target Milestone: --- → 3.12
Comment on attachment 271658 [details] [diff] [review]
Patch v1.

The problem I had with this first patch was that it used the presence of ANY output from certutil as an indicator of success.  So, even an error message would be treated as a successful result.  The second patch solves that problem by looking for specific output to indicate success.

Also, the first patch caused the output of certutil to no longer appear in the log.  The second patch corrects that.
Attachment #271658 - Flags: review-
Attachment #272664 - Flags: superreview?(julien.pierre.boogz)
This is targetted at 3.12, so is a 2nd review needed ?
Aren't there 2 reviews required for the branch ? This patch is ready also for 3.11.8 and I think it's better to keep the trunk and the branch synchronized. 
Slavo, yes, 2 reviews are required for the branch, but the "Target milestone" field of this bug is 3.12. If you want it in 3.11.8, you need to change that.
Target Milestone: 3.12 → 3.11.8
Comment on attachment 272664 [details] [diff] [review]
Patch v2.

The Patch is OK.

However, a block of 8 new lines appears 5 times identically in it - the part about "List the FIPS module certificates". Instead of being copied and pasted, could this possibly be put into a function that would be called 5 times ? Also, having a string argument and changing the string slightly would help to figure out where we are at when reading output logs.
Attachment #272664 - Flags: superreview?(julien.pierre.boogz) → superreview+

Checking in;
/cvsroot/mozilla/security/nss/tests/fips/,v  <--
new revision: 1.30; previous revision: 1.29

Checking in;
/cvsroot/mozilla/security/nss/tests/fips/,v  <--
new revision:; previous revision:

To comment #11:
Yes, maybe it can be put into special function, but the same with other certutil parameters, not only -L for listing. If you think this is important rather open new bug for this.

Closed: 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.