Re: [Openvpn-devel] [PATCH v3] Use separate list for per-client push options

2016-10-10 Thread Steffan Karger 
Hi,

On 10-10-16 16:54, Lev Stipakov wrote:
> Move client-specific push options (currently peer-id and cipher) to
> separate list, which is deallocated after push_reply
> has been send. This makes sure that options fit into buf,
> not duplicated nor leak memory on renegotiation.

Feature-ACK.  Very needed refactoring.

But some very minor comments:

> - * @param o  The current connection's options
> - * @param msglevel   The message level to use when printing errors
> + * @param gc GC arena where options are allocated
> + * @param push_list Push list containing options
> + * @param msglevel  The message level to use when printing errors
>   * @param fmtFormat string for the option

Some whitespace inconsistencies here (looks funny with ts=8).

> +prepare_push_reply (struct context *c, struct gc_arena *gc, struct push_list 
> *push_list)

> +  if (c->c2.push_ifconfig_defined && c->c2.push_ifconfig_local && 
> c->c2.push_ifconfig_remote_netmask)

> +   push_option_fmt(gc, push_list, M_USAGE, "peer-id %d", 
> tls_multi->peer_id);

These lines exceed 80 chars (and won't get much harder to read from
wrapping), so please wrap them.

I ran some smoke tests, but my jenkins broke down do didn't test very
thoroughly.  Still, the code looks very reasonable and smoke tests
succeed, so ACK if the above nitpicks are taken care of.

-Steffan

--
Check out the vibrant tech community on one of the world's most 
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

[Openvpn-devel] [PATCH v3] Use separate list for per-client push options

2016-10-10 Thread Lev Stipakov 
v3:
 - rebase on master

v2:
 - Also move ifconfig and ipv6-ifconfig to separate options list

Move client-specific push options (currently peer-id and cipher) to
separate list, which is deallocated after push_reply
has been send. This makes sure that options fit into buf,
not duplicated nor leak memory on renegotiation.

Signed-off-by: Lev Stipakov 
---
 src/openvpn/push.c | 188 +
 1 file changed, 104 insertions(+), 84 deletions(-)

diff --git a/src/openvpn/push.c b/src/openvpn/push.c
index df4f596..1f28826 100644
--- a/src/openvpn/push.c
+++ b/src/openvpn/push.c
@@ -40,26 +40,29 @@
 
 #if P2MP
 
+static char push_reply_cmd[] = "PUSH_REPLY";
+
 /**
- * Add an option to the push list by providing a format string.
+ * Add an option to the given 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 oThe current connection's options
- * @param msglevel The message level to use when printing errors
+ * @param gc   GC arena where options are allocated
+ * @param push_list Push list containing 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,
+static bool push_option_fmt(struct gc_arena *gc, struct push_list *push_list, 
int msglevel,
 const char *fmt, ...)
 #ifdef __GNUC__
 #if __USE_MINGW_ANSI_STDIO
-__attribute__ ((format (gnu_printf, 3, 4)))
+__attribute__ ((format (gnu_printf, 4, 5)))
 #else
-__attribute__ ((format (__printf__, 3, 4)))
+__attribute__ ((format (__printf__, 4, 5)))
 #endif
 #endif
 ;
@@ -295,16 +298,39 @@ send_push_request (struct context *c)
 /**
  * Prepare push options, based on local options and available peer info.
  *
- * @param options  Connection options
- * @param tls_multiTLS state structure for the current tunnel
+ * @param context  context structure storing data for VPN tunnel
+ * @param gc   gc arena for allocating push options
+ * @param push_listpush list to where options are added
  *
  * @return true on success, false on failure.
  */
 static bool
-prepare_push_reply (struct options *o, struct tls_multi *tls_multi)
+prepare_push_reply (struct context *c, struct gc_arena *gc, struct push_list 
*push_list)
 {
   const char *optstr = NULL;
+  const struct tls_multi *tls_multi = c->c2.tls_multi;
   const char * const peer_info = tls_multi->peer_info;
+  struct options *o = >options;
+
+  /* ipv6 */
+  if (c->c2.push_ifconfig_ipv6_defined && !o->push_ifconfig_ipv6_blocked)
+{
+  push_option_fmt (gc, push_list, M_USAGE, "ifconfig-ipv6 %s/%d %s",
+  print_in6_addr (c->c2.push_ifconfig_ipv6_local, 0, gc),
+  c->c2.push_ifconfig_ipv6_netbits,
+  print_in6_addr (c->c2.push_ifconfig_ipv6_remote, 0, gc));
+}
+
+  /* ipv4 */
+  if (c->c2.push_ifconfig_defined && c->c2.push_ifconfig_local && 
c->c2.push_ifconfig_remote_netmask)
+{
+  in_addr_t ifconfig_local = c->c2.push_ifconfig_local;
+  if (c->c2.push_ifconfig_local_alias)
+   ifconfig_local = c->c2.push_ifconfig_local_alias;
+  push_option_fmt (gc, push_list, M_USAGE, "ifconfig %s %s",
+  print_in_addr_t (ifconfig_local, 0, gc),
+  print_in_addr_t (c->c2.push_ifconfig_remote_netmask, 0, 
gc));
+}
 
   /* Send peer-id if client supports it */
   optstr = peer_info ? strstr(peer_info, "IV_PROTO=") : NULL;
@@ -314,8 +340,7 @@ prepare_push_reply (struct options *o, struct tls_multi 
*tls_multi)
   int r = sscanf(optstr, "IV_PROTO=%d", );
   if ((r == 1) && (proto >= 2))
{
- push_remove_option(o, "peer-id");
- push_option_fmt(o, M_USAGE, "peer-id %d", tls_multi->peer_id);
+ push_option_fmt(gc, push_list, M_USAGE, "peer-id %d", 
tls_multi->peer_id);
}
 }
 
@@ -325,7 +350,7 @@ prepare_push_reply (struct options *o, struct tls_multi 
*tls_multi)
   /* if we have already created our key, we cannot change our own
* cipher, so disable NCP and warn = explain why
*/
-  struct tls_session *session = _multi->session[TM_ACTIVE];
+  const struct tls_session *session = _multi->session[TM_ACTIVE];
   if ( session->key[KS_PRIMARY].crypto_options.key_ctx_bi.initialized )
{
   msg( M_INFO, "PUSH: client wants to negotiate cipher (NCP), but "
@@ -336,87 +361,76 @@ prepare_push_reply (struct options *o, struct tls_multi 
*tls_multi)
{
  /* Push the first cipher from --ncp-ciphers to the client.
   * TODO: actual negotiation, instead of server dictatorship. */
- char *push_cipher =