Hi Alexey,
I've looked through LdapCtx/LdapClient/SaslBind changes:
Do we need to check if CHANNEL_BINDING is set explicitly for all
connection types? Maybe we can move the check inside 'if (conn.sock
instanceof SSLSocket) {' block.
Also, instead of setting CHANNEL_BINDING in context environment and then
removing it in finally block, it would be better to clone the
environment, put calculated CHANNEL_BINDING into it, and pass the cloned
one to Sasl.createSaslClient.
Another suggestion about the code that verifies if both properties are
set before connection is started:
As you've already mentioned the new code in LdapCtx is only needed for
checking if timeout is set. Could we try to remove LdapCtx::cbType field
and all related methods from LdapCtx (this class is already
over-complicated and hard to read) and replace it with some static
method in LdapSasl? It will help to localize all changes to LdapSasl
except for one line in LdapCtx.
I mean something like this:
Replace
+
+ // verify LDAP channel binding property
+ if (cbType != null && connectTimeout == -1)
+ throw new
NamingException(TlsChannelBinding.CHANNEL_BINDING_TYPE +
+ " property requires " +
+ CONNECT_TIMEOUT +
+ " property is set.");
With
+
LdapSasl.checkCbParameters((String)envprops.get(TlsChannelBinding.CHANNEL_BINDING_TYPE),
connectTimeout);
And add something like that to LdapSasl (or maybe pass the full env here):
+ public static void checkCbParameters(String cbTypePropertyValue, int
connectTimeout) throws NamingException {
+ TlsChannelBindingType cbType =
TlsChannelBinding.parseType(cbTypePropertyValue);
+ // verify LDAP channel binding property
+ if (cbType != null && connectTimeout == -1) {
+ throw new NamingException(TlsChannelBinding.CHANNEL_BINDING_TYPE +
+ " property requires com.sun.jndi.ldap.connect.timeout" +
+ " property is set.");
+ }
+ }
Other LdapCtx/LdapClient/SaslBind changes look fine to me.
With Kind Regards,
Aleksei
On 06/06/2020 20:45, Alexey Bakhtin 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.w...@oracle.com> wrote:
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.
