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

[Michael Osipov]

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
* The word "cannot" is incorrectly spelled throughout all exception messages

> +            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.

> +            } 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
* 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.

Stupid question: how can one create a GSS security context when the TLS
security context has not been established yet?

> --- 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.

>      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.


> +                                secCtx.setChannelBinding(new
ChannelBinding(null, null, cbData));

Why not use new ChannelBinding(cbData)?

> +            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
* 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.

* Exception messages are inconsistent. Sometimes "TLS channel binding",
sometimes just "channel binding". To avoid misunderstandings it should
always read "TLS channel binding..".

Generally, I have two fundemental issues:
* How do you know that address type must be UNSPEC and not NULLADDR?
Trial and error?
* Changing the default address type against the RFC will break every
installation using channel bindings relying on it with cross GSS-API
implementations.

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.