On 26/05/14 15:25, Daniel Kubec wrote: > Add support for TLS Keying Material Exporters [RFC 5705]. > > Keying Material Exporter allow additional keying material to be derived from > existing TLS channel. > This exported keying material can then be used for a variety of purposes. >
I've finally had some time to compile and test this patch, as well as
done code review of it. First of all, this is a good piece of work! So
I'm looking forward to see this arrive in OpenVPN. It will surely help
develop more advanced authentication methods.
The patch does indeed work quite well. It provides a tls_binding_key
environment variable to plugins which supports OPENVPN_PLUGIN_TLS_FINAL.
Here's my comments, thoughts and questions ...
a) Is the intention that this tls_binding_key variable should only be
available when the OPENVPN_PLUGIN_TLS_FINAL plugin call happens?
Currently tls_binding_key is available to all plugin *and* script
calls happening after OPENVPN_PLUGIN_TLS_FINAL has been called. But
*only* after a plugin which supports this hook has run. This smells
like a bug.
I think I would recommend this variable only to available for the
OPENVPN_PLUGIN_TLS_FINAL call. Which means you would need to add a
setenv_del() in ssl.c:key_method_2_read() after plugin_call().
I spotted tls_binding_key in the following hooks (using a slightly
modified sample-plugin/log/log.c)
Client side: PLUGIN_IPCHANGE, PLUGIN_UP, PLUGIN_ROUTE_UP
and PLUGIN_ROUTE_DOWN
Server side: PLUGIN_IPCHANGE, PLUGIN_CLIENT_CONNECT_V2*,
PLUGIN_LEARN_ADDRESS and PLUGIN_CLIENT_DISCONNECT
* Most likely PLUGIN_CLIENT_CONNECT too
I also tried to use a --client-connect script which dumps 'printenv'
to a file. When this log.so plug-in was used, I saw tls_binding_key
in the printenv dump. It was not present when log.so was not loaded.
b) Should there be a script-hook available for this as well?
This could make it possible to make authentication methods using
scripts as an addition to plug-ins written in C. Just thinking
aloud, not saying it's a requirement.
c) The format is currently like this:
0f3d5a7a 8b78fe1a 8a7fce61 58b89142
Is this format the preferred formatting?
Other formats are available through format_hex_ex(). I would
probably recommend another separator than space, if a separator is
needed at all.
d) This patch breaks building with PolarSSL.
ssl.o: In function `key_method_2_read':
~/openvpn.git/src/openvpn/ssl.c:2129: undefined reference to
`key_state_export_keying_material'
collect2: error: ld returned 1 exit status
I would recommend to add a kind of wrapper function in
ssl_polarssl.c which adds this function. Not sure if it would
be appropriate to do an ASSERT(0) or do something else. Maybe it
should not set anything, just return without adding the
tls_binding_key. When plug-ins expecting to see this fails to find
this variable, it will the plug-in's responsibility to not explode.
e) Has this patch been tested with OpenSSL < 1.0.1?
Is it really needed with the #else ASSERT(0); ?
f) It should be stated better in the man page that this feature depends
on a plug-in supporting OPENVPN_PLUGIN_TLS_FINAL.
--
kind regards,
David Sommerseth
signature.asc
Description: OpenPGP digital signature
