Hi, Thanks for the review. Attached a v2 of the patch, and responses inline below.
On Mon, Feb 15, 2016 at 8:58 PM, Gert Doering <g...@greenie.muc.de> wrote: > On Sun, Feb 07, 2016 at 08:47:18PM +0100, Steffan Karger wrote: >> This patch: >> * Makes the server advertise "IV_NCP=2", if --push-peer-info 2 is enabled. > > I'd rather not do that - it won't do harm, but technically, it's not > correct (we don't support "negotiable" ciphers in that direction either) - > and then, nothing at the client ever *looks* at the peer-info variables, > so it's somewhat moot. And that makes it "unneccessary extra code"... Ok, removed from the patch. > As a side note: I was wondering if o->gc is save to use here (when is it > cleaned up? is it ever cleaned up?) and under the assumption that the > existing code in push_option_ex() is correct, the new code is correct > as well - the existing code uses o->gc too. Yes, o->gc is the right thing here. It is freshly initialized by init_instance() when a new 'child' (connection/session) instance is created, and cleaned up by multi_close_instance() when the child is instance is destroyed. > A minor comment comment: > >> #if P2MP >> >> +/** >> + * Add an option to the push list by providing a format string. >> + * >> + * The string added to the push options is allocated in o->gc, so fmt may be >> + * free'd by the caller before the option is sent to the peer. > > Since nobody ever free()s *fmt*, maybe word that slightly differently, > like: > > * The resulting string added to the push options is allocated in o->gc, > * so the caller does not have to preserve anything > > or so Yes, much better. Updated. -Steffan
From b0a9c3f22b39c7b658aa2f59786cad217256c9d6 Mon Sep 17 00:00:00 2001 From: Steffan Karger <stef...@karger.me> Date: Mon, 15 Feb 2016 21:07:11 +0100 Subject: [PATCH 10/10 v2] Add preliminary server-side support for negotiable crypto parameters Add preliminary support for Negotiable Crypto Parameters 'level 2' (IV_NCP=2), as proposed by James Yonan on the openvpn-devel mailinglist: http://comments.gmane.org/gmane.network.openvpn.devel/9385 This patch makes a server push a 'cipher XXX' directive to the client, if the client advertises "IV_NCP=2", where XXX is the cipher set in the server config file. This enables clients that have support for IV_NCP to connect to a server, even when the client does not have the correct cipher specified in it's config file. Since pushing the cipher directive is quite similar to pushing peer-id, I moved peer-id pushing to the same prepare_push_reply() function I created for pushing cipher. Adding these directives as regular push options allows us to use the existing 'push-continuation' infrastructure. Note that we should not reduce safe_cap in send_push_reply, because it was never increased to account for peer-id. This is a preliminary patch, which will be followed by more patches to add client support, and configurability. v2: * Reword doxygen of push_options_fmt() * No longer push IV_NCP as a server Signed-off-by: Steffan Karger <stef...@karger.me> --- src/openvpn/push.c | 97 +++++++++++++++++++++++++++++++++++++++++++++--------- src/openvpn/push.h | 2 -- 2 files changed, 82 insertions(+), 17 deletions(-) diff --git a/src/openvpn/push.c b/src/openvpn/push.c index d4f3cb6..c29093b 100644 --- a/src/openvpn/push.c +++ b/src/openvpn/push.c @@ -40,6 +40,30 @@ #if P2MP +/** + * Add an option to the push list by providing a format string. + * + * The string added to the push options is allocated in o->gc, so the caller + * does not have to preserve anything. + * + * @param o The current connection's options + * @param msglevel The message level to use when printing errors + * @param fmt Format string for the option + * @param ... Format string arguments + * + * @return true on success, false on failure. + */ +static bool push_option_fmt(struct options *o, int msglevel, + const char *fmt, ...) +#ifdef __GNUC__ +#if __USE_MINGW_ANSI_STDIO + __attribute__ ((format (gnu_printf, 3, 4))) +#else + __attribute__ ((format (__printf__, 3, 4))) +#endif +#endif + ; + /* * Auth username/password * @@ -239,7 +263,47 @@ send_push_request (struct context *c) #if P2MP_SERVER -bool +/** + * Prepare push options, based on local options and available peer info. + * + * @param options Connection options + * @param tls_multi TLS state structure for the current tunnel + * + * @return true on success, false on failure. + */ +static bool +prepare_push_reply (struct options *o, struct tls_multi *tls_multi) +{ + const char *optstr = NULL; + const char * const peer_info = tls_multi->peer_info; + + /* Send peer-id if client supports it */ + optstr = peer_info ? strstr(peer_info, "IV_PROTO=") : NULL; + if (optstr) + { + int proto = 0; + int r = sscanf(optstr, "IV_PROTO=%d", &proto); + if ((r == 1) && (proto >= 2)) + { + push_option_fmt(o, M_USAGE, "peer-id %d", tls_multi->peer_id); + } + } + + /* Push cipher if client supports Negotiable Crypto Parameters */ + optstr = peer_info ? strstr(peer_info, "IV_NCP=") : NULL; + if (optstr) + { + int ncp = 0; + int r = sscanf(optstr, "IV_NCP=%d", &ncp); + if ((r == 1) && (ncp == 2)) + { + push_option_fmt(o, M_USAGE, "cipher %s", o->ciphername); + } + } + return true; +} + +static bool send_push_reply (struct context *c) { struct gc_arena gc = gc_new (); @@ -309,19 +373,6 @@ send_push_reply (struct context *c) if (multi_push) buf_printf (&buf, ",push-continuation 1"); - /* Send peer-id if client supports it */ - if (c->c2.tls_multi->peer_info) - { - const char* proto_str = strstr(c->c2.tls_multi->peer_info, "IV_PROTO="); - if (proto_str) - { - int proto = 0; - int r = sscanf(proto_str, "IV_PROTO=%d", &proto); - if ((r == 1) && (proto >= 2)) - buf_printf(&buf, ",peer-id %d", c->c2.tls_multi->peer_id); - } - } - if (BLEN (&buf) > sizeof(cmd)-1) { const bool status = send_control_channel_string (c, BSTR (&buf), D_PUSH); @@ -409,6 +460,21 @@ push_options (struct options *o, char **p, int msglevel, struct gc_arena *gc) push_option (o, opt, msglevel); } +static bool push_option_fmt(struct options *o, int msglevel, + const char *format, ...) +{ + va_list arglist; + char tmp[256] = {0}; + int len = -1; + va_start (arglist, format); + len = vsnprintf (tmp, sizeof(tmp), format, arglist); + va_end (arglist); + if (len > sizeof(tmp)-1) + return false; + push_option (o, string_alloc (tmp, &o->gc), msglevel); + return true; +} + void push_reset (struct options *o) { @@ -442,7 +508,8 @@ process_incoming_push_request (struct context *c) } else { - if (send_push_reply (c)) + if (prepare_push_reply(&c->options, c->c2.tls_multi) && + send_push_reply (c)) { ret = PUSH_MSG_REQUEST; c->c2.sent_push_reply_expiry = now + 30; diff --git a/src/openvpn/push.h b/src/openvpn/push.h index fa06e08..19bbf5e 100644 --- a/src/openvpn/push.h +++ b/src/openvpn/push.h @@ -62,8 +62,6 @@ void push_options (struct options *o, char **p, int msglevel, struct gc_arena *g void push_reset (struct options *o); -bool send_push_reply (struct context *c); - void remove_iroutes_from_push_route_list (struct options *o); void send_auth_failed (struct context *c, const char *client_reason); -- 2.5.0
