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
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
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
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
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
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
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
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
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:
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
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
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.
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
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
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 :)
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.
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
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
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
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
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 =
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
> 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
> 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
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
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
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
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
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
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
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
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
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,
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
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:
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
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
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
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
(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
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
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
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
> 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
> 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
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
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
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
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
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
AT?)
>>>
>>> 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?
>>>
>>> Gruss
>>> Ber
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:
>
>
>
: 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 in the latest
tin
> Gesendet: Mittwoch, Mai 27, 2020 11:43 AM
> An: Valerie Peng
> Cc: security-...@openjdk.java.net; core-libs-dev@openjdk.java.net; Thomas
> Maslen
> Betreff: Re: RFR: 8245527: LDAP Cnannel Binding support for Java GSS/Kerberos
>
> Hello Valerie, Unfortunately, Windows L
> 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
: 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
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:
> 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
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
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
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
An: Weijun Wang
Cc: security-...@openjdk.java.net ;
core-libs-dev@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
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
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
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
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
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
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/
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
74 matches
Mail list logo
