Re: RFR: 8259707: LDAP channel binding does not work with StartTLS extension [v2]

2021-01-21 Thread Alexey Bakhtin 
On Thu, 21 Jan 2021 18:19:10 GMT, Aleksei Efimov  wrote:

>> Alexey Bakhtin has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   separate tlsHandshakeCompleted for every StartTLS connection
>
> src/java.naming/share/classes/com/sun/jndi/ldap/Connection.java line 1034:
> 
>> 1032: }
>> 1033: 
>> 1034: private HandshakeListener tlsHandshakeListener;
> 
> I believe that `volatile` modifier should be added here. And it could be 
> beneficial for future maintainers to have here a comment block with a brief 
> description of when `tlsHandshakeListener` is used.

Agree. Added comments and volatile modifier.

> src/java.naming/share/classes/com/sun/jndi/ldap/Connection.java line 1059:
> 
>> 1057: private class HandshakeListener implements 
>> HandshakeCompletedListener {
>> 1058: 
>> 1059: private CompletableFuture 
>> tlsHandshakeCompleted =
> 
> `tlsHandshakeCompleted` could be made `final`

Thank you. Made it final

-

PR: https://git.openjdk.java.net/jdk/pull/2085

Re: RFR: 8259707: LDAP channel binding does not work with StartTLS extension [v2]

2021-01-21 Thread Aleksei Efimov 
On Thu, 21 Jan 2021 13:13:38 GMT, Alexey Bakhtin  wrote:

>> Please review a small patch to enable LDAP TLS Channel Binding with StartTLS 
>> Extension.
>> Test from the bug report and jtreg javax/naming tests are passed.
>
> Alexey Bakhtin has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   separate tlsHandshakeCompleted for every StartTLS connection

src/java.naming/share/classes/com/sun/jndi/ldap/Connection.java line 1034:

> 1032: }
> 1033: 
> 1034: private HandshakeListener tlsHandshakeListener;

I believe that `volatile` modifier should be added here. And it could be 
beneficial for future maintainers to have here a comment block with a brief 
description of when `tlsHandshakeListener` is used.

src/java.naming/share/classes/com/sun/jndi/ldap/Connection.java line 1059:

> 1057: private class HandshakeListener implements 
> HandshakeCompletedListener {
> 1058: 
> 1059: private CompletableFuture 
> tlsHandshakeCompleted =

`tlsHandshakeCompleted` could be made `final`

-

PR: https://git.openjdk.java.net/jdk/pull/2085

Re: RFR: 8259707: LDAP channel binding does not work with StartTLS extension [v2]

2021-01-21 Thread Alexey Bakhtin 
On Wed, 20 Jan 2021 15:54:41 GMT, Daniel Fuchs  wrote:

>> New ChannelBinding Data will be recreated for every TLS connection and 
>> provided to SASL Client in the new environment properties set (cloned from 
>> the original). 
>> LdapSasl.java lines 133 - 136:
>> TlsChannelBinding tlsCB =
>> TlsChannelBinding.create(cert);
>> envProps = (Hashtable) env.clone();
>> envProps.put(TlsChannelBinding.CHANNEL_BINDING, 
>> tlsCB.getData());
>
>> New ChannelBinding Data will be recreated for every TLS connection and 
>> provided to SASL Client in the new environment properties set (cloned from 
>> the original).
>> LdapSasl.java lines 133 - 136:
>> 
>> ```
>> TlsChannelBinding tlsCB =
>> TlsChannelBinding.create(cert);
>> envProps = (Hashtable) env.clone();
> 
> Hi Alexey, 
> 
> Aleksei and I have concern because this code uses a `cert` that is obtained 
> from a CompletableFuture, and the completable future  can be completed only 
> once. The second time around - you will therefore find the same `cert` that 
> was set when the first StartTLSResponse was negotiated. This may - or may not 
> matter - depending on whether the `cert` certificate returned by the server 
> the second time around should be the same - or not.
> Could you test this scenario?
> It may be that it's a niche scenario that makes no sense or that we don't 
> want to support - I'm not sure how STARTTLS is used in the wild. Do you have 
> any insights on this?
> 
> best regards,
> 
> -- daniel

Hi Daniel, Aleksei,

You are right, the second StartTLS request works incorrectly because of a 
single CompletableFuture. I do not know if several StartTLS sessions are used 
in the wild, but there are no such restrictions in the spec. I have updated the 
review to create new CompletableFuture for each TLS session. Updated test, 
suggested by Aleksei is passed. I have verified that the Channel Binding data 
is created on the base of a new cert object every TLS session.

-

PR: https://git.openjdk.java.net/jdk/pull/2085

Re: RFR: 8259707: LDAP channel binding does not work with StartTLS extension [v2]

2021-01-21 Thread Alexey Bakhtin 
> Please review a small patch to enable LDAP TLS Channel Binding with StartTLS 
> Extension.
> Test from the bug report and jtreg javax/naming tests are passed.

Alexey Bakhtin has updated the pull request incrementally with one additional 
commit since the last revision:

  separate tlsHandshakeCompleted for every StartTLS connection

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2085/files
  - new: https://git.openjdk.java.net/jdk/pull/2085/files/d2f470e7..30a29e07

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=2085=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2085=00-01

  Stats: 71 lines in 2 files changed: 37 ins; 25 del; 9 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2085.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2085/head:pull/2085

PR: https://git.openjdk.java.net/jdk/pull/2085