On 7/7/20 12:29 PM, Alexey Bakhtin wrote:
Hello Sean,
Thank you for review.
You are right, we can eliminate requirements for connection timeout property.
I’ve added handshakeComletedListener to the LDAP over SSl. In this case we’ll
have no possible performance impact caused by synchronous handshake.
Great! And one less property to set too.
Also, it allows to exclude changes in the LdapCtx class.
Updated webrev : http://cr.openjdk.java.net/~abakhtin/8245527/webrev.v12/
You will also need to update the CSR to remove the connection timeout
property.
Also, you should document the "com.sun.jndi.ldap.tls.cbtype" property in
the java.naming module-info file as was done for other properties in
Daniel's recent RFR, once he has pushed it [1].
* src/java.naming/share/classes/com/sun/jndi/ldap/Connection.java
It looks like there is a possibility of deadlock if
getTlsServerCertificate() is called before handshakeCompleted(). In that
case the lock could be obtained first and the thread would end up
holding the lock and waiting forever.
Here are a few other mostly minor comments on the code:
*
src/java.naming/share/classes/com/sun/jndi/ldap/sasl/TlsChannelBinding.java
61 * Channel binding on the base of TLS Finished message
I think you mean to say "basis", not "base". Same comment on line 65.
87 throw new NamingException( "Channel binding type " +
Remove extra space before "Channel.
114 * Construct tls-server-point-point Channel Binding data
Typo: "point-point" -> "end-point".
*
src/java.security.jgss/share/classes/sun/security/jgss/krb5/InitialToken.java
380 // LDAP TLS Channel Binding requires
CHANNEL_BINDING_AF_UNSPEC address type
381 // for unspecified initiator and acceptor addresses
382 // CHANNEL_BINDING_AF_NULL_ADDR value should be used for
unspecified address
383 // in all other cases
This would read better if there were periods at the end of each sentence
on lines 381 and 383. The same comment applies to the comment block on
lines 207-210 in src/java.security.jgss/share/native/libj2gss/GSSLibStub.c
--Sean
[1] https://mail.openjdk.java.net/pipermail/net-dev/2020-July/014148.html
Regards
Alexey
On 7 Jul 2020, at 02:30, Sean Mullan <sean.mul...@oracle.com> wrote:
Thanks for that extra info.
I think it would be much cleaner to avoid having to set that property and instead start
the handshake synchronously if the "com.sun.jndi.ldap.tls.cbtype" property is
set. This way only one property needs to be set and you don't need to guess what an
acceptable value is for the timeout property, which could also cause the connection to be
interrupted before the TLS handshake is complete if you use too small of a value.
Or better yet, there may be another way to do this with JSSE where you wait for
the TLS connection to complete. I'll ask my team and get back to you.
--Sean
On 7/6/20 6:06 PM, Aleks Efimov wrote:
Hi Sean,
Alexey answered the same question for me:
I mean “com.sun.jndi.ldap.connect.timeout” property.
The positive value forces to start TLS handshake and wait for it completion
during the connectTimeout milliseconds:
Connection.java
if (connectTimeout > 0) {
int socketTimeout = sslSocket.getSoTimeout();
sslSocket.setSoTimeout(connectTimeout); // reuse full timeout value
sslSocket.startHandshake();
sslSocket.setSoTimeout(socketTimeout);
}
Without this property handshake is started later asynchronously.
As result
certs = ssock.getSession().getPeerCertificates();
in the LdapClient.java could return SSLPeerUnverifiedException().
This exception will be wrapped to NamingException and thrown to application.
This is not usually happens but I saw it on the slow connection
The full context of LDAP Connection code that initiates the SSL handshake could
be viewed here:
https://github.com/openjdk/jdk/blob/master/src/java.naming/share/classes/com/sun/jndi/ldap/Connection.java#L345
-- Aleksei
On 06/07/2020 21:11, Sean Mullan wrote:
Hi Alexey,
This may have been discussed already, but can you explain why the
"com.sun.jndi.ldap.connect.timeout" property needs to be set in order to use
this feature? That property is mostly used in tests to avoid long socket timeouts, etc.
Why does that need to be set? What problem are you trying to solve?
--Sean
On 7/3/20 11:31 AM, Alexey Bakhtin wrote:
I would suggest removing it. At least for the SASL GSS-API mech, it seems the
GSSContext object will not be leaked and no one has a chance to call
setChannelBinding again on it.
There is no spec saying setChannelBinding() can only be called once, so I'd
rather we don't enforce that, although you might say there is no need to call
it twice.
OK.
GSSContextImpl class is removed from patch.
Webrev : http://cr.openjdk.java.net/~abakhtin/8245527/webrev.v11
Thank you
Alexey
