RFR: 8245527: LDAP Cnannel Binding support for Java GSS/Kerberos
Aleks Efimov
aleksej.efimov at oracle.com
Mon Jun 8 13:07:21 UTC 2020
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.
More information about the security-dev
mailing list