webrev: http://cr.openjdk.java.net/~xuelei/7011497/webrev/
On 1/27/2011 1:41 AM, Sean Mullan wrote:
On 1/24/11 9:28 PM, Xuelei Fan wrote:
webrev: http://cr.openjdk.java.net/~xuelei/7011497/webrev/
Changed to use X509CertSelector to select the trust certificate.
Comparing with the previous webrev, the following files are updated to
use X509CertSelector:
DistributionPointFetcher.java
ForwardBuilder.java
PKIXCertPathValidator.java
AdaptableX509CertSelector.java
You can only review the above updates.
* PKIXCertPathValidator
[234-238]: these fields are set to null by default. I don't think you
need to invoke the set methods.
Yes. But in order to remind that we don't want to check the validity, I
have one line comment:
"// no certificate and private key valid check for trusted certificate"
[240,248]: s/trust/trusted
Updated.
[242]: You don't need to check the length of the array here. If
X509Certificate.getKeyUsage returned an array of the wrong length then
that would be a non-compliant implementation and you should just let
the code throw an ArrayIndexOOB exception.
Good catch.
I don't like runtime exception here, as it expose to DOS attack if the
client send improper data. I updated the code to throw CRLException or
CertPathValidatorException, "Encounter non-compliant X.509 certificate:
no enought KeyUsage bits". Does it sound reasonable to you?
* AdaptableX509CertSelector
[151-154]: this block could be moved into the previous conditional
block since version is < 3. Also, can you explain this block? It seems
like you would want to return false (as X509CertSelector.match does)
if there is no subject key id, but I assume there is an important use
case that I'm missing here.
Ooops, there was a mistake in my code. The logic connector should be
"||" but not "&&":
- 151 if (version< 3&& xcert.getExtensionValue("2.5.29.14") == null) {
+ 151 if (version< 3 || xcert.getExtensionValue("2.5.29.14") == null) {
The purpose of the block is to ensure, conservatively, that if there is
no SubjectKID extension in the certificate, we don't check it by force
for compatibility.
* DistributionPointFetcher
[703-707] Same comment as PKIXCertPathValidator[234-238] above.
[711] Same comment as PKIXCertPathValidator[242] above.
Updated as the updates in PKIXCertPathValidator.
Why did you remove the signature verification check of the CRL that
was previously in ForwardBuilder.issues. Is that because we verify it
later (line 642)?
Yes, I think the signature will be finally be verified later in line 642.
Another point that I think we can remove the check is that for version 1
and 2 certificates, the CA should not issue multiple CRL signers with
the same subject; and for version 3 certificate, the AKID and AKID
should be used instead. So we don't need to verify the signature twice.
Thanks,
Xuelei
--Sean
Thanks,
Xuelei
On 1/15/2011 5:37 AM, Sean Mullan wrote:
Hi Andrew,
Did you consider using the existing X509CertSelector class to match on
the authority key identifier? I actually think this should work, and it
will avoid having to create the AKIDMatchState class. Take a look at
the
ForwardBuilder.getMatchingCACerts method towards the end, where it gets
the AuthorityKeyIdentifierExtension. Can you create a similar
X509CertSelector to select the proper trust anchor?
--Sean
On 1/14/11 12:32 PM, Xuelei Fan wrote:
On 1/15/2011 1:30 AM, Xuelei Fan wrote:
Hi Sean,
webrev:
http://cr.openjdk.java.net/~xuelei/7011497/webrev/
Would you please review the update again. I integrate the fix for
7011497 and 7012357 together.
Comparing with previous webrev, the following updates are unchanged:
src/share/classes/java/security/cert/CertPathValidatorException.java
src/share/classes/sun/security/provider/certpath/AlgorithmChecker.java
src/share/classes/sun/security/validator/SimpleValidator.java
other test files.
The following are new changes for CR 7012357:
src/share/classes/sun/security/provider/certpath/ForwardBuilder.java
src/share/classes/sun/security/provider/certpath/PKIXCertPathValidator.java
test/sun/security/provider/certpath/DisabledAlgorithms/CPValidatorEndEntity.java
Thanks,
Xuelei
On 1/14/2011 11:10 AM, Xuelei Fan wrote:
We don't checking the SKID and AKID during searching for the trust
anchor.
I have filled a new CR for the issue, 7012357, Improve trust anchor
searching method during cert path validation.
I will have this commented out block in CPValidatorEndEntity.java. I
will use this test case for CR 7012357.
Thanks,
Xuelei
On 1/14/2011 12:44 AM, Xuelei Fan wrote:
I just realized, if subject KID and issuer KID works, the cert path
validation should be able to find the proper trust anchor. I will
look
into the issue tomorrow.
Xuelei
On 1/14/2011 12:27 AM, Xuelei Fan wrote:
On 1/14/2011 12:05 AM, Sean Mullan wrote:
On 1/13/11 6:38 AM, Xuelei Fan wrote:
Hi Sean,
Would you please review the fix for CR 7011497?
http://cr.openjdk.java.net/~xuelei/7011497/webrev/
Thanks,
Xuelei
CPValidatorEndEntity.java:
307 /* coment out useless trust anchor
308 is = new
ByteArrayInputStream(trustAnchor_SHA1withRSA_512.getBytes());
309 cert = cf.generateCertificate(is);
310 anchor = new TrustAnchor((X509Certificate)cert,
null);
311 anchors.add(anchor);
312 */
Why do you leave this code in with this comment?
If I have this block. The cert path validation cannot find the
proper
trust anchor. As there are two trusted certificates, they are
almost the
same except the key size (one key size is 1024, another one is
512).
In cert path validation, once a trust anchor found, if the
signature is
not valid, I think no more effort to test more trust anchors.
I was wondering whether it is worthy to try more trust anchors.
It's
expensive!
Thanks for the review.
Xuelei
Otherwise, looks good.
--Sean
