Hello Aleks, Daniel, Thank you for your comments. I don’t like that the code is split into several modules too. The reason of doing it is I can not get TLS server certificate from the JGSS/Kerberos code. The only place Where I can fetch it is LdapClient. If I understand your idea correctly, I can extract TLS server certificate in the LdapClient and put it into internal environment property as byte array without mention about Channel Binding. It will be done for every LDAPS connection. In case of “com.sun.security.sasl.channelbinding=tls-server-end-point” property specified, GssKrb5Client extracts certificate from the internal environment property and use it to create TLS Channel Binding. In this case almost all Channel Binding code could be moved to GssKrb5Client. LdapClient still changed but not mention about Channel Binding. Changes in the LdapCtx could be omitted.
Regards Alexey > On 26 May 2020, at 17:55, Aleks Efimov <aleksej.efi...@oracle.com> wrote: > > 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/ >>
