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