> 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.
>
>>
>> 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
