Re: [8] Request for Review: 8007967: Infinite loop can happen in sun.security.provider.certpath.SunCertPathBuilder.depthFirstSearchForward()
Thanks, Sean. I've updated the webrev with your suggestions. Also, I've changed the check for loops in ForwardBuilder.java to bail out whenever the same certificate is encountered twice. Please note that none of the policy mapping tests in the NIST PKITS use loops. Also, RFC 4158's section on Certification Path building states that X.509 requires that certificates are not repeated when building paths, but does not mention policy mapping. It should therefore be okay to make this change. Please take a look: http://cr.openjdk.java.net/~juh/8007967/webrev.01/ Thanks, Jason On 12/04/2013 01:18 PM, Sean Mullan wrote: Just 2 comments on DistributionPointFetcher: You can eliminate some duplication of code by changing the existing getCRLs method to just call the new method with a null prevCert parameter. On lines 659-663, you don't need to add @code tags, that is only for javadoc comments. --Sean On 12/03/2013 01:51 PM, Jason Uh wrote: Could I please get a review for this change? This change fixes some issues in CertPath building and CRL verification. The main components of this fix are: 1. Proper setting of TrustAnchors when verifying indirect CRLs obtained from CRL Distribution Points. I added an overloaded getCRLs() method to DistributionPointFetcher for this. 2. Terminating the CertPath build immediately when the target cert is found to be revoked. 3. Some clarification in the comments. Webrev: http://cr.openjdk.java.net/~juh/8007967/webrev.00/ Bug: https://bugs.openjdk.java.net/browse/JDK-8007967 Thanks, Jason
Re: [8] Request for Review: 8007967: Infinite loop can happen in sun.security.provider.certpath.SunCertPathBuilder.depthFirstSearchForward()
Looks good, just one nit - you can remove the imports of PolicyMappingsExtension and X509CertImpl from ForwardBuilder. --Sean On 12/05/2013 04:04 PM, Jason Uh wrote: Thanks, Sean. I've updated the webrev with your suggestions. Also, I've changed the check for loops in ForwardBuilder.java to bail out whenever the same certificate is encountered twice. Please note that none of the policy mapping tests in the NIST PKITS use loops. Also, RFC 4158's section on Certification Path building states that X.509 requires that certificates are not repeated when building paths, but does not mention policy mapping. It should therefore be okay to make this change. Please take a look: http://cr.openjdk.java.net/~juh/8007967/webrev.01/ Thanks, Jason On 12/04/2013 01:18 PM, Sean Mullan wrote: Just 2 comments on DistributionPointFetcher: You can eliminate some duplication of code by changing the existing getCRLs method to just call the new method with a null prevCert parameter. On lines 659-663, you don't need to add @code tags, that is only for javadoc comments. --Sean On 12/03/2013 01:51 PM, Jason Uh wrote: Could I please get a review for this change? This change fixes some issues in CertPath building and CRL verification. The main components of this fix are: 1. Proper setting of TrustAnchors when verifying indirect CRLs obtained from CRL Distribution Points. I added an overloaded getCRLs() method to DistributionPointFetcher for this. 2. Terminating the CertPath build immediately when the target cert is found to be revoked. 3. Some clarification in the comments. Webrev: http://cr.openjdk.java.net/~juh/8007967/webrev.00/ Bug: https://bugs.openjdk.java.net/browse/JDK-8007967 Thanks, Jason
Re: [8] Request for Review: 8007967: Infinite loop can happen in sun.security.provider.certpath.SunCertPathBuilder.depthFirstSearchForward()
On 12/05/2013 01:26 PM, Sean Mullan wrote: Looks good, just one nit - you can remove the imports of PolicyMappingsExtension and X509CertImpl from ForwardBuilder. Done. Also, updated copyright date in ForwardBuilder.java. http://cr.openjdk.java.net/~juh/8007967/webrev.02/ --Sean On 12/05/2013 04:04 PM, Jason Uh wrote: Thanks, Sean. I've updated the webrev with your suggestions. Also, I've changed the check for loops in ForwardBuilder.java to bail out whenever the same certificate is encountered twice. Please note that none of the policy mapping tests in the NIST PKITS use loops. Also, RFC 4158's section on Certification Path building states that X.509 requires that certificates are not repeated when building paths, but does not mention policy mapping. It should therefore be okay to make this change. Please take a look: http://cr.openjdk.java.net/~juh/8007967/webrev.01/ Thanks, Jason On 12/04/2013 01:18 PM, Sean Mullan wrote: Just 2 comments on DistributionPointFetcher: You can eliminate some duplication of code by changing the existing getCRLs method to just call the new method with a null prevCert parameter. On lines 659-663, you don't need to add @code tags, that is only for javadoc comments. --Sean On 12/03/2013 01:51 PM, Jason Uh wrote: Could I please get a review for this change? This change fixes some issues in CertPath building and CRL verification. The main components of this fix are: 1. Proper setting of TrustAnchors when verifying indirect CRLs obtained from CRL Distribution Points. I added an overloaded getCRLs() method to DistributionPointFetcher for this. 2. Terminating the CertPath build immediately when the target cert is found to be revoked. 3. Some clarification in the comments. Webrev: http://cr.openjdk.java.net/~juh/8007967/webrev.00/ Bug: https://bugs.openjdk.java.net/browse/JDK-8007967 Thanks, Jason
Re: [8] Request for Review: 8007967: Infinite loop can happen in sun.security.provider.certpath.SunCertPathBuilder.depthFirstSearchForward()
Looks good. On Dec 5, 2013, at 6:58 PM, Jason Uh jason...@oracle.com wrote: On 12/05/2013 01:26 PM, Sean Mullan wrote: Looks good, just one nit - you can remove the imports of PolicyMappingsExtension and X509CertImpl from ForwardBuilder. Done. Also, updated copyright date in ForwardBuilder.java. http://cr.openjdk.java.net/~juh/8007967/webrev.02/ --Sean On 12/05/2013 04:04 PM, Jason Uh wrote: Thanks, Sean. I've updated the webrev with your suggestions. Also, I've changed the check for loops in ForwardBuilder.java to bail out whenever the same certificate is encountered twice. Please note that none of the policy mapping tests in the NIST PKITS use loops. Also, RFC 4158's section on Certification Path building states that X.509 requires that certificates are not repeated when building paths, but does not mention policy mapping. It should therefore be okay to make this change. Please take a look: http://cr.openjdk.java.net/~juh/8007967/webrev.01/ Thanks, Jason On 12/04/2013 01:18 PM, Sean Mullan wrote: Just 2 comments on DistributionPointFetcher: You can eliminate some duplication of code by changing the existing getCRLs method to just call the new method with a null prevCert parameter. On lines 659-663, you don't need to add @code tags, that is only for javadoc comments. --Sean On 12/03/2013 01:51 PM, Jason Uh wrote: Could I please get a review for this change? This change fixes some issues in CertPath building and CRL verification. The main components of this fix are: 1. Proper setting of TrustAnchors when verifying indirect CRLs obtained from CRL Distribution Points. I added an overloaded getCRLs() method to DistributionPointFetcher for this. 2. Terminating the CertPath build immediately when the target cert is found to be revoked. 3. Some clarification in the comments. Webrev: http://cr.openjdk.java.net/~juh/8007967/webrev.00/ Bug: https://bugs.openjdk.java.net/browse/JDK-8007967 Thanks, Jason
Re: [8] Request for Review: 8007967: Infinite loop can happen in sun.security.provider.certpath.SunCertPathBuilder.depthFirstSearchForward()
Just 2 comments on DistributionPointFetcher: You can eliminate some duplication of code by changing the existing getCRLs method to just call the new method with a null prevCert parameter. On lines 659-663, you don't need to add @code tags, that is only for javadoc comments. --Sean On 12/03/2013 01:51 PM, Jason Uh wrote: Could I please get a review for this change? This change fixes some issues in CertPath building and CRL verification. The main components of this fix are: 1. Proper setting of TrustAnchors when verifying indirect CRLs obtained from CRL Distribution Points. I added an overloaded getCRLs() method to DistributionPointFetcher for this. 2. Terminating the CertPath build immediately when the target cert is found to be revoked. 3. Some clarification in the comments. Webrev: http://cr.openjdk.java.net/~juh/8007967/webrev.00/ Bug: https://bugs.openjdk.java.net/browse/JDK-8007967 Thanks, Jason
[8] Request for Review: 8007967: Infinite loop can happen in sun.security.provider.certpath.SunCertPathBuilder.depthFirstSearchForward()
Could I please get a review for this change? This change fixes some issues in CertPath building and CRL verification. The main components of this fix are: 1. Proper setting of TrustAnchors when verifying indirect CRLs obtained from CRL Distribution Points. I added an overloaded getCRLs() method to DistributionPointFetcher for this. 2. Terminating the CertPath build immediately when the target cert is found to be revoked. 3. Some clarification in the comments. Webrev: http://cr.openjdk.java.net/~juh/8007967/webrev.00/ Bug: https://bugs.openjdk.java.net/browse/JDK-8007967 Thanks, Jason
