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 - GssKrb5Client uses props.remove() to read and remove internal property Regards Alexey > On 5 Jun 2020, at 06:41, Weijun Wang <weijun.w...@oracle.com> wrote: > > Hi Alexey, > > It's so unfortunate that different addressType must be used. I'm OK with the > new TlsChannelBindingImpl class. > > One thing I'm not comfortable is the > java.security.sasl/share/classes/module-info.java change. We've struggled > hard to avoid these kind of secret tunnels. Is it possible to pass the > tlsCB.getData() directly to the SASL mechanism? The property name is clear > enough to avoid any misuse. > > Another question: can an application user set the > "com.sun.security.sasl.tlschannelbinding" property? and can they read it > after it's set internally? I cannot think of a good attack now but at least > it seems they have no need to access that property value. If we keep it > internal then we also have the chance to modify the approach later. > > Thanks, > Max > > >> On Jun 5, 2020, at 2:15 AM, Alexey Bakhtin <ale...@azul.com> wrote: >> >> Hello, >> >> Could you please review new version of the patch: >> http://cr.openjdk.java.net/~abakhtin/8245527/webrev.v3/ >> >> I’ve moved all logic for creating TLS Channel Binding data out of >> GssKrb5Client.java file. >> All data is prepared inside TlsChannelBinding class. >> Internal property name is renamed to >> “com.sun.security.sasl.tlschannelbinding”. >> TlsChannelBinding object is still used to pass channel binding data from >> LdapClient to GssKrb5Client. >> I do not change it to byte[] because of TlsChannelBinding class is still >> used for internal property name. >> Also, I’ve updated TlsChannelBinding class to select SHA-256 hash algorithm >> if it can not be derived >> from the certificate signature name. >> >> Regards >> Alexey >> >> >>> On 26 May 2020, at 17:50, Weijun Wang <weijun.w...@oracle.com> wrote: >>> >>> I have a question on GssKrb5Client.java: >>> >>> Do you think it's a good idea to let the SASL mechanism understand what TLS >>> binding is? Instead of passing the whole TlsChannelBinding object through a >>> SASL property, is it possible to only pass the actual cbData? After all, >>> the name "com.sun.security.sasl.channelbinding" suggests it's just a >>> general ChannelBinding which is independent with any application level info. >>> >>> From my reading of the code change, it looks like this cbData can be >>> calculated on the LDAP side. >>> >>> Thanks, >>> Max >>> >>>> On May 21, 2020, at 3:35 PM, Alexey Bakhtin <ale...@azul.com> wrote: >>>> >>>> Hello, >>>> >>>> Could you please review the following patch: >>>> >>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8245527 >>>> Webrev: http://cr.openjdk.java.net/~abakhtin/8245527/webrev.v0/ >>>> >>>> The Windows LDAP server with LdapEnforceChannelBinding=2 uses the >>>> tls-server-end-point channel binding >>>> (based on the TLS server certificate). The channel binding data is >>>> calculated as following : >>>> • The client calculates a hash of the TLS server certificate. >>>> The hash algorithm is selected on the base of the certificate >>>> signature algorithm. >>>> Also, the client should use SHA-256 algorithm, in case of the >>>> certificate signature algorithm is SHA1 or MD5 based >>>> • The channel binding information is the same as defined in rfc4121 [1] >>>> with small corrections: >>>> • initiator and acceptor addresses should be set to NULL >>>> • initiator and acceptor address types should be zero. >>>> It contradicts to the “Using Channel Bindings in GSS-API” >>>> document [2] that say that >>>> the address type should be set to GSS_C_AF_NULLADDR=0xFF, >>>> instead of GSS_C_AF_UNSPEC=0x00. >>>> >>>> This patch introduces changes in the LDAP, SASL and JGSS modules >>>> to generate channel binding data automatically if requested by an >>>> application. >>>> This patch reuses existing org.ietf.jgss.ChannelBinding class >>>> implementation but changes >>>> initial unspecified address type from CHANNEL_BINDING_AF_NULL_ADDR to >>>> CHANNEL_BINDING_AF_UNSPEC. >>>> The patch introduces new environment property >>>> “com.sun.jndi.ldap.tls.cbtype” that indicates >>>> Channel Binding type that should be used in the LDAPS connection over >>>> JGSS/Kerberos. >>>> Right now "tls-server-end-point" Channel Binding type is supported only. >>>> The client extracts server certificate from the underlying TLS connection >>>> and creates >>>> Channel Binding data for JGSS/Kerberos authentication if application >>>> indicates >>>> com.sun.jndi.ldap.tls.cbtype=tls-server-end-point property. >>>> Client application should also specify existing >>>> "com.sun.jndi.ldap.connect.timeout” property >>>> to force and wait TLS handshake completion before JGSS/Kerberos >>>> authentication data is generated. >>>> >>>> [1] - https://tools.ietf.org/html/rfc4121#section-4.1.1.2 >>>> >>>> [2] - >>>> https://docs.oracle.com/cd/E19120-01/open.solaris/819-2145/overview-52/index.html >>>> >>>> Initial discussion of this issue is available at security-dev list: >>>> https://mail.openjdk.java.net/pipermail/security-dev/2019-December/021052.html >>>> https://mail.openjdk.java.net/pipermail/security-dev/2020-January/021140.html >>>> https://mail.openjdk.java.net/pipermail/security-dev/2020-February/021278.html >>>> https://mail.openjdk.java.net/pipermail/security-dev/2020-May/021864.html >>
