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.fu...@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 >
