RFR: 8245527: LDAP Cnannel Binding support for Java GSS/Kerberos

Alexey Bakhtin alexey at azul.com
Wed Jun 17 11:26:25 UTC 2020


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 <daniel.fuchs at oracle.com> 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
> 

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Message signed with OpenPGP
URL: <https://mail.openjdk.org/pipermail/security-dev/attachments/20200617/d4372908/signature.asc>


More information about the security-dev mailing list