On Thu, 11 Jan 2024 17:31:37 GMT, Sean Mullan <mul...@openjdk.org> wrote:
>> This enhancement simplifies and improves the performance of the Comparator >> that the PKIX CertPathBuilder uses to sort candidate certificates. >> >> [RFC 5280](https://www.rfc-editor.org/rfc/rfc5280#section-4.2.1.1) requires >> that certificates include authority and subject key identifiers to >> facilitate cert path discovery. When the certificates comply with RFC 5280, >> the sorting algorithm is fast and efficient. However, there may be cases >> where certificates do not include the proper KIDs, for legacy or other >> reasons. This enhancement targets those cases and has shown an increase in >> performance of `CertPathBuilder.build` by up to 2x in tests involving >> certificates that do not contain KIDs. Specific changes include: >> >> - Removed and simplified some of the steps in `PKIXCertComparator.compare` >> method. Some of these steps were not a good representation of common >> certificate hierarchies and were overly expensive to perform. >> - Several methods in `X500Name` and `Builder` have been made obsolete and >> thus removed. >> - `X500Name` has been changed to use shared secrets instead of reflection to >> access non-public members of `X500Principal`, and vice-versa. >> - The `CertificateBuilder` test code has been enhanced to set reasonable >> defaults for serial number and validity fields of a certificate > > Sean Mullan has updated the pull request incrementally with one additional > commit since the last revision: > > Fix whitespace error. Improve debugging. Change return value of > distanceToCommonAncestor(). LGTM. Only 2 style comments for src and another for test. src/java.base/share/classes/sun/security/provider/certpath/ForwardBuilder.java line 496: > 494: String debugMsg = null; > 495: if (debug != null) { > 496: debug.println(METHOD_NME +" SAME NAMESPACE AS TRUSTED > TEST..."); Add a space after `+`. src/java.base/share/classes/sun/security/provider/certpath/ForwardBuilder.java line 509: > 507: X500Name tSubjectName = X500Name.asX500Name(tSubject); > 508: int d1 = distanceToCommonAncestor(tSubjectName, > cIssuer1Name); > 509: int d2 = distanceToCommonAncestor(tSubjectName, > cIssuer2Name); The logic below is correct. Somehow I think it will be clearer if you print out the debug lines before all the checks. Something like: if (debug != null) { if (d1 != -1) debug.println(...); if (d2 != -1) debug.println(...); } test/jdk/sun/security/provider/certpath/PKIXCertComparator/Order.java line 58: > 56: kpg.initialize(2048); > 57: > 58: // Create top-level root CA cert with KIDs (Subject and Auth > KeyIds) Should a root CA have AKID? ------------- Marked as reviewed by weijun (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17248#pullrequestreview-1829925340 PR Review Comment: https://git.openjdk.org/jdk/pull/17248#discussion_r1457589230 PR Review Comment: https://git.openjdk.org/jdk/pull/17248#discussion_r1457598419 PR Review Comment: https://git.openjdk.org/jdk/pull/17248#discussion_r1457613256
