Re: RFR: 8259707: LDAP channel binding does not work with StartTLS extension [v4]
On Fri, 22 Jan 2021 18:16:52 GMT, Daniel Fuchs wrote: >> Alexey Bakhtin has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Update copyright year > > LGTM. Thanks for taking this on! I will sponsor this! - PR: https://git.openjdk.java.net/jdk/pull/2085
Re: RFR: 8259707: LDAP channel binding does not work with StartTLS extension [v4]
On Fri, 22 Jan 2021 14:42:10 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: > > Update copyright year LGTM. Thanks for taking this on! - Marked as reviewed by dfuchs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/2085
Re: RFR: 8259707: LDAP channel binding does not work with StartTLS extension [v4]
On Fri, 22 Jan 2021 14:42:10 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: > > Update copyright year Marked as reviewed by aefimov (Committer). - PR: https://git.openjdk.java.net/jdk/pull/2085
Re: RFR: 8259707: LDAP channel binding does not work with StartTLS extension [v4]
> 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: Update copyright year - Changes: - all: https://git.openjdk.java.net/jdk/pull/2085/files - new: https://git.openjdk.java.net/jdk/pull/2085/files/e3bf8c36..40447456 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=2085=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2085=02-03 Stats: 2 lines in 2 files changed: 0 ins; 0 del; 2 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
Re: RFR: 8259707: LDAP channel binding does not work with StartTLS extension [v3]
On Thu, 21 Jan 2021 19:57:04 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: > > Add comments and volatile modifier for tlsHandshakeListener Hi Alexey, The latest changes look good to me. Thanks for handling a case of sequential StartTLS requests on one LDAP context and running the modified test. I've also checked that existing LDAP tests shows no failures with the proposed changed. You might also want to update last modification years to `2021` in both files. - Marked as reviewed by aefimov (Committer). PR: https://git.openjdk.java.net/jdk/pull/2085
Re: RFR: 8259707: LDAP channel binding does not work with StartTLS extension [v2]
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 [v3]
> 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: Add comments and volatile modifier for tlsHandshakeListener - Changes: - all: https://git.openjdk.java.net/jdk/pull/2085/files - new: https://git.openjdk.java.net/jdk/pull/2085/files/30a29e07..e3bf8c36 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=2085=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2085=01-02 Stats: 7 lines in 1 file changed: 5 ins; 0 del; 2 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
Re: RFR: 8259707: LDAP channel binding does not work with StartTLS extension [v2]
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]
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]
> 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
Re: RFR: 8259707: LDAP channel binding does not work with StartTLS extension
On Wed, 20 Jan 2021 15:34:24 GMT, Alexey Bakhtin 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(); 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 - PR: https://git.openjdk.java.net/jdk/pull/2085
Re: RFR: 8259707: LDAP channel binding does not work with StartTLS extension
On Wed, 20 Jan 2021 15:08:56 GMT, Aleksei Efimov wrote:
>> That look reasonable to me. But what would happen if at some point after
>> performing some LDAP operations, you called StartTLSResponse::close and then
>> after some more time you tried to again create a StartTLSRequest on the same
>> context? Would that work - or would you be using a possibly obsolete channel
>> binding obtained from the first upgrade?
>
> The change looks valid to me too.
> I believe Daniel question could be illustrated with the following change to
> `CBwithTLS` reproducer attached to the bug report:
> --- CBwithTLS_Original.java 2021-01-20 14:56:09.620844903 +
> +++ CBwithTLS.java2021-01-20 15:01:47.253982227 +
> @@ -45,7 +45,7 @@
> System.out.println(ctxt.getAttributes("", new
> String[]{"defaultNamingContext"}).get("defaultNamingContext").get());
>
> // Switch to TLS
> -
> +for (int i=0; i<2; i++) {
> StartTlsResponse tls = (StartTlsResponse) ctxt.extendedOperation(new
> StartTlsRequest());
> tls.negotiate();
>
> @@ -64,6 +64,9 @@
> lc.login();
>
> Subject.doAs(lc.getSubject(), (PrivilegedAction)
> CBwithTLS::run);
> +lc.logout();
> +tls.close();
> +}
> }
>
> private static Void run() {
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());
-
PR: https://git.openjdk.java.net/jdk/pull/2085
Re: RFR: 8259707: LDAP channel binding does not work with StartTLS extension
On Wed, 20 Jan 2021 14:41:26 GMT, Daniel Fuchs 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.
>
> That look reasonable to me. But what would happen if at some point after
> performing some LDAP operations, you called StartTLSResponse::close and then
> after some more time you tried to again create a StartTLSRequest on the same
> context? Would that work - or would you be using a possibly obsolete channel
> binding obtained from the first upgrade?
The change looks valid to me too.
I believe Daniel question could be illustrated with the following change to
`CBwithTLS` reproducer attached to the bug report:
--- CBwithTLS_Original.java 2021-01-20 14:56:09.620844903 +
+++ CBwithTLS.java 2021-01-20 15:01:47.253982227 +
@@ -45,7 +45,7 @@
System.out.println(ctxt.getAttributes("", new
String[]{"defaultNamingContext"}).get("defaultNamingContext").get());
// Switch to TLS
-
+for (int i=0; i<2; i++) {
StartTlsResponse tls = (StartTlsResponse) ctxt.extendedOperation(new
StartTlsRequest());
tls.negotiate();
@@ -64,6 +64,9 @@
lc.login();
Subject.doAs(lc.getSubject(), (PrivilegedAction) CBwithTLS::run);
+lc.logout();
+tls.close();
+}
}
private static Void run() {
-
PR: https://git.openjdk.java.net/jdk/pull/2085
Re: RFR: 8259707: LDAP channel binding does not work with StartTLS extension
On Thu, 14 Jan 2021 19:28:27 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. That look reasonable to me. But what would happen if at some point after performing some LDAP operations, you called StartTLSResponse::close and then after some more time you tried to again create a StartTLSRequest on the same context? Would that work - or would you be using a possibly obsolete channel binding obtained from the first upgrade? - PR: https://git.openjdk.java.net/jdk/pull/2085
Re: RFR: 8259707: LDAP channel binding does not work with StartTLS extension
On Wed, 20 Jan 2021 14:01:45 GMT, Sean Mullan 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. > > Marked as reviewed by mullan (Reviewer). Sean, Thank you for review - PR: https://git.openjdk.java.net/jdk/pull/2085
Re: RFR: 8259707: LDAP channel binding does not work with StartTLS extension
On Thu, 14 Jan 2021 19:28:27 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. Marked as reviewed by mullan (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/2085
Re: RFR: 8259707: LDAP channel binding does not work with StartTLS extension
On Wed, 20 Jan 2021 07:21:22 GMT, Alexey Bakhtin wrote: > Unfortunately, I can not find any LDAP StartTLS Extended Operation regression > tests. security/infra area contains RevocationChecker tests. They can not be > used for this scenario. Ok, please add a noreg-hard label to the bug. - PR: https://git.openjdk.java.net/jdk/pull/2085
Re: RFR: 8259707: LDAP channel binding does not work with StartTLS extension
On Tue, 19 Jan 2021 20:24:21 GMT, Sean Mullan 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. > > Can you add a test for this or is it too hard? There are existing tests for > StartTLS in the security/infra area -- could they be enhanced to test this > case? Unfortunately, I can not find any LDAP StartTLS Extended Operation regression tests. security/infra area contains RevocationChecker tests. They can not be used for this scenario. - PR: https://git.openjdk.java.net/jdk/pull/2085
Re: RFR: 8259707: LDAP channel binding does not work with StartTLS extension
On Thu, 14 Jan 2021 19:28:27 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. Can you add a test for this or is it too hard? There are existing tests for StartTLS in the security/infra area -- could they be enhanced to test this case? - PR: https://git.openjdk.java.net/jdk/pull/2085
