Closed
Bug 952572
Opened 10 years ago
Closed 10 years ago
Hard code ANSSI(DCISS) to french gov dns space
Categories
(NSS :: Libraries, defect, P1)
NSS
Libraries
Tracking
(Not tracked)
RESOLVED
FIXED
3.16.1
People
(Reporter: cviecco, Assigned: cviecco)
References
Details
Attachments
(3 files, 7 obsolete files)
7.56 KB,
patch
|
KaiE
:
review+
|
Details | Diff | Splinter Review |
4.98 KB,
patch
|
cviecco
:
review+
|
Details | Diff | Splinter Review |
1.33 KB,
patch
|
ryan.sleevi
:
review+
wtc
:
checked-in+
|
Details | Diff | Splinter Review |
The IGC/A / ANSSI root needs to be constrained to gouv.fr to limit the risk of missuasage of the cert.
Assignee | ||
Comment 1•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → cviecco
Comment 2•10 years ago
|
||
The CA has requested that we use the same list that Google will be using. To my knowledge the list is: .fr, .gp, .gf, .mq, .re, .yt, .pm, .bl, .mf, .wf, .pf, .nc, .tf Limiting certificate issuance to *.gouv.fr would be too restrictive, because there are several institutions that do not belong to to the "gouvernement" and don't use the ".gouv.fr" extension, such as jurisdictions, universities, Parliament, etc. However, they belong to the public sector and are allowed to be certified by the IGC/A root.
Updated•10 years ago
|
Summary: Name contraint ANSSI to DNS:gouv.fr → Add ability to apply name constraints to root certs, and name contraint ANSSI to DNS:gouv.fr
Comment 3•10 years ago
|
||
Please prefer adding tests to the NSS test suite, rather than to a specific Mozilla application.
Comment 4•10 years ago
|
||
Comment on attachment 8350708 [details] [diff] [review] name-contrain-ANSSI What's the meaning of comment "delta =4" ? In the past, when we hardcoded comparison to identify a particular certificate, we preferred to compare the binary encoding, to ensure that encoding of visible text won't cause mistakes. Take a look at PSM's nsIdentityChecking.cpp for the comparison strategy that we had agreed on in the past. You may use "pp -t certificate-identity" to print certificate encodings that's suitable for embedding in the C code.
Comment 5•10 years ago
|
||
Please either remove the printf statements, or add conditional compilation like using #ifdef DEBUG_cviecco. Typo "manuak".
Assignee | ||
Comment 6•10 years ago
|
||
This has the current name constraint value, a psm layer test but has an awful mechanism to compare certs.
Attachment #8350708 -
Attachment is obsolete: true
Comment 7•10 years ago
|
||
Camilo, Thanks for working on this. I can't tell from the patch which domains the root is being constrained to. Can you add comments to make it more easily readable/clear?
Comment 8•10 years ago
|
||
(In reply to Kathleen Wilson from comment #7) > Camilo, Thanks for working on this. I can't tell from the patch which > domains the root is being constrained to. Can you add comments to make it > more easily readable/clear? I just checked again, and it looks good. Not sure why I couldn't see it before. So you can ignore Comment #7. Thanks!
Updated•10 years ago
|
Target Milestone: --- → 3.16.1
Assignee | ||
Comment 9•10 years ago
|
||
Attachment #8374381 -
Attachment is obsolete: true
Assignee | ||
Comment 10•10 years ago
|
||
Attachment #8390129 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8390152 -
Flags: feedback?(brian)
Assignee | ||
Comment 11•10 years ago
|
||
Comment on attachment 8390152 [details] [diff] [review] name-contrain-ANSSI (v2) Review of attachment 8390152 [details] [diff] [review]: ----------------------------------------------------------------- Acutally I think this is ready for review as we had discussed this before
Attachment #8390152 -
Flags: feedback?(brian) → review?(brian)
Comment 12•10 years ago
|
||
Comment on attachment 8390152 [details] [diff] [review] name-contrain-ANSSI (v2) Review of attachment 8390152 [details] [diff] [review]: ----------------------------------------------------------------- Camilo, as we discussed before, let's write the insanity::pkix version and then backport it to NSS. I will review this after I review the insanity::pkix version.
Attachment #8390152 -
Flags: review?(brian)
Assignee | ||
Comment 13•10 years ago
|
||
Attachment #8390152 -
Attachment is obsolete: true
Assignee | ||
Comment 14•10 years ago
|
||
Attachment #8400763 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8400776 -
Flags: review?(kaie)
Assignee | ||
Comment 15•10 years ago
|
||
Attachment #8400777 -
Flags: review?(kaie)
Assignee | ||
Updated•10 years ago
|
Summary: Add ability to apply name constraints to root certs, and name contraint ANSSI to DNS:gouv.fr → Hard code ANSSI(DCISS) to french gov dns space
Comment 16•10 years ago
|
||
Comment on attachment 8400776 [details] [diff] [review] name-constrain-DCISS Code looks good, I'd just like to ask for a few minor changes. Please change at least (a) and (b). (a) Please don't use char*, rather use const char [], because it allows you to use sizeof() Instead of the constants 136/95, please use sizeof(name-of-array)-1 (minus 1 to exclude the zero terminator), this saves me from counting manually to confirm your numbers. (b) Your getNameExtensionsBuiltIn function has an arena parameter, but you don't use it. In CERT_FindNameConstraintsExten, the data returned from either CERT_FindCertExtension or your getNameExtensionsBuiltIn will be freed unconditionally with PORT_Free(constraintsExtension.data). This means, it's correct that your call to SECITEM_CopyItem uses a NULL parameter for the arena. Conclusion: Please remove the arena parameter from getNameExtensionsBuiltIn. (nits) please add a comment that mentions bug # 952572 please consistently use const/non-const for both char arrays maybe instead of permit_france_gov, call it restrict_france_gov or constraint_france_gov? you could move SECStatus rv into the scope where you call SECITEM_CopyItem
Attachment #8400776 -
Flags: review?(kaie) → review-
Comment 17•10 years ago
|
||
Also, next to the rawANSSISubject variable, could you please add a comment with a human-readable version of the subject string?
Assignee | ||
Comment 18•10 years ago
|
||
Attachment #8400776 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8402848 -
Flags: review?(kaie)
Comment 19•10 years ago
|
||
Comment on attachment 8402848 [details] [diff] [review] name-constrain-DCISS (v2) r=kaie typo: "fo bug 952572"
Attachment #8402848 -
Flags: review?(kaie) → review+
Comment 20•10 years ago
|
||
Comment on attachment 8400777 [details] [diff] [review] dciss-tests The NSS tests execute in 4 cycles, standard, pkix, upgradedb, sharedb (see all.sh) You create the test certs in a file that seems to belong to libpkix, tests/libpkix/certs/make-nc, that's why I'm asking: Did you double check that your test gets executed in the "standard" test cycle, which uses the classic NSS engine? You can restrict the cycles being executed with NSS_CYCLES=standard prior to running all.sh If the test gets executed with the classic verification engine, too, then it's sufficient and r=kaie
Attachment #8400777 -
Flags: review?(kaie) → review+
Updated•10 years ago
|
Flags: needinfo?(cviecco)
Assignee | ||
Comment 21•10 years ago
|
||
Using HOST=localhost NSS_CYCLES=standard NSS_TESTS=chains time ./all.sh the tests pass through my code.
Flags: needinfo?(cviecco)
Assignee | ||
Comment 22•10 years ago
|
||
Fixing typo, keeping r+ from kaie
Attachment #8400777 -
Attachment is obsolete: true
Attachment #8402942 -
Flags: review+
Updated•10 years ago
|
Attachment #8400777 -
Attachment is obsolete: false
Updated•10 years ago
|
Attachment #8402848 -
Attachment is obsolete: true
Comment 23•10 years ago
|
||
Both patches checked in: https://hg.mozilla.org/projects/nss/rev/742307da0792
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Updated•10 years ago
|
OS: Linux → All
Priority: -- → P1
Hardware: x86_64 → All
Comment 24•10 years ago
|
||
Ryan already reviewed this patch at https://codereview.chromium.org/316353002 Patch checked in: https://hg.mozilla.org/projects/nss/rev/2a7348f013cb
Attachment #8435426 -
Flags: review?(ryan.sleevi)
Attachment #8435426 -
Flags: checked-in+
Comment 25•10 years ago
|
||
Comment on attachment 8435426 [details] [diff] [review] Fix compiler warnings in lib/certdb/genname.c, function getNameExtensionsBuiltIn Review of attachment 8435426 [details] [diff] [review]: ----------------------------------------------------------------- r+ as reviewed
Attachment #8435426 -
Flags: review?(ryan.sleevi) → review+
You need to log in
before you can comment on or make changes to this bug.
Description
•