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

Status: RESOLVED FIXED

(NSS :: Test, defect, P2)

Description

[Julien Pierre]

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 .

Comment 1

[Slavomir Katuscak]

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. 

Comment 2

[Slavomir Katuscak]

Attachment: 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).

Comment 3

[Slavomir Katuscak]

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

Comment 4

[Slavomir Katuscak]

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

Comment 5

[Slavomir Katuscak]

Attachment: 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).

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 272664 [details] [diff] [review]
Patch v2.

r=nelson

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

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.

Comment 8

[Julien Pierre]

This is targetted at 3.12, so is a 2nd review needed ?

Comment 9

[Slavomir Katuscak]

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. 

Comment 10

[Julien Pierre]

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.

Comment 11

[Julien Pierre]

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.

Comment 12

[Slavomir Katuscak]

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

Comment 13

[Slavomir Katuscak]

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.