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/
