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

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: refactor-CheckNameConstraints.patch

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).

Comment 1

[Dana Keeler (she/her) (use needinfo) (:keeler for reviews)]

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

Comment 2

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

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

Comment 3

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 4

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(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.

Comment 5

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Re-pushed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5d4c04e1ccac

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5d4c04e1ccac