351756 - Add 7 new root CA certs to NSS trunk and 3.11 branch

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Kai Engert [:KaiE:]]

This bug is to add the new root CA certs from several bugs in a single step.

bug 341022, bug 343662, bug 344394, bug 347880, bug 347883

Comment 1

[Kai Engert [:KaiE:]]

Attachment: NSS trunk patch

Comment 2

[Kai Engert [:KaiE:]]

Attachment: NSS 3.11 branch patch

Comment 3

[Kai Engert [:KaiE:]]

Attachment: Text dump of the new certs

Comment 4

[Wan-Teh Chang]

Comment on attachment 237257 [details] [diff] [review]
NSS 3.11 branch patch

r=wtc.

In the future please don't include the diffs for the
generated file certdata.c.

I verified that the trust settings are correct.

I verified that the new version in nssckbi.h is correct.

I verified that the nicknames are correct.

Two issues:

1. It seems that you didn't use nicknames for the Firmaprofessional
and Wells Fargo CAs.

2. Since the Firmaprofessional CA doesn't have an
Organization (O) field, its display in the PSM
Certificate Manager looks strange.  You will find
it at the top of the list because its organization,
which I guess is an empty string, is sorted first.

Comment 5

[Wan-Teh Chang]

I found that you specified CKA_LABEL for the Firmaprofessional
and Wells Fargo CAs, so it may be a PSM bug that their nicknames
are not used in the PSM Certificate Manager display.

Please look at these new CAs in the PSM Certificate Manager and
let me know if you see the same problems.

Comment 6

[Wan-Teh Chang]

Comment on attachment 237255 [details] [diff] [review]
NSS trunk patch

Since we haven't produced any NSS release based on the NSS trunk,
it's not necessary to change the version in nssckbi.h.  This is
the only problem with this patch.

Comment 7

[Wan-Teh Chang]

Comment on attachment 237257 [details] [diff] [review]
NSS 3.11 branch patch

I have one optional suggested change: put the three new GeoTrust
CAs after the existing "GeoTrust Global CA" in certdata.txt.

I have just done another review of this patch.

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

> 1. It seems that you didn't use nicknames for the Firmaprofessional 
> and Wells Fargo CAs.

If it is true tht there are no nicknames for these CAs, that is a stopper
for this patch and should cause the patch to get r-.

Comment 9

[Wan-Teh Chang]

I found that the PSM Certificate Manager displays the CA
cert's nickname only if the cert doesn't have a Common Name (CN)
field.  For many certs, their Common Names and nicknames are
the same, so it wasn't obvious to me why only some certs'
nicknames are displayed.

So I'm now happy with Kai's NSS 3.11 branch patch now.

Comment 10

[Kai Engert [:KaiE:]]

I would like to confirm that I did assign nicknames for all CA certs when I added them.

I used the following commands:

addbuiltin -n "Taiwan GRCA" -t C,C,C < /home/kaie/moz/nss/rootca/GRCA.cer >> certdata.txt
addbuiltin -n "Firmaprofesional Root CA" -t C,C, < /home/kaie/moz/nss/rootca/firmapro.cer >> certdata.txt
addbuiltin -n "Wells Fargo Root CA" -t C,C,C < /home/kaie/moz/nss/rootca/wellsfargo.cer >> certdata.txt
addbuiltin -n "Swisscom Root CA 1" -t C,C,C < /home/kaie/moz/nss/rootca/swisscom-sdcs.cer >> certdata.txt
addbuiltin -n "GeoTrust Global CA 2" -t C,C,C < /home/kaie/moz/nss/rootca/GeoTrust_Global_CA2.cer >> certdata.txt
addbuiltin -n "GeoTrust Universal CA" -t C,C,C < /home/kaie/moz/nss/rootca/GeoTrust_Universal_CA.cer >> certdata.txt
addbuiltin -n "GeoTrust Universal CA 2" -t C,C,C < /home/kaie/moz/nss/rootca/GeoTrust_Universal_CA2.cer >> certdata.txt


Yes, I confirm the display in PSM looks strange for the Firmaprofesional cert, because of the missing O=

I propose we display an alternativ string, if O is empty.

My initial idea was to display the nickname instead, but unfortunately all nicknames are prefixed with the token name when using the PSM APIs to obtain it, so the UI would display "Builtin Object Token:Firma...."
I think that is ugly.

I propose we fall back to the common name.

Comment 11

[Kai Engert [:KaiE:]]

(In reply to comment #10)
> 
> I propose we fall back to the common name.

I filed bug 352401 and attached a patch.

Comment 12

[Kai Engert [:KaiE:]]

Attachment: Trunk Patch v2

new patch v2:
- grouped all Geotrust roots
- no longer change the version number
- this patch does not include the generated changes in the .c file

Comment 13

[Kai Engert [:KaiE:]]

Attachment: 3.11 branch Patch v2

new patch v2:
- grouped all Geotrust roots
- this patch does not include the generated changes in the .c file

Comment 14

[Wan-Teh Chang]

Comment on attachment 238058 [details] [diff] [review]
Trunk Patch v2

r=wtc.  Please check in this change on the NSS trunk.
It's easier for me to verify the patch's correctness
that way.

Comment 15

[Wan-Teh Chang]

Comment on attachment 238059 [details] [diff] [review]
3.11 branch Patch v2

r=wtc.

Do not check in this patch on the NSS_3_11_BRANCH until Christophe
Ravel has announced the NSS 3.11.3 release.

Please go ahead and request approval to check this in on the
MOZILLA_1_8_BRANCH.

Comment 16

[Nelson Bolyard (seldom reads bugmail)]

(In reply to comment #10)
> I would like to confirm that I did assign nicknames for all CA certs when I
> added them.

Let me suggest that you use the command:
  cerutil -d dbdir -L -h all
to list all the certs in the root module, and grep the results to see if 
the new nicknames appear there.

Comment 17

[Kai Engert [:KaiE:]]

(In reply to comment #16)
> 
> Let me suggest that you use the command:
>   cerutil -d dbdir -L -h all
> to list all the certs in the root module, and grep the results to see if 
> the new nicknames appear there.

[kaie@kaiefast 98ap5uix.slt]$ certutil -d /home/kaie/.mozilla/default/98ap5uix.slt -L -h all | egrep -i "firmapro|swissc|grca|geotr|fargo"
Builtin Object Token:GeoTrust Global CA                      C,C,C
Builtin Object Token:GeoTrust Global CA 2                    C,C,C
Builtin Object Token:GeoTrust Universal CA                   C,C,C
Builtin Object Token:GeoTrust Universal CA 2                 C,C,C
Builtin Object Token:Taiwan GRCA                             C,C,C
Builtin Object Token:Firmaprofesional Root CA                C,C,p
Builtin Object Token:Wells Fargo Root CA                     C,C,C
Builtin Object Token:Swisscom Root CA 1                      C,C,C

Comment 18

[Kai Engert [:KaiE:]]

Comment on attachment 238059 [details] [diff] [review]
3.11 branch Patch v2

Requesting approval for MOZILLA_1_8_BRANCH checkin for Firefox 2 et al.

This patch adds new Root CA Certificates that have been approved by Frank Hecker following Mozilla's policy.

This is static data only - no new code is being added.

Also requesting Nelson's approval for NSS 3.11 branch checkin.

Comment 19

[Kai Engert [:KaiE:]]

fixed on trunk

Checking in lib/ckfw/builtins/certdata.c;
/cvsroot/mozilla/security/nss/lib/ckfw/builtins/certdata.c,v  <--  certdata.c
new revision: 1.40; previous revision: 1.39
done
Checking in lib/ckfw/builtins/certdata.txt;
/cvsroot/mozilla/security/nss/lib/ckfw/builtins/certdata.txt,v  <--  certdata.txt
new revision: 1.40; previous revision: 1.39
done

Comment 20

[Mike Schroepfer]

Frank - can you confirm that all of these certs have been approved?

Comment 21

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 238059 [details] [diff] [review]
3.11 branch Patch v2

Two questions:
1) 
The insertion of new certs into the middle of the list caused many
certs to be renumered.  Was that intentional?  

>-static const NSSItem nss_builtins_items_123 [] = {
>+static const NSSItem nss_builtins_items_129 [] = {

>-static const NSSItem nss_builtins_items_124 [] = {
>+static const NSSItem nss_builtins_items_130 [] = {

I'd prefer that that renumbering not happen, but am willing to be
persuaded if there's a good reason for it.

2) This addition demonstrates one of the flaws in addbuiltins,
namely the setting of the 'p' (valid peer) trust flag in all 
categories (ssl, smime, object signing) where no other trust 
flag was set.  

Since all existing root CA certs also have these unwanted 'p' 
trust flags, that's no reason to stop this patch.  But I wonder
if we want to try to fix it before adding more.  
See bug 348882

Comment 22

[Frank Hecker]

(In reply to comment #20)
> Frank - can you confirm that all of these certs have been approved?

Yes, they have; see bug 342426 (Firmaprofesional), bug 294916 (GeoTrust), 274106 (GRCA), bug 342470 (Swisscom), and bug 342926 (Wells Fargo) for the original requests and my approvals thereof. These bug numbers can also be found from the dependency graph for this bug, or from looking at <http://www.hecker.org/mozilla/ca-certificate-list>.

Comment 23

[Mike Schroepfer]

Comment on attachment 238059 [details] [diff] [review]
3.11 branch Patch v2

a=schrep for drivers.

Comment 24

[Kai Engert [:KaiE:]]

Nelson,

(In reply to comment #21)
> The insertion of new certs into the middle of the list caused many
> certs to be renumered.  Was that intentional?  

The entries that you see got renumbered when the .c file with static data got automatically produced from the .txt file.

Per Wan-Teh's proposal in comment 7 we inserted the new GeoTrust certs in the middle of the file, in order to group all GeoTrust certs in a single section in that file.

I don't see how that renumbering of static data can cause any harm.

And we did it before on the 3.11 branch, see:
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=MOZILLA_1_8_BRANCH&branchtype=match&dir=mozilla%2Fsecurity%2Fnss%2Flib%2Fckfw&file=&filetype=match&who=kaie%25kuix.de&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2006-06-11&maxdate=2006-06-13&cvsroot=%2Fcvsroot

At that time an additional Netlock cert was added to the group of other Netlock certs.

Comment 25

[Wan-Teh Chang]

The renumbering of certs in the generated file certdata.c is
caused by my suggestion of putting the three new GeoTrust root
CAs right after the existing GeoTrust root CA in certdata.txt.
Kai forgot to exclude the diffs for certdata.c in the patch.
Code reviewers don't need to review the human unreadable diffs
for generated files.

Comment 26

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 238059 [details] [diff] [review]
3.11 branch Patch v2

r=nelson

Comment 27

[Kai Engert [:KaiE:]]

Not yet checked in to 1.8 branch, because it is currently closed.
Will check in as soon as it reopens.

Now checked in on NSS 3.11 branch:

Checking in lib/ckfw/builtins/certdata.c;
/cvsroot/mozilla/security/nss/lib/ckfw/builtins/certdata.c,v  <--  certdata.c
new revision: 1.36.24.3; previous revision: 1.36.24.2
done
Checking in lib/ckfw/builtins/certdata.txt;
/cvsroot/mozilla/security/nss/lib/ckfw/builtins/certdata.txt,v  <--  certdata.txt
new revision: 1.37.24.3; previous revision: 1.37.24.2
done
Checking in lib/ckfw/builtins/nssckbi.h;
/cvsroot/mozilla/security/nss/lib/ckfw/builtins/nssckbi.h,v  <--  nssckbi.h
new revision: 1.14.2.2; previous revision: 1.14.2.1
done

Comment 28

[Kai Engert [:KaiE:]]

fixed1.8.1

Checking in lib/ckfw/builtins/certdata.c;
/cvsroot/mozilla/security/nss/lib/ckfw/builtins/certdata.c,v  <--  certdata.c
new revision: 1.36.14.2; previous revision: 1.36.14.1
done
Checking in lib/ckfw/builtins/certdata.txt;
/cvsroot/mozilla/security/nss/lib/ckfw/builtins/certdata.txt,v  <--  certdata.txt
new revision: 1.37.14.2; previous revision: 1.37.14.1
done
Checking in lib/ckfw/builtins/nssckbi.h;
/cvsroot/mozilla/security/nss/lib/ckfw/builtins/nssckbi.h,v  <--  nssckbi.h
new revision: 1.13.14.3; previous revision: 1.13.14.2
done