RFR: 8245527: LDAP Cnannel Binding support for Java GSS/Kerberos
Valerie Peng
valerie.peng at oracle.com
Tue May 26 20:37:07 UTC 2020
I am also concerned about the changes in GSSLibStub.c about the default
value being GSS_C_AF_UNSPECinstead 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-06 at 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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/security-dev/attachments/20200526/17376680/attachment.htm>
More information about the security-dev
mailing list