RFR: 8245527: LDAP Cnannel Binding support for Java GSS/Kerberos

[Aleks Efimov]

Hi Alexey,

Thank you for removing the dependency on the timeout property and adding 
tests for TLS handshake cases.

Please, find the comments about the latest webrev below:

Not quite sure why the CF is completed at two places. Probably that’s 
OK, but it would be good to know the reason :)

The ExecutionException could be unpacked instead of using it directly - 
and its cause used instead.

'getTlsServerCertificate' should return null if 'isTlsConnection()' is 
false - rather than waiting forever.

java.naming/share/classes/module-info.java: could we try to improve the 
formatting of the possible values for the new system property - maybe 
format them as a list.

Connection.java:995 - you could use diamond operator here

Formatting: Connection.java:1027 'catch ('

Could we use the test/jdk/com/sun/jndi/ldap/lib/BaseLdapServer.java from 
the test library to implement dummy LDAPS server, I believe it could 
help to increase the test verbosity and reduce its code size.

LdapCBPropertiesTest.java:122 - could use no param Hastable constructor

I've also run your latest patch through our CI system and it showed no 
failures with your latest changes.

-Aleksei

On 09/07/2020 20:34, Alexey Bakhtin wrote:
> Hello Sean, Daniel,
>
> Thank you for review
> I’ve added “com.sun.jndi.ldap.tls.cbtype” property into the module-info file
> and updated synchronisation using CompletableFuture.
> Also, I have added new test cases : successful and unsuccessful TLS handshake,
> synchronous and asynchronous TLS handshake.
>
> New webrev at : http://cr.openjdk.java.net/~abakhtin/8245527/webrev.v13
>
> Connection property is removed from CSR :
> https://bugs.openjdk.java.net/browse/JDK-8247311
>
> Regards
> Alexey
>
>> On 8 Jul 2020, at 17:41, Daniel Fuchs <daniel.fuchs at oracle.com> wrote:
>>
>> Thanks Sean, Alexey,
>>
>> On 08/07/2020 13:25, Sean Mullan wrote:
>>>> Updated webrev : http://cr.openjdk.java.net/~abakhtin/8245527/webrev.v12/
>>> You will also need to update the CSR to remove the connection timeout property.
>>> Also, you should document the "com.sun.jndi.ldap.tls.cbtype" property in the java.naming module-info file as was done for other properties in Daniel's recent RFR, once he has pushed it [1].
>> I have pushed the fix:
>> https://hg.openjdk.java.net/jdk/jdk/rev/ed375ae6c779
>>
>> Alexey, you should now be able to document your new "com.sun.jndi.ldap.tls.cbtype"
>> property in that @implNote as well, and update your CSR
>> consequently.
>>
>>> * src/java.naming/share/classes/com/sun/jndi/ldap/Connection.java
>>> It looks like there is a possibility of deadlock if getTlsServerCertificate() is called before handshakeCompleted(). In that case the lock could be obtained first and the thread would end up holding the lock and waiting forever.
>> I also have a concern with the use of wait/notify in that code.
>> I would suggest prototyping using a CompletableFuture instead
>> (or can new certificates be exchanged later on the same
>> connection?)
>>
>> CompletableFuture would allow you to relay and propagate any
>> exception raised in the callback handler as well, and would
>> avoid running into deadlocks.
>>
>> best regards,
>>
>> -- daniel