(resending to all lists on the review)
I'm just catching up a bit on this review.
Sorry if this has mentioned before, but are you planning to write a CSR
and release note? I think this is needed for the
com.sun.jndi.ldap.tls.cbtype property. I'm also wondering if this
property should be documented in the javadocs, and why it is not a
standard property (i.e. "java.naming.ldap.tls.cbtype").
I was also wondering what relation this has to the "G2" standard SASL
mechanisms defined in RFC 5801 [1], and whether that is something we
should be using to negotiate this channel binding, and if not, why not.
Or if this is something that is implementation-specific and will only
work with Microsoft LDAP technology, in which case, we might want to
make that more explicit, perhaps by including "microsoft" or something
like that in the property name.
Thanks,
Sean
[1] https://tools.ietf.org/html/rfc5801
On 6/8/20 9:07 AM, Aleks Efimov wrote:
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.
