+++ This bug was initially created as a clone of Bug #1033561 +++ Right now, TrustDomain::FindPotentialIssuers returns a CERTCertList that BuildForward iterators through. In order to resolve bug 1033561, we need to remove this usage of CERTCertList. When rsleevi reviewed the original code, he noted that it would be better to change this to be an iterator interface. In particular, he was thinking ahead of supporting AIA caIssuer cert fetching. I doubt we'll do AIA caIssuer cert fetching in Firefox, but this refactoring will make that decision up to the TrustDomain, instead of being structurally impossible due to the design of the mozilla::pkix API. The idea is that BuildForward will call FindPotentialIssuers like this: rv = trustDomain.FindPotentialIssuers(&subject.GetIssuer(), time, issuerVerifier); and the next iteration of NSSCertDBTrustDomain::FindPotentialIssuers implementation will look like this: { ScopedCERTCertList candidates(CERT_CreateSubjectCertList(...)); for (CERTCertListNode* n = CERT_LIST_HEAD(candidates); !CERT_LIST_END(n, candidates); n = CERT_LIST_NEXT(n)) { bool found; rv = issuerVerifier.CheckPotentialIssuer(n->cert->derCert, found); if (rv != SECSuccess || found) { return rv; } } return SECSuccess; } Then, the next iteration of NSSCertDBTrustDomain::FindPotentialIssuers will replace the use of CERT_CreateSubjectCertList with something similar, but which does not use CERTCertificate/CERTCertList.

try: https://tbpl.mozilla.org/?tree=Try&rev=e1d3b0e83214

Comment on attachment 8450014 [details] [diff] [review] Part 2: Convert mozilla::pkix::BuildForwardInner into an iterator-type thing Review of attachment 8450014 [details] [diff] [review]: ----------------------------------------------------------------- LGTM with comments addressed. ::: security/pkix/lib/pkixbuild.cpp @@ +90,5 @@ > +}; > + > +SECStatus > +PathBuildingStep::RecordResult(PRErrorCode newResult, > + /*out*/ bool& keepGoing) It seems like the keepGoing boolean is replacing the Success / RecoverableError/ FatalError paradigm (i.e. (SECsuccess, keepGoing = false) === Success, (SECSuccess, keepGoing = true) === RecoverableError, and SECFailure === FatalError)). Is your intention to move away from that to this new system? Would it be better to re-implement this change in terms of what we're using now? It seems like using the boolean is necessary so the public interface can use SECStatus instead of mozilla::pkix::Result. I guess I'm ok with this, but it's something to think about. @@ +154,5 @@ > if (SECITEM_ItemsAreEqual(&potentialIssuer.GetSubjectPublicKeyInfo(), > &prev->GetSubjectPublicKeyInfo()) && > SECITEM_ItemsAreEqual(&potentialIssuer.GetSubject(), > &prev->GetSubject())) { > + // XXX: error code As long as we're here, we may as well be more specific about what's wrong. The issue is we don't have an error code specific to "issuer loop detected; don't continue searching this path", right? @@ +192,3 @@ > } > > + return RecordResult(Success, keepGoing); RecordResult takes a PRErrorCode as its first argument - not a mozilla::pkix:Result.

Comment on attachment 8450015 [details] [diff] [review] Part 3: Change mozilla::pkix::TrustDomain::FindPotentialIssuers API to be iterator-like Review of attachment 8450015 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. ::: security/apps/AppTrustDomain.cpp @@ +95,5 @@ > PR_SetError(PR_INVALID_STATE_ERROR, 0); > return SECFailure; > } > > + // TODO(bug XXXXXXX): If/when mozilla::pkix relaxes the restriction that "but XXXXXXX" probably isn't useful unless we tie it to a specific bug ::: security/pkix/include/pkix/pkixtypes.h @@ +158,5 @@ > + // Search for a CA certificate with the given name. The implementation must > + // call checker.Check with the DER encoding of the potential issuer > + // certificate. The implementation must follow these rules: > + // > + // * The subject name of the certificate given to checker.Check must be equal This is probably something that should be enforced by the library, just in case. @@ +195,5 @@ > + // [...] > + // > + // checker.Check is responsible for limiting the recursion to a reasonable > + // limit. > + virtual SECStatus FindPotentialIssuers(const SECItem& encodedIssuerName, Maybe "CheckPotentialIssuers"? Or "IterateThroughPotentialIssuers"?

(In reply to David Keeler (:keeler) [use needinfo?] from comment #5) > It seems like the keepGoing boolean is replacing the Success / > RecoverableError/ FatalError paradigm (i.e. (SECsuccess, keepGoing = false) > === Success, (SECSuccess, keepGoing = true) === RecoverableError, and > SECFailure === FatalError)). Is your intention to move away from that to > this new system? Would it be better to re-implement this change in terms of > what we're using now? > It seems like using the boolean is necessary so the public interface can use > SECStatus instead of mozilla::pkix::Result. I guess I'm ok with this, but > it's something to think about. Your understanding is basically correct. Because we use SECStatus, the TrustDomain can't distinguish between Fatal and Recoverable errors the same way as the internals of mozilla::pkix do. And, mixing Result, der::Result, and SECstatus all over in mozilla::pkix is bad and I feel bad; it will get fixed soon. > @@ +154,5 @@ > > if (SECITEM_ItemsAreEqual(&potentialIssuer.GetSubjectPublicKeyInfo(), > > &prev->GetSubjectPublicKeyInfo()) && > > SECITEM_ItemsAreEqual(&potentialIssuer.GetSubject(), > > &prev->GetSubject())) { > > + // XXX: error code > > As long as we're here, we may as well be more specific about what's wrong. > The issue is we don't have an error code specific to "issuer loop detected; > don't continue searching this path", right? Right. But, this is no different before or after this patch. If you think we should improve this (perhaps so we can write tests for the loop detection?) then let's do it in another bug. > RecordResult takes a PRErrorCode as its first argument - not a > mozilla::pkix:Result. So it does! Another reason why mozilla::pkix::Result ended up not working out as well as I had hoped at the beginning. Thanks for the review!

(In reply to David Keeler (:keeler) [use needinfo?] from comment #6) > > + // TODO(bug XXXXXXX): If/when mozilla::pkix relaxes the restriction that > > "but XXXXXXX" probably isn't useful unless we tie it to a specific bug Updated to reference the new bug, bug 1035418. > > + // * The subject name of the certificate given to checker.Check must be equal > > This is probably something that should be enforced by the library, just in > case. I agree. Bug 1035414. > > + virtual SECStatus FindPotentialIssuers(const SECItem& encodedIssuerName, > > Maybe "CheckPotentialIssuers"? Or "IterateThroughPotentialIssuers"? I agree FindPotentialIssuers is not a good name any more. I changed it to FindIssuer. I really appreciate your two suggestions, but I didn't use either of them because I feel like they imply that we're going to always check or iterate through every potential issuer, instead of stopping when we find an acceptable one. Thanks for the helpful comments and quick review!

https://hg.mozilla.org/integration/mozilla-inbound/rev/c21ea604d839 https://hg.mozilla.org/integration/mozilla-inbound/rev/072f999edcc4 https://hg.mozilla.org/integration/mozilla-inbound/rev/44c19e8283c2

https://hg.mozilla.org/mozilla-central/rev/c21ea604d839 https://hg.mozilla.org/mozilla-central/rev/072f999edcc4 https://hg.mozilla.org/mozilla-central/rev/44c19e8283c2