RFR: 8245527: LDAP Cnannel Binding support for Java GSS/Kerberos

[Alexey Bakhtin]

Hello Max, Daniel,

Thank you for review and suggestions.

Could you please check the updated webrev:
http://cr.openjdk.java.net/~abakhtin/8245527/webrev.v6/

This version contains the following changes:
1. Updated license for new files
2. Comments about default address type usage in the InitialToken.java and GSSLibStub.c
3. internal property is renamed to “jdk.internal.sasl.tlschannelbinding"
4. I did not create test but provided detailed reproducer in the bug description
5. Property verification is moved from LdapCTX into LdapSasl as suggested by Aleks
6. Use clone of enviroment properties as suggested by Aleks

I did not move internal property check under 'if (conn.sock instanceof SSLSocket) {' block.
It was already discussed in [1] :
"If not TLS, this property value be kept there and visible inside the SASL mech."

[1] - https://mail.openjdk.java.net/pipermail/core-libs-dev/2020-June/067042.html

Thank you
Alexey

> On 7 Jun 2020, at 04:47, Weijun Wang <weijun.wang at oracle.com> wrote:
> 
> Some comments:
> 
> 1. I just noticed your 2 new files are using the plain GPU license, it should be GPL + Classpath. Read another source file for an example.
> 
> 2. Please add some comments in InitialToken.java on what happens to the default type.
> 
> 3. I still think "com.sun.security.sasl.tlschannelbinding" sounds like sun.com will support it.
> 
> 4. If an automatic test is not feasible, please provide some instructions so our SQE can see if we can setup some internal tests. Then we can add noreg-hard with some justification to the test and go on.
> 
> I cannot comment on LdapCtx.java, but the others look fine to me.
> 
> Thanks,
> Max
> 
>> On Jun 7, 2020, at 3:45 AM, Alexey Bakhtin <alexey at azul.com> 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.wang at oracle.com> wrote:
>>> 
>>> 
>>> 
>>>> On Jun 6, 2020, at 2:41 PM, Weijun Wang <weijun.wang at oracle.com> wrote:
>>>> 
>>>> 
>>>> 
>>>>> On Jun 5, 2020, at 11:03 PM, Alexey Bakhtin <alexey at 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.
>> 

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Message signed with OpenPGP
URL: <https://mail.openjdk.org/pipermail/security-dev/attachments/20200608/189faa8e/signature.asc>