Hello Aleks, Thank you very much for review.
I’ve fixed missed spaces and removed casting from LdapSasl.java Failure of the SaslMutual test was caused by prop.remove() in the GssKrb5Client This operation is not required any more. GssKrb5Client receives temporary copy of the properties. Fixed Also, I’ve added references to RFC-5929 and RFC-5056 into the TlsChannelBinding class Updated patch is located at: http://cr.openjdk.java.net/~abakhtin/8245527/webrev.v7/ Regards Alexey > On 9 Jun 2020, at 15:59, Aleks Efimov <aleksej.efi...@oracle.com> wrote: > > Hi Alexey, > > Thank you for incorporating LdapCtx and LdapSasl changes. I've reviewed both > classes and they look good to me, with few minor comments in LdapSasl.java: > missing spaces in the following lines: 78, 152 > With your last changes we can remove explicit cast of 'envProps' on > line 176 > > I've also run your changes through our CI and one test is failing due to the > changes in GssKrb5Client: > The failed test name: sun/security/krb5/auto/SaslMutual.java > > The observed failure: > java.lang.UnsupportedOperationException > at > java.base/java.util.ImmutableCollections.uoe(ImmutableCollections.java:127) > at > java.base/java.util.ImmutableCollections$AbstractImmutableMap.remove(ImmutableCollections.java:910) > at > jdk.security.jgss/com.sun.security.sasl.gsskerb.GssKrb5Client.<init>(GssKrb5Client.java:156) > at > jdk.security.jgss/com.sun.security.sasl.gsskerb.FactoryImpl.createSaslClient(FactoryImpl.java:63) > at > java.security.sasl/javax.security.sasl.Sasl.createSaslClient(Sasl.java:433) > at SaslMutual.main(SaslMutual.java:50) > at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native > Method) > at > java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:64) > at > java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) > at java.base/java.lang.reflect.Method.invoke(Method.java:564) > at > com.sun.javatest.regtest.agent.MainWrapper$MainThread.run(MainWrapper.java:127) > at java.base/java.lang.Thread.run(Thread.java:832) > > > For information about CSR process you can start from this wiki page: > https://wiki.openjdk.java.net/display/csr > > Best Regards, > Aleksei > > On 08/06/2020 22:33, 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. >> >> 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. >>>>>>> >
signature.asc
Description: Message signed with OpenPGP
