> 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

Reply via email to