Code Review Request: JDK-6491070 (Support for RFC 5929-Channel Bindings)
Martin Balao
mbalao at redhat.com
Mon Oct 23 13:45:08 UTC 2017
Hi Xuelei,
I would like to propose Webrev 03:
*
http://cr.openjdk.java.net/~sgehwolf/webrevs/mbalaoal/JDK-6491070/webrev.03/
(browse online)
*
http://cr.openjdk.java.net/~sgehwolf/webrevs/mbalaoal/JDK-6491070/webrev.03/6491070.webrev.03.zip
(download)
What's new in Webrev 03:
* New interface to expose Finished messages verify data handshake material
(from both client and server) through a HandshakeListener. It's now up to
the API client to figure out which is the tls-unique binding value from
that data. TlsChannelBindingTest.java is an example of how to use the
proposed API.
* The new API allows disabling renegotiations (TLS handshake protocol).
* Added HandshakeCompletedListener to SSLEngine as in SSLSocket. It's
necessary to be able to get the server certificate used on a handshake for
the certificate binding value.
* HandshakeListener and HandshakeCompletedListener could be merged into
HandshakeCompletedListener, but I think the name "Completed" does not fit
-particularly for the callback that gets rid of renegotiations-. Obviously
renaming HandshakeCompletedListener to HandshakeListener would be a major
and breaking API change.
* TlsChannelBindingTest.java updated to the new API.
I've done testing running the following test categories and didn't notice
any regression:
* javax/net/ssl
* sun/security/ssl
Look forward to your comments.
Kind regards,
Martin.-
On Thu, Aug 31, 2017 at 10:32 PM, Xuelei Fan <xuelei.fan at oracle.com> wrote:
> The failure-and-retry mechanism could a nightmare for some applications.
> Please think more how could we avoid it. If need more APIs, what the
> update may looks like and how complicated it could be?
>
> If required, Bernd's proposal can be extended a little bit to support
> operations other than listening.
>
> APIs maintain is very complicated, a good design may need more time
> currently, but could save much more in the future.
>
> Thanks,
> Xuelei
>
> On 8/31/2017 11:41 AM, Martin Balao wrote:
>
>> Hi,
>>
>> The material is already cached in the handshaker if secure renegotiation
>> is enabled. However, I agree with you that we are going to cache the value
>> even when secure renegotiation is not enabled, thus, wasting roughly 12
>> bytes (as bytes for an empty array are already consumed). In fact, the
>> exact case -adding a few conditionals to webrev.02- is the one in which
>> secure renegotiation is disabled and extended master secret is enabled.
>> GnuTls and OpenSSL -including its derivatives like Boring SSL and Python
>> (through OpenSSL)- always cache this information.
>>
>> As for the empty / null cases, the API consumer was expected to ask for
>> the binding information after the TLS connection is established. It's on
>> the API consumer knowledge that asking for the information before (i.e.:
>> just after creating a disconnected socket) or while the handshake is taking
>> place, makes no sense and no valid value will be obtained (either we define
>> this as null or empty). For those providers that do not support this
>> feature, the information wouldn't have been available after the handshake.
>> However, I agree with you that before the handshake is completed there is
>> no means of knowing if the provider does support this API. My first webrev
>> (webrev.01) was throwing an UnsupportedOperationException to make this case
>> explicit but I had doubts regarding the real value it provides for the API
>> consumer. The proposed API was similar to Python, SSLBoring and GnuTLs.
>> However, handshake listener callbacks -as Bernd suggests- and the idea of
>> just exposing the handshake material (as a lower level API) sounds good to
>> me. I can give it a try. I propose then to bring the handshake information
>> as part of a HandshakeCompletedEvent instance, even though the callback
>> registrant may not consume it. Would that work for you?
>>
>> In regard to the handshake material update -which I assumed unlikely-,
>> the point in which a renegotiation may take place (from the server side) is
>> when reading data, not when writing. That cannot be controlled by the
>> application because it's JSSE internal and not exposed. Thus, an
>> application may read from the socket, get the handshake material and write
>> a message using the binding value -which we can make sure that is the valid
>> one at that point-. However, as soon as the application reads again from
>> the socket, a renegotiation -if requested by the client- may be processed
>> and the binding value gets updated. The higher level protocol may fail
>> -because the binding value was already sent but not processed on the other
>> side- and a re-try needed. This looks independent of whether we use the
>> originally proposed API or the handshake listener callback interface (or
>> even a sync callback), because the underlying problem is that the
>> application cannot really control the renegotiation flow in the lower layer
>> (as RFC 5929 suggest). The options I see are adding more complexity to the
>> API and let the application control the renegotiation flow or live with
>> that and expect the application to retry.
>>
>> Thanks,
>> Martin.-
>>
>> On Tue, Aug 29, 2017 at 4:34 PM, Xuelei Fan <xuelei.fan at oracle.com
>> <mailto:xuelei.fan at oracle.com>> wrote:
>>
>> On 8/26/2017 2:56 PM, Bernd Eckenfels wrote:
>> > How about only passing it to an extended handshake listener. The
>> > material does not have to be cached (the app can do it if needed)
>> and
>> > renegotiation works the same way. This can also be helpful for
>> things
>> > like logging the master secret (for wireshark decryption). It can
>> even
>> > handle auditing of session resumptions
>> Martin, what do you think about Bernd's proposal above and similar
>> callback mechanism?
>>
>> More comment inlines.
>>
>> On 8/29/2017 11:50 AM, Martin Balao wrote:
>>
>> Hi Xuelei,
>>
>> >There are a few protocols that can benefits from exporting
>> SSL/TLS handshake materials, including RFC 5929, RFC 5056, token
>> binding and TLS 1.3 itself. Can we define a general API so as
>> to exposing the handshake materials, so as to mitigate the
>> inflating of JSSE APIs? I may suggest make further evaluation
>> before move on to following design and code.
>>
>> Do you prefer an API like "public byte[]
>> getTlsHandshakeMaterial(String materialType)" (in SSLSocket and
>> SSLEngine) where "materialType" can eventually be
>> "clientFinishedMessage"/"finishedMessage" or
>> "serverFinishedMessage"/"peerFinishedMessage"?
>>
>> The problem of the APIs like that is, when applications call the
>> method, it is not always return the expected result, and the
>> implementation may have to cache the message even if an application
>> never use it. See more in the following example.
>>
>> I cannot think of "serverCertificate" or "masterKey" as this is
>> more related to a Session and not neccessarily to a handshake.
>> getTlsHandshakeMaterial would be a lower level API and would
>> move the burden of knowing which information is required for
>> "tls-unique" TLS channel binding to the API consumer. Looks more
>> like the OpenSSL approach (instead of the Python, SSLBoring or
>> GnuTls approaches). However, OpenSSL have specific methods for
>> each piece of information instead of a generic and parametrized
>> one. I.e.: SSL_get_finished or SSL_get_peer_finished. What other
>> information do you expect the Handshaker to provide?
>>
>> >The SunJSSE provider happens to cache the finished messages
>> in its implementation so you can use it for tls-unique, but it
>> may not be true for other provider or other channel bindings.
>> Need to define a more reliable approach to get the handshake
>> materials.
>>
>> I focused on SunJSSE provider. I'm not sure about how other
>> providers may implement this API and where they can get the
>> required information from, without knowing their internals. In
>> regard to SunJSSE and "tls-unique" binding type, I leveraged on
>> existing data. If data weren't already there, I would have to
>> figure out how to get it from the handshake -doing the same that
>> was already done would have been an option-. Do you prefer the
>> Handshaker to provide a function to get different information
>> and not just the finished hash? (as for the public
>> SSLSocket/SSLEngine "getTlsHandshakeMaterial" API). Which other
>> information may be useful to get from the Handshaker? What do
>> you mean by reliable? (given that this is all SunJSSE internal
>> and we have no external dependencies).
>>
>> Let consider the use of the API.
>> byte[] getTlsChannelBinding("tls_unique");
>>
>> I'm confusing when I try to use it by myself:
>> 1. provider does not implement this method
>> return null or empty?
>>
>> It happens because an old provider should still work in new JDK, but
>> old provider does not implement new APIs, or a new provider does not
>> support this feature.
>>
>> 2. the method is called before handshaking
>> return null or empty?
>>
>> 3. the method is called during handshaking
>> return null, empty or the channel binding value?
>>
>> 4. the method is called at the same time the handshaking completed?
>> return the channel binding value?
>>
>> 5. the method is called after the handshaking
>> return the channel binding value?
>>
>> 6. the method is called during renegoitation
>> return null, empty, the old binding value, or the new binding
>> value?
>>
>> 7. the method is called after handshaking
>> return old binding value, or the new binding value?
>>
>> 8. the method is called after the initial handshaking, but the
>> binding value is changed shortly after because of renegotiation.
>> how could application use the binding value?
>>
>> We need a clear define of the behavior of the method. It could be
>> complicated if the method is designed as
>> getTlsChannelBinding("tls_unique").
>>
>> I feel that handshake material should be captured when
>> 1. it is requested to capture the handshake material, and
>> 2. the handshake material get produced.
>>
>> For the getTlsChannelBinding("tls_unique") API, it is unknown:
>> 1. Is it required to capture the handshake material?
>> 2. Is the handshake material produced?
>>
>> The two points could result in a few unexpected problems, as the
>> above 8 items that we may want to consider.
>>
>> In regard to other channel bindings, it'll depend on the binding
>> type the way in which the information is obtained. I.e.:
>> "tls-unique" SunJSSE implementation leverages on cached finished
>> messages. However, "tls-server-end-point" leverages on stored
>> certificates that are obtained from the Session (not from the
>> handshaker). Is there any specific channel binding you are
>> concerned with?
>>
>> >If the channel binding is not required, it may be not
>> necessary to expose the handshake materials. Need to define a
>> solution to indicate the need of the exporting.
>>
>> Do you mean a lower layer knowing if the upper layer is going to
>> require that information and decide to provide it or not based
>> on that knowledge? I think I didn't get your point here.
>>
>> I mean, if an application want to support channel binding, the
>> provider can provider the channel binding service; If the an
>> application does not want channel binding, the provider should be
>> perform the channel binding service. The getTlsChannelBinding()
>> make the provider MUST perform channel binding cache or calculation
>> no matter application want it or not.
>>
>> >2. No way to know the update of the underlying handshake
>> materials.
>> >If renegotiation can takes place, need to define a interface
>> to indicate that so that application can response accordingly.
>> See section 3 and 7 of RFC 5929.
>>
>> I intentionally skipped this -at the cost of a spurious
>> authentication- to avoid adding complexity to the API. An
>> spurious authentication -which does not appear likely to me- can
>> easily be retried by the application. The RFC 5929 suggests APIs
>> through which the application can *control* the flow (i.e.: hold
>> a renegotitation). This would expose JSSE internals. This is
>> more than notifying. Notification, in my opinion, adds no value:
>> what if the application already used the binding token before
>> receiving the notification? The spurious authentication will
>> happen anyways and has to be handled -i.e. retried-. It's just a
>> timing issue. The real value is controlling the flow as the RFC
>> suggests, but at the cost of exposing JSSE internals.
>>
>> My understanding, the block of the protocol is to make sure
>> application is performing the channel binding with the right value,
>> or updating the value accordingly if necessary. If you skip this
>> and when renegotiation happen, the channel binding could be limited,
>> or may not work as expected.
>>
>> Thanks,
>> Xuelei
>>
>> Kind regards,
>> Martin.-
>>
>>
>> On Sat, Aug 26, 2017 at 5:25 PM, Xuelei Fan
>> <xuelei.fan at oracle.com <mailto:xuelei.fan at oracle.com>
>> <mailto:xuelei.fan at oracle.com <mailto:xuelei.fan at oracle.com>>>
>>
>> wrote:
>>
>> Hi Marin,
>>
>> Sorry for the delay.
>>
>> There are a few protocols that can benefits from exporting
>> SSL/TLS
>> handshake materials, including RFC 5929, RFC 5056, token
>> binding and
>> TLS 1.3 itself. Can we define a general API so as to
>> exposing the
>> handshake materials, so as to mitigate the inflating of
>> JSSE APIs? I may suggest make further evaluation before move
>> on to following
>> design and code.
>>
>> >
>> http://cr.openjdk.java.net/~sgehwolf/webrevs/mbalaoal/JDK-64
>> 91070/webrev.02/
>> <http://cr.openjdk.java.net/~sgehwolf/webrevs/mbalaoal/JDK-6
>> 491070/webrev.02/>
>> <http://cr.openjdk.java.net/~s
>> gehwolf/webrevs/mbalaoal/JDK-6491070/webrev.02/
>> <http://cr.openjdk.java.net/~sgehwolf/webrevs/mbalaoal/JDK-6
>> 491070/webrev.02/>>
>> I have two concerns about the design:
>>
>> 1. Channel binding may be not always required.
>> SSLSocket/SSLEngine.getTlsChannelBinding(String
>> bindingType);
>>
>> The SunJSSE provider happens to cache the finished messages
>> in its
>> implementation so you can use it for tls-unique, but it may
>> not be
>> true for other provider or other channel bindings. Need to
>> define a
>> more reliable approach to get the handshake materials.
>>
>> If the channel binding is not required, it may be not
>> necessary to
>> expose the handshake materials. Need to define a solution to
>> indicate the need of the exporting.
>>
>> 2. No way to know the update of the underlying handshake
>> materials.
>> If renegotiation can takes place, need to define a interface
>> to
>> indicate that so that application can response
>> accordingly. See
>> section 3 and 7 of RFC 5929.
>>
>> Thanks,
>> Xuelei
>>
>> On 7/31/2017 8:53 AM, Martin Balao wrote:
>>
>> Hi,
>>
>> Here it is an update for the proposed TLS Channel
>> Bindings
>> support in OpenJDK:
>>
>> *
>> http://cr.openjdk.java.net/~sgehwolf/webrevs/mbalaoal/JDK-64
>> 91070/webrev.02/
>> <http://cr.openjdk.java.net/~sgehwolf/webrevs/mbalaoal/JDK-6
>> 491070/webrev.02/>
>> <http://cr.openjdk.java.net/~s
>> gehwolf/webrevs/mbalaoal/JDK-6491070/webrev.02/
>> <http://cr.openjdk.java.net/~sgehwolf/webrevs/mbalaoal/JDK-6
>> 491070/webrev.02/>>
>> (browse online)
>> *
>> http://cr.openjdk.java.net/~sgehwolf/webrevs/mbalaoal/JDK-64
>> 91070/webrev.02/6491070.webrev.02.zip
>> <http://cr.openjdk.java.net/~sgehwolf/webrevs/mbalaoal/JDK-6
>> 491070/webrev.02/6491070.webrev.02.zip>
>> <http://cr.openjdk.java.net/~s
>> gehwolf/webrevs/mbalaoal/JDK-6491070/webrev.02/6491070.webrev.02.zip
>> <http://cr.openjdk.java.net/~sgehwolf/webrevs/mbalaoal/JDK-6
>> 491070/webrev.02/6491070.webrev.02.zip>>
>> (download)
>>
>> Changes since v01:
>>
>> * getTlsChannelBinding API changed to return null by
>> default
>> (if not implemented), instead of throwing an
>> UnsupportedOperationException.
>>
>> * "tls-server-end-point" TLS channel binding now
>> supported.
>>
>> Kind regards,
>> Martin.-
>>
>> On Wed, Jul 26, 2017 at 4:12 PM, Martin Balao
>> <mbalao at redhat.com <mailto:mbalao at redhat.com>
>> <mailto:mbalao at redhat.com <mailto:mbalao at redhat.com>>
>> <mailto:mbalao at redhat.com <mailto:mbalao at redhat.com>
>>
>> <mailto:mbalao at redhat.com <mailto:mbalao at redhat.com>>>>
>>
>> wrote:
>>
>> Hi,
>>
>> Here it is my proposal for JDK-6491070 (Support
>> for RFC
>> 5929-Channel
>> Bindings: e.g. public API to obtain TLS finished
>> message) [1]:
>>
>> *
>> http://cr.openjdk.java.net/~sgehwolf/webrevs/mbalaoal/JDK-64
>> 91070/webrev.01/
>> <http://cr.openjdk.java.net/~sgehwolf/webrevs/mbalaoal/JDK-6
>> 491070/webrev.01/>
>> <http://cr.openjdk.java.net/~s
>> gehwolf/webrevs/mbalaoal/JDK-6491070/webrev.01/
>> <http://cr.openjdk.java.net/~sgehwolf/webrevs/mbalaoal/JDK-6
>> 491070/webrev.01/>>
>> <http://cr.openjdk.java.net/~s
>> gehwolf/webrevs/mbalaoal/JDK-6491070/webrev.01/
>> <http://cr.openjdk.java.net/~sgehwolf/webrevs/mbalaoal/JDK-6
>> 491070/webrev.01/>
>> <http://cr.openjdk.java.net/~s
>> gehwolf/webrevs/mbalaoal/JDK-6491070/webrev.01/
>> <http://cr.openjdk.java.net/~sgehwolf/webrevs/mbalaoal/JDK-6
>> 491070/webrev.01/>>>
>> *
>> http://cr.openjdk.java.net/~sgehwolf/webrevs/mbalaoal/JDK-64
>> 91070/webrev.01/6491070.webrev.01.zip
>> <http://cr.openjdk.java.net/~sgehwolf/webrevs/mbalaoal/JDK-6
>> 491070/webrev.01/6491070.webrev.01.zip>
>> <http://cr.openjdk.java.net/~s
>> gehwolf/webrevs/mbalaoal/JDK-6491070/webrev.01/6491070.webrev.01.zip
>> <http://cr.openjdk.java.net/~sgehwolf/webrevs/mbalaoal/JDK-6
>> 491070/webrev.01/6491070.webrev.01.zip>>
>> <http://cr.openjdk.java.net/~s
>> gehwolf/webrevs/mbalaoal/JDK-6491070/webrev.01/6491070.webrev.01.zip
>> <http://cr.openjdk.java.net/~sgehwolf/webrevs/mbalaoal/JDK-6
>> 491070/webrev.01/6491070.webrev.01.zip>
>> <http://cr.openjdk.java.net/~s
>> gehwolf/webrevs/mbalaoal/JDK-6491070/webrev.01/6491070.webrev.01.zip
>> <http://cr.openjdk.java.net/~sgehwolf/webrevs/mbalaoal/JDK-6
>> 491070/webrev.01/6491070.webrev.01.zip>>>
>>
>> Notes:
>> * Implementation based on Channel Bindings for
>> TLS (RFC
>> 5929) [2]
>>
>> * Only "tls-unique" currently supported
>>
>> Look forward to your comments.
>>
>> Kind regards,
>> Martin.-
>>
>> --
>> [1] -
>> https://bugs.openjdk.java.net/browse/JDK-6491070
>> <https://bugs.openjdk.java.net/browse/JDK-6491070>
>> <https://bugs.openjdk.java.net/browse/JDK-6491070
>> <https://bugs.openjdk.java.net/browse/JDK-6491070>>
>> <https://bugs.openjdk.java.net/browse/JDK-6491070
>> <https://bugs.openjdk.java.net/browse/JDK-6491070>
>> <https://bugs.openjdk.java.net/browse/JDK-6491070
>> <https://bugs.openjdk.java.net/browse/JDK-6491070>>>
>> [2] - https://tools.ietf.org/html/rfc5929
>> <https://tools.ietf.org/html/rfc5929>
>> <https://tools.ietf.org/html/rfc5929
>> <https://tools.ietf.org/html/rfc5929>>
>> <https://tools.ietf.org/html/rfc5929
>> <https://tools.ietf.org/html/rfc5929>
>> <https://tools.ietf.org/html/rfc5929
>> <https://tools.ietf.org/html/rfc5929>>>
>>
>>
>>
>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/security-dev/attachments/20171023/73dea668/attachment.htm>
More information about the security-dev
mailing list