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

RESOLVED FIXED in 3.11.8

Status

NSS
Test
P2
normal
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: Julien Pierre, Assigned: Slavomir Katuscak)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

5.65 KB, patch
Nelson Bolyard (seldom reads bugmail)
: review+
Julien Pierre
: superreview+
Details | Diff | Splinter Review
(Reporter)

Description

11 years ago
Currently, there is a test that says it checks certutil -D in fips.sh, 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 .
(Assignee)

Comment 1

11 years ago
Currents steps related to certificates export/import in fips.sh:

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. 

(Assignee)

Comment 2

11 years ago
Created attachment 271658 [details] [diff] [review]
Patch v1.

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)
(Reporter)

Updated

11 years ago
Attachment #271658 - Flags: review?(julien.pierre.boogz) → review+
(Assignee)

Comment 3

11 years ago
Trunk:
Checking in fips.sh;
/cvsroot/mozilla/security/nss/tests/fips/fips.sh,v  <--  fips.sh
new revision: 1.28; previous revision: 1.27
done
(Assignee)

Updated

11 years ago
Attachment #271658 - Flags: superreview?(nelson)
(Assignee)

Comment 4

11 years ago
Backed out last patch:
Checking in fips.sh;
/cvsroot/mozilla/security/nss/tests/fips/fips.sh,v  <--  fips.sh
new revision: 1.29; previous revision: 1.28
done
(Assignee)

Comment 5

11 years ago
Created attachment 272664 [details] [diff] [review]
Patch v2.

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.

r=nelson
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-
(Assignee)

Updated

11 years ago
Attachment #272664 - Flags: superreview?(julien.pierre.boogz)
(Reporter)

Comment 8

11 years ago
This is targetted at 3.12, so is a 2nd review needed ?
(Assignee)

Comment 9

11 years ago
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. 
(Reporter)

Comment 10

11 years ago
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.
(Assignee)

Updated

11 years ago
Target Milestone: 3.12 → 3.11.8
(Reporter)

Comment 11

11 years ago
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+
(Assignee)

Comment 12

11 years ago
Trunk:

Checking in fips.sh;
/cvsroot/mozilla/security/nss/tests/fips/fips.sh,v  <--  fips.sh
new revision: 1.30; previous revision: 1.29
done
(Assignee)

Comment 13

11 years ago
Branch:

Checking in fips.sh;
/cvsroot/mozilla/security/nss/tests/fips/fips.sh,v  <--  fips.sh
new revision: 1.19.28.8; previous revision: 1.19.28.7
done

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.

Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.