RFR: 8245527: LDAP Cnannel Binding support for Java GSS/Kerberos

[Aleks Efimov]

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.wang at oracle.com> wrote:
>>
>>
>>
>>> On Jun 6, 2020, at 2:41 PM, Weijun Wang <weijun.wang at oracle.com> wrote:
>>>
>>>
>>>
>>>> On Jun 5, 2020, at 11:03 PM, Alexey Bakhtin <alexey at 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.