[Openvpn-devel] [PATCH applied] Re: Print remote IPv4 address on a dual-stack v6 socket in IPv4 format

[Gert Doering]

Patch has been applied to the master branch.

commit 0b1a68fffa33e175c320c2828604cdc7dfb097e7 (master)

Author: Gert Doering
List-Post: openvpn-devel@lists.sourceforge.net
Date:   Mon Dec 29 18:48:45 2014 +0100

 Print remote IPv4 address on a dual-stack v6 socket in IPv4 format

 Signed-off-by: Gert Doering 
 Acked-by: Arne Schwabe 
 Message-Id: <1419875325-96015-1-git-send-email-g...@greenie.muc.de>
 URL: http://article.gmane.org/gmane.network.openvpn.devel/9363


--
kind regards,

Gert Doering

Re: [Openvpn-devel] [PATCH v3] Mac OS X Keychain management client

[Gert Doering]

Hi,

On Sun, Feb 15, 2015 at 10:05:07PM +0100, Arne Schwabe wrote:
> Am 24.01.15 um 18:04 schrieb Vasily Kulikov:
[..]
> > OpenVPN itself gets new 'NEED-CERTIFICATE" command which is called when
> > --management-external-cert is used.  It is implemented as a multiline
> > command very similar to an existing 'RSA-SIGN' command.
> >
> > The patch is against commit 3341a98c2852d1d0c1eafdc70a3bdb218ec29049.
> ACK from me to the OpenVPN part. I also tested the patch in OpenVPN for
> Android and the RSA-SIGN still works as expected. I have not reviewed
> the OS X contrib program (other than a quick glance at the code) but I
> think marking it as contrib it should be allowed to be still included.

I hear Arne, and James also ACKed this ("based on testing", which Arne
did).

I'm not merging it yet, though - Vasily, please provide a v4 of the patch
that adds:

 - documentation of --management-external-cert in doc/openvpn.8
 - documentation of the new management command and response in 
   doc/management-notes.txt
 - fix the typos in the options here (please fix the other one, too):

@@ -2221,6 +2230,8 @@ options_postprocess_verify_ce (const struct options *optio
ns, const struct conne
 #ifdef MANAGMENT_EXTERNAL_KEY
   if (options->management_flags & MF_EXTERNAL_KEY)
msg(M_USAGE, "Parameter --external-management-key cannot be used whe
n --pkcs12 is also specified."); 
+  if (options->management_flags & MF_EXTERNAL_CERT)
+   msg(M_USAGE, "Parameter --external-management-cert cannot be used wh
en --pkcs12 is also specified.");
 #endif
 #endif
 }

... it's "--management-external-*", not "--external-management-*".

With that, I'll merge right away :-)

thanks,

gert

-- 
USENET is *not* the non-clickable part of WWW!
   //www.muc.de/~gert/
Gert Doering - Munich, Germany g...@greenie.muc.de
fax: +49-89-35655025g...@net.informatik.tu-muenchen.de


pgp1k4t7kJD0_.pgp
Description: PGP signature

Re: [Openvpn-devel] [PATCH v3] Mac OS X Keychain management client

[Arne Schwabe]

Am 24.01.15 um 18:04 schrieb Vasily Kulikov:
> This patch adds support for using certificates stored in the Mac OSX
> Keychain to authenticate with the OpenVPN server.  This works with
> certificates stored on the computer as well as certificates on hardware
> tokens that support Apple's tokend interface.
>
> This patch version implements management client which handles RSA-SIGN
> command for RSA offloading.  Also it handles new 'NEED-CERTIFICATE'
> request to pass a certificate from the keychain to OpenVPN.
>
> OpenVPN itself gets new 'NEED-CERTIFICATE" command which is called when
> --management-external-cert is used.  It is implemented as a multiline
> command very similar to an existing 'RSA-SIGN' command.
>
> The patch is against commit 3341a98c2852d1d0c1eafdc70a3bdb218ec29049.
>
>
ACK from me to the OpenVPN part. I also tested the patch in OpenVPN for
Android and the RSA-SIGN still works as expected. I have not reviewed
the OS X contrib program (other than a quick glance at the code) but I
think marking it as contrib it should be allowed to be still included.

Arne



signature.asc
Description: OpenPGP digital signature

Re: [Openvpn-devel] [PATCH] Print remote IPv4 address on a dual-stack v6 socket in IPv4 format

[Gert Doering]

Hi,

On Mon, Feb 09, 2015 at 10:04:03AM +0100, Arne Schwabe wrote:
> > --- a/src/openvpn/mroute.c
> > +++ b/src/openvpn/mroute.c
> > @@ -426,8 +426,16 @@ mroute_addr_print_ex (const struct mroute_addr *ma,
> >   break;
> > case MR_ADDR_IPV6:
> >   {
> > -   buf_printf (, "%s",
> > - print_in6_addr( *(struct in6_addr*), 0, gc)); 
> > +   if ( IN6_IS_ADDR_V4MAPPED( (struct in6_addr*) ) )
> > + {
> > +   buf_printf (, "%s",
> > +print_in_addr_t( *(in_addr_t*)([12]), 
> > IA_NET_ORDER, gc));
> Have you checked that this is endian safe?

Yep, both by running on i386 and on sparc64 - and then by staring at the
code and wondering why it works.

Thing is that print_in_addr_t() takes a flag, IA_NET_ORDER, which tells it
"that 32bit thingie you're printing now is in network byte order", so it
will not call htonl() on it before passing to inet_ntoa(), which wants
network byte order.

(And the IPv6 address is "just an array of 16 octets", no possible confusion
here with 32bit integer representation)

From the Linux man page:

 snip 
   The inet_ntoa() function converts the Internet host address  in,  given
   in  network  byte  order,  to a string in IPv4 dotted-decimal notation.
   The string is returned in a statically allocated buffer,  which  subse-
   quent calls will overwrite.
 snip 

(interesting enough, the FreeBSD man page only mentions "network byte
order" for inet_ntop()...)


> > +   {
> > + struct in_addr ia;
> > + ia.s_addr = *(in_addr_t *)>addr.in6.sin6_addr.s6_addr[12] ;
> > + openvpn_snprintf (name_buf, sizeof (name_buf), "%s_ip", name_prefix);
> > + openvpn_snprintf (buf, sizeof(buf), "%s", inet_ntoa(ia) );
> Same as above.

Same as above :-) - I admit that this is all amazingly ugly, but it is
technically correct.  The IPv6 address is always in network byte order,
and inet_ntoa() wants "its" 32bit that way...

In the end, this is the same code as in the first place, except that we
have no gc around here, and print_in_addr_t() wants one...   so I did it
with the existing buf.

> Otherwise ACK from me.

Thanks.  Will proceed to merge this bikeshed :-)

gert

-- 
USENET is *not* the non-clickable part of WWW!
   //www.muc.de/~gert/
Gert Doering - Munich, Germany g...@greenie.muc.de
fax: +49-89-35655025g...@net.informatik.tu-muenchen.de


pgpxb_47kNe0k.pgp
Description: PGP signature

[Openvpn-devel] [PATCH v2 for 2.3] New approach to handle peer-id related changes to link-mtu.

[Gert Doering]

Instead of statically increasing link-mtu by +3, keep the old value for
OCC compatibility with old servers/clients, and only increase link-mtu
if peer-id option is enabled (right now: is pushed by server).

If link-mtu has been set in the config, keep configured value, and log
warning (because the extra overhead has to decrease tun-mtu).

Reserve extra +3 bytes in frame->extra_link.

v2: use frame->extra_link, not frame->extra_buffer (receive path on server)
introduce frame_add_to_link_mtu() to manipulate frame->link_mtu value
rework comments to make more clear what is happening

Adaption to 2.3: reserve +8 bytes in frame->extra_buffer - if compression
is not enabled, the 2.3 code does not reserve space for compression
overhead (2.4 code does), so the buffer ends up being too small.
+3 is not sufficient because the buffer handling code also does some
alignment tricks...

This reverts commit 4ec70ca227370380011d072c09b739135e236183.

Signed-off-by: Gert Doering 
Message-Id: <1423390725-13438-1-git-send-email-g...@greenie.muc.de>
URL: http://article.gmane.org/gmane.network.openvpn.devel/9450
(cherry picked from commit 9e0963c11aa439deb382d7d6bc40b6ade999401c)
---
 src/openvpn/init.c | 24 
 src/openvpn/mtu.h  |  6 ++
 src/openvpn/ssl.c  | 10 ++
 3 files changed, 36 insertions(+), 4 deletions(-)

diff --git a/src/openvpn/init.c b/src/openvpn/init.c
index 4cfa132..48b28fc 100644
--- a/src/openvpn/init.c
+++ b/src/openvpn/init.c
@@ -1798,6 +1798,19 @@ do_deferred_options (struct context *c, const unsigned 
int found)
   msg (D_PUSH, "OPTIONS IMPORT: peer-id set");
   c->c2.tls_multi->use_peer_id = true;
   c->c2.tls_multi->peer_id = c->options.peer_id;
+  frame_add_to_extra_frame(>c2.frame, +3);  /* peer-id overhead */
+  if ( !c->options.ce.link_mtu_defined )
+   {
+ frame_add_to_link_mtu(>c2.frame, +3);
+ msg (D_PUSH, "OPTIONS IMPORT: adjusting link_mtu to %d",
+   EXPANDED_SIZE(>c2.frame));
+   }
+  else
+   {
+ msg (M_WARN, "OPTIONS IMPORT: WARNING: peer-id set, but link-mtu"
+   " fixed by config - reducing tun-mtu to %d, expect"
+   " MTU problems", TUN_MTU_SIZE(>c2.frame) );
+   }
 }
 #endif
 }
@@ -2400,6 +2413,17 @@ do_init_frame (struct context *c)
*/
   frame_finalize_options (c, NULL);

+  /* packets with peer-id (P_DATA_V2) need 3 extra bytes in frame (on client)
+   * and need link_mtu+3 bytes on socket reception (on server).
+   *
+   * accomodate receive path in f->extra_link
+   *send path in f->extra_buffer (+leave room for alignment)
+   *
+   * f->extra_frame is adjusted when peer-id option is push-received
+   */
+  frame_add_to_extra_link(>c2.frame, 3);
+  frame_add_to_extra_buffer(>c2.frame, 8);
+
 #ifdef ENABLE_FRAGMENT
   /*
* Set frame parameter for fragment code.  This is necessary because
diff --git a/src/openvpn/mtu.h b/src/openvpn/mtu.h
index 29ec21f..bccd681 100644
--- a/src/openvpn/mtu.h
+++ b/src/openvpn/mtu.h
@@ -258,6 +258,12 @@ frame_headroom (const struct frame *f, const unsigned int 
flag_mask)
  */

 static inline void
+frame_add_to_link_mtu (struct frame *frame, const int increment)
+{
+  frame->link_mtu += increment;
+}
+
+static inline void
 frame_add_to_extra_frame (struct frame *frame, const int increment)
 {
   frame->extra_frame += increment;
diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
index b62dc12..423aedb 100644
--- a/src/openvpn/ssl.c
+++ b/src/openvpn/ssl.c
@@ -267,14 +267,16 @@ tls_get_cipher_name_pair (const char * cipher_name, 
size_t len) {
   return NULL;
 }

-/**
- * Max number of bytes we will add for data structures common to both data and
- * control channel packets (1 byte opcode + 3 bytes peer-id).
+/*
+ * Max number of bytes we will add
+ * for data structures common to both
+ * data and control channel packets.
+ * (opcode only).
  */
 void
 tls_adjust_frame_parameters(struct frame *frame)
 {
-  frame_add_to_extra_frame (frame, 1 + 3); /* space for opcode + peer-id */
+  frame_add_to_extra_frame (frame, 1); /* space for opcode */
 }

 /*
-- 
2.1.3

[Openvpn-devel] [PATCH applied] Re: New approach to handle peer-id related changes to link-mtu.

[Gert Doering]

Patch has been applied to the master branch.  

Release/2.3 needs a slightly different patch due to different buffer 
space reservation in the compression code (which means that the extra 
bytes we need are not there unless comp-lzo is used).

commit 9e0963c11aa439deb382d7d6bc40b6ade999401c (master)

Author: Gert Doering
List-Post: openvpn-devel@lists.sourceforge.net
Date:   Sun Feb 8 11:18:45 2015 +0100

 New approach to handle peer-id related changes to link-mtu.

 Signed-off-by: Gert Doering 
 Acked-by: Arne Schwabe 
 Message-Id: <1423390725-13438-1-git-send-email-g...@greenie.muc.de>
 URL: http://article.gmane.org/gmane.network.openvpn.devel/9450


--
kind regards,

Gert Doering

[Openvpn-devel] [PATCH applied] Re: Disable SSL compression

[Gert Doering]

ACKmetoo.

Your patch has been applied to the master and release/2.3 branch.

commit 5d5233778868ddd568140c394adfcfc8e3453245 (master)
commit 5b46cf43432e69bb55747830494f613115a2af0c (release/2.3)

Author: Steffan Karger
List-Post: openvpn-devel@lists.sourceforge.net
Date:   Sun Feb 15 15:24:26 2015 +0100

 Disable SSL compression

 Signed-off-by: Steffan Karger 
 Acked-by: Arne Schwabe 
 Acked-by: Gert Doering 
 Message-Id: <1424010266-5910-1-git-send-email-stef...@karger.me>
 URL: http://article.gmane.org/gmane.network.openvpn.devel/9453
 Signed-off-by: Gert Doering 


--
kind regards,

Gert Doering

Re: [Openvpn-devel] [PATCH] Disable SSL compression

[Steffan Karger]

On 15-02-15 16:00, Arne Schwabe wrote:
> ACK from me. Sounds sensible to me. If do not support 0.9.8 anymore (in
> -master perhaps?) I would like this to be commited without ifdef.

Agreed, but we went from 0.9.6+ in 2.3 to 0.9.8+ in 2.4/master.

I put 0.9.8 explicitly in the comment, so that if we decide to drop
support for 0.9.8, it is easy to spot that we can remove these ifdefs.
(and if we drop 0.9.8, we can probably remove a lot more code :) ).

-Steffan

Re: [Openvpn-devel] [PATCH] Disable SSL compression

[Arne Schwabe]

On 15.02.2015 15:24, Steffan Karger wrote:
> As reported in trac #502, SSL compression can cause problems in some corner
> cases.  OpenVPN does not need SSL compression, since the control channel is
> low bandwidth.  This does not influence the data channel compressen (i.e.
> --comp or --comp-lzo).
>
> Even though this has not yet been relevant for OpenVPN (since an attacker
> can not easily control contents of control channel messages), SSL
> compression has been used in the CRIME and BREACH attacks on TLS.  TLS 1.3
> will probably even remove support for compression all together, for
> exactly this reason.
>
> Since we don't need it, and SSL compression causes issues, let's just
> disable it in OpenSSL builds.  PolarSSL has no run-time flag to disable
> compression, but is by default compiled without compression.
>
ACK from me. Sounds sensible to me. If do not support 0.9.8 anymore (in
-master perhaps?) I would like this to be commited without ifdef.

Arne

[Openvpn-devel] [PATCH] Disable SSL compression

[Steffan Karger]

As reported in trac #502, SSL compression can cause problems in some corner
cases.  OpenVPN does not need SSL compression, since the control channel is
low bandwidth.  This does not influence the data channel compressen (i.e.
--comp or --comp-lzo).

Even though this has not yet been relevant for OpenVPN (since an attacker
can not easily control contents of control channel messages), SSL
compression has been used in the CRIME and BREACH attacks on TLS.  TLS 1.3
will probably even remove support for compression all together, for
exactly this reason.

Since we don't need it, and SSL compression causes issues, let's just
disable it in OpenSSL builds.  PolarSSL has no run-time flag to disable
compression, but is by default compiled without compression.

Signed-off-by: Steffan Karger 
---
 src/openvpn/ssl_openssl.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/src/openvpn/ssl_openssl.c b/src/openvpn/ssl_openssl.c
index 48c0571..d9abc6e 100644
--- a/src/openvpn/ssl_openssl.c
+++ b/src/openvpn/ssl_openssl.c
@@ -203,6 +203,10 @@ tls_ctx_set_options (struct tls_root_ctx *ctx, unsigned 
int ssl_flags)
 if (tls_ver_min > TLS_VER_1_2 || tls_ver_max < TLS_VER_1_2)
   sslopt |= SSL_OP_NO_TLSv1_2;
 #endif
+#ifdef SSL_OP_NO_COMPRESSION
+/* Disable compression - flag not available in OpenSSL 0.9.8 */
+sslopt |= SSL_OP_NO_COMPRESSION;
+#endif
 SSL_CTX_set_options (ctx->ctx, sslopt);
   }

-- 
2.1.0