On 6/9/20 12:40 PM, Xuelei Fan wrote:
About the prefix, it may follow RFC 5056 (See page 7, section 2.1).
o Specifications of channel bindings for any secure channels MUST
provide for a single, canonical octet string encoding of the
channel bindings. Under this framework, channel bindings MUST
start with the channel binding unique prefix followed by a colon
(ASCII 0x3A).
Thanks! Easy to miss.
I would recommend adding more comments in the code (ex, in
TLSChannelBinding) pointing to that RFC section, and other RFCs such
5929 for the tls cbtypes. Also, this RFC (and other specifications such
as RFC 5959) should be listed in the CSR so that we document precisely
what encodings and types the JDK implementation supports and is using.
--Sean
Xuelei
On 6/9/2020 8:52 AM, Alexey Bakhtin wrote:
Hello Sean,
Thank you for the link. I’ll follow it to create CSR
I could not find any clear document or specification for this Channel
Binding format.
The only document I found that describes this format is the following:
https://docs.microsoft.com/en-us/archive/blogs/openspecification/ntlm-and-channel-binding-hash-aka-extended-protection-for-authentication
So, it is hard to say - is it a standard or Microsoft implementation
specific
Regards
Alexey
On 9 Jun 2020, at 18:35, Sean Mullan <sean.mul...@oracle.com> wrote:
On 6/8/20 5:33 PM, Alexey Bakhtin wrote:
Hello Sean,
Yes, I think we'll need CSR and release notes as soon as this patch
adds new property.
I do not know exact process for it, so I will be grateful if you
could explain me exact steps.
The CSR process is documented at
https://wiki.openjdk.java.net/display/csr/Main. It should be fairly
self-explanatory but let me know if you have questions.
For the release note, we can tackle that later once the CSR is
approved now I have tagged the issue with the "release-note=yes"
label so we don't forget it.
This patch was developed to address compatibility issue with new
LDAP authentication over SSL/TLS announced by Microsoft [1]. It is
not related to RFC 5801. In my opinion
“com.sun.jndi.ldap.tls.cbtype” name looks more suitable for this
property and should allow backport it to early JDK versions.
Good point about backporting.
What RFC or specification defines the format you are using for the
channel binding in TlsChannelBinding.java, specifically where the
type prefix is encoded as "tls-server-end-point:" followed by the
binding data? I have looked through various RFCs but I can't find
exactly where this format is defined, so I am wondering if this is a
standard encoding or not.
Thanks,
Sean
[1] -
https://support.microsoft.com/en-au/help/4034879/how-to-add-the-ldapenforcechannelbinding-registry-entry
Regards
Alexey
On 8 Jun 2020, at 22:03, Sean Mullan <sean.mul...@oracle.com> wrote:
(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.
