Re: [Openvpn-devel] [PATCH v3] Add per session pseudo-random jitter to --reneg-sec intervals

[Simon Matter]

Hi Steffan,

While running your v3 version of the patch I found an issue with the
modified logging. It gives the following error while building

gcc -DHAVE_CONFIG_H -I. -I../.. -I../../include  -I../../include
-I../../src/compat   -DPLUGIN_LIBDIR=\"/usr/lib64/openvpn/plugins\" 
-O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions
-fstack-protector-strong --param=ssp-buffer-size=4 -grecord-gcc-switches  
-m64 -mtune=generic -std=c99 -c -o ssl.o ssl.c
ssl.c: In function 'tls_process':
ssl.c:2735:9: warning: format '%lld' expects argument of type 'long long
int', but argument 3 has type 'time_t' [-Wformat=]
 msg(D_TLS_DEBUG_LOW, "TLS: soft reset sec=%lld/%d bytes="
counter_format
 ^
and the log output is also corrupted

TLS: soft reset sec=14774687501680/336605974
bytes=18446744069414584320/581335 pkts=0/0
TLS: soft reset sec=14787572403571/77375 bytes=18446744069414584320/885
pkts=0/-1080308296
TLS: soft reset sec=14164802145506/6746662
bytes=18446744069414584320/86778 pkts=0/0
TLS: soft reset sec=15264313773538/186529 bytes=18446744069414584320/1108
pkts=0/13935244

Please have a look at the attached v4 patch which fixes it for me. The
output is now

TLS: soft reset sec=3474/3474 bytes=8470454/-1 pkts=108673/0
TLS: soft reset sec=3489/3489 bytes=429623769/-1 pkts=656033/0
TLS: soft reset sec=3517/3517 bytes=89365/-1 pkts=877/0
TLS: soft reset sec=3537/3537 bytes=206142/-1 pkts=1123/0

which seems reasonable to me.
In the patch I've modified the lines below:

 msg(D_TLS_DEBUG_LOW,
-"TLS: soft reset sec=%d bytes=" counter_format "/%d pkts="
counter_format "/%d",
-(int)(ks->established + session->opt->renegotiate_seconds -
now),
+"TLS: soft reset sec=%d/%d bytes=" counter_format "/%d pkts="
counter_format "/%d",
+(int)(now - ks->established), session->opt->renegotiate_seconds,
 ks->n_bytes, session->opt->renegotiate_bytes,

I'm not a developer and not a git user, so please accept my hand crafted
patch work :-)

Thanks and regards,
Simon

> From: Simon Matter 
>
> While we were suffering from the "TLS Renegotiation Slowdown" bug here
> https://community.openvpn.net/openvpn/ticket/854 we realized that there is
> still room for improvement in our use case.
>
> It appears that TLS renegotiation is getting more and more expensive in
> terms of CPU cycles with recent changes for more security. To make things
> worse, we realized that most renegotiation procedures took place at almost
> the same time and increased the CPU load too much during these periods.
> That's especially true on large, multi-instance openvpn setups.
>
> I've created attached patch to add a per session pseudo-random component
> to
> the --reneg-sec intervals so that renegotiation is evenly spread over
> time.
> It is configured by simply adding a second value to --reneg-sec as
> described in the --help text:
>
> --reneg-sec max [min] : Renegotiate data chan. key after at most max
>   (default=3600) and at least min (default 90% of max on
>   servers and equal to max on clients).
>
> The jitter is only enabled by default on servers, because the actual reneg
> time is min(reneg_server, reneg_client).  Introducing jitter at both ends
> would bias the actual reneg time to the min value.
>
> Note that the patch also slightly changes the log output to show the sec
> value in the same way as the bytes/pkts values:
>
> TLS: soft reset sec=3084/3084 bytes=279897/-1 pkts=1370/0
>
> The idea and first versions of this patch are from Simon Matter.  Steffan
> Karger later incorporated the mailing list comments into this patch.  So
> credits go to Simon, and all bugs are Steffan's fault ;-)
>
> Signed-off-by: Simon Matter 
> Signed-off-by: Steffan Karger 
> ---
> v3: incorporate comments from openvpn-devel@, don't add jitter by
> default on the client side.
>
>  doc/openvpn.8 | 26 +-
>  src/openvpn/init.c| 15 ++-
>  src/openvpn/options.c | 11 +--
>  src/openvpn/options.h |  1 +
>  src/openvpn/ssl.c |  6 +++---
>  5 files changed, 48 insertions(+), 11 deletions(-)
>
> diff --git a/doc/openvpn.8 b/doc/openvpn.8
> index a4189ac2..eb5258f9 100644
> --- a/doc/openvpn.8
> +++ b/doc/openvpn.8
> @@ -33,7 +33,7 @@
>  .\" .ft -- normal face
>  .\" .in +|-{n} -- indent
>  .\"
> -.TH openvpn 8 "25 August 2016"
> +.TH openvpn 8 "04 April 2017"
>  .\"*
>  .SH NAME
>  openvpn \- secure IP tunnel daemon.
> @@ -4957,10 +4957,26 @@ Renegotiate data channel key after
>  packets sent and received (disabled by default).
>  .\"*
>  .TP
> -.B \-\-reneg\-sec n
> -Renegotiate data channel key after
> -.B n
> -seconds (default=3600).
> +.B \-\-reneg\-sec max [min]
> +Renegotiate data channel key after at most
> +.B max
> 

Re: [Openvpn-devel] [PATCH] Implement "status 4" (JSON) for management interface

[David Sommerseth]

On 14/11/17 12:02, Gert Doering wrote:
> JSON is very trivial to produce (unlike XML, or netlink).  The escaping
> rules on producing are also very easy - basically, encode things in double
> quotes, and escape the set of { BS, FF, NL, CR, Tab, Backslash, Quote }
> with a single backslash before the corresponding character.
>
>


Right, all those are single byte characters - and that's fairly simple
... but that ignores various quirks which easily appears with multi-byte
characters - especially when "looping" through a value, byte by byte.
We support UTF-8 in certificate subject fields.  So this escaper needs
to handle those classes the stackoverflow mentions, plus beware of
multi-byte strings (so we need to use the plethora of mb* related
functions).

In a clean 8-bit ASCII only-world, things are less complicated.

Heiko and I have looked into the "simple" world of revamping the argv
parser (to avoid our own "homebrewed" printf-like processing and base it
on what is in the C library) and even this pre-parsing we need to do
have popped up with surprises.  The argv caller scope mostly covers
parsing strings which is defined by us developers so the variations are
not as broad, and luckily format strings is not expected to contain
UTF-8 chars.  But I do not for a second think processing certificate
subject strings will as easy as those values we need to parse (typically
CN) are not generated by us but a broad range of users.  Who knows what
kind of funny tricks they'll throw at our code?

But sure, we can start with our own string parser to escape JSON and see
how it works and then revisit using a library if the maintenance gets
too annoying.


-- 
kind regards,

David Sommerseth
OpenVPN, Inc




signature.asc
Description: OpenPGP digital signature
--
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

Re: [Openvpn-devel] [PATCH] Implement "status 4" (JSON) for management interface

[Jonathan K. Bullard]

Hi,

On Tue, Nov 14, 2017 at 3:31 AM, Gert Doering  wrote:
> Hi,
>
> On Mon, Nov 13, 2017 at 01:16:46PM +0100, David Sommerseth wrote:
>> But we should consider if we want to make use of a JSON library
>> producing the JSON streams.  The reason is to ensure the output is
>> according to the specification and that escaping if contents is
>> consistent and proper.  Imagine if someone puts a double-quote into the
>> CN field of a certificate?
>>
>>  CN="} Lets explode things, O=Hacktivist0
>>
>> Or other characters which needs escaping.
>
> I'm not convinced we want to import a new library dependency and a heap
> of #ifdef for this.
>
> Escaping on *output* is pretty trivial ("characters from 
> need to be encoded ") - and as long as we do not need to parse
> *incoming* JSON, a full-blown new library is mainly adding complications
> (like, configure flags, #ifdefs, library version dependencies, ...).

+1

Best regards,

Jon Bullard

--
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

Re: [Openvpn-devel] [PATCH] Implement "status 4" (JSON) for management interface

[Gert Doering]

Hi,

On Tue, Nov 14, 2017 at 11:56:10AM +0100, David Sommerseth wrote:
> And to me this feels like it actually contradicts our netlink discussion
> on the Hackathon, where you initially wanted to use an external library
> ... ;-)

It's a matter of complexity.   Netlink is binary, complex, and hard to
verify (unless you write your own decoder, which might have bugs on its
own).

JSON is very trivial to produce (unlike XML, or netlink).  The escaping
rules on producing are also very easy - basically, encode things in double
quotes, and escape the set of { BS, FF, NL, CR, Tab, Backslash, Quote }
with a single backslash before the corresponding character.

https://stackoverflow.com/questions/19176024/how-to-escape-special-characters-in-building-a-json-string


For *parsing*, I'd agree with you...

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


signature.asc
Description: PGP signature
--
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

Re: [Openvpn-devel] [PATCH] Implement "status 4" (JSON) for management interface

[David Sommerseth]

On 14/11/17 09:31, Gert Doering wrote:
> Hi,
> 
> On Mon, Nov 13, 2017 at 01:16:46PM +0100, David Sommerseth wrote:
>> But we should consider if we want to make use of a JSON library
>> producing the JSON streams.  The reason is to ensure the output is
>> according to the specification and that escaping if contents is
>> consistent and proper.  Imagine if someone puts a double-quote into the
>> CN field of a certificate?
>>
>>  CN="} Lets explode things, O=Hacktivist0
>>
>> Or other characters which needs escaping.
> 
> I'm not convinced we want to import a new library dependency and a heap
> of #ifdef for this.
> 
> Escaping on *output* is pretty trivial ("characters from  
> need to be encoded ") - and as long as we do not need to parse 
> *incoming* JSON, a full-blown new library is mainly adding complications
> (like, configure flags, #ifdefs, library version dependencies, ...).
> 
> But you knew that this response would be coming :-)
Not surprised ;-)

But I do disagree.  I tend to dislike re-inventing wheels when others do
the job for you.  Quite some years ago, I would probably have agreed
with you.  But I've been in projects where I implemented producing XML
data manually in a similar manner.  After fixing numerous of escaping
errors, I switched to a library and all these bugs disappeared and never
came back.  It is so easy to think you know exactly what needs to be
escaped beforehand and firmly believe you have covered each corner-case,
but it always sneaks back at you - as you didn't spot the hidden doors.

And to me this feels like it actually contradicts our netlink discussion
on the Hackathon, where you initially wanted to use an external library
... ;-)

That said, json-c is packaged and available on Linux, FreeBSD and even
OpenWRT/LEDE got a recent version available. I don't know the situation
on macOS (but that's similar to FreeBSD, I'd presume) and Windows is
also not clear to me (if this is a relevant feature there).  json-c is
also fairly small (approx 7k lines of code, stripped binary lib ~60KB)
and is being maintained (last commits from late October).  So this isn't
a big massive dependency.

With ./configure --enable-json we also don't change the current
behaviour in OpenVPN.  Which should only require a single #ifdef
ENABLE_JSON in the multi_print_status() [multi.c:859] function.  But I
also see that the status printing stuff could be improved as well, to
simplify the implementation a bit too (which is a different task).


-- 
kind regards,

David Sommerseth
OpenVPN, Inc




signature.asc
Description: OpenPGP digital signature
--
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

Re: [Openvpn-devel] [PATCH] Implement "status 4" (JSON) for management interface

[Antonio Quartulli]

Hi,

On 14/11/17 16:31, Gert Doering wrote:
> Hi,
> 
> On Mon, Nov 13, 2017 at 01:16:46PM +0100, David Sommerseth wrote:
>> But we should consider if we want to make use of a JSON library
>> producing the JSON streams.  The reason is to ensure the output is
>> according to the specification and that escaping if contents is
>> consistent and proper.  Imagine if someone puts a double-quote into the
>> CN field of a certificate?
>>
>>  CN="} Lets explode things, O=Hacktivist0
>>
>> Or other characters which needs escaping.
> 
> I'm not convinced we want to import a new library dependency and a heap
> of #ifdef for this.
> 
> Escaping on *output* is pretty trivial ("characters from  
> need to be encoded ") - and as long as we do not need to parse 
> *incoming* JSON, a full-blown new library is mainly adding complications
> (like, configure flags, #ifdefs, library version dependencies, ...).
> 
> But you knew that this response would be coming :-)

+1

if we want to do output only, we should not need another library.
Especially if we are not going to do complex stuff.


Cheers,


-- 
Antonio Quartulli



signature.asc
Description: OpenPGP digital signature
--
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

Re: [Openvpn-devel] [PATCH] Implement "status 4" (JSON) for management interface

[Gert Doering]

Hi,

On Mon, Nov 13, 2017 at 01:16:46PM +0100, David Sommerseth wrote:
> But we should consider if we want to make use of a JSON library
> producing the JSON streams.  The reason is to ensure the output is
> according to the specification and that escaping if contents is
> consistent and proper.  Imagine if someone puts a double-quote into the
> CN field of a certificate?
> 
>  CN="} Lets explode things, O=Hacktivist0
> 
> Or other characters which needs escaping.

I'm not convinced we want to import a new library dependency and a heap
of #ifdef for this.

Escaping on *output* is pretty trivial ("characters from  
need to be encoded ") - and as long as we do not need to parse 
*incoming* JSON, a full-blown new library is mainly adding complications
(like, configure flags, #ifdefs, library version dependencies, ...).

But you knew that this response would be coming :-)

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


signature.asc
Description: PGP signature
--
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