Hello Valerie, Unfortunately, Windows LDAP server with LdapEnforceChannelBinding=2 does not accept GSS_C_AF_NULLADDR address type. This is exact reason of these changes. I’ve tried to fix inconsistency of address type value in the latest webrev: http://cr.openjdk.java.net/~abakhtin/8245527/webrev.v2/ This fix allows to set GSS_C_AF_UNSPEC address type for the tls-server-end-point channel binding only
Regards Alexey > On 26 May 2020, at 23:37, Valerie Peng <valerie.p...@oracle.com> wrote: > > I am also concerned about the changes in GSSLibStub.c about the default value > being GSS_C_AF_UNSPEC instead of GSS_C_AF_NULLADDR (line 194-195). > > Can you try and see if Window works with GSS_C_AF_NULLADDR? If yes, I'd > prefer to not changing the default value for addresstype for the same reason > which Michael has already stated. > > Thanks, > Valerie > > > On 5/25/2020 8:33 AM, Alexey Bakhtin wrote: >> 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. >>>
