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

[Michael Osipov]

Am 2020-05-24 um 22:29 schrieb Thomas Maslen:
> [Off-list reply just to you two gents.  Feel free to forward it to the list
> if it helps, or ignore it if it doesn't]
>
> I'm just adding my 0.02 on zero vs 255 for the "no address" type in a
> channel binding
>
> ---------- Forwarded message ----------
>> From: Michael Osipov <1983-01-06 at gmx.net>
>> To: Alexey Bakhtin <alexey at azul.com>, "core-libs-dev at openjdk.java.net" <
>> core-libs-dev at openjdk.java.net>
>> Cc: "security-dev at openjdk.java.net" <security-dev at openjdk.java.net>
>> Bcc:
>> Date: Sun, 24 May 2020 01:38:06 +0200
>> Subject: Re: RFR: 8245527: LDAP Cnannel Binding support for Java
>> GSS/Kerberos
>> 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:
>>
>
> [...]
>
>>       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.

Understood, then this requires an inline comment.

> Usually I would agree, but in this case there is a perverse relationship
> between 255 and 0 (see below), so grouping them together like this _may_
> help the reader to understand that there's something funny going on.
>
>>       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.
>>
>
> Michael:  what's going on here, I believe, is that in theory of course one
> should do what the RFC says and use GSS_C_AF_NULLADDR (255) but instead all
> the main implementations with which one would want to interoperate blithely
> use zero (because, IMO, they were written by bloody C programmers) -- and
> Alexey is doing the best he can to interoperate with those rather than with
> the letter of the spec that noone implements.
>
> viz previous discussion on the IETF kitten list:
>
> https://mailarchive.ietf.org/arch/msg/kitten/Jx8ok5BBHu5GIrpdu8i6-4NtsZM/
>
>
> which refers to:
>
> https://mailarchive.ietf.org/arch/msg/kitten/ZGkBijVg080plyyQOv53bL08qRU/
>
>
>
>>
>>>     /* 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.
>>
>
> [...]
>
>
>> Generally, I have two fundemental issues:
>> * How do you know that address type must be UNSPEC and not NULLADDR?
>> Trial and error?
>>
>
> Pretty much...
>
>
>> * Changing the default address type against the RFC will break every
>> installation using channel bindings relying on it with cross GSS-API
>> implementations.
>>
>
> In theory, yes;  in practice, the opposite.

So I read the discussions. RFC 2744 shall not be changed, people
admitted that the spec of SASL GS2 mechs is wrong and should be changed,
but this has not happened yet. It remained at UNSPEC all the years.

So we have several issues here:
* GSS-API C bindings and SASL requests are two distinct RFCs which
require/mandate differnt things.
* The change in JGSS in unrelated to this patch because GSS-API knows
nothing about SASL and its fauly spec.

Since we are doing LDAP over SASL here and RFC 5801 requires to be
UNSPEC (0) the SASL TlsChannelBinding class must take that into account.
Unfortunately, JGSS implies with the args of the ChannelBinding the type
fo the adress. So will change my opinion a bit:

No property for AD/non-AD is necessary, but handling of UNSPEC is
required. JGSS shall remain at NULLADDR. The subtype
UnspecEmptyInetAddress should be at least evaluated.

Michael