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

[Alexey Bakhtin]

Hello Aleksei,

Thank you for review.
Please see my comments below.

Updated webrev: http://cr.openjdk.java.net/~abakhtin/8245527/webrev.v14/

Regards
Alexey

> On 10 Jul 2020, at 19:40, Aleks Efimov <aleksej.efimov at oracle.com> wrote:
> 
> 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 :)

HandshakeCompletedListener is called in case of successful handshake only.
In case of async handshake failed we close the connection and complete CF exceptionally to release CF.get()

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

Thank you. Fixed in Connection.java and LdapCBPropertiesTest.java
> 
> 'getTlsServerCertificate' should return null if 'isTlsConnection()' is false - rather than waiting forever.

We call isTlsConnection() in the LdapSasl before getTlsServerCertificate().
But your are right, we can double check it to prevent possible deadlock in the future, if code changed.
Fixed in Connection.java

> 
> 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.
Done
> 
> Connection.java:995 - you could use diamond operator here
Updated
> 
> Formatting: Connection.java:1027 'catch (‘
Updated
> 
> 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.
Thank you for suggestion. Updated to use BaseLdapServer in the test

> 
> LdapCBPropertiesTest.java:122 - could use no param Hastable constructor
Fixed
> 
> 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