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

[Alexey Bakhtin]

Hi Daniel,

Thank you for review
Yes, I can move TlsChannelBinding class into the com.sun.jndi.ldap.sasl package and LdapClient related changes into the LdapSasl.saslBind method.
Also, you are right with exceptions. I will rename them to the NamingException.

However, I’d like to parse TLS Channel Binding property in the LdapCtx class. The reason is “com.sun.jndi.ldap.connect.timeout” property. This property should be set together with TLS Channel Binding. So, I’d like to verify if both properties are set before connection is started. The best place for it is LdapCtx.initEnv()
Is it acceptable ?

Thank you
Alexey

> On 5 Jun 2020, at 18:17, Daniel Fuchs <daniel.fuchs at oracle.com> wrote:
> 
> Hi Alexey,
> 
> Could we move the new code from LdapClient.java and LdapCtxt.java
> into the com.sun.jndi.ldap.sasl package, and possibly delay
> its execution until the com.sun.jndi.ldap.sasl.LdapSasl.saslBind
> method is called?
> 
> The new TlsChannelBinding class should also be preferably moved
> to com.sun.jndi.ldap.sasl since it's apparently only useful
> with SASL.
> 
> Also:
> 
> 2573             if (cbTypeProp.equals(TlsChannelBindingType.TLS_UNIQUE.getName())) {
> 2574                 throw new UnsupportedOperationException( "Channel binding type " +
> 2575                         TlsChannelBindingType.TLS_UNIQUE.getName() +
> 2576                         " is not supported");
> 2577             } else if (cbTypeProp.equals(TlsChannelBindingType.TLS_SERVER_END_POINT.getName())) {
> 2578                 if (connectTimeout == -1)
> 2579                     throw new IllegalStateException(CHANNEL_BINDING_TYPE + " property requires " +
> 2580                             CONNECT_TIMEOUT + " property is set.");
> 2581                 cbType = TlsChannelBindingType.TLS_SERVER_END_POINT;
> 2582             } else {
> 2583                 throw new IllegalArgumentException("Illegal value for " +
> 2584                         CHANNEL_BINDING_TYPE + " property.");
> 2585             }
> 
> is not correct - as IllegalArgumentException, IllegalStateException and UnsupportedOperationException will percolate up to the calling code, and
> are not documented at the public API level.
> These should really be NamingException.
> 
> best regards,
> 
> -- daniel
> 
> 
> 
> On 05/06/2020 16:03, Alexey Bakhtin 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
>> 	- GssKrb5Client uses props.remove() to read and remove internal property
>> Regards
>> Alexey

-------------- 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/20200605/ae1ea1b5/signature.asc>