Hi Xuelei,
I am not sure how the updates in SimpleValidator relate to the template
for JSSE tests. 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.
SSLSocketTemplate.java
- Fields in ContextParameters can be final
- 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.
- 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.
- 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. It also might make sense to make
createSSLContext() a part of public API which I think would make the
template more flexible.
- 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.
ProxyAuthTest.java
- line 153, I believe try-with-resources doesn't make it worse.
- lines 114-123, this code is used quite often by tests, why don't we
keep it in SSLSocketTemplate?
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
