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/5/20 12:45 PM, Daniel Fuchs wrote:
Hi Alexey,
On 05/06/2020 17:33, Alexey Bakhtin wrote:
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 ?
Yes - I am OK with that.
Also - you will need a test. Ideally we'd want a test that verifies
that setting the new property works as expected.
Best regards,
-- daniel
Thank you
Alexey
