Bug ID: JDK-8260429 Internal LDAP channel binding property should be public

Type: CSR
Component: security-libs
Sub-Component: java.security

Priority: P4
Status: Closed
Resolution: Withdrawn
Fix Versions: 17

Submitted: 2021-01-26
Updated: 2021-12-15
Resolved: 2021-12-15

Summary
-------

Make internal LDAP channel binding property public.

Problem
-------

The fix for https://bugs.openjdk.java.net/browse/JDK-8245527 introduced an internal property "jdk.internal.sasl.tlschannelbinding" which is used to pass the TLS channel binding data to the SASL client.
"jdk.internal.sasl.tlschannelbinding" property is not documented, so custom SASL Client providers can not rely on the name of this property.

Solution
--------

1.  Rename internal property to the "com.sun.sasl.tls.cbdata"
2.  Document property in the src/java.security.sasl/share/classes/module-info.java file

Specification
-------------
Add the following section into src/java.security.sasl/share/classes/module-info.java

    * @implNote
    * The following implementation specific property is supported by the
    * default SASL implementation in the JDK:
    * <ul>
    *     <li>{@code com.sun.sasl.tls.cbdata}:
    *         <br>The value of this property is the byte array representing the
    *         TLS Channel Binding data as defined in RFC-5929 and RFC-5056.
    *         LDAP Client generates TLS Channel Binding data on the base of
    *         TLS handshake and provides it to the SASL Client for authentication.
    *         "com.sun.sasl.tls.cbdata" property should not be specified
    *         explicitly. It is used internally to pass Channel Binding data from
    *         LDAP to SASL client.
    *     </li>
    * </ul>

Hello Sean, Thank you for review and proposal. I think you are right. GSSAPI SaslServer also should support this property to get CB data on the base of the server certificate. This data will be used to verify ChannelBinding data received from Client. Server part was not implemented by JDK-8245527, should be added. JDK-8245527 follows the Channel Binding encoding rules defined in the RFC 5801, section 5.1 (address type encoding). So, it could be considered as an intermediate step to "GS2" SASL mechanism. "GS2" requires channel binding on the base of TLS, so I think this property will be valid with "GS2" too
16-02-2021
Mentioning that the JDK default JNDI/LDAP provider sets the `jdk.sasl.tls.cbdata` property to provide the channel binding data to the underlying SASL implementation is an implementation detail, but seems reasonable if the underlying SASL implementation can be substituted. This could be a footnote in the `@implNote` that talks about channel binding in the `java.naming` module description, and could link back to the place in the SASL API documentation where the property is defined.
16-02-2021
I have a few comments: 1. I think we should rename the property "jdk.sasl.tls.cbdata". I don't see a reason to use "com.sun" for new properties unless there is an overriding need for consistency. And this is the first JDK-specific property for SASL AFAIK. 2. You should not mention here that the JDK LDAP impl uses this property. That information should be included in the LDAP javadocs/release notes. I think we should document that it is passed in as a property when creating a SaslClient in an @implNote in the java.naming module, but I would ask [~dfuchs] what he thinks. However, you should mention that the JDK GSSAPI SaslClient mechanism uses this property for channel binding. 3. RFC 5056 does not specify how SASL should use channel binding data, so it should not be mentioned. 4. I see no reason to limit this property to LDAP, and we can't really do that anyway since this is a property that any code can now pass in. So I think we should remove the sentence that the property should not be used explicitly. I would propose the following re-wording: ``` * @implNote * The JDK implementation of the "GSSAPI" {@code SaslClient} mechanism * supports the following property: * <ul> * <li>{@code jdk.sasl.tls.cbdata}: * <br>The value of this property is the byte array representing the * TLS Channel Binding data as defined in RFC 5929. The JDK GSSAPI * implementation sets the channel binding data to this value. * </li> * </ul> ``` A few follow-on questions: 1. Shouldn't the GSSAPI `SaslServer` mechanism implementation also support this property? 2. Any idea how this relates to the "GS2" Sasl mechanism (RFC 5801) and whether we could leverage this property if we decide to support that mechanism in the future?
16-02-2021
Hi Daniel, Thank you for review. The property is moved to java.security.sasl module
12-02-2021
The `java.naming` module description is not the correct place to expose that property. If it is documented, it should be documented as a JDK specific property somewhere in the `java.security.sasl` module, since this is there that the property is consumed. In other words - `java.naming` default implementation of the LDAP provider depends on SASL, and depends on an internal property that the JDK default implementation of SASL defines.
11-02-2021
Just FYI, the point in the previous comment won't affect us since our custom SaslClient uses a custom GSS implementation also.
08-02-2021
Hi Richard, Sean. I just found that we also have a private contact between SunSASL and SunJGSS providers. It is sun.security.jgss.krb5.internal.TlsChannelBindingImpl class. This contract should be changed to public also. Otherwise, it won’t be possible to use SunJGSS provider from the custom SaslClient implementation.
08-02-2021
If the property name changes between JDK 16 and 17 then third-party code using the old name will need changing to work with 17.
08-02-2021
Here's some code from our SaslClient implementation which works with a native Windows GSS library: // Channel binding if (props != null) { Object bdata = props.get("jdk.internal.sasl.tlschannelbinding") if (bdata instanceof byte []) { ctxt.setChannelBinding(new ChannelBinding((byte []) bdata)); } } We need to read the channel binding data set earlier in com/sun/jndi/ldap/sasl/LdapSasl.java. Hence the request that the property name should be a bit more public.
08-02-2021
[~abakhtin] Do you have some test code to show how this property would be used? I would like to see the flow of code and when the channel binding data would be made available to the application. This would help me review the change.
03-02-2021
Moving to Provisional, not Approved. [~abakhtin], please have one or more other engineers review the CSR before it is finalized for the second round of review.
01-02-2021
I think the scope is similar to CSR for JDK-8245527 (see https://bugs.openjdk.java.net/browse/JDK-8247311) which is JDK. The changes in the src/java.naming/share/classes/module-info.java file will be located in the @ImplNote section
01-02-2021
I assume the property is JDK-specific, in which case it should be in an implNote.
31-01-2021
It seems that the Scope should be "SE", not "JDK"
31-01-2021
I plan to review it but may need a few days.
27-01-2021
Marking the request as pended since it is Finalized without a reviewer. Please have one or more engineers in the security area, such as [~mullan], review the request and then Propose or Finalize it. Note that JDK 16 is ramp down 2 and getting fixes in requires extra effort; see the fix request process http://openjdk.java.net/jeps/3#Fix-Request-Process or target 17.
27-01-2021
Minimal code changes: only property renaming
26-01-2021