Closed Bug 351756 Opened 14 years ago Closed 14 years ago

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

Categories

(NSS :: Libraries, defect)

3.11.3
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
3.11.4

People

(Reporter: KaiE, Assigned: KaiE)

References

Details

(Keywords: fixed1.8.1)

Attachments

(3 files, 2 obsolete files)

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
Attached patch NSS trunk patch (obsolete) — Splinter Review
Assignee: nobody → kengert
Status: NEW → ASSIGNED
Attachment #237255 - Flags: review?(wtchang)
Attached patch NSS 3.11 branch patch (obsolete) — Splinter Review
Attachment #237257 - Flags: review?(wtchang)
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.
Attachment #237257 - Flags: review?(wtchang) → review+
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 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.
Attachment #237255 - Flags: review?(wtchang) → review-
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.
> 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-.
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.
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.
(In reply to comment #10)
> 
> I propose we fall back to the common name.

I filed bug 352401 and attached a patch.
Attached patch Trunk Patch v2Splinter Review
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
Attachment #237255 - Attachment is obsolete: true
Attachment #238058 - Flags: review?(wtchang)
new patch v2:
- grouped all Geotrust roots
- this patch does not include the generated changes in the .c file
Attachment #237257 - Attachment is obsolete: true
Attachment #238059 - Flags: review?(wtchang)
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.
Attachment #238058 - Flags: review?(wtchang) → review+
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.
Attachment #238059 - Flags: review?(wtchang) → review+
(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.
(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 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.
Attachment #238059 - Flags: superreview?(nelson)
Attachment #238059 - Flags: approval1.8.1?
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
Summary: Add 7 new root CA certs to NSS → Add 7 new root CA certs to NSS trunk and 3.11 branch
Frank - can you confirm that all of these certs have been approved?
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
(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 on attachment 238059 [details] [diff] [review]
3.11 branch Patch v2

a=schrep for drivers.
Attachment #238059 - Flags: approval1.8.1? → approval1.8.1+
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.
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 on attachment 238059 [details] [diff] [review]
3.11 branch Patch v2

r=nelson
Attachment #238059 - Flags: superreview?(nelson) → superreview+
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
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Target Milestone: --- → 3.11.4
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
Keywords: fixed1.8.1
You need to log in before you can comment on or make changes to this bug.