527759 - Add multiple roots to NSS (single patch)

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Kai Engert (:KaiE:)]

Add multiple roots to NSS (single patch)

See list of bugs this one blocks.

Comment 1

[Kai Engert (:KaiE:)]

Attachment: Patch v1

I used the following 4 commands to add the 4 new root certs, using friendly names and trust flags as indicated in the related 4 bugs.

addbuiltin -n "certSIGN ROOT CA" -t C,C,C <  ~/moz/nss/head/cert-526532.der >> certdata.txt

addbuiltin -n "CNNIC ROOT" -t C,,  <  ~/moz/nss/head/cert-525008 >> certdata.txt

addbuiltin -n "ApplicationCA - Japanese Government" -t C,,C  <  ~/moz/nss/head/cert-523434.der >> certdata.txt

addbuiltin -n "GeoTrust Primary Certification Authority - G3" -t C,C,C  <  ~/moz/nss/head/cert-517234.der >> certdata.txt

Comment 2

[Kai Engert (:KaiE:)]

Comment on attachment 411492 [details] [diff] [review]
Patch v1

Maybe you'll want to postpone the review until the test build has been confirmed. I'll create a try server build, ask CAs for confirmation, and afterwards ask you again for review.

Comment 3

[Kai Engert (:KaiE:)]

adding 2 more bugs, 4 more certs

Comment 4

[Kai Engert (:KaiE:)]

for the additional thawte and verisign roots I used the following commands:

addbuiltin -n "thawte Primary Root CA - G2" -t C,,C  <  ~/moz/nss/head/cert-521869-g2 >> certdata.txt

addbuiltin -n "thawte Primary Root CA - G3" -t C,,C  <  ~/moz/nss/head/cert-521869-g3 >> certdata.txt

addbuiltin -n "Verisign Class 1 Public Primary Certification Authority" -t ,C,  <  ~/moz/nss/head/cert-515462-pca1.der >> certdata.txt

addbuiltin -n "Verisign Class 3 Public Primary Certification Authority" -t C,C,C  <  ~/moz/nss/head/cert-515462-pca3.der >> certdata.txt

Comment 5

[Kai Engert (:KaiE:)]

I'll produce a test build based on stable Firefox 3.5.x, mozilla-1.9.1 branch

That branch still uses NSS 3.12.4, I'll copy certdata.c and certdata.txt from my patched trunk version of NSS to the NSS snapshot currently used in mozilla-1.9.1

(can't simply apply the patch because additional roots have been added recently, after 3.12.4)

Comment 6

[Kai Engert (:KaiE:)]

Attachment: Patch v2, 8 new roots

Comment 7

[Kai Engert (:KaiE:)]

test build:
https://build.mozilla.org/tryserver-builds/[e-mail]-bug527759/

I've updated the individual bugs, asking for testing and feedback.

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 411639 [details] [diff] [review]
Patch v2, 8 new roots

Behold the rubber stamp.

Comment 9

[Kai Engert (:KaiE:)]

Attachment: Patch v3, 11 new roots

We missed 3 roots!
I added 3 more dependent bugs, added them to this patch.

Patch v3 is equivalent to v2, it just adds 3 additional roots.

Here are the commands I had used to add them:

addbuiltin -n "GeoTrust Primary Certificate Authority - G2" -t C,C,C < ~/moz/nss/head/cert-517242.der >> certdata.txt

addbuiltin -n "VeriSign Universal Root Certification Authority" -t C,C,C < ~/moz/nss/head/cert-515470 >> certdata.txt

addbuiltin -n "VeriSign Class 3 Public Primary Certificate Authority - G4" -t C,C,C < ~/moz/nss/head/cert-515472 >> certdata.txt

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 412641 [details] [diff] [review]
Patch v3, 11 new roots

Per bug 515472 comment 7, two of these should have different nicknames.

"GeoTrust Primary Certificate Authority - G2" 
should be
"GeoTrust Primary Certification Authority - G2" 

"VeriSign Class 3 Public Primary Certificate Authority - G4"
should be
"VeriSign Class 3 Public Primary Certification Authority - G4"

Comment 11

[Kai Engert (:KaiE:)]

Attachment: Patch v4

I reverted my tree to the clean original cvs contents.

I repeated my 11 commands, now using the 2 corrected nicknames.

addbuiltin -n "certSIGN ROOT CA" -t C,C,C <  ~/moz/nss/head/cert-526532.der >> certdata.txt
addbuiltin -n "CNNIC ROOT" -t C,,  <  ~/moz/nss/head/cert-525008 >> certdata.txt
addbuiltin -n "ApplicationCA - Japanese Government" -t C,,C  < ~/moz/nss/head/cert-523434.der >> certdata.txt
addbuiltin -n "GeoTrust Primary Certification Authority - G3" -t C,C,C  < ~/moz/nss/head/cert-517234.der >> certdata.txt
addbuiltin -n "thawte Primary Root CA - G2" -t C,,C  < ~/moz/nss/head/cert-521869-g2 >> certdata.txt
addbuiltin -n "thawte Primary Root CA - G3" -t C,,C  < ~/moz/nss/head/cert-521869-g3 >> certdata.txt
addbuiltin -n "Verisign Class 1 Public Primary Certification Authority" -t ,C, <  ~/moz/nss/head/cert-515462-pca1.der >> certdata.txt
addbuiltin -n "Verisign Class 3 Public Primary Certification Authority" -t C,C,C  <  ~/moz/nss/head/cert-515462-pca3.der >> certdata.txt
addbuiltin -n "GeoTrust Primary Certification Authority - G2" -t C,C,C < ~/moz/nss/head/cert-517242.der >> certdata.txt
addbuiltin -n "VeriSign Universal Root Certification Authority" -t C,C,C < ~/moz/nss/head/cert-515470 >> certdata.txt
addbuiltin -n "VeriSign Class 3 Public Primary Certification Authority - G4" -t C,C,C < ~/moz/nss/head/cert-515472 >> certdata.txt

and ran "make generate"

This gave me this updated patch.

I compared previous patch v3 and this patch v4.
The only difference is in nickname strings and string length constants.

(I think another test build is not necessary.)

Comment 12

[Kai Engert (:KaiE:)]

Please delay reviewing this patch until we have feedback from verisign.

They complained that trust bits are missing, but I can't confirm their impression.

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 413313 [details] [diff] [review]
Patch v4

Since you've asked me to delay the review, I'm canceling the review request.  Please request review again when you're ready.

Comment 14

[Kai Engert (:KaiE:)]

It appears bug 515462 needs some more analysis. I therefore propose to omit it from this round of cert additions, in order to get the other certs shipped soon.

Comment 15

[Kai Engert (:KaiE:)]

Attachment: Patch v5, roots

Comment 16

[Kai Engert (:KaiE:)]

Patch v5 adds 9 roots.
(When compared to the previously attached patch, I removed the 2 roots from bug 515462)

NSS 3.12.5 got released with ckbi v 1.76.
When adding the 9 roots, I propose to upgrade the ckbi version number to v 1.77.
I propose to release ckbi v 1.77 immediately after adding the roots and produce a NSS 3.12.4 + ckbi 1.77 tag.

The first part of the patch updates the version number.


As NSS 3.12 turns out to be a surprisingly long lived branch, I propose to increase the range of version numbers we reserve for 3.12.

The second part of the patch adds this proposal and states the proposed new version number ranges.

Comment 17

[Kai Engert (:KaiE:)]

Attachment: Patch v6, version numbers

Comment 18

[Kai Engert (:KaiE:)]

Please review both patches, thanks in advance.

Comment 19

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 415100 [details] [diff] [review]
Patch v6, version numbers

I see no problem with this.

Comment 20

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 415099 [details] [diff] [review]
Patch v5, roots

r=nelson
I have verified that the roots in this patch have the nicknames requested in their respective NSS RFEs. 
I have not verified the trust flags.  I am relying on the CAs, who have each approved their respective cert additions, for this purpose.

Comment 21

[Kai Engert (:KaiE:)]

Thanks for the reviews.

Marking resolved fixed.

I forgot to copy the output from cvs commit, but here are the version's I've committed:

/certdata.c/1.58/Thu Dec  3 21:22:36 2009//TNSSCKBI_1_77_RTM
/certdata.txt/1.57/Thu Dec  3 21:22:36 2009//TNSSCKBI_1_77_RTM
/nssckbi.h/1.22/Thu Dec  3 21:22:36 2009//TNSSCKBI_1_77_RTM

Comment 22

[Daniel Veditz [:dveditz]]

Comment on attachment 415099 [details] [diff] [review]
Patch v5, roots

I assume you'll want to add these to Firefox 3.0 and 3.5 as well

Comment 23

[Daniel Veditz [:dveditz]]

Comment on attachment 415099 [details] [diff] [review]
Patch v5, roots

Firefox 3.0.x is still on NSS 3.12.3.1 and basically EOL, let's not.

Comment 24

[Kai Engert (:KaiE:)]

Johnathan's comment in Bug 528277 comment 3 should be seen as approval-1.9.2+ for both r+'ed patches in this bug (attachment 415099 [details] [diff] [review] and attachment 415100 [details] [diff] [review])

Comment 25

[Daniel Veditz [:dveditz]]

Comment on attachment 415099 [details] [diff] [review]
Patch v5, roots

Approved for 1.9.1.8, a=dveditz for release-drivers

Comment 26

[Daniel Veditz [:dveditz]]

Comment on attachment 415100 [details] [diff] [review]
Patch v6, version numbers

Approved for 1.9.1.8, a=dveditz for release-drivers

Comment 27

[Daniel Veditz [:dveditz]]

This has been fixed for 1.9.2 in bug 528277.

Comment 28

[Kai Engert (:KaiE:)]

pushed
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/96aa722da7ab