Hello Max, Daniel, Thank you for review.
Please review new version of the patch : http://cr.openjdk.java.net/~abakhtin/8245527/webrev.v5/ In this version: - TlsChannelBinding class is moved into the com.sun.jndi.ldap.sasl package - SSL Ceritificate related code is moved from LdapClient into the LdapSasl.saslBind method - verification and removal of internal property is also moved to LdapSasl.saslBind method - verification of connectTimeout property is moved to LdapCtx.connect. I’ve found that connectionTimeout could be assigned later then cbType The test for this issue is not ready yet. I did not find any suitable test case that can be reused. Thank you Alexey > On 6 Jun 2020, at 09:44, Weijun Wang <weijun.w...@oracle.com> wrote: > > > >> On Jun 6, 2020, at 2:41 PM, Weijun Wang <weijun.w...@oracle.com> wrote: >> >> >> >>> On Jun 5, 2020, at 11:03 PM, Alexey Bakhtin <ale...@azul.com> wrote: >>> >>> Hello Max, >>> >>> Thank you a lot for review. >>> >>> Could you check the new version of the patch : >>> http://cr.openjdk.java.net/~abakhtin/8245527/webrev.v4/ >>> >>> I’ve made the following changes: >>> - TlsChannelBinding class is moved to java.naming module. >>> java.security.sasl module is not affected any more >>> - I pass tlsCB.getData() directly to the SASL mechanism as you suggested >>> - I’ve made some guards to prevent application from using >>> "com.sun.security.sasl.tlschannelbinding” property directly: >>> - LdapClient verifies if internal property is not set >> >> 245 // Prepare TLS Channel Binding data >> 246 if (conn.sock instanceof SSLSocket) { >> 247 // Internal property cannot be set explicitly >> 248 if (env.get(TlsChannelBinding.CHANNEL_BINDING) >> != null) { >> 249 throw new >> NamingException(TlsChannelBinding.CHANNEL_BINDING + >> 250 " property cannot be set >> explicitly"); >> 251 } >> >> If not TLS, this property value be kept there and visible inside the SASL >> mech. >> >>> - GssKrb5Client uses props.remove() to read and remove internal property > > Maybe you can remove the value in LdapClient.java, in case the mech used is > not GSSAPI. You can remove it in a finally block after line 303. > > --Max > >> >> Traditionally, we use "com.sun..." name which is a JDK supported name >> (although not at Java SE level), you might want to use a name which is even >> more internal. >> >> >> Thanks, >> Max >> >> p.s. I see that NTLM also supports ChannelBinding. I'll see if I can improve >> the NTLM SASL mech to support it.
