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

[Aleks Efimov]

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/
>