Hello Sean,

Yes, I think we'll need CSR and release notes as soon as this patch adds new 
property.
I do not know exact process for it, so I will be grateful if you could explain 
me exact steps.

This patch was developed to address compatibility issue with new LDAP 
authentication over SSL/TLS announced by Microsoft [1]. It is not related to 
RFC 5801. In my opinion “com.sun.jndi.ldap.tls.cbtype” name looks more suitable 
for this property and should allow backport it to early JDK versions.

[1] - 
https://support.microsoft.com/en-au/help/4034879/how-to-add-the-ldapenforcechannelbinding-registry-entry
Regards
Alexey

> On 8 Jun 2020, at 22:03, Sean Mullan <sean.mul...@oracle.com> wrote:
> 
> (resending to all lists on the review)
> 
> I'm just catching up a bit on this review.
> 
> Sorry if this has mentioned before, but are you planning to write a CSR and 
> release note? I think this is needed for the com.sun.jndi.ldap.tls.cbtype 
> property. I'm also wondering if this property should be documented in the 
> javadocs, and why it is not a standard property (i.e. 
> "java.naming.ldap.tls.cbtype").
> 
> I was also wondering what relation this has to the "G2" standard SASL 
> mechanisms defined in RFC 5801 [1], and whether that is something we should 
> be using to negotiate this channel binding, and if not, why not. Or if this 
> is something that is implementation-specific and will only work with 
> Microsoft LDAP technology, in which case, we might want to make that more 
> explicit, perhaps by including "microsoft" or something like that in the 
> property name.
> 
> Thanks,
> Sean
> 
> [1] https://tools.ietf.org/html/rfc5801
> 
> On 6/8/20 9:07 AM, Aleks Efimov wrote:
>> Hi Alexey,
>> I've looked through LdapCtx/LdapClient/SaslBind changes:
>> Do we need to check if CHANNEL_BINDING is set explicitly for all connection 
>> types? Maybe we can move the check inside 'if (conn.sock instanceof 
>> SSLSocket) {' block.
>> Also, instead of setting CHANNEL_BINDING in context environment and then 
>> removing it in finally block, it would be better to clone the environment, 
>> put calculated CHANNEL_BINDING into it, and pass the cloned one to 
>> Sasl.createSaslClient.
>> Another suggestion about the code that verifies if both properties are set 
>> before connection is started:
>> As you've already mentioned the new code in LdapCtx is only needed for 
>> checking if timeout is set. Could we try to remove LdapCtx::cbType field and 
>> all related methods from LdapCtx (this class is already over-complicated and 
>> hard to read) and replace it with some static method in LdapSasl? It will 
>> help to localize all changes to LdapSasl except for one line in LdapCtx.
>> I mean something like this:
>> Replace
>> +
>> +            // verify LDAP channel binding property
>> +            if (cbType != null && connectTimeout == -1)
>> +                    throw new 
>> NamingException(TlsChannelBinding.CHANNEL_BINDING_TYPE +
>> +                            " property requires " +
>> +                            CONNECT_TIMEOUT +
>> +                            " property is set.");
>> With
>> + 
>> LdapSasl.checkCbParameters((String)envprops.get(TlsChannelBinding.CHANNEL_BINDING_TYPE),
>>  connectTimeout);
>> And add something like that to LdapSasl (or maybe pass the full env here):
>> + public static void checkCbParameters(String cbTypePropertyValue, int 
>> connectTimeout) throws NamingException {
>> +     TlsChannelBindingType cbType = 
>> TlsChannelBinding.parseType(cbTypePropertyValue);
>> +     // verify LDAP channel binding property
>> +     if (cbType != null && connectTimeout == -1) {
>> +         throw new NamingException(TlsChannelBinding.CHANNEL_BINDING_TYPE +
>> +                 " property requires com.sun.jndi.ldap.connect.timeout" +
>> +                 " property is set.");
>> +     }
>> + }
>> Other LdapCtx/LdapClient/SaslBind  changes look fine to me.
>> With Kind Regards,
>> Aleksei
>> On 06/06/2020 20:45, Alexey Bakhtin wrote:
>>> 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