Hello Sean, Thank you for the link. I’ll follow it to create CSR
I could not find any clear document or specification for this Channel Binding format. The only document I found that describes this format is the following: https://docs.microsoft.com/en-us/archive/blogs/openspecification/ntlm-and-channel-binding-hash-aka-extended-protection-for-authentication So, it is hard to say - is it a standard or Microsoft implementation specific Regards Alexey > On 9 Jun 2020, at 18:35, Sean Mullan <sean.mul...@oracle.com> wrote: > > On 6/8/20 5:33 PM, Alexey Bakhtin wrote: >> 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. > > The CSR process is documented at > https://wiki.openjdk.java.net/display/csr/Main. It should be fairly > self-explanatory but let me know if you have questions. > > For the release note, we can tackle that later once the CSR is approved now I > have tagged the issue with the "release-note=yes" label so we don't forget it. > >> 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. > > Good point about backporting. > > What RFC or specification defines the format you are using for the channel > binding in TlsChannelBinding.java, specifically where the type prefix is > encoded as "tls-server-end-point:" followed by the binding data? I have > looked through various RFCs but I can't find exactly where this format is > defined, so I am wondering if this is a standard encoding or not. > > Thanks, > Sean > >> [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.
signature.asc
Description: Message signed with OpenPGP
