Re: RFR: 8245527: LDAP Cnannel Binding support for Java GSS/Kerberos
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:23, Sean Mullan wrote: 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 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 that you need to specify in the environment map. best regards, -- daniel On 14/08/2020 14:18, Sean Mullan wrote: 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 @systemProperty to be consistent. I think it is fine to do that as part of this fix. --Sean
Re: RFR: 8245527: LDAP Cnannel Binding support for Java GSS/Kerberos
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
Re: RFR: 8245527: LDAP Cnannel Binding support for Java GSS/Kerberos
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 > 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 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 >> that you need to specify in the environment map. >> best regards, >> -- daniel >> On 14/08/2020 14:18, Sean Mullan wrote: >>> 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 @systemProperty to be consistent. I >>> think it is fine to do that as part of this fix. >>> >>> --Sean
Re: RFR: 8245527: LDAP Cnannel Binding support for Java GSS/Kerberos
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 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 that you need to specify in the environment map. best regards, -- daniel On 14/08/2020 14:18, Sean Mullan wrote: 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 @systemProperty to be consistent. I think it is fine to do that as part of this fix. --Sean
Re: RFR: 8245527: LDAP Cnannel Binding support for Java GSS/Kerberos
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 that you need to specify in the environment map. best regards, -- daniel On 14/08/2020 14:18, Sean Mullan wrote: 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 @systemProperty to be consistent. I think it is fine to do that as part of this fix. --Sean
Re: RFR: 8245527: LDAP Cnannel Binding support for Java GSS/Kerberos
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.jndi.ldap.tls.cbtype=tls-server-end-point". Best Regards, Aleksei On 14/08/2020 14:26, Alexey Bakhtin wrote: 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 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 @systemProperty to be consistent. I think it is fine to do that as part of this fix. --Sean 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 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 AM, Daniel Fuchs wrote: 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
Re: RFR: 8245527: LDAP Cnannel Binding support for Java GSS/Kerberos
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 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 @systemProperty to be consistent. I > think it is fine to do that as part of this fix. > > --Sean > >> 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 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 AM, Daniel Fuchs wrote: 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
Re: RFR: 8245527: LDAP Cnannel Binding support for Java GSS/Kerberos
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 @systemProperty to be consistent. I think it is fine to do that as part of this fix. --Sean 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 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 AM, Daniel Fuchs wrote: 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
Re: RFR: 8245527: LDAP Cnannel Binding support for Java GSS/Kerberos
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 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 AM, Daniel Fuchs wrote: >> 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
Re: RFR: 8245527: LDAP Cnannel Binding support for Java GSS/Kerberos
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 AM, Daniel Fuchs wrote: 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
Re: RFR: 8245527: LDAP Cnannel Binding support for Java GSS/Kerberos
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
Re: RFR: 8245527: LDAP Cnannel Binding support for Java GSS/Kerberos
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
Re: RFR: 8245527: LDAP Cnannel Binding support for Java GSS/Kerberos
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.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(NoSuchAlgorithmException | CertificateEncodingException e)
> {
>
> missing spaces around `catch`; No need for a new webrev.
>
> Please make sure to update the CSR, and in particular update
> the specification section with the diffs from
>
> src/java.naming/share/classes/module-info.java
>
> Also I am not sure the links that are currently in the
> specification section are at their place. They may be better
> placed in the Solution section (the solution is to implement
> the client part of the channel binding as described by these
> documents in the default JNDI/LDAP/GSS provider).
>
> Since we don't really have any end-to-end regression test
> (what we have is mostly a smoke test) - it would be good if
> you could describe in more details what you did to test your
> fix against a real server in a JBS comment in JDK-8245527 - so
> that someone (future or current maintainers) could reproduce
> this testing to verify that nothing is broken by future evolutions.
> In particular - if anything specific needs to be
> installed/configured on the test machine (LDAP server? Which?
> Is that all?)
>
>
> best regards,
>
> -- daniel
Re: RFR: 8245527: LDAP Cnannel Binding support for Java GSS/Kerberos
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. 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 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 :) HandshakeCompletedListener is called in case of successful handshake only. In case of async handshake failed we close the connection and complete CF exceptionally to release CF.get() The ExecutionException could be unpacked instead of using it directly - and its cause used instead. Thank you. Fixed in Connection.java and LdapCBPropertiesTest.java 'getTlsServerCertificate' should return null if 'isTlsConnection()' is false - rather than waiting forever. We call isTlsConnection() in the LdapSasl before getTlsServerCertificate(). But your are right, we can double check it to prevent possible deadlock in the future, if code changed. Fixed in Connection.java java.naming/share/classes/module-info.java: could we try to improve the formatting of the possible values for the new system property - maybe format them as a list. Done Connection.java:995 - you could use diamond operator here Updated Formatting: Connection.java:1027 'catch (‘ Updated Could we use the test/jdk/com/sun/jndi/ldap/lib/BaseLdapServer.java from the test library to implement dummy LDAPS server, I believe it could help to increase the test verbosity and reduce its code size. Thank you for suggestion. Updated to use BaseLdapServer in the test LdapCBPropertiesTest.java:122 - could use no param Hastable constructor Fixed I've also run your latest patch through our CI system and it showed no failures with your latest changes. -Aleksei On 09/07/2020 20:34, Alexey Bakhtin wrote: 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 webrev at : http://cr.openjdk.java.net/~abakhtin/8245527/webrev.v13 Connection property is removed from CSR : https://bugs.openjdk.java.net/browse/JDK-8247311 Regards Alexey On 8 Jul 2020, at 17:41, Daniel Fuchs wrote: 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 the java.naming module-info file as was done for other properties in Daniel's recent RFR, once he has pushed it [1]. I have pushed the fix: https://hg.openjdk.java.net/jdk/jdk/rev/ed375ae6c779 Alexey, you should now be able to document your new "com.sun.jndi.ldap.tls.cbtype" property in that @implNote as well, and update your CSR consequently. * src/java.naming/share/classes/com/sun/jndi/ldap/Connection.java It looks like there is a possibility of deadlock if getTlsServerCertificate() is called before handshakeCompleted(). In that case the lock could be obtained first and the thread would end up holding the lock and waiting forever. I also have a concern with the use of wait/notify in that code. I would suggest prototyping using a CompletableFuture instead (or can new certificates be exchanged later on the same connection?) CompletableFuture would allow you to relay and propagate any exception raised in the callback handler as well, and would avoid running into deadlocks. best regards, -- daniel
Re: RFR: 8245527: LDAP Cnannel Binding support for Java GSS/Kerberos
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(NoSuchAlgorithmException |
CertificateEncodingException e) {
missing spaces around `catch`; No need for a new webrev.
Please make sure to update the CSR, and in particular update
the specification section with the diffs from
src/java.naming/share/classes/module-info.java
Also I am not sure the links that are currently in the
specification section are at their place. They may be better
placed in the Solution section (the solution is to implement
the client part of the channel binding as described by these
documents in the default JNDI/LDAP/GSS provider).
Since we don't really have any end-to-end regression test
(what we have is mostly a smoke test) - it would be good if
you could describe in more details what you did to test your
fix against a real server in a JBS comment in JDK-8245527 - so
that someone (future or current maintainers) could reproduce
this testing to verify that nothing is broken by future evolutions.
In particular - if anything specific needs to be
installed/configured on the test machine (LDAP server? Which?
Is that all?)
best regards,
-- daniel
Re: RFR: 8245527: LDAP Cnannel Binding support for Java GSS/Kerberos
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 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 :) HandshakeCompletedListener is called in case of successful handshake only. In case of async handshake failed we close the connection and complete CF exceptionally to release CF.get() > > The ExecutionException could be unpacked instead of using it directly - and > its cause used instead. Thank you. Fixed in Connection.java and LdapCBPropertiesTest.java > > 'getTlsServerCertificate' should return null if 'isTlsConnection()' is false > - rather than waiting forever. We call isTlsConnection() in the LdapSasl before getTlsServerCertificate(). But your are right, we can double check it to prevent possible deadlock in the future, if code changed. Fixed in Connection.java > > java.naming/share/classes/module-info.java: could we try to improve the > formatting of the possible values for the new system property - maybe format > them as a list. Done > > Connection.java:995 - you could use diamond operator here Updated > > Formatting: Connection.java:1027 'catch (‘ Updated > > Could we use the test/jdk/com/sun/jndi/ldap/lib/BaseLdapServer.java from the > test library to implement dummy LDAPS server, I believe it could help to > increase the test verbosity and reduce its code size. Thank you for suggestion. Updated to use BaseLdapServer in the test > > LdapCBPropertiesTest.java:122 - could use no param Hastable constructor Fixed > > I've also run your latest patch through our CI system and it showed no > failures with your latest changes. > > -Aleksei > > On 09/07/2020 20:34, Alexey Bakhtin wrote: >> 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 webrev at : http://cr.openjdk.java.net/~abakhtin/8245527/webrev.v13 >> >> Connection property is removed from CSR : >> https://bugs.openjdk.java.net/browse/JDK-8247311 >> >> Regards >> Alexey >> >>> On 8 Jul 2020, at 17:41, Daniel Fuchs wrote: >>> >>> 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 the java.naming module-info file as was done for other properties in Daniel's recent RFR, once he has pushed it [1]. >>> I have pushed the fix: >>> https://hg.openjdk.java.net/jdk/jdk/rev/ed375ae6c779 >>> >>> Alexey, you should now be able to document your new >>> "com.sun.jndi.ldap.tls.cbtype" >>> property in that @implNote as well, and update your CSR >>> consequently. >>> * src/java.naming/share/classes/com/sun/jndi/ldap/Connection.java It looks like there is a possibility of deadlock if getTlsServerCertificate() is called before handshakeCompleted(). In that case the lock could be obtained first and the thread would end up holding the lock and waiting forever. >>> I also have a concern with the use of wait/notify in that code. >>> I would suggest prototyping using a CompletableFuture instead >>> (or can new certificates be exchanged later on the same >>> connection?) >>> >>> CompletableFuture would allow you to relay and propagate any >>> exception raised in the callback handler as well, and would >>> avoid running into deadlocks. >>> >>> best regards, >>> >>> -- daniel
Re: RFR: 8245527: LDAP Cnannel Binding support for Java GSS/Kerberos
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 :)
The ExecutionException could be unpacked instead of using it directly -
and its cause used instead.
'getTlsServerCertificate' should return null if 'isTlsConnection()' is
false - rather than waiting forever.
java.naming/share/classes/module-info.java: could we try to improve the
formatting of the possible values for the new system property - maybe
format them as a list.
Connection.java:995 - you could use diamond operator here
Formatting: Connection.java:1027 'catch ('
Could we use the test/jdk/com/sun/jndi/ldap/lib/BaseLdapServer.java from
the test library to implement dummy LDAPS server, I believe it could
help to increase the test verbosity and reduce its code size.
LdapCBPropertiesTest.java:122 - could use no param Hastable constructor
I've also run your latest patch through our CI system and it showed no
failures with your latest changes.
-Aleksei
On 09/07/2020 20:34, Alexey Bakhtin wrote:
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 webrev at : http://cr.openjdk.java.net/~abakhtin/8245527/webrev.v13
Connection property is removed from CSR :
https://bugs.openjdk.java.net/browse/JDK-8247311
Regards
Alexey
On 8 Jul 2020, at 17:41, Daniel Fuchs wrote:
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 the
java.naming module-info file as was done for other properties in Daniel's recent RFR,
once he has pushed it [1].
I have pushed the fix:
https://hg.openjdk.java.net/jdk/jdk/rev/ed375ae6c779
Alexey, you should now be able to document your new
"com.sun.jndi.ldap.tls.cbtype"
property in that @implNote as well, and update your CSR
consequently.
* src/java.naming/share/classes/com/sun/jndi/ldap/Connection.java
It looks like there is a possibility of deadlock if getTlsServerCertificate()
is called before handshakeCompleted(). In that case the lock could be obtained
first and the thread would end up holding the lock and waiting forever.
I also have a concern with the use of wait/notify in that code.
I would suggest prototyping using a CompletableFuture instead
(or can new certificates be exchanged later on the same
connection?)
CompletableFuture would allow you to relay and propagate any
exception raised in the callback handler as well, and would
avoid running into deadlocks.
best regards,
-- daniel
Re: RFR: 8245527: LDAP Cnannel Binding support for Java GSS/Kerberos
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 webrev at : http://cr.openjdk.java.net/~abakhtin/8245527/webrev.v13 Connection property is removed from CSR : https://bugs.openjdk.java.net/browse/JDK-8247311 Regards Alexey > On 8 Jul 2020, at 17:41, Daniel Fuchs wrote: > > 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 the >> java.naming module-info file as was done for other properties in Daniel's >> recent RFR, once he has pushed it [1]. > > I have pushed the fix: > https://hg.openjdk.java.net/jdk/jdk/rev/ed375ae6c779 > > Alexey, you should now be able to document your new > "com.sun.jndi.ldap.tls.cbtype" > property in that @implNote as well, and update your CSR > consequently. > >> * src/java.naming/share/classes/com/sun/jndi/ldap/Connection.java >> It looks like there is a possibility of deadlock if >> getTlsServerCertificate() is called before handshakeCompleted(). In that >> case the lock could be obtained first and the thread would end up holding >> the lock and waiting forever. > > I also have a concern with the use of wait/notify in that code. > I would suggest prototyping using a CompletableFuture instead > (or can new certificates be exchanged later on the same > connection?) > > CompletableFuture would allow you to relay and propagate any > exception raised in the callback handler as well, and would > avoid running into deadlocks. > > best regards, > > -- daniel
Re: RFR: 8245527: LDAP Cnannel Binding support for Java GSS/Kerberos
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 the java.naming module-info file as was done for other properties in Daniel's recent RFR, once he has pushed it [1]. I have pushed the fix: https://hg.openjdk.java.net/jdk/jdk/rev/ed375ae6c779 Alexey, you should now be able to document your new "com.sun.jndi.ldap.tls.cbtype" property in that @implNote as well, and update your CSR consequently. * src/java.naming/share/classes/com/sun/jndi/ldap/Connection.java It looks like there is a possibility of deadlock if getTlsServerCertificate() is called before handshakeCompleted(). In that case the lock could be obtained first and the thread would end up holding the lock and waiting forever. I also have a concern with the use of wait/notify in that code. I would suggest prototyping using a CompletableFuture instead (or can new certificates be exchanged later on the same connection?) CompletableFuture would allow you to relay and propagate any exception raised in the callback handler as well, and would avoid running into deadlocks. best regards, -- daniel
Re: RFR: 8245527: LDAP Cnannel Binding support for Java GSS/Kerberos
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 handshake.
Great! And one less property to set too.
Also, it allows to exclude changes in the LdapCtx class.
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
the java.naming module-info file as was done for other properties in
Daniel's recent RFR, once he has pushed it [1].
* src/java.naming/share/classes/com/sun/jndi/ldap/Connection.java
It looks like there is a possibility of deadlock if
getTlsServerCertificate() is called before handshakeCompleted(). In that
case the lock could be obtained first and the thread would end up
holding the lock and waiting forever.
Here are a few other mostly minor comments on the code:
*
src/java.naming/share/classes/com/sun/jndi/ldap/sasl/TlsChannelBinding.java
61 * Channel binding on the base of TLS Finished message
I think you mean to say "basis", not "base". Same comment on line 65.
87 throw new NamingException( "Channel binding type " +
Remove extra space before "Channel.
114 * Construct tls-server-point-point Channel Binding data
Typo: "point-point" -> "end-point".
*
src/java.security.jgss/share/classes/sun/security/jgss/krb5/InitialToken.java
380 // LDAP TLS Channel Binding requires
CHANNEL_BINDING_AF_UNSPEC address type
381 // for unspecified initiator and acceptor addresses
382 // CHANNEL_BINDING_AF_NULL_ADDR value should be used for
unspecified address
383 // in all other cases
This would read better if there were periods at the end of each sentence
on lines 381 and 383. The same comment applies to the comment block on
lines 207-210 in src/java.security.jgss/share/native/libj2gss/GSSLibStub.c
--Sean
[1] https://mail.openjdk.java.net/pipermail/net-dev/2020-July/014148.html
Regards
Alexey
On 7 Jul 2020, at 02:30, Sean Mullan wrote:
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 value is for the timeout property, which could also cause the connection to be
interrupted before the TLS handshake is complete if you use too small of a value.
Or better yet, there may be another way to do this with JSSE where you wait for
the TLS connection to complete. I'll ask my team and get back to you.
--Sean
On 7/6/20 6:06 PM, Aleks Efimov wrote:
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 = sslSocket.getSoTimeout();
sslSocket.setSoTimeout(connectTimeout); // reuse full timeout value
sslSocket.startHandshake();
sslSocket.setSoTimeout(socketTimeout);
}
Without this property handshake is started later asynchronously.
As result
certs = ssock.getSession().getPeerCertificates();
in the LdapClient.java could return SSLPeerUnverifiedException().
This exception will be wrapped to NamingException and thrown to application.
This is not usually happens but I saw it on the slow connection
The full context of LDAP Connection code that initiates the SSL handshake could
be viewed here:
https://github.com/openjdk/jdk/blob/master/src/java.naming/share/classes/com/sun/jndi/ldap/Connection.java#L345
-- Aleksei
On 06/07/2020 21:11, Sean Mullan wrote:
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 trying to solve?
--Sean
On 7/3/20 11:31 AM, Alexey Bakhtin wrote:
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, although you might say there is no need to call
it twice.
OK.
GSSContextImpl class is removed from patch.
Webrev : http://cr.openjdk.java.net/~abakhtin/8245527/webrev.v11
Thank you
Alexey
Re: RFR: 8245527: LDAP Cnannel Binding support for Java GSS/Kerberos
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 the LdapCtx class.
Updated webrev : http://cr.openjdk.java.net/~abakhtin/8245527/webrev.v12/
Regards
Alexey
> On 7 Jul 2020, at 02:30, Sean Mullan wrote:
>
> 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 value is for
> the timeout property, which could also cause the connection to be interrupted
> before the TLS handshake is complete if you use too small of a value.
>
> Or better yet, there may be another way to do this with JSSE where you wait
> for the TLS connection to complete. I'll ask my team and get back to you.
>
> --Sean
>
>
> On 7/6/20 6:06 PM, Aleks Efimov wrote:
>> 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 = sslSocket.getSoTimeout();
> sslSocket.setSoTimeout(connectTimeout); // reuse full timeout value
> sslSocket.startHandshake();
> sslSocket.setSoTimeout(socketTimeout);
> }
>>> Without this property handshake is started later asynchronously.
>>> As result
>certs = ssock.getSession().getPeerCertificates();
>>> in the LdapClient.java could return SSLPeerUnverifiedException().
>>> This exception will be wrapped to NamingException and thrown to application.
>>>
>>> This is not usually happens but I saw it on the slow connection
>> The full context of LDAP Connection code that initiates the SSL handshake
>> could be viewed here:
>> https://github.com/openjdk/jdk/blob/master/src/java.naming/share/classes/com/sun/jndi/ldap/Connection.java#L345
>> -- Aleksei
>> On 06/07/2020 21:11, Sean Mullan wrote:
>>> 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 trying to solve?
>>>
>>> --Sean
>>>
>>>
>>> On 7/3/20 11:31 AM, Alexey Bakhtin wrote:
> 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, although you might say there is no need
> to call it twice.
OK.
GSSContextImpl class is removed from patch.
Webrev : http://cr.openjdk.java.net/~abakhtin/8245527/webrev.v11
Thank you
Alexey
Re: RFR: 8245527: LDAP Cnannel Binding support for Java GSS/Kerberos
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
value is for the timeout property, which could also cause the connection
to be interrupted before the TLS handshake is complete if you use too
small of a value.
Or better yet, there may be another way to do this with JSSE where you
wait for the TLS connection to complete. I'll ask my team and get back
to you.
--Sean
On 7/6/20 6:06 PM, Aleks Efimov wrote:
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 = sslSocket.getSoTimeout();
sslSocket.setSoTimeout(connectTimeout); // reuse full timeout value
sslSocket.startHandshake();
sslSocket.setSoTimeout(socketTimeout);
}
Without this property handshake is started later asynchronously.
As result
certs = ssock.getSession().getPeerCertificates();
in the LdapClient.java could return SSLPeerUnverifiedException().
This exception will be wrapped to NamingException and thrown to application.
This is not usually happens but I saw it on the slow connection
The full context of LDAP Connection code that initiates the SSL
handshake could be viewed here:
https://github.com/openjdk/jdk/blob/master/src/java.naming/share/classes/com/sun/jndi/ldap/Connection.java#L345
-- Aleksei
On 06/07/2020 21:11, Sean Mullan wrote:
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 trying to solve?
--Sean
On 7/3/20 11:31 AM, Alexey Bakhtin wrote:
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, although you might say there is
no need to call it twice.
OK.
GSSContextImpl class is removed from patch.
Webrev : http://cr.openjdk.java.net/~abakhtin/8245527/webrev.v11
Thank you
Alexey
Re: RFR: 8245527: LDAP Cnannel Binding support for Java GSS/Kerberos
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 = sslSocket.getSoTimeout();
sslSocket.setSoTimeout(connectTimeout); // reuse full timeout value
sslSocket.startHandshake();
sslSocket.setSoTimeout(socketTimeout);
}
Without this property handshake is started later asynchronously.
As result
certs = ssock.getSession().getPeerCertificates();
in the LdapClient.java could return SSLPeerUnverifiedException().
This exception will be wrapped to NamingException and thrown to application.
This is not usually happens but I saw it on the slow connection
The full context of LDAP Connection code that initiates the SSL
handshake could be viewed here:
https://github.com/openjdk/jdk/blob/master/src/java.naming/share/classes/com/sun/jndi/ldap/Connection.java#L345
-- Aleksei
On 06/07/2020 21:11, Sean Mullan wrote:
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 trying to solve?
--Sean
On 7/3/20 11:31 AM, Alexey Bakhtin wrote:
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, although you might say there is
no need to call it twice.
OK.
GSSContextImpl class is removed from patch.
Webrev : http://cr.openjdk.java.net/~abakhtin/8245527/webrev.v11
Thank you
Alexey
Re: RFR: 8245527: LDAP Cnannel Binding support for Java GSS/Kerberos
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 trying to solve? --Sean On 7/3/20 11:31 AM, Alexey Bakhtin wrote: 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, although you might say there is no need to call it twice. OK. GSSContextImpl class is removed from patch. Webrev : http://cr.openjdk.java.net/~abakhtin/8245527/webrev.v11 Thank you Alexey
Re: RFR: 8245527: LDAP Cnannel Binding support for Java GSS/Kerberos
> 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, although you might say there is no need to call > it twice. OK. GSSContextImpl class is removed from patch. Webrev : http://cr.openjdk.java.net/~abakhtin/8245527/webrev.v11 Thank you Alexey
Re: RFR: 8245527: LDAP Cnannel Binding support for Java GSS/Kerberos
> 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 suggested by Max I’ve moved TlsChannelBindingImpl class into sub-package, > so module-info.java exports TlsChannelBindingImpl only. Thanks. > >> >> 2. Is GSSContextImpl::setChannelBinding really necessary? I don't know if >> there are people using null to erase a CB set earlier. > > I think these changes could be useful to exclude situations when application > trying to set Channel Binding with GSSContext::setChannelBinding and > “com.sun.jndi.ldap.tls.cbtype” property altogether. I can remove it, if you > think it is not necessary. 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, although you might say there is no need to call it twice. > > Also, I've fixed Exception text and parseType(String prop) parameter name as > suggested by Michael. > Unfortunately, I can not completely exclude usage of the literal names > because of module import issues. Fixed in the TlsChannelBinding class only. > > Webrev: http://cr.openjdk.java.net/~abakhtin/8245527/webrev.v10/ Thanks, Max > > Regards > Alexey >
Re: RFR: 8245527: LDAP Cnannel Binding support for Java GSS/Kerberos
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 module-info.java exports TlsChannelBindingImpl only. > > 2. Is GSSContextImpl::setChannelBinding really necessary? I don't know if > there are people using null to erase a CB set earlier. I think these changes could be useful to exclude situations when application trying to set Channel Binding with GSSContext::setChannelBinding and “com.sun.jndi.ldap.tls.cbtype” property altogether. I can remove it, if you think it is not necessary. Also, I've fixed Exception text and parseType(String prop) parameter name as suggested by Michael. Unfortunately, I can not completely exclude usage of the literal names because of module import issues. Fixed in the TlsChannelBinding class only. Webrev: http://cr.openjdk.java.net/~abakhtin/8245527/webrev.v10/ Regards Alexey
Re: RFR: 8245527: LDAP Cnannel Binding support for Java GSS/Kerberos
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 GssKrb5Client and directly check its class name in InitialToken? This is a little ugly so you're free to ignore it. 2. Is GSSContextImpl::setChannelBinding really necessary? I don't know if there are people using null to erase a CB set earlier. Thanks, Max > On Jul 3, 2020, at 2:33 AM, Sean Mullan wrote: > > 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 is the expert in JGSS. > > Given we are around US holidays/vacation, expect some delay. > > I suppose this is a bit of a grey area, but given the scope and the > introduction of new properties and initial support for TLS channel binding, > it seems more like an Enhancement than a Bug to me. That would kind of rule > it out for JDK 15, but I suspect that what is more important is backporting > this to prior releases, and that can be figured out later. So, JDK 16 as an > initial target seems ok to me. > > Thanks, > Sean
Re: RFR: 8245527: LDAP Cnannel Binding support for Java GSS/Kerberos
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 is the expert in JGSS. Given we are around US holidays/vacation, expect some delay. I suppose this is a bit of a grey area, but given the scope and the introduction of new properties and initial support for TLS channel binding, it seems more like an Enhancement than a Bug to me. That would kind of rule it out for JDK 15, but I suspect that what is more important is backporting this to prior releases, and that can be figured out later. So, JDK 16 as an initial target seems ok to me. Thanks, Sean
Re: RFR: 8245527: LDAP Cnannel Binding support for Java GSS/Kerberos
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
Re: RFR: 8245527: LDAP Cnannel Binding support for Java GSS/Kerberos
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" doesn't yield back
InetAddress.getLoopbackAddress():
--
* @library /test/lib
...
import java.net.URI;
import jdk.test.lib.net.URIBuilder;
...
URI uri = URIBuilder.newBuilder()
.scheme("ldaps")
.loopback()
.port(serverPort)
.build();
env.put(Context.PROVIDER_URL, uri.toString());
--
So we have now two new properties:
jdk.internal.sasl.tlschannelbinding which is a private contract between
java.naming and jdk.security.jgss;
com.sun.jndi.ldap.tls.cbtype which is a new JDK implementation specific
environment property for the InitialLdapContext, and is depending
on another JDK specific environment property:
"com.sun.jndi.ldap.connect.timeout"
None of these properties are currently documented in the JDK itself.
Although jdk.internal.sasl.tlschannelbinding probably doesn't need
to be documented (but I'll defer to the security experts for that),
the other two probably should. Where is the question.
If we had a jdk.namimg.ldap module then the documentation for
these jndi properties would probably need to go in its module-info.java
API documentation, but we don't. Obviously we will want to write
a release note for this fix that documents the new
com.sun.jndi.ldap.tls.cbtype property - but will that be
sufficient? The CSR committee might wish for more.
Anyone has advice to share on this?
best regards,
-- daniel
On 17/06/2020 12:26, Alexey Bakhtin wrote:
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 webrev
at:http://cr.openjdk.java.net/~abakhtin/8245527/webrev.v8/
The link to CSR for this feature
:https://bugs.openjdk.java.net/browse/JDK-8247311
Regards
Alexey
Re: RFR: 8245527: LDAP Cnannel Binding support for Java GSS/Kerberos
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 webrev at:
http://cr.openjdk.java.net/~abakhtin/8245527/webrev.v8/
The link to CSR for this feature :
https://bugs.openjdk.java.net/browse/JDK-8247311
Regards
Alexey
> On 12 Jun 2020, at 12:20, Daniel Fuchs wrote:
>
> 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 that it does so. Since the
> exceptions are all supposed to be thrown before the actual
> binding happens, there's no need to have an actual LDAP server
> that supports any kind of authentication to test that.
>
> The simple dummy servers that we have in the test base should
> be enough to write such a test.
>
> In addition, there are a couple of places where stray exceptions
> could theoretically be thrown, and where they should be wrapped in
> NamingException (but are not). Maybe it's OK, because this has
> been checked before - but it would be better if we had a test that
> proves that it is so:
>
> For instance: LdapSasl.java
> 82 connectTimeout = Integer.parseInt((String)propVal);
> What if the value of the propVal is "Foo" - or not a String?
> What unexpected exception might be relayed to the calling code?
>
> Still in that same file:
> 84 if (connectTimeout == -1)
>
> should probably be if (connectTimeout <= 0) since
> Connection.java checks for connectTimeout > 0 to determine
> whether to start the initial handshake.
>
> Which makes me wonder if we should find a better way to
> instruct Connection to start the initial handshake...
>
> TlsChannelBinding.java:
>
> It would be much better if static factories methods were used instead of
> public constructors. That would allow you to check all arguments before
> actually constructing your object:
>
> public static TlsChannelBinding create((byte[] finishedMessage) throws
> SaslException {
>throw new UnsupportedOperationException("tls-unique channel binding is not
> supported");
> }
>
> public statuic TlsChannelBinding create(X509Certificate serverCertificate)
> throws SaslException {
> var cbType = TlsChannelBindingType.TLS_SERVER_END_POINT;
> byte[] cbData;
> try {
> // compute cbdata
> ...
>
> return new TlsChannelBinding(cbType, cbData);
> }
>
> private TlsChannelBinding(TlsChannelBindingType cbType, byte[] cbData) {
> this.cbType = cbType;
> this.cbData = cbData;
> }
>
> That's all I have for now.
>
> best regards,
>
> -- daniel
>
Re: RFR: 8245527: LDAP Cnannel Binding support for Java GSS/Kerberos
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 that it does so. Since the
exceptions are all supposed to be thrown before the actual
binding happens, there's no need to have an actual LDAP server
that supports any kind of authentication to test that.
The simple dummy servers that we have in the test base should
be enough to write such a test.
In addition, there are a couple of places where stray exceptions
could theoretically be thrown, and where they should be wrapped in
NamingException (but are not). Maybe it's OK, because this has
been checked before - but it would be better if we had a test that
proves that it is so:
For instance: LdapSasl.java
82 connectTimeout = Integer.parseInt((String)propVal);
What if the value of the propVal is "Foo" - or not a String?
What unexpected exception might be relayed to the calling code?
Still in that same file:
84 if (connectTimeout == -1)
should probably be if (connectTimeout <= 0) since
Connection.java checks for connectTimeout > 0 to determine
whether to start the initial handshake.
Which makes me wonder if we should find a better way to
instruct Connection to start the initial handshake...
TlsChannelBinding.java:
It would be much better if static factories methods were used instead of
public constructors. That would allow you to check all arguments before
actually constructing your object:
public static TlsChannelBinding create((byte[] finishedMessage) throws
SaslException {
throw new UnsupportedOperationException("tls-unique channel binding
is not supported");
}
public statuic TlsChannelBinding create(X509Certificate
serverCertificate) throws SaslException {
var cbType = TlsChannelBindingType.TLS_SERVER_END_POINT;
byte[] cbData;
try {
// compute cbdata
...
return new TlsChannelBinding(cbType, cbData);
}
private TlsChannelBinding(TlsChannelBindingType cbType, byte[] cbData) {
this.cbType = cbType;
this.cbData = cbData;
}
That's all I have for now.
best regards,
-- daniel
On 09/06/2020 22:03, Alexey Bakhtin wrote:
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
Also, I’ve added references to RFC-5929 and RFC-5056 into the TlsChannelBinding
class
Updated patch is located at:
http://cr.openjdk.java.net/~abakhtin/8245527/webrev.v7/
Regards
Alexey
Re: RFR: 8245527: LDAP Cnannel Binding support for Java GSS/Kerberos
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 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
Re: RFR: 8245527: LDAP Cnannel Binding support for Java GSS/Kerberos
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
Also, I’ve added references to RFC-5929 and RFC-5056 into the TlsChannelBinding
class
Updated patch is located at:
http://cr.openjdk.java.net/~abakhtin/8245527/webrev.v7/
Regards
Alexey
> On 9 Jun 2020, at 15:59, Aleks Efimov wrote:
>
> 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
> line 176
>
> I've also run your changes through our CI and one test is failing due to the
> changes in GssKrb5Client:
> The failed test name: sun/security/krb5/auto/SaslMutual.java
>
> The observed failure:
> java.lang.UnsupportedOperationException
> at
> java.base/java.util.ImmutableCollections.uoe(ImmutableCollections.java:127)
> at
> java.base/java.util.ImmutableCollections$AbstractImmutableMap.remove(ImmutableCollections.java:910)
> at
> jdk.security.jgss/com.sun.security.sasl.gsskerb.GssKrb5Client.(GssKrb5Client.java:156)
> at
> jdk.security.jgss/com.sun.security.sasl.gsskerb.FactoryImpl.createSaslClient(FactoryImpl.java:63)
> at
> java.security.sasl/javax.security.sasl.Sasl.createSaslClient(Sasl.java:433)
> at SaslMutual.main(SaslMutual.java:50)
> at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native
> Method)
> at
> java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:64)
> at
> java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
> at java.base/java.lang.reflect.Method.invoke(Method.java:564)
> at
> com.sun.javatest.regtest.agent.MainWrapper$MainThread.run(MainWrapper.java:127)
> at java.base/java.lang.Thread.run(Thread.java:832)
>
>
> For information about CSR process you can start from this wiki page:
> https://wiki.openjdk.java.net/display/csr
>
> Best Regards,
> Aleksei
>
> On 08/06/2020 22:33, 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.
>>
>> 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.
>>
>> [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
>>> 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
Re: RFR: 8245527: LDAP Cnannel Binding support for Java GSS/Kerberos
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 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 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 +
+ "
Re: RFR: 8245527: LDAP Cnannel Binding support for Java GSS/Kerberos
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).
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 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 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 {
+
Re: RFR: 8245527: LDAP Cnannel Binding support for Java GSS/Kerberos
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 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 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 =
Re: RFR: 8245527: LDAP Cnannel Binding support for Java GSS/Kerberos
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 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
-
Re: RFR: 8245527: LDAP Cnannel Binding support for Java GSS/Kerberos
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 line 176
I've also run your changes through our CI and one test is failing due to
the changes in GssKrb5Client:
The failed test name: sun/security/krb5/auto/SaslMutual.java
The observed failure:
java.lang.UnsupportedOperationException
at
java.base/java.util.ImmutableCollections.uoe(ImmutableCollections.java:127)
at
java.base/java.util.ImmutableCollections$AbstractImmutableMap.remove(ImmutableCollections.java:910)
at
jdk.security.jgss/com.sun.security.sasl.gsskerb.GssKrb5Client.(GssKrb5Client.java:156)
at
jdk.security.jgss/com.sun.security.sasl.gsskerb.FactoryImpl.createSaslClient(FactoryImpl.java:63)
at
java.security.sasl/javax.security.sasl.Sasl.createSaslClient(Sasl.java:433)
at SaslMutual.main(SaslMutual.java:50)
at
java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native
Method)
at
java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:64)
at
java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.base/java.lang.reflect.Method.invoke(Method.java:564)
at
com.sun.javatest.regtest.agent.MainWrapper$MainThread.run(MainWrapper.java:127)
at java.base/java.lang.Thread.run(Thread.java:832)
For information about CSR process you can start from this wiki page:
https://wiki.openjdk.java.net/display/csr
Best Regards,
Aleksei
On 08/06/2020 22:33, 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.
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.
[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 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
Re: RFR: 8245527: LDAP Cnannel Binding support for Java GSS/Kerberos
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 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.
[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 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 wrote:
> On Jun 6, 2020, at 2:41 PM, Weijun Wang wrote:
>
>
>
>> On Jun 5, 2020, at 11:03 PM, Alexey Bakhtin wrote:
>>
>> Hello Max,
>>
>>
Re: RFR: 8245527: LDAP Cnannel Binding support for Java GSS/Kerberos
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 InitialToken.java and
GSSLibStub.c
3. internal property is renamed to “jdk.internal.sasl.tlschannelbinding"
4. I did not create test but provided detailed reproducer in the bug description
5. Property verification is moved from LdapCTX into LdapSasl as suggested by
Aleks
6. Use clone of enviroment properties as suggested by Aleks
I did not move internal property check under 'if (conn.sock instanceof
SSLSocket) {' block.
It was already discussed in [1] :
"If not TLS, this property value be kept there and visible inside the SASL
mech."
[1] -
https://mail.openjdk.java.net/pipermail/core-libs-dev/2020-June/067042.html
Thank you
Alexey
> On 7 Jun 2020, at 04:47, Weijun Wang wrote:
>
> 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.tlschannelbinding" sounds like
> sun.com will support it.
>
> 4. If an automatic test is not feasible, please provide some instructions so
> our SQE can see if we can setup some internal tests. Then we can add
> noreg-hard with some justification to the test and go on.
>
> I cannot comment on LdapCtx.java, but the others look fine to me.
>
> Thanks,
> Max
>
>> On Jun 7, 2020, at 3:45 AM, 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 wrote:
>>>
>>>
>>>
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 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.
>>
Re: RFR: 8245527: LDAP Cnannel Binding support for Java GSS/Kerberos
(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 wrote:
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 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
Re: RFR: 8245527: LDAP Cnannel Binding support for Java GSS/Kerberos
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 wrote:
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 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.
Re: RFR: 8245527: LDAP Cnannel Binding support for Java GSS/Kerberos
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.tlschannelbinding" sounds like sun.com
will support it.
4. If an automatic test is not feasible, please provide some instructions so
our SQE can see if we can setup some internal tests. Then we can add noreg-hard
with some justification to the test and go on.
I cannot comment on LdapCtx.java, but the others look fine to me.
Thanks,
Max
> On Jun 7, 2020, at 3:45 AM, 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 wrote:
>>
>>
>>
>>> 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 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.
>
Re: RFR: 8245527: LDAP Cnannel Binding support for Java GSS/Kerberos
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 wrote:
>
>
>
>> 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 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.
Re: RFR: 8245527: LDAP Cnannel Binding support for Java GSS/Kerberos
> 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 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.
>
>>
>> Regards
>> Alexey
>>
>>> On 5 Jun 2020, at 06:41, Weijun Wang wrote:
>>>
>>> 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 pass the
>>> tlsCB.getData() directly to the SASL mechanism? The property name is clear
>>> enough to avoid any misuse.
>>>
>>> Another question: can an application user set the
>>> "com.sun.security.sasl.tlschannelbinding" property? and can they read it
>>> after it's set internally? I cannot think of a good attack now but at least
>>> it seems they have no need to access that property value. If we keep it
>>> internal then we also have the chance to modify the approach later.
>>>
>>> Thanks,
>>> Max
>>>
>>>
On Jun 5, 2020, at 2:15 AM, Alexey Bakhtin wrote:
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
“com.sun.security.sasl.tlschannelbinding”.
TlsChannelBinding object is still used to pass channel binding data from
LdapClient to GssKrb5Client.
I do not change it to byte[] because of TlsChannelBinding class is still
used for internal property name.
Also, I’ve updated TlsChannelBinding class to select SHA-256 hash
algorithm if it can not be derived
from the certificate signature name.
Regards
Alexey
> On 26 May 2020, at 17:50, Weijun Wang wrote:
>
> 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.sasl.channelbinding" suggests it's
> just a general ChannelBinding which is independent with any application
> level info.
>
> From my reading of the code change, it looks like this cbData can be
> calculated on the LDAP side.
>
> Thanks,
> Max
>
>> On May 21, 2020, at 3:35 PM, Alexey Bakhtin wrote:
>>
>> 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 server certificate). The channel binding data is
>> calculated as following :
>> • The client calculates a hash of the TLS server certificate.
>> The hash algorithm is selected on the base of the
Re: RFR: 8245527: LDAP Cnannel Binding support for Java GSS/Kerberos
> 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.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
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.
>
> Regards
> Alexey
>
>> On 5 Jun 2020, at 06:41, Weijun Wang wrote:
>>
>> 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 pass the
>> tlsCB.getData() directly to the SASL mechanism? The property name is clear
>> enough to avoid any misuse.
>>
>> Another question: can an application user set the
>> "com.sun.security.sasl.tlschannelbinding" property? and can they read it
>> after it's set internally? I cannot think of a good attack now but at least
>> it seems they have no need to access that property value. If we keep it
>> internal then we also have the chance to modify the approach later.
>>
>> Thanks,
>> Max
>>
>>
>>> On Jun 5, 2020, at 2:15 AM, Alexey Bakhtin wrote:
>>>
>>> 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
>>> “com.sun.security.sasl.tlschannelbinding”.
>>> TlsChannelBinding object is still used to pass channel binding data from
>>> LdapClient to GssKrb5Client.
>>> I do not change it to byte[] because of TlsChannelBinding class is still
>>> used for internal property name.
>>> Also, I’ve updated TlsChannelBinding class to select SHA-256 hash algorithm
>>> if it can not be derived
>>> from the certificate signature name.
>>>
>>> Regards
>>> Alexey
>>>
>>>
On 26 May 2020, at 17:50, Weijun Wang wrote:
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.sasl.channelbinding" suggests it's
just a general ChannelBinding which is independent with any application
level info.
From my reading of the code change, it looks like this cbData can be
calculated on the LDAP side.
Thanks,
Max
> On May 21, 2020, at 3:35 PM, Alexey Bakhtin wrote:
>
> 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 server certificate). The channel binding data is
> calculated as following :
> • The client calculates a hash of the TLS server certificate.
> 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 based
> • The channel binding information is the same as defined in rfc4121 [1]
> with small corrections:
> • initiator and acceptor addresses should be set to
Re: RFR: 8245527: LDAP Cnannel Binding support for Java GSS/Kerberos
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
Re: RFR: 8245527: LDAP Cnannel Binding support for Java GSS/Kerberos
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 ?
Thank you
Alexey
> On 5 Jun 2020, at 18:17, Daniel Fuchs wrote:
>
> 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.jndi.ldap.sasl since it's apparently only useful
> with SASL.
>
> Also:
>
> 2573 if
> (cbTypeProp.equals(TlsChannelBindingType.TLS_UNIQUE.getName())) {
> 2574 throw new UnsupportedOperationException( "Channel
> binding type " +
> 2575 TlsChannelBindingType.TLS_UNIQUE.getName() +
> 2576 " is not supported");
> 2577 } else if
> (cbTypeProp.equals(TlsChannelBindingType.TLS_SERVER_END_POINT.getName())) {
> 2578 if (connectTimeout == -1)
> 2579 throw new IllegalStateException(CHANNEL_BINDING_TYPE
> + " property requires " +
> 2580 CONNECT_TIMEOUT + " property is set.");
> 2581 cbType = TlsChannelBindingType.TLS_SERVER_END_POINT;
> 2582 } else {
> 2583 throw new IllegalArgumentException("Illegal value for " +
> 2584 CHANNEL_BINDING_TYPE + " property.");
> 2585 }
>
> is not correct - as IllegalArgumentException, IllegalStateException and
> UnsupportedOperationException will percolate up to the calling code, and
> are not documented at the public API level.
> These should really be NamingException.
>
> best regards,
>
> -- daniel
>
>
>
> On 05/06/2020 16:03, 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.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
>> - GssKrb5Client uses props.remove() to read and remove internal property
>> Regards
>> Alexey
Re: RFR: 8245527: LDAP Cnannel Binding support for Java GSS/Kerberos
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.jndi.ldap.sasl since it's apparently only useful
with SASL.
Also:
2573 if
(cbTypeProp.equals(TlsChannelBindingType.TLS_UNIQUE.getName())) {
2574 throw new UnsupportedOperationException( "Channel
binding type " +
2575 TlsChannelBindingType.TLS_UNIQUE.getName() +
2576 " is not supported");
2577 } else if
(cbTypeProp.equals(TlsChannelBindingType.TLS_SERVER_END_POINT.getName())) {
2578 if (connectTimeout == -1)
2579 throw new
IllegalStateException(CHANNEL_BINDING_TYPE + " property requires " +
2580 CONNECT_TIMEOUT + " property is set.");
2581 cbType = TlsChannelBindingType.TLS_SERVER_END_POINT;
2582 } else {
2583 throw new IllegalArgumentException("Illegal value
for " +
2584 CHANNEL_BINDING_TYPE + " property.");
2585 }
is not correct - as IllegalArgumentException, IllegalStateException and
UnsupportedOperationException will percolate up to the calling code, and
are not documented at the public API level.
These should really be NamingException.
best regards,
-- daniel
On 05/06/2020 16:03, 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.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
- GssKrb5Client uses props.remove() to read and remove internal property
Regards
Alexey
Re: RFR: 8245527: LDAP Cnannel Binding support for Java GSS/Kerberos
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 - GssKrb5Client uses props.remove() to read and remove internal property Regards Alexey > On 5 Jun 2020, at 06:41, Weijun Wang wrote: > > 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 pass the > tlsCB.getData() directly to the SASL mechanism? The property name is clear > enough to avoid any misuse. > > Another question: can an application user set the > "com.sun.security.sasl.tlschannelbinding" property? and can they read it > after it's set internally? I cannot think of a good attack now but at least > it seems they have no need to access that property value. If we keep it > internal then we also have the chance to modify the approach later. > > Thanks, > Max > > >> On Jun 5, 2020, at 2:15 AM, Alexey Bakhtin wrote: >> >> 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 >> “com.sun.security.sasl.tlschannelbinding”. >> TlsChannelBinding object is still used to pass channel binding data from >> LdapClient to GssKrb5Client. >> I do not change it to byte[] because of TlsChannelBinding class is still >> used for internal property name. >> Also, I’ve updated TlsChannelBinding class to select SHA-256 hash algorithm >> if it can not be derived >> from the certificate signature name. >> >> Regards >> Alexey >> >> >>> On 26 May 2020, at 17:50, Weijun Wang wrote: >>> >>> 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.sasl.channelbinding" suggests it's just a >>> general ChannelBinding which is independent with any application level info. >>> >>> From my reading of the code change, it looks like this cbData can be >>> calculated on the LDAP side. >>> >>> Thanks, >>> Max >>> On May 21, 2020, at 3:35 PM, Alexey Bakhtin wrote: 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 server certificate). The channel binding data is calculated as following : • The client calculates a hash of the TLS server certificate. 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 based • The channel binding information is the same as defined in rfc4121 [1] with small corrections: • initiator and acceptor addresses should be set to NULL • initiator and acceptor address types should be zero. It contradicts to the “Using Channel Bindings in GSS-API” document [2] that say that the address type should be set to GSS_C_AF_NULLADDR=0xFF, instead of GSS_C_AF_UNSPEC=0x00. This patch introduces changes in the LDAP, SASL and JGSS modules to generate channel binding data automatically if requested by an application. This patch reuses existing org.ietf.jgss.ChannelBinding class implementation but changes initial unspecified address type from CHANNEL_BINDING_AF_NULL_ADDR to CHANNEL_BINDING_AF_UNSPEC. The patch introduces new environment property “com.sun.jndi.ldap.tls.cbtype” that indicates Channel Binding type that should be used in the LDAPS connection over JGSS/Kerberos. Right now "tls-server-end-point" Channel Binding type is supported only. The client extracts server certificate from the underlying TLS connection
Re: RFR: 8245527: LDAP Cnannel Binding support for Java GSS/Kerberos
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 pass the tlsCB.getData() directly to the SASL mechanism? The property name is clear enough to avoid any misuse. Another question: can an application user set the "com.sun.security.sasl.tlschannelbinding" property? and can they read it after it's set internally? I cannot think of a good attack now but at least it seems they have no need to access that property value. If we keep it internal then we also have the chance to modify the approach later. Thanks, Max > On Jun 5, 2020, at 2:15 AM, Alexey Bakhtin wrote: > > 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 > “com.sun.security.sasl.tlschannelbinding”. > TlsChannelBinding object is still used to pass channel binding data from > LdapClient to GssKrb5Client. > I do not change it to byte[] because of TlsChannelBinding class is still used > for internal property name. > Also, I’ve updated TlsChannelBinding class to select SHA-256 hash algorithm > if it can not be derived > from the certificate signature name. > > Regards > Alexey > > >> On 26 May 2020, at 17:50, Weijun Wang wrote: >> >> 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.sasl.channelbinding" suggests it's just a general >> ChannelBinding which is independent with any application level info. >> >> From my reading of the code change, it looks like this cbData can be >> calculated on the LDAP side. >> >> Thanks, >> Max >> >>> On May 21, 2020, at 3:35 PM, Alexey Bakhtin wrote: >>> >>> 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 server certificate). The channel binding data is >>> calculated as following : >>> • The client calculates a hash of the TLS server certificate. >>> 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 based >>> • The channel binding information is the same as defined in rfc4121 [1] >>> with small corrections: >>> • initiator and acceptor addresses should be set to NULL >>> • initiator and acceptor address types should be zero. >>>It contradicts to the “Using Channel Bindings in GSS-API” >>> document [2] that say that >>>the address type should be set to GSS_C_AF_NULLADDR=0xFF, >>>instead of GSS_C_AF_UNSPEC=0x00. >>> >>> This patch introduces changes in the LDAP, SASL and JGSS modules >>> to generate channel binding data automatically if requested by an >>> application. >>> This patch reuses existing org.ietf.jgss.ChannelBinding class >>> implementation but changes >>> initial unspecified address type from CHANNEL_BINDING_AF_NULL_ADDR to >>> CHANNEL_BINDING_AF_UNSPEC. >>> The patch introduces new environment property >>> “com.sun.jndi.ldap.tls.cbtype” that indicates >>> Channel Binding type that should be used in the LDAPS connection over >>> JGSS/Kerberos. >>> Right now "tls-server-end-point" Channel Binding type is supported only. >>> The client extracts server certificate from the underlying TLS connection >>> and creates >>> Channel Binding data for JGSS/Kerberos authentication if application >>> indicates >>> com.sun.jndi.ldap.tls.cbtype=tls-server-end-point property. >>> Client application should also specify existing >>> "com.sun.jndi.ldap.connect.timeout” property >>> to force and wait TLS handshake completion before JGSS/Kerberos >>> authentication data is generated. >>> >>> [1] - https://tools.ietf.org/html/rfc4121#section-4.1.1.2 >>> >>> [2] - >>> https://docs.oracle.com/cd/E19120-01/open.solaris/819-2145/overview-52/index.html >>> >>> Initial discussion of this issue is available at security-dev list: >>> https://mail.openjdk.java.net/pipermail/security-dev/2019-December/021052.html >>> https://mail.openjdk.java.net/pipermail/security-dev/2020-January/021140.html >>>
Re: RFR: 8245527: LDAP Cnannel Binding support for Java GSS/Kerberos
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 “com.sun.security.sasl.tlschannelbinding”. TlsChannelBinding object is still used to pass channel binding data from LdapClient to GssKrb5Client. I do not change it to byte[] because of TlsChannelBinding class is still used for internal property name. Also, I’ve updated TlsChannelBinding class to select SHA-256 hash algorithm if it can not be derived from the certificate signature name. Regards Alexey > On 26 May 2020, at 17:50, Weijun Wang wrote: > > 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.sasl.channelbinding" suggests it's just a general > ChannelBinding which is independent with any application level info. > > From my reading of the code change, it looks like this cbData can be > calculated on the LDAP side. > > Thanks, > Max > >> On May 21, 2020, at 3:35 PM, Alexey Bakhtin wrote: >> >> 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 server certificate). The channel binding data is >> calculated as following : >> • The client calculates a hash of the TLS server certificate. >> 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 based >> • The channel binding information is the same as defined in rfc4121 [1] >> with small corrections: >> • initiator and acceptor addresses should be set to NULL >> • initiator and acceptor address types should be zero. >> It contradicts to the “Using Channel Bindings in GSS-API” >> document [2] that say that >> the address type should be set to GSS_C_AF_NULLADDR=0xFF, >> instead of GSS_C_AF_UNSPEC=0x00. >> >> This patch introduces changes in the LDAP, SASL and JGSS modules >> to generate channel binding data automatically if requested by an >> application. >> This patch reuses existing org.ietf.jgss.ChannelBinding class implementation >> but changes >> initial unspecified address type from CHANNEL_BINDING_AF_NULL_ADDR to >> CHANNEL_BINDING_AF_UNSPEC. >> The patch introduces new environment property “com.sun.jndi.ldap.tls.cbtype” >> that indicates >> Channel Binding type that should be used in the LDAPS connection over >> JGSS/Kerberos. >> Right now "tls-server-end-point" Channel Binding type is supported only. >> The client extracts server certificate from the underlying TLS connection >> and creates >> Channel Binding data for JGSS/Kerberos authentication if application >> indicates >> com.sun.jndi.ldap.tls.cbtype=tls-server-end-point property. >> Client application should also specify existing >> "com.sun.jndi.ldap.connect.timeout” property >> to force and wait TLS handshake completion before JGSS/Kerberos >> authentication data is generated. >> >> [1] - https://tools.ietf.org/html/rfc4121#section-4.1.1.2 >> >> [2] - >> https://docs.oracle.com/cd/E19120-01/open.solaris/819-2145/overview-52/index.html >> >> Initial discussion of this issue is available at security-dev list: >> https://mail.openjdk.java.net/pipermail/security-dev/2019-December/021052.html >> https://mail.openjdk.java.net/pipermail/security-dev/2020-January/021140.html >> https://mail.openjdk.java.net/pipermail/security-dev/2020-February/021278.html >> https://mail.openjdk.java.net/pipermail/security-dev/2020-May/021864.html
Re: RFR: 8245527: LDAP Cnannel Binding support for Java GSS/Kerberos
Hello Aleks,
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 = sslSocket.getSoTimeout();
>> sslSocket.setSoTimeout(connectTimeout); // reuse full timeout value
>> sslSocket.startHandshake();
>> sslSocket.setSoTimeout(socketTimeout);
>> }
Without this property handshake is started later asynchronously.
As result
>>certs = ssock.getSession().getPeerCertificates();
in the LdapClient.java could return SSLPeerUnverifiedException().
This exception will be wrapped to NamingException and thrown to application.
This is not usually happens but I saw it on the slow connection
Alexey
> On 27 May 2020, at 16:13, Aleks Efimov wrote:
>
> Hi Alexey,
>
> I have question about timeouts:
> LdapCtx has 2 timeout properties: connectTimeout and readTimeout.
> First one is for controlling the Socket::connect timeout
> (Connection::createSocket), another - for reading out the replies
> (Connection::readReply).
> Both of them have default values set to '-1' which means that the LDAP stack
> would not set any timeouts for connect and/or reading operations.
>
> Could you, please, share the failures you're observing with no connect
> timeout set?
>
> Best,
> Aleksei
>
> On 27/05/2020 11:48, Alexey Bakhtin wrote:
>> Hello Bernd,
>>
>>> On 27 May 2020, at 13:12, Bernd Eckenfels wrote:
>>>
>>> LdapCtxt:
>>> 2568 /**
>>> 2569 * Sets the read timeout value
>>> 2570 */
>>> 2571 private void setChannelBindingType(String cbTypeProp) {
>>
>> Thank you. This is misprint. Should be “Sets the channel binding type”
>> About timeout - Yes. It is required. Otherwise, LdapClient does not wait for
>> TLS handshake completion and we can not get TLS server certificate before
>> Channel Binding data is populated.
>> Actually, we can force to wait for handshake completion but what timeout
>> should be set in this case.
>> As mentioned by Danial, information about new property and timeout should be
>> documented in the release notes.
>> For the TlsChannelBindingType.TLS_SERVER_END_POINT_COMPAT type name, I do
>> not think it is good approach. If you think different servers could accept
>> different address types for the same Channel Binding type, It would be
>> better to introduce separate boolean compatibility property like
>> “com.sun.security.sasl.addrtype.compat”. In this case it would be applied
>> not only for tls-server-end-point but for any provided Channel Bindings
>>
>>
>>> Not sure if that javadoc is the right one? And I also wonder if enforcing
>>> the timeout is needed, and if yes if it should be documented why. Was not
>>> obvious to me,
>>>
>>> what about having two type names
>>> (TlsChannelBindingType.TLS_SERVER_END_POINT and
>>> TlsChannelBindingType.TLS_SERVER_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?
>>>
>>> Gruss
>>> Bernd
>>>
>>> Von: security-dev im Auftrag von
>>> Alexey Bakhtin
>>> 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 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.v2/
Re: RFR: 8245527: LDAP Cnannel Binding support for Java GSS/Kerberos
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: > > > >> 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 https://www.rfc-editor.org/rfc/rfc5929#section-4.1, this is the > right approach. I'm just curious if you have seen newer signature algorithms > like RSASSA-PSS and EdDSA used in reality, since the latest TLS spec already > defined ciphersuites around them. > > Thanks, > Max
Re: RFR: 8245527: LDAP Cnannel Binding support for Java GSS/Kerberos
Hi Alexey,
I have question about timeouts:
LdapCtx has 2 timeout properties: connectTimeout and readTimeout.
First one is for controlling the Socket::connect timeout
(Connection::createSocket), another - for reading out the replies
(Connection::readReply).
Both of them have default values set to '-1' which means that the LDAP
stack would not set any timeouts for connect and/or reading operations.
Could you, please, share the failures you're observing with no connect
timeout set?
Best,
Aleksei
On 27/05/2020 11:48, Alexey Bakhtin wrote:
Hello Bernd,
On 27 May 2020, at 13:12, Bernd Eckenfels wrote:
LdapCtxt:
2568 /**
2569 * Sets the read timeout value
2570 */
2571 private void setChannelBindingType(String cbTypeProp) {
Thank you. This is misprint. Should be “Sets the channel binding type”
About timeout - Yes. It is required. Otherwise, LdapClient does not wait for
TLS handshake completion and we can not get TLS server certificate before
Channel Binding data is populated.
Actually, we can force to wait for handshake completion but what timeout should
be set in this case.
As mentioned by Danial, information about new property and timeout should be
documented in the release notes.
For the TlsChannelBindingType.TLS_SERVER_END_POINT_COMPAT type name, I do not
think it is good approach. If you think different servers could accept
different address types for the same Channel Binding type, It would be better
to introduce separate boolean compatibility property like
“com.sun.security.sasl.addrtype.compat”. In this case it would be applied not
only for tls-server-end-point but for any provided Channel Bindings
Not sure if that javadoc is the right one? And I also wonder if enforcing the
timeout is needed, and if yes if it should be documented why. Was not obvious
to me,
what about having two type names (TlsChannelBindingType.TLS_SERVER_END_POINT
and TlsChannelBindingType.TLS_SERVER_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?
Gruss
Bernd
Von: security-dev im Auftrag von Alexey
Bakhtin
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 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.v2/
Re: RFR: 8245527: LDAP Cnannel Binding support for Java GSS/Kerberos
Hello Bernd,
> On 27 May 2020, at 13:12, Bernd Eckenfels wrote:
>
> LdapCtxt:
> 2568 /**
> 2569 * Sets the read timeout value
> 2570 */
> 2571 private void setChannelBindingType(String cbTypeProp) {
Thank you. This is misprint. Should be “Sets the channel binding type”
About timeout - Yes. It is required. Otherwise, LdapClient does not wait for
TLS handshake completion and we can not get TLS server certificate before
Channel Binding data is populated.
Actually, we can force to wait for handshake completion but what timeout should
be set in this case.
As mentioned by Danial, information about new property and timeout should be
documented in the release notes.
For the TlsChannelBindingType.TLS_SERVER_END_POINT_COMPAT type name, I do not
think it is good approach. If you think different servers could accept
different address types for the same Channel Binding type, It would be better
to introduce separate boolean compatibility property like
“com.sun.security.sasl.addrtype.compat”. In this case it would be applied not
only for tls-server-end-point but for any provided Channel Bindings
>
> Not sure if that javadoc is the right one? And I also wonder if enforcing the
> timeout is needed, and if yes if it should be documented why. Was not obvious
> to me,
>
> what about having two type names (TlsChannelBindingType.TLS_SERVER_END_POINT
> and TlsChannelBindingType.TLS_SERVER_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?
>
> Gruss
> Bernd
>
> Von: security-dev im Auftrag von
> Alexey Bakhtin
> 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 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.v2/
Re: RFR: 8245527: LDAP Cnannel Binding support for Java GSS/Kerberos
> 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 https://www.rfc-editor.org/rfc/rfc5929#section-4.1, this is the right approach. I'm just curious if you have seen newer signature algorithms like RSASSA-PSS and EdDSA used in reality, since the latest TLS spec already defined ciphersuites around them. Thanks, Max
Re: RFR: 8245527: LDAP Cnannel Binding support for Java GSS/Kerberos
LdapCtxt:
2568 /**
2569 * Sets the read timeout value
2570 */
2571 private void setChannelBindingType(String cbTypeProp) {
Not sure if that javadoc is the right one? And I also wonder if enforcing the
timeout is needed, and if yes if it should be documented why. Was not obvious
to me,
what about having two type names (TlsChannelBindingType.TLS_SERVER_END_POINT
and TlsChannelBindingType.TLS_SERVER_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?
Gruss
Bernd
Von: security-dev im Auftrag von Alexey
Bakhtin
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 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.v2/
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 in the latest webrev:
http://cr.openjdk.java.net/~abakhtin/8245527/webrev.v2/
This fix allows to set GSS_C_AF_UNSPEC address type for the
tls-server-end-point channel binding only
Regards
Alexey
> On 26 May 2020, at 23:37, Valerie Peng wrote:
>
> I am also concerned about the changes in GSSLibStub.c about the default value
> being GSS_C_AF_UNSPEC instead 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
> which Michael has already stated.
>
> Thanks,
> Valerie
>
>
> On 5/25/2020 8:33 AM, Alexey Bakhtin wrote:
>> 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, please see my comments below.
>>
>> Regards
>> Alexey
>>
>>
>>> On 24 May 2020, at 02:38, Michael Osipov <1983-01...@gmx.net>
>>> wrote:
>>>
>>> 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
>>>
>> Thank you. Fixed in Jira
>>
>>> * The word "cannot" is incorrectly spelled throughout all exception messages
>>>
>> Fixed from “can not” to “cannot"
>>
+if
(cbTypeProp.equals(TlsChannelBindingType.TLS_UNIQUE.getName())) {
+throw new UnsupportedOperationException("LdapCtx: " +
+TlsChannelBindingType.TLS_UNIQUE.getName() + "
type is not supported");
>>>
>>> "LdapCtx: " is redundant because the stacktrace will contain the class
>>> name already. A better message would be: "Channel binding type '%s' is
>>> not supported". Not just the plain value.
>>>
>> Exception message is corrected
>>
+} else if
(cbTypeProp.equals(TlsChannelBindingType.TLS_SERVER_END_POINT.getName())) {
+if (connectTimeout == -1)
+throw new
IllegalArgumentException(CHANNEL_BINDING_TYPE + " property requires " +
+CONNECT_TIMEOUT + " property is set.");
>>> * Same here with the message
>>>
>> Not sure, What’s wrong with the message ?
>>
>>> * The IAE is wrong because passed value is correct, but leads to an
>>> invalid state because connection timeout is -1. You need an
>>> IllegalStateException here.
>>>
>> Thank you. You are right again. Changed to IllegalStateException
>>
>>> Stupid question: how can one create a GSS security context when the TLS
>>> security context has not been established yet?
>>>
>> This logic already existed here. It could be a reason for it and I don’t
>> want change it without strong purpose.
>> The only changes here is to prevent double setting of channel binding data.
>>
>>
---
old/src/java.security.jgss/share/classes/sun/security/jgss/GSSContextImpl.java
2020-05-18 19:39:46.0 +0300
+++
new/src/java.security.jgss/share/classes/sun/security/jgss/GSSContextImpl.java
2020-05-18 19:39:46.0 +0300
@@ -531,9 +531,12 @@
public void setChannelBinding(ChannelBinding channelBindings)
throws GSSException {
-if (mechCtxt == null)
+if (mechCtxt == null) {
+if (this.channelBindings != null) {
+throw new GSSException(GSSException.BAD_BINDINGS);
+}
this.channelBindings = channelBindings;
-
+}
}
>>> I don't understand the purpose of this hunk. Is this safeguard to set
>>> bindings only once?
>>>
>>>
private static final int CHANNEL_BINDING_AF_INET = 2;
private static final int CHANNEL_BINDING_AF_INET6 = 24;
private static final int CHANNEL_BINDING_AF_NULL_ADDR = 255;
+private static final int CHANNEL_BINDING_AF_UNSPEC = 0;
>>> This should sort from 0 to 255 and not at the end.
>>>
>> OK. Moved to the top.
>>
>>
private int getAddrType(InetAddress addr) {
-int addressType = CHANNEL_BINDING_AF_NULL_ADDR;
+int addressType = CHANNEL_BINDING_AF_UNSPEC;
// initialize addrtype in CB first
- cb->initiator_addrtype = GSS_C_AF_NULLADDR;
- cb->acceptor_addrtype = GSS_C_AF_NULLADDR;
+
Re: RFR: 8245527: LDAP Cnannel Binding support for Java GSS/Kerberos
> 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 sure if I understand, what do "Different SASL protocols" mean? Here LdapClient is the application and therefore the "protocol". Are you worried that another SASL mechanism might choose a different prefix? I really find the TLS word in a SASL mechanism strange. It belongs to a different layer. --Max > This is a reason I create TLSChannelBinding class and calculate hash from the > LdapClient and add “tls-server-end-point:” prefix in the JGSS/Kerberos. > > Alexey > >> On 26 May 2020, at 17:50, Weijun Wang wrote: >> >> 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.sasl.channelbinding" suggests it's just a general >> ChannelBinding which is independent with any application level info. >> >> From my reading of the code change, it looks like this cbData can be >> calculated on the LDAP side. >> >> Thanks, >> Max >> >>> On May 21, 2020, at 3:35 PM, Alexey Bakhtin wrote: >>> >>> 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 server certificate). The channel binding data is >>> calculated as following : >>> • The client calculates a hash of the TLS server certificate. >>> 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 based >>> • The channel binding information is the same as defined in rfc4121 [1] >>> with small corrections: >>> • initiator and acceptor addresses should be set to NULL >>> • initiator and acceptor address types should be zero. >>>It contradicts to the “Using Channel Bindings in GSS-API” >>> document [2] that say that >>>the address type should be set to GSS_C_AF_NULLADDR=0xFF, >>>instead of GSS_C_AF_UNSPEC=0x00. >>> >>> This patch introduces changes in the LDAP, SASL and JGSS modules >>> to generate channel binding data automatically if requested by an >>> application. >>> This patch reuses existing org.ietf.jgss.ChannelBinding class >>> implementation but changes >>> initial unspecified address type from CHANNEL_BINDING_AF_NULL_ADDR to >>> CHANNEL_BINDING_AF_UNSPEC. >>> The patch introduces new environment property >>> “com.sun.jndi.ldap.tls.cbtype” that indicates >>> Channel Binding type that should be used in the LDAPS connection over >>> JGSS/Kerberos. >>> Right now "tls-server-end-point" Channel Binding type is supported only. >>> The client extracts server certificate from the underlying TLS connection >>> and creates >>> Channel Binding data for JGSS/Kerberos authentication if application >>> indicates >>> com.sun.jndi.ldap.tls.cbtype=tls-server-end-point property. >>> Client application should also specify existing >>> "com.sun.jndi.ldap.connect.timeout” property >>> to force and wait TLS handshake completion before JGSS/Kerberos >>> authentication data is generated. >>> >>> [1] - https://tools.ietf.org/html/rfc4121#section-4.1.1.2 >>> >>> [2] - >>> https://docs.oracle.com/cd/E19120-01/open.solaris/819-2145/overview-52/index.html >>> >>> Initial discussion of this issue is available at security-dev list: >>> https://mail.openjdk.java.net/pipermail/security-dev/2019-December/021052.html >>> https://mail.openjdk.java.net/pipermail/security-dev/2020-January/021140.html >>> https://mail.openjdk.java.net/pipermail/security-dev/2020-February/021278.html >>> https://mail.openjdk.java.net/pipermail/security-dev/2020-May/021864.html >
Re: RFR: 8245527: LDAP Cnannel Binding support for Java GSS/Kerberos
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
* The word "cannot" is incorrectly spelled throughout all exception messages
+if (cbTypeProp.equals(TlsChannelBindingType.TLS_UNIQUE.getName()))
{
+throw new UnsupportedOperationException("LdapCtx: " +
+TlsChannelBindingType.TLS_UNIQUE.getName() + " type is not
supported");
"LdapCtx: " is redundant because the stacktrace will contain the class
name already. A better message would be: "Channel binding type '%s' is
not supported". Not just the plain value.
+} else if
(cbTypeProp.equals(TlsChannelBindingType.TLS_SERVER_END_POINT.getName())) {
+if (connectTimeout == -1)
+throw new IllegalArgumentException(CHANNEL_BINDING_TYPE + "
property requires " +
+CONNECT_TIMEOUT + " property is set.");
* Same here with the message
* The IAE is wrong because passed value is correct, but leads to an
invalid state because connection timeout is -1. You need an
IllegalStateException here.
Stupid question: how can one create a GSS security context when the TLS
security context has not been established yet?
---
old/src/java.security.jgss/share/classes/sun/security/jgss/GSSContextImpl.java
2020-05-18 19:39:46.0 +0300
+++
new/src/java.security.jgss/share/classes/sun/security/jgss/GSSContextImpl.java
2020-05-18 19:39:46.0 +0300
@@ -531,9 +531,12 @@
public void setChannelBinding(ChannelBinding channelBindings)
throws GSSException {
-if (mechCtxt == null)
+if (mechCtxt == null) {
+if (this.channelBindings != null) {
+throw new GSSException(GSSException.BAD_BINDINGS);
+}
this.channelBindings = channelBindings;
-
+}
}
I don't understand the purpose of this hunk. Is this safeguard to set
bindings only once?
private static final int CHANNEL_BINDING_AF_INET = 2;
private static final int CHANNEL_BINDING_AF_INET6 = 24;
private static final int CHANNEL_BINDING_AF_NULL_ADDR = 255;
+private static final int CHANNEL_BINDING_AF_UNSPEC = 0;
This should sort from 0 to 255 and not at the end.
private int getAddrType(InetAddress addr) {
-int addressType = CHANNEL_BINDING_AF_NULL_ADDR;
+int addressType = CHANNEL_BINDING_AF_UNSPEC;
// initialize addrtype in CB first
- cb->initiator_addrtype = GSS_C_AF_NULLADDR;
- cb->acceptor_addrtype = GSS_C_AF_NULLADDR;
+ cb->initiator_addrtype = GSS_C_AF_UNSPEC;
+ cb->acceptor_addrtype = GSS_C_AF_UNSPEC;
This looks wrong to me -- as you already mentioned -- this violates RFC
2744, section 3.11, last sentence:
or omit addressing information, specifying
GSS_C_AF_NULLADDR as the address-types.
/* release initiator address */
- if (cb->initiator_addrtype != GSS_C_AF_NULLADDR) {
+ if (cb->initiator_addrtype != GSS_C_AF_NULLADDR &&
+ cb->initiator_addrtype != GSS_C_AF_UNSPEC) {
resetGSSBuffer(&(cb->initiator_address));
}
/* release acceptor address */
- if (cb->acceptor_addrtype != GSS_C_AF_NULLADDR) {
+ if (cb->acceptor_addrtype != GSS_C_AF_NULLADDR &&
+ cb->acceptor_addrtype != GSS_C_AF_UNSPEC) {
resetGSSBuffer(&(cb->acceptor_address));
}
Unspecified does not mean that it is null.
+final byte[] prefix =
(TlsChannelBindingType.TLS_SERVER_END_POINT.getName() + ":").getBytes();
+byte[] cbData = Arrays.copyOf(prefix,
+prefix.length + tlsCB.getData().length
);
+System.arraycopy(tlsCB.getData(), 0, cbData,
prefix.length, tlsCB.getData().length);
Since you are calling "tlsCB.getData()" multiple times, this should go
into a variable.
+secCtx.setChannelBinding(new
ChannelBinding(null, null, cbData));
Why not use new ChannelBinding(cbData)?
+String hashAlg = serverCertificate.getSigAlgName().
+replace("SHA", "SHA-").toUpperCase();
+int ind = hashAlg.indexOf("WITH");
+if (ind > 0) {
+hashAlg = hashAlg.substring(0, ind);
+if (hashAlg.equals("MD5") || hashAlg.equals("SHA-1")) {
+hashAlg = "SHA-256";
+}
I have several problems with that, some might be hypothetical:
* toUpperCase() should be qualified with Locale.ROOT or Locate.ENGLISH
* Looking at https://tools.ietf.org/html/rfc5280#section-4.1.1.2, then
at sun.security.x509.AlgorithmId.getName() it uses nameTable to
translate OIDs to readible names.
With indexOf("WITH") you are implying that the cert's sig alg
Re: RFR: 8245527: LDAP Cnannel Binding support for Java GSS/Kerberos
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 course needs to be properly passed to native providers too. GssKrb5Client would also need to know that this channel binding is explicitly for Active Directory and not some other, spec-compliant, SASL peer (property on LdapCtx?) Giving this more thought. I believe you have also found a bug in InitialToken#getAddrType(InetAddress). It would return CHANNEL_BINDING_AF_NULL_ADDR if addr is neither Inet6 nor Inet6, but is not null which is wrong. But this is just a hypothetical case. M
Re: RFR: 8245527: LDAP Cnannel Binding support for Java GSS/Kerberos
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 which Michael has already stated.
Thanks,
Valerie
On 5/25/2020 8:33 AM, Alexey Bakhtin wrote:
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, please see my comments below.
Regards
Alexey
On 24 May 2020, at 02:38, Michael Osipov <1983-01...@gmx.net> wrote:
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
Thank you. Fixed in Jira
* The word "cannot" is incorrectly spelled throughout all exception messages
Fixed from “can not” to “cannot"
+if (cbTypeProp.equals(TlsChannelBindingType.TLS_UNIQUE.getName()))
{
+throw new UnsupportedOperationException("LdapCtx: " +
+TlsChannelBindingType.TLS_UNIQUE.getName() + " type is not
supported");
"LdapCtx: " is redundant because the stacktrace will contain the class
name already. A better message would be: "Channel binding type '%s' is
not supported". Not just the plain value.
Exception message is corrected
+} else if
(cbTypeProp.equals(TlsChannelBindingType.TLS_SERVER_END_POINT.getName())) {
+if (connectTimeout == -1)
+throw new IllegalArgumentException(CHANNEL_BINDING_TYPE + "
property requires " +
+CONNECT_TIMEOUT + " property is set.");
* Same here with the message
Not sure, What’s wrong with the message ?
* The IAE is wrong because passed value is correct, but leads to an
invalid state because connection timeout is -1. You need an
IllegalStateException here.
Thank you. You are right again. Changed to IllegalStateException
Stupid question: how can one create a GSS security context when the TLS
security context has not been established yet?
This logic already existed here. It could be a reason for it and I don’t want
change it without strong purpose.
The only changes here is to prevent double setting of channel binding data.
---
old/src/java.security.jgss/share/classes/sun/security/jgss/GSSContextImpl.java
2020-05-18 19:39:46.0 +0300
+++
new/src/java.security.jgss/share/classes/sun/security/jgss/GSSContextImpl.java
2020-05-18 19:39:46.0 +0300
@@ -531,9 +531,12 @@
public void setChannelBinding(ChannelBinding channelBindings)
throws GSSException {
-if (mechCtxt == null)
+if (mechCtxt == null) {
+if (this.channelBindings != null) {
+throw new GSSException(GSSException.BAD_BINDINGS);
+}
this.channelBindings = channelBindings;
-
+}
}
I don't understand the purpose of this hunk. Is this safeguard to set
bindings only once?
private static final int CHANNEL_BINDING_AF_INET = 2;
private static final int CHANNEL_BINDING_AF_INET6 = 24;
private static final int CHANNEL_BINDING_AF_NULL_ADDR = 255;
+private static final int CHANNEL_BINDING_AF_UNSPEC = 0;
This should sort from 0 to 255 and not at the end.
OK. Moved to the top.
private int getAddrType(InetAddress addr) {
-int addressType = CHANNEL_BINDING_AF_NULL_ADDR;
+int addressType = CHANNEL_BINDING_AF_UNSPEC;
// initialize addrtype in CB first
- cb->initiator_addrtype = GSS_C_AF_NULLADDR;
- cb->acceptor_addrtype = GSS_C_AF_NULLADDR;
+ cb->initiator_addrtype = GSS_C_AF_UNSPEC;
+ cb->acceptor_addrtype = GSS_C_AF_UNSPEC;
This looks wrong to me -- as you already mentioned -- this violates RFC
2744, section 3.11, last sentence:
or omit addressing information, specifying
GSS_C_AF_NULLADDR as the address-types.
/* release initiator address */
- if (cb->initiator_addrtype != GSS_C_AF_NULLADDR) {
+ if (cb->initiator_addrtype != GSS_C_AF_NULLADDR &&
+ cb->initiator_addrtype != GSS_C_AF_UNSPEC) {
resetGSSBuffer(&(cb->initiator_address));
}
/* release acceptor address */
- if (cb->acceptor_addrtype != GSS_C_AF_NULLADDR) {
+ if (cb->acceptor_addrtype != GSS_C_AF_NULLADDR &&
+ cb->acceptor_addrtype != GSS_C_AF_UNSPEC) {
resetGSSBuffer(&(cb->acceptor_address));
}
Unspecified does not mean that it is null.
+final byte[] prefix =
(TlsChannelBindingType.TLS_SERVER_END_POINT.getName() + ":").getBytes();
+byte[] cbData =
Re: RFR: 8245527: LDAP Cnannel Binding support for Java GSS/Kerberos
Not completely sure about which of the involved apIs have what possible extensions. Maybe we can somehow make two mechanisms one which is the compatible default and one would be the rfc compliant method. Then SASL can be configured and use different mechanism names with a new propert? That would help jgss for the other mechanisms for channel bindings (cbt) as well. Not sure how jgss and JSSE will talk to each other.. via SASL? -- http://bernd.eckenfels.net Von: core-libs-dev im Auftrag von Alexey Bakhtin Gesendet: Tuesday, May 26, 2020 7:46:11 PM 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 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 LdapClient and add “tls-server-end-point:” prefix in the JGSS/Kerberos. Alexey > On 26 May 2020, at 17:50, Weijun Wang wrote: > > 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.sasl.channelbinding" suggests it's just a general > ChannelBinding which is independent with any application level info. > > From my reading of the code change, it looks like this cbData can be > calculated on the LDAP side. > > Thanks, > Max > >> On May 21, 2020, at 3:35 PM, Alexey Bakhtin wrote: >> >> 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 server certificate). The channel binding data is >> calculated as following : >> • The client calculates a hash of the TLS server certificate. >> 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 based >> • The channel binding information is the same as defined in rfc4121 >> [1] with small corrections: >> • initiator and acceptor addresses should be set to NULL >> • initiator and acceptor address types should be zero. >> It contradicts to the “Using Channel Bindings in GSS-API” >> document [2] that say that >> the address type should be set to GSS_C_AF_NULLADDR=0xFF, >> instead of GSS_C_AF_UNSPEC=0x00. >> >> This patch introduces changes in the LDAP, SASL and JGSS modules >> to generate channel binding data automatically if requested by an >> application. >> This patch reuses existing org.ietf.jgss.ChannelBinding class implementation >> but changes >> initial unspecified address type from CHANNEL_BINDING_AF_NULL_ADDR to >> CHANNEL_BINDING_AF_UNSPEC. >> The patch introduces new environment property “com.sun.jndi.ldap.tls.cbtype” >> that indicates >> Channel Binding type that should be used in the LDAPS connection over >> JGSS/Kerberos. >> Right now "tls-server-end-point" Channel Binding type is supported only. >> The client extracts server certificate from the underlying TLS connection >> and creates >> Channel Binding data for JGSS/Kerberos authentication if application >> indicates >> com.sun.jndi.ldap.tls.cbtype=tls-server-end-point property. >> Client application should also specify existing >> "com.sun.jndi.ldap.connect.timeout” property >> to force and wait TLS handshake completion before JGSS/Kerberos >> authentication data is generated. >> >> [1] - https://tools.ietf.org/html/rfc4121#section-4.1.1.2 >> >> [2] - >> https://docs.oracle.com/cd/E19120-01/open.solaris/819-2145/overview-52/index.html >> >> Initial discussion of this issue is available at security-dev list: >> https://mail.openjdk.java.net/pipermail/security-dev/2019-December/021052.html >> https://mail.openjdk.java.net/pipermail/security-dev/2020-January/021140.html >> https://mail.openjdk.java.net/pipermail/security-dev/2020-February/021278.html >> https://mail.openjdk.java.net/pipermail/security-dev/2020-May/021864.html
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 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 LdapClient and add “tls-server-end-point:” prefix in the JGSS/Kerberos. Alexey > On 26 May 2020, at 17:50, Weijun Wang wrote: > > 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.sasl.channelbinding" suggests it's just a general > ChannelBinding which is independent with any application level info. > > From my reading of the code change, it looks like this cbData can be > calculated on the LDAP side. > > Thanks, > Max > >> On May 21, 2020, at 3:35 PM, Alexey Bakhtin wrote: >> >> 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 server certificate). The channel binding data is >> calculated as following : >> • The client calculates a hash of the TLS server certificate. >> 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 based >> • The channel binding information is the same as defined in rfc4121 [1] >> with small corrections: >> • initiator and acceptor addresses should be set to NULL >> • initiator and acceptor address types should be zero. >> It contradicts to the “Using Channel Bindings in GSS-API” >> document [2] that say that >> the address type should be set to GSS_C_AF_NULLADDR=0xFF, >> instead of GSS_C_AF_UNSPEC=0x00. >> >> This patch introduces changes in the LDAP, SASL and JGSS modules >> to generate channel binding data automatically if requested by an >> application. >> This patch reuses existing org.ietf.jgss.ChannelBinding class implementation >> but changes >> initial unspecified address type from CHANNEL_BINDING_AF_NULL_ADDR to >> CHANNEL_BINDING_AF_UNSPEC. >> The patch introduces new environment property “com.sun.jndi.ldap.tls.cbtype” >> that indicates >> Channel Binding type that should be used in the LDAPS connection over >> JGSS/Kerberos. >> Right now "tls-server-end-point" Channel Binding type is supported only. >> The client extracts server certificate from the underlying TLS connection >> and creates >> Channel Binding data for JGSS/Kerberos authentication if application >> indicates >> com.sun.jndi.ldap.tls.cbtype=tls-server-end-point property. >> Client application should also specify existing >> "com.sun.jndi.ldap.connect.timeout” property >> to force and wait TLS handshake completion before JGSS/Kerberos >> authentication data is generated. >> >> [1] - https://tools.ietf.org/html/rfc4121#section-4.1.1.2 >> >> [2] - >> https://docs.oracle.com/cd/E19120-01/open.solaris/819-2145/overview-52/index.html >> >> Initial discussion of this issue is available at security-dev list: >> https://mail.openjdk.java.net/pipermail/security-dev/2019-December/021052.html >> https://mail.openjdk.java.net/pipermail/security-dev/2020-January/021140.html >> https://mail.openjdk.java.net/pipermail/security-dev/2020-February/021278.html >> https://mail.openjdk.java.net/pipermail/security-dev/2020-May/021864.html
Re: RFR: 8245527: LDAP Cnannel Binding support for Java GSS/Kerberos
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 can extract TLS server certificate in the LdapClient and put it into internal environment property as byte array without mention about Channel Binding. It will be done for every LDAPS connection. In case of “com.sun.security.sasl.channelbinding=tls-server-end-point” property specified, GssKrb5Client extracts certificate from the internal environment property and use it to create TLS Channel Binding. In this case almost all Channel Binding code could be moved to GssKrb5Client. LdapClient still changed but not mention about Channel Binding. Changes in the LdapCtx could be omitted. Regards Alexey > On 26 May 2020, at 17:55, Aleks Efimov wrote: > > 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 > implementation (GssKrb5Client class, "GSSAPI" authentication mechanism name): > - That may require to store the server certificate extracted from SSLSocket > into new context environment property > - The code that processes and checks the String value of channel binding type > value could also be moved to GssKrb5Client or to TlsChannelBinding > - Add TlsChannelBinding factory method that creates an object from the server > certificate and the string value of the environment property could also be > useful here > > All of that will allow us to keep the fix specific to "JGSS/Kerberos" area > and will keep LdapCtx/LdapClient code changes minimal and clear of > "JGSS/Kerberos" details > > Best Regards, > Aleksei > > On 26/05/2020 15:14, Daniel Fuchs wrote: >> 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 >> UnsupportedOperationExceptions which are undocumented. >> I believe they should be replaced by NamingException which >> is documented at the public API level. >> >> Also it would be good to have a test that validates that >> the proposed changes works as expected. >> >> I will not comment on the security libs changes - I'm clearly >> out of my depth there. It's a bit distasteful that the >> LdapCtxt/LdapClient have to have knowledge of channel binding >> and extract the certificates from the SSLSocket to pass them to >> the lower layer. Ideally this should rather be handled by the >> SASL layers? >> >> best regards, >> >> -- daniel >> >> >> On 25/05/2020 16:33, Alexey Bakhtin wrote: >>> Updated webrev is available >>> at:http://cr.openjdk.java.net/~abakhtin/8245527/webrev.v1/ >>
Re: RFR: 8245527: LDAP Cnannel Binding support for Java GSS/Kerberos
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 implementation (GssKrb5Client class, "GSSAPI" authentication mechanism name): - That may require to store the server certificate extracted from SSLSocket into new context environment property - The code that processes and checks the String value of channel binding type value could also be moved to GssKrb5Client or to TlsChannelBinding - Add TlsChannelBinding factory method that creates an object from the server certificate and the string value of the environment property could also be useful here All of that will allow us to keep the fix specific to "JGSS/Kerberos" area and will keep LdapCtx/LdapClient code changes minimal and clear of "JGSS/Kerberos" details Best Regards, Aleksei On 26/05/2020 15:14, Daniel Fuchs wrote: 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 UnsupportedOperationExceptions which are undocumented. I believe they should be replaced by NamingException which is documented at the public API level. Also it would be good to have a test that validates that the proposed changes works as expected. I will not comment on the security libs changes - I'm clearly out of my depth there. It's a bit distasteful that the LdapCtxt/LdapClient have to have knowledge of channel binding and extract the certificates from the SSLSocket to pass them to the lower layer. Ideally this should rather be handled by the SASL layers? best regards, -- daniel On 25/05/2020 16:33, Alexey Bakhtin wrote: Updated webrev is available at:http://cr.openjdk.java.net/~abakhtin/8245527/webrev.v1/
Re: RFR: 8245527: LDAP Cnannel Binding support for Java GSS/Kerberos
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.sasl.channelbinding" suggests it's just a general ChannelBinding which is independent with any application level info. From my reading of the code change, it looks like this cbData can be calculated on the LDAP side. Thanks, Max > On May 21, 2020, at 3:35 PM, Alexey Bakhtin wrote: > > 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 server certificate). The channel binding data is calculated > as following : > • The client calculates a hash of the TLS server certificate. > 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 based > • The channel binding information is the same as defined in rfc4121 [1] > with small corrections: > • initiator and acceptor addresses should be set to NULL > • initiator and acceptor address types should be zero. > It contradicts to the “Using Channel Bindings in GSS-API” > document [2] that say that > the address type should be set to GSS_C_AF_NULLADDR=0xFF, > instead of GSS_C_AF_UNSPEC=0x00. > > This patch introduces changes in the LDAP, SASL and JGSS modules > to generate channel binding data automatically if requested by an application. > This patch reuses existing org.ietf.jgss.ChannelBinding class implementation > but changes > initial unspecified address type from CHANNEL_BINDING_AF_NULL_ADDR to > CHANNEL_BINDING_AF_UNSPEC. > The patch introduces new environment property “com.sun.jndi.ldap.tls.cbtype” > that indicates > Channel Binding type that should be used in the LDAPS connection over > JGSS/Kerberos. > Right now "tls-server-end-point" Channel Binding type is supported only. > The client extracts server certificate from the underlying TLS connection and > creates > Channel Binding data for JGSS/Kerberos authentication if application indicates > com.sun.jndi.ldap.tls.cbtype=tls-server-end-point property. > Client application should also specify existing > "com.sun.jndi.ldap.connect.timeout” property > to force and wait TLS handshake completion before JGSS/Kerberos > authentication data is generated. > > [1] - https://tools.ietf.org/html/rfc4121#section-4.1.1.2 > > [2] - > https://docs.oracle.com/cd/E19120-01/open.solaris/819-2145/overview-52/index.html > > Initial discussion of this issue is available at security-dev list: > https://mail.openjdk.java.net/pipermail/security-dev/2019-December/021052.html > https://mail.openjdk.java.net/pipermail/security-dev/2020-January/021140.html > https://mail.openjdk.java.net/pipermail/security-dev/2020-February/021278.html > https://mail.openjdk.java.net/pipermail/security-dev/2020-May/021864.html
Re: RFR: 8245527: LDAP Cnannel Binding support for Java GSS/Kerberos
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 UnsupportedOperationExceptions which are undocumented. I believe they should be replaced by NamingException which is documented at the public API level. Also it would be good to have a test that validates that the proposed changes works as expected. I will not comment on the security libs changes - I'm clearly out of my depth there. It's a bit distasteful that the LdapCtxt/LdapClient have to have knowledge of channel binding and extract the certificates from the SSLSocket to pass them to the lower layer. Ideally this should rather be handled by the SASL layers? best regards, -- daniel On 25/05/2020 16:33, Alexey Bakhtin wrote: Updated webrev is available at:http://cr.openjdk.java.net/~abakhtin/8245527/webrev.v1/
Re: RFR: 8245527: LDAP Cnannel Binding support for Java GSS/Kerberos
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 UnspecEmptyInetAddress approach but it could help to distinguish between TLS channel binding and Channel Bindings set by application. Later we can remove this workaround In case we prove that UNSPEC type should be used in all types of Channel Bindings. Updated webrev : http://cr.openjdk.java.net/~abakhtin/8245527/webrev.v2/ Regards Alexey > On 26 May 2020, at 06:04, Thomas Maslen wrote: > > 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. > > So we have several issues here: > * GSS-API C bindings and SASL requests are two distinct RFCs which > require/mandate differnt things. > * The change in JGSS in unrelated to this patch because GSS-API knows > nothing about SASL and its fauly spec. > > Since we are doing LDAP over SASL here and RFC 5801 requires to be > UNSPEC (0) the SASL TlsChannelBinding class must take that into account. > Unfortunately, JGSS implies with the args of the ChannelBinding the type > fo the adress. So will change my opinion a bit: > > No property for AD/non-AD is necessary, but handling of UNSPEC is > required. JGSS shall remain at NULLADDR. The subtype > UnspecEmptyInetAddress should be at least evaluated. > > Michael > > No. This isn't just about RFC 5801. As Alexey Bakhtin observed, this also > applies to channel bindings for HTTP Negotiate Authentication (loosely aka > "SPNEGO"), not only for NTLM (which probably isn't at issue here) but also > for Kerberos -- that's where I first encountered this, working on a > proprietary Java Kerberos implementation. > > More generally, if you want channel bindings to interoperate in the GSSAPI > Kerberos Mechanism for any protocol -- SASL GS2, HTTP Negotiate > Authentication, or anything else -- ignore the fact that RFC 2744 specifies > 255 for the "no address" case and do what everyone actually does: use zero. > > Here is a test from MIT Kerberos that (implicitly) uses zero: > https://github.com/krb5/krb5/blob/master/src/tests/gssapi/t_bindings.c > > And here is one from Heimdal: > https://github.com/heimdal/heimdal/blob/5057d04f6a47f05f1ed7c617458722104d4c17dc/lib/gssapi/test_context.c
Re: RFR: 8245527: LDAP Cnannel Binding support for Java GSS/Kerberos
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, please see my comments below.
Regards
Alexey
> On 24 May 2020, at 02:38, Michael Osipov <1983-01...@gmx.net> wrote:
>
> 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
Thank you. Fixed in Jira
> * The word "cannot" is incorrectly spelled throughout all exception messages
Fixed from “can not” to “cannot"
>
>> +if
>> (cbTypeProp.equals(TlsChannelBindingType.TLS_UNIQUE.getName())) {
>> +throw new UnsupportedOperationException("LdapCtx: " +
>> +TlsChannelBindingType.TLS_UNIQUE.getName() + " type
>> is not supported");
>
>
> "LdapCtx: " is redundant because the stacktrace will contain the class
> name already. A better message would be: "Channel binding type '%s' is
> not supported". Not just the plain value.
Exception message is corrected
>
>> +} else if
>> (cbTypeProp.equals(TlsChannelBindingType.TLS_SERVER_END_POINT.getName())) {
>> +if (connectTimeout == -1)
>> +throw new IllegalArgumentException(CHANNEL_BINDING_TYPE
>> + " property requires " +
>> +CONNECT_TIMEOUT + " property is set.");
>
> * Same here with the message
Not sure, What’s wrong with the message ?
> * The IAE is wrong because passed value is correct, but leads to an
> invalid state because connection timeout is -1. You need an
> IllegalStateException here.
Thank you. You are right again. Changed to IllegalStateException
>
> Stupid question: how can one create a GSS security context when the TLS
> security context has not been established yet?
This logic already existed here. It could be a reason for it and I don’t want
change it without strong purpose.
The only changes here is to prevent double setting of channel binding data.
>
>> ---
>> old/src/java.security.jgss/share/classes/sun/security/jgss/GSSContextImpl.java
>>2020-05-18 19:39:46.0 +0300
>> +++
>> new/src/java.security.jgss/share/classes/sun/security/jgss/GSSContextImpl.java
>>2020-05-18 19:39:46.0 +0300
>> @@ -531,9 +531,12 @@
>> public void setChannelBinding(ChannelBinding channelBindings)
>> throws GSSException {
>>
>> -if (mechCtxt == null)
>> +if (mechCtxt == null) {
>> +if (this.channelBindings != null) {
>> +throw new GSSException(GSSException.BAD_BINDINGS);
>> +}
>> this.channelBindings = channelBindings;
>> -
>> +}
>> }
>
> I don't understand the purpose of this hunk. Is this safeguard to set
> bindings only once?
>
>> private static final int CHANNEL_BINDING_AF_INET = 2;
>> private static final int CHANNEL_BINDING_AF_INET6 = 24;
>> private static final int CHANNEL_BINDING_AF_NULL_ADDR = 255;
>> +private static final int CHANNEL_BINDING_AF_UNSPEC = 0;
>
> This should sort from 0 to 255 and not at the end.
OK. Moved to the top.
>
>> private int getAddrType(InetAddress addr) {
>> -int addressType = CHANNEL_BINDING_AF_NULL_ADDR;
>> +int addressType = CHANNEL_BINDING_AF_UNSPEC;
>
>> // initialize addrtype in CB first
>> - cb->initiator_addrtype = GSS_C_AF_NULLADDR;
>> - cb->acceptor_addrtype = GSS_C_AF_NULLADDR;
>> + cb->initiator_addrtype = GSS_C_AF_UNSPEC;
>> + cb->acceptor_addrtype = GSS_C_AF_UNSPEC;
>
> This looks wrong to me -- as you already mentioned -- this violates RFC
> 2744, section 3.11, last sentence:
>> or omit addressing information, specifying
>> GSS_C_AF_NULLADDR as the address-types.
>
>> /* release initiator address */
>> - if (cb->initiator_addrtype != GSS_C_AF_NULLADDR) {
>> + if (cb->initiator_addrtype != GSS_C_AF_NULLADDR &&
>> + cb->initiator_addrtype != GSS_C_AF_UNSPEC) {
>> resetGSSBuffer(&(cb->initiator_address));
>> }
>> /* release acceptor address */
>> - if (cb->acceptor_addrtype != GSS_C_AF_NULLADDR) {
>> + if (cb->acceptor_addrtype != GSS_C_AF_NULLADDR &&
>> + cb->acceptor_addrtype != GSS_C_AF_UNSPEC) {
>> resetGSSBuffer(&(cb->acceptor_address));
>> }
>
> Unspecified does not mean that it is null.
>
>> +final byte[] prefix =
>> (TlsChannelBindingType.TLS_SERVER_END_POINT.getName() + ":").getBytes();
>> +byte[] cbData = Arrays.copyOf(prefix,
>> +prefix.length +
>> tlsCB.getData().length );
>> +
RFR: 8245527: LDAP Cnannel Binding support for Java GSS/Kerberos
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 server certificate). The channel binding data is calculated as following : • The client calculates a hash of the TLS server certificate. 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 based • The channel binding information is the same as defined in rfc4121 [1] with small corrections: • initiator and acceptor addresses should be set to NULL • initiator and acceptor address types should be zero. It contradicts to the “Using Channel Bindings in GSS-API” document [2] that say that the address type should be set to GSS_C_AF_NULLADDR=0xFF, instead of GSS_C_AF_UNSPEC=0x00. This patch introduces changes in the LDAP, SASL and JGSS modules to generate channel binding data automatically if requested by an application. This patch reuses existing org.ietf.jgss.ChannelBinding class implementation but changes initial unspecified address type from CHANNEL_BINDING_AF_NULL_ADDR to CHANNEL_BINDING_AF_UNSPEC. The patch introduces new environment property “com.sun.jndi.ldap.tls.cbtype” that indicates Channel Binding type that should be used in the LDAPS connection over JGSS/Kerberos. Right now "tls-server-end-point" Channel Binding type is supported only. The client extracts server certificate from the underlying TLS connection and creates Channel Binding data for JGSS/Kerberos authentication if application indicates com.sun.jndi.ldap.tls.cbtype=tls-server-end-point property. Client application should also specify existing "com.sun.jndi.ldap.connect.timeout” property to force and wait TLS handshake completion before JGSS/Kerberos authentication data is generated. [1] - https://tools.ietf.org/html/rfc4121#section-4.1.1.2 [2] - https://docs.oracle.com/cd/E19120-01/open.solaris/819-2145/overview-52/index.html Initial discussion of this issue is available at security-dev list: https://mail.openjdk.java.net/pipermail/security-dev/2019-December/021052.html https://mail.openjdk.java.net/pipermail/security-dev/2020-January/021140.html https://mail.openjdk.java.net/pipermail/security-dev/2020-February/021278.html https://mail.openjdk.java.net/pipermail/security-dev/2020-May/021864.html
