Hello Michael, Thomas, Thank you a lot for review and suggestions. I’ve fixed most of the issues except of fundamental one I need more time to evaluate suggested usage of UnspecEmptyInetAddress subtype.
Updated webrev is available at: http://cr.openjdk.java.net/~abakhtin/8245527/webrev.v1/ Also, please see my comments below. Regards Alexey > On 24 May 2020, at 02:38, Michael Osipov <1983-01...@gmx.net> wrote: > > Am 2020-05-21 um 09:35 schrieb Alexey Bakhtin: >> Hello, >> >> Could you please review the following patch: >> >> JBS: https://bugs.openjdk.java.net/browse/JDK-8245527 >> Webrev: http://cr.openjdk.java.net/~abakhtin/8245527/webrev.v0/ > > Let's go through your changes statically: > > * The JIRA issue title has a typo Thank you. Fixed in Jira > * The word "cannot" is incorrectly spelled throughout all exception messages Fixed from “can not” to “cannot" > >> + if >> (cbTypeProp.equals(TlsChannelBindingType.TLS_UNIQUE.getName())) { >> + throw new UnsupportedOperationException("LdapCtx: " + >> + TlsChannelBindingType.TLS_UNIQUE.getName() + " type >> is not supported"); > > > "LdapCtx: " is redundant because the stacktrace will contain the class > name already. A better message would be: "Channel binding type '%s' is > not supported". Not just the plain value. Exception message is corrected > >> + } else if >> (cbTypeProp.equals(TlsChannelBindingType.TLS_SERVER_END_POINT.getName())) { >> + if (connectTimeout == -1) >> + throw new IllegalArgumentException(CHANNEL_BINDING_TYPE >> + " property requires " + >> + CONNECT_TIMEOUT + " property is set."); > > * Same here with the message Not sure, What’s wrong with the message ? > * The IAE is wrong because passed value is correct, but leads to an > invalid state because connection timeout is -1. You need an > IllegalStateException here. Thank you. You are right again. Changed to IllegalStateException > > Stupid question: how can one create a GSS security context when the TLS > security context has not been established yet? This logic already existed here. It could be a reason for it and I don’t want change it without strong purpose. The only changes here is to prevent double setting of channel binding data. > >> --- >> old/src/java.security.jgss/share/classes/sun/security/jgss/GSSContextImpl.java >> 2020-05-18 19:39:46.000000000 +0300 >> +++ >> new/src/java.security.jgss/share/classes/sun/security/jgss/GSSContextImpl.java >> 2020-05-18 19:39:46.000000000 +0300 >> @@ -531,9 +531,12 @@ >> public void setChannelBinding(ChannelBinding channelBindings) >> throws GSSException { >> >> - if (mechCtxt == null) >> + if (mechCtxt == null) { >> + if (this.channelBindings != null) { >> + throw new GSSException(GSSException.BAD_BINDINGS); >> + } >> this.channelBindings = channelBindings; >> - >> + } >> } > > I don't understand the purpose of this hunk. Is this safeguard to set > bindings only once? > >> private static final int CHANNEL_BINDING_AF_INET = 2; >> private static final int CHANNEL_BINDING_AF_INET6 = 24; >> private static final int CHANNEL_BINDING_AF_NULL_ADDR = 255; >> + private static final int CHANNEL_BINDING_AF_UNSPEC = 0; > > This should sort from 0 to 255 and not at the end. OK. Moved to the top. > >> private int getAddrType(InetAddress addr) { >> - int addressType = CHANNEL_BINDING_AF_NULL_ADDR; >> + int addressType = CHANNEL_BINDING_AF_UNSPEC; > >> // initialize addrtype in CB first >> - cb->initiator_addrtype = GSS_C_AF_NULLADDR; >> - cb->acceptor_addrtype = GSS_C_AF_NULLADDR; >> + cb->initiator_addrtype = GSS_C_AF_UNSPEC; >> + cb->acceptor_addrtype = GSS_C_AF_UNSPEC; > > This looks wrong to me -- as you already mentioned -- this violates RFC > 2744, section 3.11, last sentence: >> or omit addressing information, specifying >> GSS_C_AF_NULLADDR as the address-types. > >> /* release initiator address */ >> - if (cb->initiator_addrtype != GSS_C_AF_NULLADDR) { >> + if (cb->initiator_addrtype != GSS_C_AF_NULLADDR && >> + cb->initiator_addrtype != GSS_C_AF_UNSPEC) { >> resetGSSBuffer(&(cb->initiator_address)); >> } >> /* release acceptor address */ >> - if (cb->acceptor_addrtype != GSS_C_AF_NULLADDR) { >> + if (cb->acceptor_addrtype != GSS_C_AF_NULLADDR && >> + cb->acceptor_addrtype != GSS_C_AF_UNSPEC) { >> resetGSSBuffer(&(cb->acceptor_address)); >> } > > Unspecified does not mean that it is null. > >> + final byte[] prefix = >> (TlsChannelBindingType.TLS_SERVER_END_POINT.getName() + ":").getBytes(); >> + byte[] cbData = Arrays.copyOf(prefix, >> + prefix.length + >> tlsCB.getData().length ); >> + System.arraycopy(tlsCB.getData(), 0, >> cbData, prefix.length, tlsCB.getData().length); > > Since you are calling "tlsCB.getData()" multiple times, this should go > into a variable. Temporary variable is added > > >> + secCtx.setChannelBinding(new > ChannelBinding(null, null, cbData)); > > Why not use new ChannelBinding(cbData)? Right. updated > >> + String hashAlg = serverCertificate.getSigAlgName(). >> + replace("SHA", "SHA-").toUpperCase(); >> + int ind = hashAlg.indexOf("WITH"); >> + if (ind > 0) { >> + hashAlg = hashAlg.substring(0, ind); >> + if (hashAlg.equals("MD5") || hashAlg.equals("SHA-1")) { >> + hashAlg = "SHA-256"; >> + } > > I have several problems with that, some might be hypothetical: > > * toUpperCase() should be qualified with Locale.ROOT or Locate.ENGLISH As you mentioned below AlgorithmId.getName() uses nameTable to return algorithm name. Looking at implementation I do not think it is realistic to get name in the non-English locale. I do not think it should be fixed > * Looking at https://tools.ietf.org/html/rfc5280#section-4.1.1.2, then > at sun.security.x509.AlgorithmId.getName() it uses nameTable to > translate OIDs to readible names. > > With indexOf("WITH") you are implying that the cert's sig alg may not > use a cryptographic function, but this would mean that if the OID is > 1.3.14.3.2.26 you'd turn "SHA-X" into "SHA--X" according to nameTable, > wouldn't you? > I also don't know how realistic "PBEWith..." is. > > This likely needs more thought or I am missing something. > I do not think it is realistic scenario to have certificate signature algorithm without crypto function. indexOf(“WITH”) just used to find end out hash algorithm name. > * Exception messages are inconsistent. Sometimes "TLS channel binding", > sometimes just "channel binding". To avoid misunderstandings it should > always read "TLS channel binding..". > Messages are updated. > Generally, I have two fundemental issues: > * How do you know that address type must be UNSPEC and not NULLADDR? > Trial and error? I did not find any strict specification about TLS Channel Binding format in Windows server. So, it was a lot of experiments, reading related forums and docs. One of the doc, that turns me to try UNSPEC type is the following: https://docs.microsoft.com/en-us/archive/blogs/openspecification/ntlm-and-channel-binding-hash-aka-extended-protection-for-authentication > * Changing the default address type against the RFC will break every > installation using channel bindings relying on it with cross GSS-API > implementations. I do not like this conflict between UNSPEC and NULLADDR types too, but I do not know How to better solve it. The usage of UnspecEmptyInetAddress subtype is not quite clear to me yet. Who set this value and will it change org.ietf.jgss.ChannelBinding public api ? As I understand, Implementation cannot decide (without pplication request), What channel binding type is enabled on the server. > > There must be another way to solve this. A system property would also be > problematic because if you have a product which connects to MS and > non-MS backends at the same time, channel bindings would be broken again. > > What about introducing a UnspecEmptyInetAddress() which gives the proper > AF type, but #getAddress() would return null? This should satisfy the > requirements, InitialToken as well as the RFC. this of course needs to > be properly passed to native providers too. GssKrb5Client would also > need to know that this channel binding is explicitly for Active > Directory and not some other, spec-compliant, SASL peer (property on > LdapCtx?) > > One could easily use this for custom code with a SSLServerSocket paired > with Java SASL or in C, OpenSSL and Cyrus SASL. > > Michael > > PS: Yes, I am a nitpicker.
