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.

Reply via email to