Closed Bug 1032947 Opened 5 years ago Closed 5 years ago

Change mozilla::pkix::CheckNameConstraints to construct CERTCertificates as needed, temporarily

Categories

(Core :: Security: PSM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: briansmith, Assigned: briansmith)

References

Details

Attachments

(1 file)

If we stop parsing the certificate into a CERTCertificate structure in BackCert::Init, we will need to do so in CheckNameConstraints, because CERT_GetConstrainedCertificateNames requires one. This is just temporary until the patches I'm working on for bug 970542 land. I'm hoping to finish those next week. But, I'd like to land my patches for bug 1006958 sooner than the name constraint testing, since they'd benefit from increased compatibility testing on Nightly, and because separating the landing of bug 1006958 and bug 970542 into two separate Nightlies would make regression range finding easier (though, of course, we hope to not have any regressions).
Attachment #8448873 - Flags: review?(dkeeler)
Blocks: 1029247
No longer blocks: 1006958
Comment on attachment 8448873 [details] [diff] [review]
refactor-CheckNameConstraints.patch

Review of attachment 8448873 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.

::: security/pkix/lib/pkixcheck.cpp
@@ +423,5 @@
>    if (!constraints) {
>      return MapSECStatus(SECFailure);
>    }
>  
> +  for (const BackCert* prev = cert.childCert; prev; prev = prev->childCert) {

hmmm - maybe 'child' is a more illuminating name than 'prev' (particularly when making the includeCN decision)

@@ +434,3 @@
>      }
> +
> +    // owned by arena

this comment should probably go directly above names, not includeCN

@@ +435,5 @@
> +
> +    // owned by arena
> +    bool includeCN = prev->includeCN == BackCert::IncludeCN::Yes;
> +    const CERTGeneralName*
> +      names(CERT_GetConstrainedCertificateNames(nssCert.get(), arena.get(), 

nit: trailing space
Attachment #8448873 - Flags: review?(dkeeler) → review+
Thanks for the quick review! I made the changes you suggested before pushing the patch to inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f97578949399
(In reply to Wes Kocher (:KWierso) from comment #3)
> Backed out in
> https://hg.mozilla.org/integration/mozilla-inbound/rev/fc79d63461d2 because
> something from this push (this bug, or bug 1019770 or bug 1031952) broke the
> build: 
> https://tbpl.mozilla.org/php/getParsedLog.php?id=42894750&tree=Mozilla-
> Inbound

It was bug 1019770.
Flags: needinfo?(brian)
https://hg.mozilla.org/mozilla-central/rev/5d4c04e1ccac
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.