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
