Re: [Openvpn-devel] multihome broken in the presence of asymmetric routing

[Arne Schwabe]

Am 12.03.18 um 11:37 schrieb Teodor Milkov:
> Hello,
> 
> I have the following multihomed setup:
> 
> 
>    BGP1    BGP2
>     ^   ^
>     |   |
> +---+---+    +--+---+
> | IP1.1   IP1.2   IP1.3 |    | IP2.1  IP2.2   IP3.1 |
> |   |    |  |
> |   |    |  |
> |  RTR1 |    | RTR2 |
> |   |    |  |
> |   |    |  |
> |  vIP  |    | vIP  |
> +---+---+    +--+---+
>     |   |
>     |   |
>     |   |
>     |    VRRP   |
>     +---+
> 
> 
> I.e. two routers with BGP to multiple ISPs. VRRP running at inner side
> keeping one virtual IP (vIP) up at the master router.
> 
> Someday I hope I'll be able to use the vIP only for openvpn server bind
> IP ("local" config option). Untill then, for legacy reasons, I have to
> use the "multihome" config option, so that clients could connect to each
> of the 7 IPs (IP1.1, IP1.2, IP1.3, IP2.1, IP2.2, IP2.3, vIP).
> 
> Unfortunately, Linux would not respond to /some/ clients over UDP. It
> seems like this is due to the way "multihome" forces the output
> interface using IP_PKTINFO when we have asymetric path to/from vpn clients.
> 
> To provide single-socket UDP multihoming, openvpn uses the IP_PKTINFO
> data from recvmsg() and passes it as-is to sendmsg(). Apparently
> ipi_ifindex behaves in an interesting way under Linux. man 7 ip states:
> 
> If  IP_PKTINFO  is  passed  to  sendmsg(2)  and ipi_spec_dst  is  not
> zero, then it is used as the local source address for the routing table
> lookup and for setting up IP source route options.  When ipi_ifindex is
> not zero, the primary local address of the interface  specified  by
> the  index overwrites ipi_spec_dst for the routing table lookup.
> 
> 
> In my tests it's like /ipi_ifindex/ will override any routing table
> decision. But then if there is no routing table entry for the
> destination via
> that interface, the destination will be assumed to be on-link and will
> not be routed via a gateway. No error is returned to
> userspace, but if the destination does not respond to an ARP request on
> that link, the packet will be silently dropped. That's what I see with
> tcpdump on the openvpn server: arp requests for the dst address of the
> openvpn client. For example if a.b.c.100 is an openvpn client and
> x.y.z.1 is the virtual IP on the openvpn server to which the client
> tries to connect:
> 
> 11:52:40.695830 ARP, Request who-has a.b.c.100 tell x.y.z.1, length 28
> 
> I got it working by patching socket.c in the following manner:

Sorry for replying so late. I am just going through all open patches.
but with this i am not sure that it does not break the other multihomed
use cases where this behaviour is actually intended and you also only
tested it for your specific use case, so I am a bit cautious to just
include it.

Arne



signature.asc
Description: OpenPGP digital signature
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Re: [Openvpn-devel] [PATCH v2 3/5] Move NCP related function into a seperate file and add unit tests

[Lev Stipakov]

I found this discussion

https://sourceforge.net/p/openvpn/mailman/message/34405489/

and wonder if we should replace strdup with string_alloc("AES-256-GCM").

If the result of strdup is NULL because of OOM, then following is NULL

char *peer_ncp_list = tls_peer_ncp_list(peer_info);

and it is passed to

const char *token = strtok(tmp_ciphers, ":");

and then behavior depends on the previous state of strtok.

string_alloc(), on the other hand, calls check_malloc_return(), which
prints "out of memory" and exits program.


pe 14. helmik. 2020 klo 16.35 Arne Schwabe (a...@rfc2549.org) kirjoitti:
>
> Am 14.02.20 um 14:08 schrieb Lev Stipakov:
> > Hi,
> >
> > 
> >
> > I was planning to send a separate patch, but since you moved
> > this code, maybe you could fix it here.
> >
> >> +bool
> >> +tls_item_in_cipher_list(const char *item, const char *list)
> >> +{
> >> +char *tmp_ciphers = string_alloc(list, NULL);
> >> +char *tmp_ciphers_orig = tmp_ciphers;
> >
> > This is redundant. strtok() manipulates string, but doesn't modify pointer,
> > unlike strsep. So it should be safe to to call
> >
>
> This patch is just moving code that it does not touch. The original code
> is from Steffan. If we fix it, that should go into a different patch I
> think.
>
> But yes you are correct. But the compiler will optimise that away I think :)
>
> Arne
>


-- 
-Lev


___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Re: [Openvpn-devel] [PATCH v2 3/5] Move NCP related function into a seperate file and add unit tests

[Arne Schwabe]

Am 14.02.20 um 14:08 schrieb Lev Stipakov:
> Hi,
> 
> 
> 
> I was planning to send a separate patch, but since you moved
> this code, maybe you could fix it here.
> 
>> +bool
>> +tls_item_in_cipher_list(const char *item, const char *list)
>> +{
>> +    char *tmp_ciphers = string_alloc(list, NULL);
>> +    char *tmp_ciphers_orig = tmp_ciphers;
> 
> This is redundant. strtok() manipulates string, but doesn't modify pointer,
> unlike strsep. So it should be safe to to call
> 

This patch is just moving code that it does not touch. The original code
is from Steffan. If we fix it, that should go into a different patch I
think.

But yes you are correct. But the compiler will optimise that away I think :)

Arne



signature.asc
Description: OpenPGP digital signature
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Re: [Openvpn-devel] [PATCH v2 2/5] Implement dynamic NCP negotiation

[Arne Schwabe]

Am 14.02.20 um 13:23 schrieb Lev Stipakov:
> Hi,
> 
> to 13. helmik. 2020 klo 15.51 Arne Schwabe (a...@rfc2549.org
> ) kirjoitti:
>>
>> Our current NCP version is flawed in the way that it can only indicate
> support for
> 
> Built and tested on Ubuntu 18 and MSVC. Works as advertised.
> 
> Out of curiosity - you picked strsep because strtok which we have used
> so far
> relies on evil global state?

Yes, one of the calls in that functions actually calls a function that
uses strtok, so then everything blows up.

Arne


___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Re: [Openvpn-devel] [PATCH v4 2/2] Add unit tests for engine keys

[Илья Шипицин]

пт, 14 февр. 2020 г. в 18:05, James Bottomley <
james.bottom...@hansenpartnership.com>:

> On Thu, 2020-02-13 at 19:18 +0100, Arne Schwabe wrote:
> > Am 10.02.18 um 23:50 schrieb James Bottomley:
> > > Testing engines is problematic, so one of the prerequisites built
> > > for the tests is a simple openssl engine that reads a non-standard
> > > PEM guarded key.  The test is simply can we run a client/server
> > > configuration with the usual sample key replaced by an engine key.
> > > The trivial engine prints out some operations and we check for
> > > these in the log to make sure the engine was used to load the key
> > > and that it correctly got the password.
> >
> > This tests the openssl engine functionality in a sensible way. But I
> > think it is not fully ready to be commited. To get it working I
> > needed to do following changes on my Mac:
>
> That could be ... I only have a linux box to try this out on.
>
> > commit afa697cec15b4e54e720efe9de39c9b20b13c5c8 (HEAD ->
> > review/enginekeys)
> > Author: Arne Schwabe 
> > Date:   Thu Feb 13 18:13:34 2020 +0100
> >
> > foo
> >
> > diff --git a/tests/unit_tests/engine-key/Makefile.am
> > b/tests/unit_tests/engine-key/Makefile.am
> > index 73921965..6d7fc9c5 100644
> > --- a/tests/unit_tests/engine-key/Makefile.am
> > +++ b/tests/unit_tests/engine-key/Makefile.am
> > @@ -10,4 +10,6 @@ TESTS_ENVIRONMENT = srcdir="$(abs_srcdir)"; \
> >  TESTS = check_engine_keys.sh
> >
> >  libtestengine_la_SOURCES = libtestengine.c
> > -libtestengine_la_LDFLAGS = -rpath /lib -avoid-version
> > +libtestengine_la_LDFLAGS = @TEST_LDFLAGS@  -rpath /lib
> > +libtestengine_la_CFLAGS  = @TEST_CFLAGS@ -I$(openvpn_srcdir)
> > -I$(compat_srcdir)
> > +
> > diff --git a/tests/unit_tests/engine-key/libtestengine.c
> > b/tests/unit_tests/engine-key/libtestengine.c
> > index fa7f5de1..46ec1e33 100644
> > --- a/tests/unit_tests/engine-key/libtestengine.c
> > +++ b/tests/unit_tests/engine-key/libtestengine.c
> > @@ -30,7 +30,6 @@ static EVP_PKEY *engine_load_key(ENGINE *e, const
> > char
> > *key_id,
> > PKCS8_PRIV_KEY_INFO *p8inf;
> > UI *ui;
> > char auth[256];
> > -   int len;
>
> the variable is certainly unused and can go.
>
> > fprintf(stderr, "ENGINE: engine_load_key called\n");
> >
> > diff --git a/tests/unit_tests/engine-key/openssl.cnf
> > b/tests/unit_tests/engine-key/openssl.cnf
> > index 53200c46..e9513a92 100644
> > --- a/tests/unit_tests/engine-key/openssl.cnf
> > +++ b/tests/unit_tests/engine-key/openssl.cnf
> > @@ -9,4 +9,4 @@ engines = engines_section
> >  testengine = testengine_section
> >
> >  [testengine_section]
> > -dynamic_path   = $ENV::srcdir/.libs/libtestengine.so
> > +dynamic_path   = $ENV::srcdir/.libs/libtestengine.dylib
>

we use gost-engine (https://github.com/engine/gost-engine)

on both linux and osx.

for some time there was a bug in openssl:

https://github.com/openssl/openssl/issues/8950


however, for now "dylib" is used for osx. and
but we do not use "dynamic" path. we use config like that

openssl_conf = openssl_def

[openssl_def]
engines = engine_section

[engine_section]
gost = gost_section

[gost_section]
default_algorithms = ALL
engine_id = gost
CRYPT_PARAMS = id-Gost28147-89-CryptoPro-A-ParamSet


>
> This can't really be done though: the .dylib extension won't work on
> Linux because shared objects are .so files.
>
> There is a way to generate and use .so files on a MAC as well,
> according to the openssl people (half the mac engines seem to have a
> .so extension and the other half a .dylib one), I'll see if I can
> figure out what it is.
>
> James
> ___
> Openvpn-devel mailing list
> Openvpn-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/openvpn-devel
>
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Re: [Openvpn-devel] [PATCH v2 4/5] Normalise ncp-ciphers option and restrict it to 127 bytes

[Lev Stipakov]

Hi,

>
> +
> +#idef ENABLE_CRYPTO_OPENSSL

Boom.
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Re: [Openvpn-devel] [PATCH v2 3/5] Move NCP related function into a seperate file and add unit tests

[Lev Stipakov]

Hi,



I was planning to send a separate patch, but since you moved
this code, maybe you could fix it here.

> +bool
> +tls_item_in_cipher_list(const char *item, const char *list)
> +{
> +char *tmp_ciphers = string_alloc(list, NULL);
> +char *tmp_ciphers_orig = tmp_ciphers;

This is redundant. strtok() manipulates string, but doesn't modify pointer,
unlike strsep. So it should be safe to to call

 > free(tmp_ciphers)

instead of

> +free(tmp_ciphers_orig);

Same case as here:
https://sourceforge.net/p/openvpn/mailman/message/35421870/

Also could you apply following diff, which makes sure that
ssl_ncp.[ch] are displayed under correct Source/Headers folder in MSVC?

diff --git a/src/openvpn/openvpn.vcxproj.filters
b/src/openvpn/openvpn.vcxproj.filters
index 41e62d14..80eb52b3 100644
--- a/src/openvpn/openvpn.vcxproj.filters
+++ b/src/openvpn/openvpn.vcxproj.filters
@@ -243,6 +243,9 @@
 
   Source Files
 
+
+  Source Files
+
   
   
 
@@ -506,10 +509,13 @@
 
   Header Files
 
+
+  Header Files
+
   
   
 
   Resource Files
 
   
-
+
\ No newline at end of file

Other than that, looks good.
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Re: [Openvpn-devel] [PATCH v4 2/2] Add unit tests for engine keys

[James Bottomley]

On Thu, 2020-02-13 at 19:18 +0100, Arne Schwabe wrote:
> Am 10.02.18 um 23:50 schrieb James Bottomley:
> > Testing engines is problematic, so one of the prerequisites built
> > for the tests is a simple openssl engine that reads a non-standard
> > PEM guarded key.  The test is simply can we run a client/server
> > configuration with the usual sample key replaced by an engine key.
> > The trivial engine prints out some operations and we check for
> > these in the log to make sure the engine was used to load the key
> > and that it correctly got the password.
> 
> This tests the openssl engine functionality in a sensible way. But I
> think it is not fully ready to be commited. To get it working I
> needed to do following changes on my Mac:

That could be ... I only have a linux box to try this out on.

> commit afa697cec15b4e54e720efe9de39c9b20b13c5c8 (HEAD ->
> review/enginekeys)
> Author: Arne Schwabe 
> Date:   Thu Feb 13 18:13:34 2020 +0100
> 
> foo
> 
> diff --git a/tests/unit_tests/engine-key/Makefile.am
> b/tests/unit_tests/engine-key/Makefile.am
> index 73921965..6d7fc9c5 100644
> --- a/tests/unit_tests/engine-key/Makefile.am
> +++ b/tests/unit_tests/engine-key/Makefile.am
> @@ -10,4 +10,6 @@ TESTS_ENVIRONMENT = srcdir="$(abs_srcdir)"; \
>  TESTS = check_engine_keys.sh
> 
>  libtestengine_la_SOURCES = libtestengine.c
> -libtestengine_la_LDFLAGS = -rpath /lib -avoid-version
> +libtestengine_la_LDFLAGS = @TEST_LDFLAGS@  -rpath /lib
> +libtestengine_la_CFLAGS  = @TEST_CFLAGS@ -I$(openvpn_srcdir)
> -I$(compat_srcdir)
> +
> diff --git a/tests/unit_tests/engine-key/libtestengine.c
> b/tests/unit_tests/engine-key/libtestengine.c
> index fa7f5de1..46ec1e33 100644
> --- a/tests/unit_tests/engine-key/libtestengine.c
> +++ b/tests/unit_tests/engine-key/libtestengine.c
> @@ -30,7 +30,6 @@ static EVP_PKEY *engine_load_key(ENGINE *e, const
> char
> *key_id,
> PKCS8_PRIV_KEY_INFO *p8inf;
> UI *ui;
> char auth[256];
> -   int len;

the variable is certainly unused and can go.

> fprintf(stderr, "ENGINE: engine_load_key called\n");
> 
> diff --git a/tests/unit_tests/engine-key/openssl.cnf
> b/tests/unit_tests/engine-key/openssl.cnf
> index 53200c46..e9513a92 100644
> --- a/tests/unit_tests/engine-key/openssl.cnf
> +++ b/tests/unit_tests/engine-key/openssl.cnf
> @@ -9,4 +9,4 @@ engines = engines_section
>  testengine = testengine_section
> 
>  [testengine_section]
> -dynamic_path   = $ENV::srcdir/.libs/libtestengine.so
> +dynamic_path   = $ENV::srcdir/.libs/libtestengine.dylib

This can't really be done though: the .dylib extension won't work on
Linux because shared objects are .so files.

There is a way to generate and use .so files on a MAC as well,
according to the openssl people (half the mac engines seem to have a
.so extension and the other half a .dylib one), I'll see if I can
figure out what it is.

James


signature.asc
Description: This is a digitally signed message part
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Re: [Openvpn-devel] [PATCH v2 2/5] Implement dynamic NCP negotiation

[Lev Stipakov]

Hi,

to 13. helmik. 2020 klo 15.51 Arne Schwabe (a...@rfc2549.org) kirjoitti:
>
> Our current NCP version is flawed in the way that it can only indicate
support for

Built and tested on Ubuntu 18 and MSVC. Works as advertised.

Out of curiosity - you picked strsep because strtok which we have used so
far
relies on evil global state?

Acked-by: Lev Stipakov 
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel