Thanks for the review, Artem.
On 12/2/2016 2:41 PM, Artem Smotrakov wrote:
Hi Xuelei,
I am not sure how the updates in SimpleValidator relate to the template
for JSSE tests.
The certificates generated for the template have the same subject and
issuer for RSA, DSA and EC algorithms. If using the SimpleValidator, it
cannot find the right root cert for a particular end entity certificate
any more. The issue is not exposed in the past because the certs in etc
directory are self-signed and version 1 certificates. It's not common
to use version 1 and self-signed certs for general JSSE testing, so I
move to version 3 and use end entity certificate for authentication in
this update.
It might be better to separate those changes if I am not
missing something. This update in SimpleValidator looks okay to me, but
taking into account Sean's comments below, I'll let someone who is more
knowledgeable review it as well. I would prefer not to mix it with
updates for SSLSocketTemplate.
It saves time and easier to backport if using one bug. I'm OK to fix
them separately. Let's whether Sean or Weijun can have free cycle for
the review of this part.
SSLSocketTemplate.java
- Fields in ContextParameters can be final
Good suggestion!
- Looks like JSSE test are supposed to extend SSLSocketTemplate. I
suppose serverCondition/clientCondition should not be static then. Some
JSSE test may be safely run in same JVM mode, I think it is better to
make them non-static.
Good catch!
- Why did you remove Peer and Application interfaces? I think those
interfaces make SSLSocketTemplate more flexible since it allows override
doServerSide/doClientSide logic if necessary - it doesn't seem to be
worse. If there is no strong reason to remove those interfaces, I would
prefer to keep them with default static/stateless
doServerSide/doClientSide versions.
I agree with you that The Peer and Application interfaces are more
flexible. I have no strong reason to remove them. For this update, I
focus more on simple and minimal sub-classes. The Peer and Application
interfaces need some complicated coding (condition, threading, etc) in
sub-classes, so the design does not fall into the scope. I may prefer
to remove them at this update and see what happens if we moving forward
with more test update. We can add them back or create a more flexible
one if we need the flexibility in the future.
- It may be better to pass a ContextParameters instance to
createSSLContext() method because we may want to use more parameters
while creating an SSL context.
Yes.
It also might make sense to make
createSSLContext() a part of public API which I think would make the
template more flexible.
We have had the protected createClient/ServerSSLContext() methods.
- Exceptions are printed out in startServer/startClient methods, it
doesn't look necessary to use suppressed exceptions and initCause()
method. What was wrong with the code in runTest() method? The code in
runTest() method looks shorter and cleaner to me.
The main issue of runTest() is that it does not throw the original
exception. Exception tacks have more information for debugging. I want
to keep the good side of Brad's previous hardness on this point.
ProxyAuthTest.java
- line 153, I believe try-with-resources doesn't make it worse.
Right.
- lines 114-123, this code is used quite often by tests, why don't we
keep it in SSLSocketTemplate?
Good point! I don't like it in SSLSocketTemplate as it is used for HTTP
connections only, but it may be worthy to have a sub-template for HTTPS
client testing. What do you think if we address this enhancement in a
new bug?
Thanks,
Xuelei
Artem
On 12/02/2016 11:23 AM, Xue-Lei Fan wrote:
On 11/29/2016 5:22 AM, Sean Mullan wrote:
On 11/27/16 7:43 AM, Xuelei Fan wrote:
On 11/27/2016 6:04 PM, Wang Weijun wrote:
This is not only a test update.
No, I happened to find an implementation issue with the new test, so
fix
it altogether. The issue is that the simple validator
(SimpleValidator.java) does not support SKID/AKID during cert path
build. If two trusted certs has the same subject, the simple
validator
may not be able to find the right one.
We have had issues in the PKIX CertPathBuilder with matching on
AKID/SKID when building certpaths, so we want to be careful not to
introduce a similar issue. See this bug for more information:
https://bugs.openjdk.java.net/browse/JDK-8072463
I have not reviewed the fix enough to know if this issue applies here
but please double-check it.
The KID are used for best effort matching in this update. If no KIDs
get matched, the previous behavior is reserved. Should be safe, I think.
Xuelei
--Sean
Thanks,
Xuelei
On Nov 27, 2016, at 9:35 AM, Xuelei Fan <xuelei....@oracle.com>
wrote:
Hi,
Please review this test update:
http://cr.openjdk.java.net/~xuelei/8170329/webrev.00/
The new template (SSLSocketTemplate.java) could be used to avoid the
anti-free-port issues. By using sub-classes of it, the new one can
simplify the general SSLSocket test code significantly.
Thanks,
Xuelei
