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

[Daniel Fuchs]

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