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.
