Hi Alexey,
The last version looks good to me.
Best,
Aleksei
On 14/08/2020 15:42, Alexey Bakhtin wrote:
Sorry,
That’s my bad.
I have reverted @systemProperty to @code. The wording is fixed.
Webrev:
http://cr.openjdk.java.net/~abakhtin/8245527/webrev.v17/
Regards
Alexey
On 14 Aug 2020, at 17
Hi Alexey,
LGTM!
Thanks,
-- daniel
On 14/08/2020 15:42, Alexey Bakhtin wrote:
Sorry,
That’s my bad.
I have reverted @systemProperty to @code. The wording is fixed.
Webrev:
http://cr.openjdk.java.net/~abakhtin/8245527/webrev.v17/
Regards
Alexey
Sorry,
That’s my bad.
I have reverted @systemProperty to @code. The wording is fixed.
Webrev:
http://cr.openjdk.java.net/~abakhtin/8245527/webrev.v17/
Regards
Alexey
> On 14 Aug 2020, at 17:23, Sean Mullan wrote:
>
> Sorry you are right! Would be nice to have a more general @property for these
Sorry you are right! Would be nice to have a more general @property for
these types of properties and security properties, etc but that's a
different change ...
--Sean
On 8/14/20 10:18 AM, Daniel Fuchs wrote:
Hi Sean,
Wait wait... Are they system properties really?
Only system properties sh
Hi Sean,
Wait wait... Are they system properties really?
Only system properties should be documented with @systemProperty.
I can't find the place were com.sun.jndi.ldap.connect.timeout or
com.sun.jndi.ldap.read.timeout is read from System.
I believe they are just plain environment properties
th
Hi Sean, Alexey,
Adding @systemProperty doesn't look correct here since the properties
mentioned in the module-info.java are not system properties, they're
standard JNDI environment properties and setting them via system
property with the same name won't have any effect, e.g. "java
-Dcom.sun.
OK
Updated for all properties in the module-info.java
Webrev at: http://cr.openjdk.java.net/~abakhtin/8245527/webrev.v16/
Regards
Alexey
> On 14 Aug 2020, at 16:18, Sean Mullan wrote:
>
> On 8/14/20 8:41 AM, Alexey Bakhtin wrote:
>> Hello Sean,
>> Thank you for review.
>> I’ve changed wording
On 8/14/20 8:41 AM, Alexey Bakhtin wrote:
Hello Sean,
Thank you for review.
I’ve changed wording and replaced @code with @systemProperty (tested, it works
for module-info.java)
Thanks. Now that you know it works, I think you should change the other
properties documented in that file to @syst
Hello Sean,
Thank you for review.
I’ve changed wording and replaced @code with @systemProperty (tested, it works
for module-info.java)
Updated webrev at: http://cr.openjdk.java.net/~abakhtin/8245527/webrev.v15/
Regards
Alexey
> On 14 Aug 2020, at 14:50, Sean Mullan wrote:
>
> On the property
On the property wording, change "for LDAP connection" to "for an LDAP
connection".
Also, for the definition of the property, can you use the
"@systemProperty" annotation instead of "@code"? Does that work inside
the module-info.java file?
I added my name as Reviewer.
--Sean
On 7/30/20 6:14
Hi Alexey,
I have added myself as a reviewer to the CSR [1].
It would be good to get someone from security-dev to do the
same, and then move the CSR state to "Proposed".
best regards,
-- daniel
[1] https://bugs.openjdk.java.net/browse/JDK-8247311
On 30/07/2020 10:17, Alexey Bakhtin wrote:
Ge
Gentle ping
Regards
Alexey
> On 15 Jul 2020, at 14:18, Alexey Bakhtin wrote:
>
> Hello Daniel,
>
> I’ve updated CSR as you suggested and added kerberos ldap setup commands for
> the client host in the JDK-8245527
>
> Regards
> Alexey
signature.asc
Description: Message signed with OpenPGP
Hello Daniel,
I’ve updated CSR as you suggested and added kerberos ldap setup commands for
the client host in the JDK-8245527
Regards
Alexey
> On 14 Jul 2020, at 18:28, Daniel Fuchs wrote:
>
> Hi Alexey,
>
> On 10/07/2020 21:37, Alexey Bakhtin wrote:
>> Updated webrev:http://cr.openjdk.java.
Hi Alexey,
Thanks for addressing comments and answering questions. The JNDI changes
in latest webrev looks good to me.
CI is also happy with the latest changes.
Best,
Aleksei
On 10/07/2020 21:37, Alexey Bakhtin wrote:
Hello Aleksei,
Thank you for review.
Please see my comments below.
Updat
Hi Alexey,
On 10/07/2020 21:37, Alexey Bakhtin wrote:
Updated webrev:http://cr.openjdk.java.net/~abakhtin/8245527/webrev.v14/
In what the JNDI part is concerned this looks good to me now.
nit:
java.naming/share/classes/com/sun/jndi/ldap/sasl/TlsChannelBinding.java:
138 }catch(NoSuch
Hello Aleksei,
Thank you for review.
Please see my comments below.
Updated webrev: http://cr.openjdk.java.net/~abakhtin/8245527/webrev.v14/
Regards
Alexey
> On 10 Jul 2020, at 19:40, Aleks Efimov wrote:
>
> Hi Alexey,
>
> Thank you for removing the dependency on the timeout property and addi
Hi Alexey,
Thank you for removing the dependency on the timeout property and adding
tests for TLS handshake cases.
Please, find the comments about the latest webrev below:
Not quite sure why the CF is completed at two places. Probably that’s
OK, but it would be good to know the reason :)
T
Hello Sean, Daniel,
Thank you for review
I’ve added “com.sun.jndi.ldap.tls.cbtype” property into the module-info file
and updated synchronisation using CompletableFuture.
Also, I have added new test cases : successful and unsuccessful TLS handshake,
synchronous and asynchronous TLS handshake.
New
Thanks Sean, Alexey,
On 08/07/2020 13:25, Sean Mullan wrote:
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
th
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 hand
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.
Also, it allows to exclude changes in
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
valu
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 = sslSoc
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
> 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
> On Jul 3, 2020, at 7:32 PM, Alexey Bakhtin wrote:
>
> Hello All,
>
> Thank you for review.
>
>> 1. If the change in java.security.jgss/share/classes/module-info.java is
>> unavoidable, can we create a sub-package for this single class so that we
>> only need to export it?
>
> As suggest
Hello All,
Thank you for review.
> 1. If the change in java.security.jgss/share/classes/module-info.java is
> unavoidable, can we create a sub-package for this single class so that we
> only need to export it?
As suggested by Max I’ve moved TlsChannelBindingImpl class into sub-package, so
mod
The GSS and SASL changes look fine to me.
Two small questions:
1. If the change in java.security.jgss/share/classes/module-info.java is
unavoidable, can we create a sub-package for this single class so that we only
need to export it? Or, do you think we can define the sub-class inside
GssKrb5C
On 6/24/20 2:56 PM, Daniel Fuchs wrote:
The JNDI/LDAP part looks mostly good. You will need someone
from the security libs to review the security lib part of the
changes.
I have previously reviewed it but I would like to give it another once
over. Max should also review the final version as he
Hi Michael,
On 30/06/2020 15:57, Osipov, Michael wrote:
TLS channel binding is not tied to LDAP, it can be used with other
protocols, even custom ones. I see no good reason to have the property
contain jndi.ldap or use NamingException. IllegalArgumentException would
be approriate here
It is no
Am 2020-06-30 um 14:22 schrieb Alexey Bakhtin:
Hello Daniel,
Thank you for review.
I have updated LdapCBPropertiesTest according to your comments.
Updated webrev at http://cr.openjdk.java.net/~abakhtin/8245527/webrev.v9/
A few notes
+throw new
NamingException(TlsChannelBin
Hello Daniel,
Thank you for review.
I have updated LdapCBPropertiesTest according to your comments.
Updated webrev at http://cr.openjdk.java.net/~abakhtin/8245527/webrev.v9/
Regards
Alexey
signature.asc
Description: Message signed with OpenPGP
Hi Alexey,
The JNDI/LDAP part looks mostly good. You will need someone
from the security libs to review the security lib part of the
changes.
In the test I would recommend using the test URIBuilder to avoid
strange intermittent errors if the test is run on a
machine where looking up "localhost"
Hello Daniel,
Thank you for review.
As you suggested I’ve added static factory methods to create TlsChannelBinding
class and changed connectionTimeout verification to "if (connectTimeout <= 0)"
Also, I’ve added simple regression test to verify Channel Binding parameters.
Please find updated we
Hi Alexey,
This is starting to look good.
Thank you for persisting!
I have a couple of comments - on the LDAP/JNDI side.
There are several places where your new code is supposed to
trigger the throwing of a NamingException:
LdapSasl.java: lines 76, 85, 140, 168
Please write a test to verify
Hello Sean,
The link to CSR for this feature :
https://bugs.openjdk.java.net/browse/JDK-8247311
Regards
Alexey
> On 9 Jun 2020, at 19:50, Sean Mullan wrote:
>
> 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
Hello Aleks,
Thank you very much for review.
I’ve fixed missed spaces and removed casting from LdapSasl.java
Failure of the SaslMutual test was caused by prop.remove() in the GssKrb5Client
This operation is not required any more. GssKrb5Client receives temporary copy
of the properties. Fixed
Al
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, chan
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 cha
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-
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.ja
Hi Alexey,
Thank you for incorporating LdapCtx and LdapSasl changes. I've reviewed
both classes and they look good to me, with few minor comments in
LdapSasl.java:
missing spaces in the following lines: 78, 152
With your last changes we can remove explicit cast of 'envProps'
on li
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.
This patch was developed to address compatibility issue with new LDAP
authentication over SSL/TLS an
Hello Max, Daniel,
Thank you for review and suggestions.
Could you please check the updated webrev:
http://cr.openjdk.java.net/~abakhtin/8245527/webrev.v6/
This version contains the following changes:
1. Updated license for new files
2. Comments about default address type usage in the InitialTok
(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 docum
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
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 environ
Some comments:
1. I just noticed your 2 new files are using the plain GPU license, it should
be GPL + Classpath. Read another source file for an example.
2. Please add some comments in InitialToken.java on what happens to the default
type.
3. I still think "com.sun.security.sasl.tlschannelbind
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
> On Jun 6, 2020, at 2:41 PM, Weijun Wang wrote:
>
>
>
>> On Jun 5, 2020, at 11:03 PM, Alexey Bakhtin 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
> On Jun 5, 2020, at 11:03 PM, Alexey Bakhtin 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.
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
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
Hi Alexey,
Could we move the new code from LdapClient.java and LdapCtxt.java
into the com.sun.jndi.ldap.sasl package, and possibly delay
its execution until the com.sun.jndi.ldap.sasl.LdapSasl.saslBind
method is called?
The new TlsChannelBinding class should also be preferably moved
to com.sun.j
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 t
Hi Alexey,
It's so unfortunate that different addressType must be used. I'm OK with the
new TlsChannelBindingImpl class.
One thing I'm not comfortable is the
java.security.sasl/share/classes/module-info.java change. We've struggled hard
to avoid these kind of secret tunnels. Is it possible to
Hello,
Could you please review new version of the patch:
http://cr.openjdk.java.net/~abakhtin/8245527/webrev.v3/
I’ve moved all logic for creating TLS Channel Binding data out of
GssKrb5Client.java file.
All data is prepared inside TlsChannelBinding class.
Internal property name is renamed to “
_END_POINT_COMPAT?)
>>>
>>> This could be configured as a SASL property and it would add the benefit
>>> that you don't need the instance specific if in the gssstub native code if
>>> you instead have two different types values?
>>>
>>> Gr
Hi Max,
You are right, It is possible that algorithm name is not confirm
With format.
As soon as RFC5929 does not specify this situation I would suggest to use
“SHA-256” hash instead of throwing SaslException exception.
Regards
Alexey
> On 27 May 2020, at 13:25, Weijun Wang wrote:
>
>
>
>>
len
Betreff: Re: RFR: 8245527: LDAP Cnannel Binding support for Java GSS/Kerberos
Hello Valerie, Unfortunately, Windows LDAP server with
LdapEnforceChannelBinding=2 does not accept GSS_C_AF_NULLADDR address type.
This is exact reason of these changes. I ve tried to fix inconsistency of
address type
Bakhtin
> Gesendet: Mittwoch, Mai 27, 2020 11:43 AM
> An: Valerie Peng
> Cc: security-dev@openjdk.java.net; core-libs-...@openjdk.java.net; Thomas
> Maslen
> Betreff: Re: RFR: 8245527: LDAP Cnannel Binding support for Java GSS/Kerberos
>
> Hello Valerie, Unfortunately, Wind
> On May 21, 2020, at 3:35 PM, Alexey Bakhtin wrote:
>
> The hash algorithm is selected on the base of the certificate
> signature algorithm.
> Also, the client should use SHA-256 algorithm, in case of the
> certificate signature algorithm is SHA1 or MD5
According to ht
f: Re: RFR: 8245527: LDAP Cnannel Binding support for Java GSS/Kerberos
Hello Valerie, Unfortunately, Windows LDAP server with
LdapEnforceChannelBinding=2 does not accept GSS_C_AF_NULLADDR address type.
This is exact reason of these changes. I ve tried to fix inconsistency of
address type value i
Hello Valerie,
Unfortunately, Windows LDAP server with LdapEnforceChannelBinding=2 does not
accept GSS_C_AF_NULLADDR address type.
This is exact reason of these changes.
I’ve tried to fix inconsistency of address type value in the latest webrev:
http://cr.openjdk.java.net/~abakhtin/8245527/webrev
> On May 27, 2020, at 1:46 AM, Alexey Bakhtin wrote:
>
> Hello Max,
>
> Thank you review.
> If I understand correctly tls-server-end-point channel binding data is a hash
> of server certificate. Different SASL protocols could send cbData
> differently, with different prefix format.
Not sur
I am also concerned about the changes in GSSLibStub.c about the default
value being GSS_C_AF_UNSPECinstead of GSS_C_AF_NULLADDR (line 194-195).
Can you try and see if Window works with GSS_C_AF_NULLADDR? If yes, I'd
prefer to not changing the default value for addresstype for the same
reason w
: Weijun Wang
Cc: security-dev@openjdk.java.net ;
core-libs-...@openjdk.java.net ; Michael Osipov
Betreff: Re: RFR: 8245527: LDAP Cnannel Binding support for Java GSS/Kerberos
Hello Max,
Thank you review.
If I understand correctly tls-server-end-point channel binding data is a hash
of server
Hello Max,
Thank you review.
If I understand correctly tls-server-end-point channel binding data is a hash
of server certificate. Different SASL protocols could send cbData differently,
with different prefix format. This is a reason I create TLSChannelBinding class
and calculate hash from the L
Hello Aleks, Daniel,
Thank you for your comments.
I don’t like that the code is split into several modules too. The reason of
doing it is I can not get TLS server certificate from the JGSS/Kerberos code.
The only place Where I can fetch it is LdapClient.
If I understand your idea correctly, I ca
Hi Alexey,
I agree with all Daniel's comments.
Few thoughts about moving the implementation down to SASL layers:
Will it be possible to make this new code specific only for
JGSS/Kerberos authentication mechanism?
Maybe investigate moving all the new code to GssKrb5Client SASL client
implementa
I have a question on GssKrb5Client.java:
Do you think it's a good idea to let the SASL mechanism understand what TLS
binding is? Instead of passing the whole TlsChannelBinding object through a
SASL property, is it possible to only pass the actual cbData? After all, the
name "com.sun.security.sa
Hi Alexey,
This is not a review. A few high level comments however:
For that kind of change that introduce a new environment
property you will need to file a CSR, and probably provide
some release notes as well.
Your changes seem to trigger new IllegalStateException and
UnsupportedOperationExce
Hi Michael, Thomas,
Unfortunately we can not fix address type issue with the UnspecEmptyInetAddress
subclass.
We can not create subclass of InetAddress without changing public API.
We can try similar approach but create subclass of ChannelBinding class
instead. It is not so elegant like UnspecEm
On Mon, May 25, 2020 at 3:15 AM Michael Osipov <1983-01...@gmx.net> wrote:
[...]
> So I read the discussions. RFC 2744 shall not be changed, people
> admitted that the spec of SASL GS2 mechs is wrong and should be changed,
> but this has not happened yet. It remained at UNSPEC all the years.
>
> S
Hello Michael, Thomas,
Thank you a lot for review and suggestions.
I’ve fixed most of the issues except of fundamental one
I need more time to evaluate suggested usage of UnspecEmptyInetAddress subtype.
Updated webrev is available at:
http://cr.openjdk.java.net/~abakhtin/8245527/webrev.v1/
Also
age --
From: Michael Osipov <1983-01...@gmx.net>
To: Alexey Bakhtin , "core-libs-...@openjdk.java.net" <
core-libs-...@openjdk.java.net>
Cc: "security-dev@openjdk.java.net"
Bcc:
Date: Sun, 24 May 2020 01:38:06 +0200
Subject: Re: RFR: 8245527: LDAP Cnannel Bi
Am 2020-05-24 um 01:38 schrieb Michael Osipov:
Am 2020-05-21 um 09:35 schrieb Alexey Bakhtin:
...
What about introducing a UnspecEmptyInetAddress() which gives the proper
AF type, but #getAddress() would return null? This should satisfy the
requirements, InitialToken as well as the RFC. this of c
Am 2020-05-21 um 09:35 schrieb Alexey Bakhtin:
Hello,
Could you please review the following patch:
JBS: https://bugs.openjdk.java.net/browse/JDK-8245527
Webrev: http://cr.openjdk.java.net/~abakhtin/8245527/webrev.v0/
Let's go through your changes statically:
* The JIRA issue title has a typo
Hello,
Could you please review the following patch:
JBS: https://bugs.openjdk.java.net/browse/JDK-8245527
Webrev: http://cr.openjdk.java.net/~abakhtin/8245527/webrev.v0/
The Windows LDAP server with LdapEnforceChannelBinding=2 uses the
tls-server-end-point channel binding
(based on the TLS serv
79 matches
Mail list logo
