Re: [Openvpn-devel] Segmentation fault in OpenVPN 2.6_git [git:dco/fcc852a9b2ea832c]

2021-03-03 Thread Antonio Quartulli
Hi Tony,

On 04/03/2021 03:10, Tony He wrote:
> 
> Arne Schwabe mailto:a...@rfc2549.org>> 于2021年3月3日
> 周三 下午7:56写道:
> 
> Am 03.03.21 um 08:46 schrieb Tony He:
> > Hi Arne,
> >
> > I encountered segmentation fault in your dco branch. Master branch is
> > OK. I reverted the commit "Linux data-channel offload support", but it
> > still happens.
> > Anything wrong? Can you reproduce?
> 
> OpenVPN without any encryption or TLS is not something I tested.
> Consider this obscure mode broken as part of being a preview release.
> 
> 
> So can you reproduce this issue at your side? Did you just test static
> key? Can you paste your commands or configurations?

What Arne meant is that, given the following message in your log,

*** WARNING ***: All encryption and authentication features
disabled -- All data will be tunnelled as clear text and will not be
protected against man-in-the-middle changes. PLEASE DO RECONSIDER THIS
CONFIGURATION!

it appears that you are testing *without* any form of encryption.
This special case is untested at the moment.

A classic configuration with CA/certs/keys should work.
Have you tried that?

Regards,

>  
> 
> Thanks for the report.
> 
> Arne
> 
> 
> 
> ___
> Openvpn-devel mailing list
> Openvpn-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/openvpn-devel
> 

-- 
Antonio Quartulli


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


Re: [Openvpn-devel] [PATCH v5] Allow running a default configuration with TLS libraries without BF-CBC

2021-03-02 Thread Antonio Quartulli
me(_frame, crypto_max_overhead());
> +
> +
> +/* o->ciphername might be BF-CBC even though the underlying SSL 
> library
> + * does not support it. For this reason we workaround this corner 
> case
> + * by pretending to have no encryption enabled and by manually adding
> + * the required packet overhead to the MTU computation.
> + */
> +const char* ciphername = o->ciphername;
> +
> +if (strcmp(o->ciphername, "BF-CBC") == 0)
> +{
> +/* none has no overhead, so use this to later add only --auth
> + * overhead */
> +
> +/* overhead of BF-CBC: 64 bit block size, 64 bit IV size */
> +frame_add_to_extra_frame(_frame, 64/8 + 64/8);

Please add space around the / operators.

> +}
> +
> +init_key_type(_kt, ciphername, o->authname, o->keysize, true,
> +  false);
> +
>  crypto_adjust_frame_parameters(_frame, _kt, o->replay,
> 
> cipher_kt_mode_ofb_cfb(fake_kt.cipher));
>  frame_finalize(_frame, o->ce.link_mtu_defined, o->ce.link_mtu,
> @@ -3781,18 +3801,33 @@ options_string(const struct options *o,
> + (TLS_SERVER == true)
> <= 1);
>  
> -init_key_type(, o->ciphername, o->authname, o->keysize, true,
> -  false);
> +/* Skip resolving BF-CBC to allow SSL libraries without BF-CBC
> + * to work here in the default configuration */
> +const char *ciphername = o->ciphername;
> +int keysize;
> +
> +if (strcmp(o->ciphername, "BF-CBC") == 0)
> +{
> +init_key_type(, "none", o->authname, o->keysize, true,
> +  false);
> +keysize = 128;
> +}
> +else
> +{
> +init_key_type(, o->ciphername, o->authname, o->keysize, true,
> +  false);
> +ciphername = cipher_kt_name(kt.cipher);
> +keysize = kt.cipher_length * 8;
> +}
>  /* Only announce the cipher to our peer if we are willing to
>   * support it */
> -const char *ciphername = cipher_kt_name(kt.cipher);
>  if (p2p_nopull || !o->ncp_enabled
>  || tls_item_in_cipher_list(ciphername, o->ncp_ciphers))
>  {
>  buf_printf(, ",cipher %s", ciphername);
>  }
>  buf_printf(, ",auth %s", md_kt_name(kt.digest));
> -buf_printf(, ",keysize %d", kt.cipher_length * 8);
> +buf_printf(, ",keysize %d", keysize);
>  if (o->shared_secret_file)
>  {
>  buf_printf(, ",secret");
> 

Other than the whitepsace complaint, this patch makes sense to me (as it
did the previous version, but thanks Gert for spotting the BF-CBC problem).

It's ACK for me, but Gert's test rig will give its final verdict.

Acked-by: Antonio Quartulli 


-- 
Antonio Quartulli


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


Re: [Openvpn-devel] [PATCH v3 2/3] Prefer TLS libraries TLS PRF function, fix OpenVPN in FIPS mode

2021-02-08 Thread Antonio Quartulli
 -tls1_P_hash(sha1,S2,len,label,label_len,out2,olen);
> -
> -for (int i = 0; i -{
> -out1[i] ^= out2[i];
> -}
> -
> -secure_memzero(out2, olen);
> -
> -dmsg(D_SHOW_KEY_SOURCE, "tls1_PRF out[%d]: %s", olen, format_hex(out1, 
> olen, 0, ));
> -
> -gc_free();
> -}
> -
>  static void
>  openvpn_PRF(const uint8_t *secret,
>  int secret_len,
> @@ -1742,7 +1614,7 @@ openvpn_PRF(const uint8_t *secret,
>  }
>  
>  /* compute PRF */
> -tls1_PRF(BPTR(), BLEN(), secret, secret_len, output, 
> output_len);
> +ssl_tls1_PRF(BPTR(), BLEN(), secret, secret_len, output, 
> output_len);
>  
>  buf_clear();
>  free_buf();
> diff --git a/tests/unit_tests/openvpn/test_crypto.c 
> b/tests/unit_tests/openvpn/test_crypto.c
> index ea9b99b2..6c68099e 100644
> --- a/tests/unit_tests/openvpn/test_crypto.c
> +++ b/tests/unit_tests/openvpn/test_crypto.c
> @@ -38,6 +38,7 @@
>  #include 
>  
>  #include "crypto.h"
> +#include "ssl_backend.h"
>  
>  #include "mock_msg.h"
>  
> @@ -136,12 +137,43 @@ crypto_translate_cipher_names(void **state)
>  test_cipher_names("id-aes256-GCM", "AES-256-GCM");
>  }
>  
> +
> +static uint8_t good_prf[32] = {0xd9, 0x8c, 0x85, 0x18, 0xc8, 0x5e, 0x94, 
> 0x69,
> +   0x27, 0x91, 0x6a, 0xcf, 0xc2, 0xd5, 0x92, 
> 0xfb,
> +   0xb1, 0x56, 0x7e, 0x4b, 0x4b, 0x14, 0x59, 
> 0xe6,
> +   0xa9, 0x04, 0xac, 0x2d, 0xda, 0xb7, 0x2d, 
> 0x67};
> +static void
> +crypto_test_tls_prf(void **state)
> +{
> +const char *seedstr = "Quis aute iure reprehenderit in voluptate "
> +  "velit esse cillum dolore";
> +const unsigned char *seed = (const unsigned char *) seedstr;

no space after cast

> +const size_t seed_len = strlen(seedstr);
> +
> +
> +
> +
> +const char* ipsumlorem = "Lorem ipsum dolor sit amet, consectetur "
> + "adipisici elit, sed eiusmod tempor incidunt ut 
> "
> + "labore et dolore magna aliqua.";
> +
> +const unsigned char *secret = (const unsigned char *) ipsumlorem;
> +size_t secret_len = strlen((const char *)secret);
> +
> +
> +uint8_t out[32];
> +ssl_tls1_PRF(seed, seed_len, secret, secret_len, out, sizeof(out));
> +
> +assert_memory_equal(good_prf, out, sizeof(out));
> +}
> +
>  int
>  main(void)
>  {
>  const struct CMUnitTest tests[] = {
>  cmocka_unit_test(crypto_pem_encode_decode_loopback),
>  cmocka_unit_test(crypto_translate_cipher_names),
> +cmocka_unit_test(crypto_test_tls_prf)
>  };
>  
>  #if defined(ENABLE_CRYPTO_OPENSSL)
> 


Cheers,

-- 
Antonio Quartulli


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


Re: [Openvpn-devel] [PATCH v3 1/3] Check return values in md_ctx_init and hmac_ctx_init

2021-02-05 Thread Antonio Quartulli
Hi,

On 01/02/2021 18:43, Arne Schwabe wrote:
> Without this OpenVPN will later segfault on a FIPS enabled system due
> to the algorithm available but not allowed.
> 
> Patch V2: Use (!func) instead (func != 1)
> 
> Signed-off-by: Arne Schwabe 
> ---
>  src/openvpn/crypto_openssl.c | 15 ---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/src/openvpn/crypto_openssl.c b/src/openvpn/crypto_openssl.c
> index c60d4a54..e124a7b6 100644
> --- a/src/openvpn/crypto_openssl.c
> +++ b/src/openvpn/crypto_openssl.c
> @@ -954,7 +954,10 @@ md_ctx_init(EVP_MD_CTX *ctx, const EVP_MD *kt)
>  ASSERT(NULL != ctx && NULL != kt);
>  
>  EVP_MD_CTX_init(ctx);
> -EVP_DigestInit(ctx, kt);
> +if (!EVP_DigestInit(ctx, kt))
> +{
> +crypto_msg(M_FATAL, "EVP_DigestInit failed");
> +}
>  }
>  
>  void
> @@ -1011,7 +1014,10 @@ hmac_ctx_init(HMAC_CTX *ctx, const uint8_t *key, int 
> key_len,
>  ASSERT(NULL != kt && NULL != ctx);
>  
>  HMAC_CTX_reset(ctx);
> -HMAC_Init_ex(ctx, key, key_len, kt, NULL);
> +if  (!HMAC_Init_ex(ctx, key, key_len, kt, NULL))

too many spaces after "if"

> +{
> +crypto_msg(M_FATAL, "HMAC_Init_ex failed");
> +}
>  
>  /* make sure we used a big enough key */
>  ASSERT(HMAC_size(ctx) <= key_len);
> @@ -1032,7 +1038,10 @@ hmac_ctx_size(const HMAC_CTX *ctx)
>  void
>  hmac_ctx_reset(HMAC_CTX *ctx)
>  {
> -HMAC_Init_ex(ctx, NULL, 0, NULL, NULL);
> +if(!HMAC_Init_ex(ctx, NULL, 0, NULL, NULL))

missing space after "if" (it probably went in the above block :D)

> +{
> +    crypto_msg(M_FATAL, "HMAC_Init_ex failed");
> +}
>  }
>  
>  void
> 

Rest looks good!

Acked-by: Antonio Quartulli 

Maybe the spaces can be fixed on the fly while merging.

Regards,


-- 
Antonio Quartulli


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


Re: [Openvpn-devel] [PATCH 5/5] Prefer TLS libraries TLS PRF function, fix OpenVPN in FIPS mode

2021-01-29 Thread Antonio Quartulli
32] +
> - * SecurityParameters.client_random[32]);
> - *
> - * Notes:
> - *
> - * (1) key_block contains a full set of 4 keys.
> - * (2) The pre-master secret is generated by the client.
> - */
> -static void
> -tls1_PRF(const uint8_t *label,
> - int label_len,
> - const uint8_t *sec,
> - int slen,
> - uint8_t *out1,
> - int olen)
> -{
> -struct gc_arena gc = gc_new();
> -const md_kt_t *md5 = md_kt_get("MD5");
> -const md_kt_t *sha1 = md_kt_get("SHA1");
> -
> -uint8_t *out2 = (uint8_t *) gc_malloc(olen, false, );
> -
> -int len = slen/2;
> -const uint8_t *S1 = sec;
> -const uint8_t *S2 = &(sec[len]);
> -len += (slen&1); /* add for odd, make longer */
> -
> -tls1_P_hash(md5,S1,len,label,label_len,out1,olen);
> -tls1_P_hash(sha1,S2,len,label,label_len,out2,olen);
> -
> -for (int i = 0; i -{
> -out1[i] ^= out2[i];
> -}
> -
> -secure_memzero(out2, olen);
> -
> -dmsg(D_SHOW_KEY_SOURCE, "tls1_PRF out[%d]: %s", olen, format_hex(out1, 
> olen, 0, ));
> -
> -gc_free();
> -}
> -
>  static void
>  openvpn_PRF(const uint8_t *secret,
>  int secret_len,
> @@ -1757,7 +1629,7 @@ openvpn_PRF(const uint8_t *secret,
>  }
>  
>  /* compute PRF */
> -tls1_PRF(BPTR(), BLEN(), secret, secret_len, output, 
> output_len);
> +ssl_tls1_PRF(BPTR(), BLEN(), secret, secret_len, output, 
> output_len);
>  
>  buf_clear();
>  free_buf();
> diff --git a/tests/unit_tests/openvpn/test_crypto.c 
> b/tests/unit_tests/openvpn/test_crypto.c
> index ea9b99b2..6c68099e 100644
> --- a/tests/unit_tests/openvpn/test_crypto.c
> +++ b/tests/unit_tests/openvpn/test_crypto.c
> @@ -38,6 +38,7 @@
>  #include 
>  
>  #include "crypto.h"
> +#include "ssl_backend.h"
>  
>  #include "mock_msg.h"
>  
> @@ -136,12 +137,43 @@ crypto_translate_cipher_names(void **state)
>  test_cipher_names("id-aes256-GCM", "AES-256-GCM");
>  }
>  
> +
> +static uint8_t good_prf[32] = {0xd9, 0x8c, 0x85, 0x18, 0xc8, 0x5e, 0x94, 
> 0x69,

space after the opening {

> +   0x27, 0x91, 0x6a, 0xcf, 0xc2, 0xd5, 0x92, 
> 0xfb,
> +   0xb1, 0x56, 0x7e, 0x4b, 0x4b, 0x14, 0x59, 
> 0xe6,
> +   0xa9, 0x04, 0xac, 0x2d, 0xda, 0xb7, 0x2d, 
> 0x67};

space before the closing }

> +static void
> +crypto_test_tls_prf(void **state)
> +{
> +const char *seedstr = "Quis aute iure reprehenderit in voluptate "
> +  "velit esse cillum dolore";
> +const unsigned char *seed = (const unsigned char *) seedstr;
> +const size_t seed_len = strlen(seedstr);
> +
> +
> +
> +
> +const char* ipsumlorem = "Lorem ipsum dolor sit amet, consectetur "
> + "adipisici elit, sed eiusmod tempor incidunt ut 
> "
> + "labore et dolore magna aliqua.";
> +
> +const unsigned char *secret = (const unsigned char *) ipsumlorem;
> +size_t secret_len = strlen((const char *)secret);
> +
> +
> +uint8_t out[32];
> +ssl_tls1_PRF(seed, seed_len, secret, secret_len, out, sizeof(out));
> +
> +assert_memory_equal(good_prf, out, sizeof(out));
> +}
> +
>  int
>  main(void)
>  {
>  const struct CMUnitTest tests[] = {
>  cmocka_unit_test(crypto_pem_encode_decode_loopback),
>  cmocka_unit_test(crypto_translate_cipher_names),
> +cmocka_unit_test(crypto_test_tls_prf)
>  };
>  
>  #if defined(ENABLE_CRYPTO_OPENSSL)
> 


Regards,

-- 
Antonio Quartulli


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


Re: [Openvpn-devel] [PATCH v2 4/5] Check return values in md_ctx_init and hmac_ctx_init

2021-01-29 Thread Antonio Quartulli
Hi,

On 29/01/2021 11:47, Arne Schwabe wrote:
> Without this OpenVPN will later segfault on a FIPS enabled system due
> to the algorithm available but not allowed.
> 
> Patch V2: Use (!func) instead (func != 1)
> 
> Signed-off-by: Arne Schwabe 
> ---
>  src/openvpn/crypto_openssl.c | 15 ---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/src/openvpn/crypto_openssl.c b/src/openvpn/crypto_openssl.c
> index c60d4a54..e124a7b6 100644
> --- a/src/openvpn/crypto_openssl.c
> +++ b/src/openvpn/crypto_openssl.c
> @@ -954,7 +954,10 @@ md_ctx_init(EVP_MD_CTX *ctx, const EVP_MD *kt)
>  ASSERT(NULL != ctx && NULL != kt);
>  
>  EVP_MD_CTX_init(ctx);
> -EVP_DigestInit(ctx, kt);
> +if (!EVP_DigestInit(ctx, kt))
> +{
> +crypto_msg(M_FATAL, "EVP_DigestInit failed");
> +}
>  }
>  
>  void
> @@ -1011,7 +1014,10 @@ hmac_ctx_init(HMAC_CTX *ctx, const uint8_t *key, int 
> key_len,
>  ASSERT(NULL != kt && NULL != ctx);
>  
>  HMAC_CTX_reset(ctx);
> -HMAC_Init_ex(ctx, key, key_len, kt, NULL);
> +if  (!HMAC_Init_ex(ctx, key, key_len, kt, NULL))
> +{
> +crypto_msg(M_FATAL, "HMAC_Init_ex failed");
> +}
>  
>  /* make sure we used a big enough key */
>  ASSERT(HMAC_size(ctx) <= key_len);
> @@ -1032,7 +1038,10 @@ hmac_ctx_size(const HMAC_CTX *ctx)
>  void
>  hmac_ctx_reset(HMAC_CTX *ctx)
>  {
> -HMAC_Init_ex(ctx, NULL, 0, NULL, NULL);
> +if(!HMAC_Init_ex(ctx, NULL, 0, NULL, NULL))
> +{
> +crypto_msg(M_FATAL, "HMAC_Init_ex failed");
> +}
>  }

I saw that you missed this case earlier, but I thought that this call
cannot really fail.

Assuming it can fail under certain conditions, wouldn't the M_FATAL
somewhat become a DoS on the server side?

Regards,


-- 
Antonio Quartulli


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


Re: [Openvpn-devel] [PATCH 4/5] Check return values in md_ctx_init and hmac_ctx_init

2021-01-29 Thread Antonio Quartulli
Hi,

On 07/09/2020 18:22, Arne Schwabe wrote:
> Without this OpenVPN will later segfault on a FIPS enabled system due
> to the algorithm available but not allowed.
> 
> Signed-off-by: Arne Schwabe 
> ---
>  src/openvpn/crypto_openssl.c | 10 --
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/src/openvpn/crypto_openssl.c b/src/openvpn/crypto_openssl.c
> index c60d4a54..75557cca 100644
> --- a/src/openvpn/crypto_openssl.c
> +++ b/src/openvpn/crypto_openssl.c
> @@ -954,7 +954,10 @@ md_ctx_init(EVP_MD_CTX *ctx, const EVP_MD *kt)
>  ASSERT(NULL != ctx && NULL != kt);
>  
>  EVP_MD_CTX_init(ctx);
> -EVP_DigestInit(ctx, kt);
> +if (EVP_DigestInit(ctx, kt) != 1)

Based on the documentation, this function returns 0 on failure and 1 on
success (basically a bool).

How about turning this into:

if (!EVP_DigestInit(ctx, kt))

?

This way it is clear that we are not just looking for '1' as a special
return value, but simply for 'success'. (Same as we do for function
returning a real bool).

What do you think?

> +{
> +crypto_msg(M_FATAL, "EVP_DigestInit failed");
> +}
>  }
>  
>  void
> @@ -1011,7 +1014,10 @@ hmac_ctx_init(HMAC_CTX *ctx, const uint8_t *key, int 
> key_len,
>  ASSERT(NULL != kt && NULL != ctx);
>  
>  HMAC_CTX_reset(ctx);
> -HMAC_Init_ex(ctx, key, key_len, kt, NULL);
> +if  (HMAC_Init_ex(ctx, key, key_len, kt, NULL) != 1)

Same for this function.


Regards,


-- 
Antonio Quartulli


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


Re: [Openvpn-devel] [PATCH v2] Allow running a default configuration with TLS libraries without BF-CBC

2021-01-29 Thread Antonio Quartulli
Hi,

On 25/01/2021 13:43, Arne Schwabe wrote:
> Modern TLS libraries might drop Blowfish by default or distributions
> might disable Blowfish in OpenSSL/mbed TLS. We still signal OCC
> options with BF-CBC compatible strings. To avoid requiring BF-CBC
> for this, special this one usage of BF-CBC enough to avoid a hard
> requirement on Blowfish in the default configuration.
> 
> Signed-off-by: Arne Schwabe 
> 
> Patch v2: add more clarifying comment, do not warn about OCC only insecure
>   ciphers, code improvements
> 
> Signed-off-by: Arne Schwabe 

Acked-by: Antonio Quartulli 

-- 
Antonio Quartulli


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


Re: [Openvpn-devel] new coverity release

2021-01-25 Thread Antonio Quartulli
Hi,

we used to push Coverity builds via Travis, however it seems it hasn't
been pushed in a while.

It'd be good to check what's wrong and get it going again.

Regards,

On 25/01/2021 21:53, Илья Шипицин wrote:
> Hello,
> 
> how are coverity builds scheduled ?
> shouldn't we start new one ?
> 
> https://community.synopsys.com/s/question/0D52H5NeWJfSAN/announcement-upcoming-coverity-scan-upgrade-to-coverity-202009-release
> <https://community.synopsys.com/s/question/0D52H5NeWJfSAN/announcement-upcoming-coverity-scan-upgrade-to-coverity-202009-release>
> 
> 
> Ilya
> 
> 
> ___
> Openvpn-devel mailing list
> Openvpn-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/openvpn-devel
> 

-- 
Antonio Quartulli


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


Re: [Openvpn-devel] [PATCH 3/5] Allow running a default configuration with TLS libraries without BF-CBC

2021-01-22 Thread Antonio Quartulli
Hi,

On 22/01/2021 12:19, Arne Schwabe wrote:
>> I would suggest some refactoring here.
>> We can just assume that BF-CBC is not supported by the SSL library,
>> while also reducing some code duplication:
>>
>> const char *ciphername = o->ciphername;
>>
>> ...
>>
>> /* o->ciphername might be BF-CBC even though the underlying SSL library
>>  * does not support it. For this reason we workaround this corner case
>>  * by pretending to have no encryption enabled and by manually adding
>>  * the required packet overhead to the MTU computation.
>>  */
>> if (strcmp(o->ciphername, "BF-CBC") == 0)
>> {
>>ciphername = "none";
>>/* 64 bit block size, 64 bit IV size */
>>frame_add_to_extra_frame(_frame, 64/8 + 64/8);
>> }
> 
> That drops the adjusting for the HMAC size.  I will add a comment to
> clarify what the other lines are good for.

I moved the crypto_adjustment call below this block, since it is
performed both for BF-CBC and non-BF-CBC. Doesn't it work?



-- 
Antonio Quartulli


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


Re: [Openvpn-devel] [PATCH 3/5] Allow running a default configuration with TLS libraries without BF-CBC

2021-01-21 Thread Antonio Quartulli
 false);
> +
> +crypto_adjust_frame_parameters(_frame, _kt, o->replay,
> +   
> cipher_kt_mode_ofb_cfb(fake_kt.cipher));
> +/* 64 bit block size, 64 bit IV size */
> +frame_add_to_extra_frame(_frame, 64/8 + 64/8);
> +}
> +else
> +{
> +init_key_type(_kt, o->ciphername, o->authname, o->keysize, 
> true,
> +  false);
> +
> +crypto_adjust_frame_parameters(_frame, _kt, o->replay,
> +   
> cipher_kt_mode_ofb_cfb(fake_kt.cipher));
> +}

I would suggest some refactoring here.
We can just assume that BF-CBC is not supported by the SSL library,
while also reducing some code duplication:

const char *ciphername = o->ciphername;

...

/* o->ciphername might be BF-CBC even though the underlying SSL library
 * does not support it. For this reason we workaround this corner case
 * by pretending to have no encryption enabled and by manually adding
 * the required packet overhead to the MTU computation.
 */
if (strcmp(o->ciphername, "BF-CBC") == 0)
{
   ciphername = "none";
   /* 64 bit block size, 64 bit IV size */
   frame_add_to_extra_frame(_frame, 64/8 + 64/8);
}

init_key_type(_kt, ciphername, o->authname, o->keysize, true,
  false);
crypto_adjust_frame_parameters(_frame, _kt, o->replay,
   cipher_kt_mode_ofb_cfb(fake_kt.cipher));


>  frame_finalize(_frame, o->ce.link_mtu_defined, o->ce.link_mtu,
> o->ce.tun_mtu_defined, o->ce.tun_mtu);
>  msg(D_MTU_DEBUG, "%s: link-mtu %u -> %d", __func__, (unsigned int) 
> link_mtu,
> @@ -3812,18 +3833,32 @@ options_string(const struct options *o,
> + (TLS_SERVER == true)
> <= 1);
>  
> -init_key_type(, o->ciphername, o->authname, o->keysize, true,
> -  false);
> +/* Skip resolving BF-CBC to allow SSL libraries without BF-CBC
> + * to work here in the default configuration */
> +const char *ciphername = o->ciphername;
> +int keysize;
> +
> +if (strcmp(o->ciphername, "BF-CBC") == 0) {
> +init_key_type(, "none", o->authname, o->keysize, true,
> +  false);
> +ciphername = cipher_kt_name(kt.cipher);
> +keysize = 128;
> +}
> +else
> +{
> +init_key_type(, o->ciphername, o->authname, o->keysize, true,
> +  false);
> +keysize = kt.cipher_length * 8;
> +}
>      /* Only announce the cipher to our peer if we are willing to
>   * support it */
> -const char *ciphername = cipher_kt_name(kt.cipher);
>  if (p2p_nopull || !o->ncp_enabled
>  || tls_item_in_cipher_list(ciphername, o->ncp_ciphers))
>  {
>  buf_printf(, ",cipher %s", ciphername);
>  }
>  buf_printf(, ",auth %s", md_kt_name(kt.digest));
> -buf_printf(, ",keysize %d", kt.cipher_length * 8);
> +buf_printf(, ",keysize %d", keysize);
>  if (o->shared_secret_file)
>  {
>  buf_printf(, ",secret");
> 


Regards,

-- 
Antonio Quartulli


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


Re: [Openvpn-devel] [ovpn-dco]compilation error in function ‘ovpn_peer_lookup_transp_addr’

2021-01-13 Thread Antonio Quartulli
Potential fix pushed to the experimental branch.

Thanks again for reporting.

On 13/01/2021 14:18, Antonio Quartulli wrote:
> It turns our this is a bug in my experimental branch :-)
> 
> The object pointed by sa6 is not large enough, hence triggering that error.
> 
> Will come up with a fix.
> 
> Thanks!
> 
> On 13/01/2021 11:17, Tony He wrote:
>> Hi Antonio,
>>
>> Yes, I'm using latest commit. Maybe it's a compiler bug. What's your
>> compiler version?
>> Here is mine.
>> tony-vm-2004% gcc --version
>> gcc (Ubuntu 9.3.0-17ubuntu1~20.04) 9.3.0
>> Copyright (C) 2019 Free Software Foundation, Inc.
>> This is free software; see the source for copying conditions.  There is NO
>> warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
>>
>>
>>
>> Antonio Quartulli  于2021年1月13日周三 下午5:17写道:
>>
>> Also,
>>
>> are you sure you are compiling the latest experimental branch?
>> HEAD is 2555e5595088991006e57a3ee447f356dffdca92
>>
>> Regards,
>>
>> On 13/01/2021 10:12, Antonio Quartulli wrote:
>> > Hi Tony,
>> >
>> > thanks for the message.
>> > The experimental branch is still...experimental, so I expect more
>> things
>> > to crash and fail miserably :-D
>> >
>> > In any case, feel free to send a patch if you see how to fix the
>> issue!
>> > It would be nice to have external contributions :-)
>> >
>> > This said, I am not sure this is our bug - the sin6_addr member of
>> > struct sockaddr_in6 is of type struct sin6_addr:
>> >
>> >
>> https://elixir.bootlin.com/linux/v5.4/source/include/uapi/linux/in6.h#L54
>> 
>> <https://elixir.bootlin.com/linux/v5.4/source/include/uapi/linux/in6.h#L54>
>> >
>> > And I believe it is widely known that an IPv6 address is 12 bytes,
>> not 8..
>> >
>> > Maybe it's a bug in the compiler?
>> >
>> > Best Regards,
>> >
>> > On 13/01/2021 10:03, Tony He wrote:
>> >> Sorry, clicked "send" button before adding subject and CC
>> Openvpn-dev. I
>> >> will send a new mail.
>> >>
>> >> Tony He mailto:huangy...@gmail.com>
>> <mailto:huangy...@gmail.com <mailto:huangy...@gmail.com>>> 于2021年1月13
>> >> 日周三 下午4:57写道:
>> >>
>> >>     Hi Antonio,
>> >>
>> >>     I see you have pushed new commits to support multiple link to
>> peers.
>> >>     So I tried compiling, but encounter below error. My kernel
>> version
>> >>     is 5.4.0-54.
>> >>
>> >>
>> >>
>> >>     tony-vm-2004% make                                          
>>        
>> >>                                                                  
>>      
>> >>                                                                  
>>      
>> >>          
>> >>     /project/openvpn/ovpn-dco.git/gen-compat-autoconf.sh
>> >>     /project/openvpn/ovpn-dco.git/compat-autoconf.h              
>>      
>> >>                                                                  
>>      
>> >>                          
>> >>     make -C /lib/modules/5.4.0-54-generic/build
>> >>     M=/project/openvpn/ovpn-dco.git PWD=/project/openvpn/ovpn-dco.git
>> >>     REVISION=2555e55 CONFIG_OVPN_DCO=m INSTALL_MOD_DIR=updates/      
>> >>     modules                            
>> >>     make[1]: Entering directory
>> >>     '/usr/src/linux-headers-5.4.0-54-generic'                    
>>      
>> >>                                                                  
>>      
>> >>                                                    
>> >>       CC [M]
>>  /project/openvpn/ovpn-dco.git/drivers/net/ovpn-dco/main.o
>> >>                                                                  
>>      
>> >>                                                                  
>>      
>> >>            
>> >>       CC [

Re: [Openvpn-devel] [ovpn-dco]compilation error in function ‘ovpn_peer_lookup_transp_addr’

2021-01-13 Thread Antonio Quartulli
It turns our this is a bug in my experimental branch :-)

The object pointed by sa6 is not large enough, hence triggering that error.

Will come up with a fix.

Thanks!

On 13/01/2021 11:17, Tony He wrote:
> Hi Antonio,
> 
> Yes, I'm using latest commit. Maybe it's a compiler bug. What's your
> compiler version?
> Here is mine.
> tony-vm-2004% gcc --version
> gcc (Ubuntu 9.3.0-17ubuntu1~20.04) 9.3.0
> Copyright (C) 2019 Free Software Foundation, Inc.
> This is free software; see the source for copying conditions.  There is NO
> warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
> 
> 
> 
> Antonio Quartulli  于2021年1月13日周三 下午5:17写道:
> 
> Also,
> 
> are you sure you are compiling the latest experimental branch?
> HEAD is 2555e5595088991006e57a3ee447f356dffdca92
> 
> Regards,
> 
> On 13/01/2021 10:12, Antonio Quartulli wrote:
> > Hi Tony,
> >
> > thanks for the message.
> > The experimental branch is still...experimental, so I expect more
> things
> > to crash and fail miserably :-D
> >
> > In any case, feel free to send a patch if you see how to fix the
> issue!
> > It would be nice to have external contributions :-)
> >
> > This said, I am not sure this is our bug - the sin6_addr member of
> > struct sockaddr_in6 is of type struct sin6_addr:
> >
> >
> https://elixir.bootlin.com/linux/v5.4/source/include/uapi/linux/in6.h#L54
> 
> <https://elixir.bootlin.com/linux/v5.4/source/include/uapi/linux/in6.h#L54>
> >
> > And I believe it is widely known that an IPv6 address is 12 bytes,
> not 8..
> >
> > Maybe it's a bug in the compiler?
> >
> > Best Regards,
> >
> > On 13/01/2021 10:03, Tony He wrote:
> >> Sorry, clicked "send" button before adding subject and CC
> Openvpn-dev. I
> >> will send a new mail.
> >>
> >> Tony He mailto:huangy...@gmail.com>
> <mailto:huangy...@gmail.com <mailto:huangy...@gmail.com>>> 于2021年1月13
> >> 日周三 下午4:57写道:
> >>
> >>     Hi Antonio,
> >>
> >>     I see you have pushed new commits to support multiple link to
> peers.
> >>     So I tried compiling, but encounter below error. My kernel
> version
> >>     is 5.4.0-54.
> >>
> >>
> >>
> >>     tony-vm-2004% make                                          
>        
> >>                                                                  
>      
> >>                                                                  
>      
> >>          
> >>     /project/openvpn/ovpn-dco.git/gen-compat-autoconf.sh
> >>     /project/openvpn/ovpn-dco.git/compat-autoconf.h              
>      
> >>                                                                  
>      
> >>                          
> >>     make -C /lib/modules/5.4.0-54-generic/build
> >>     M=/project/openvpn/ovpn-dco.git PWD=/project/openvpn/ovpn-dco.git
> >>     REVISION=2555e55 CONFIG_OVPN_DCO=m INSTALL_MOD_DIR=updates/      
> >>     modules                            
> >>     make[1]: Entering directory
> >>     '/usr/src/linux-headers-5.4.0-54-generic'                    
>      
> >>                                                                  
>      
> >>                                                    
> >>       CC [M]
>  /project/openvpn/ovpn-dco.git/drivers/net/ovpn-dco/main.o
> >>                                                                  
>      
> >>                                                                  
>      
> >>            
> >>       CC [M]
>  /project/openvpn/ovpn-dco.git/drivers/net/ovpn-dco/bind.o
> >>                                                                  
>      
> >>                                                                  
>      
> >>            
> >>       CC [M]
> >>      /project/openvpn/ovpn-dco.git/drivers/net/ovpn-dco/crypto.o
>        
> >>                                                                  
>      
> >>              

Re: [Openvpn-devel] [ovpn-dco]

2021-01-13 Thread Antonio Quartulli
Also,

are you sure you are compiling the latest experimental branch?
HEAD is 2555e5595088991006e57a3ee447f356dffdca92

Regards,

On 13/01/2021 10:12, Antonio Quartulli wrote:
> Hi Tony,
> 
> thanks for the message.
> The experimental branch is still...experimental, so I expect more things
> to crash and fail miserably :-D
> 
> In any case, feel free to send a patch if you see how to fix the issue!
> It would be nice to have external contributions :-)
> 
> This said, I am not sure this is our bug - the sin6_addr member of
> struct sockaddr_in6 is of type struct sin6_addr:
> 
> https://elixir.bootlin.com/linux/v5.4/source/include/uapi/linux/in6.h#L54
> 
> And I believe it is widely known that an IPv6 address is 12 bytes, not 8..
> 
> Maybe it's a bug in the compiler?
> 
> Best Regards,
> 
> On 13/01/2021 10:03, Tony He wrote:
>> Sorry, clicked "send" button before adding subject and CC Openvpn-dev. I
>> will send a new mail.
>>
>> Tony He mailto:huangy...@gmail.com>> 于2021年1月13
>> 日周三 下午4:57写道:
>>
>> Hi Antonio,
>>
>> I see you have pushed new commits to support multiple link to peers.
>> So I tried compiling, but encounter below error. My kernel version
>> is 5.4.0-54.
>>
>>
>>
>> tony-vm-2004% make                                                  
>>                                                                    
>>                                                                    
>>      
>> /project/openvpn/ovpn-dco.git/gen-compat-autoconf.sh
>> /project/openvpn/ovpn-dco.git/compat-autoconf.h                    
>>                                                                    
>>                      
>> make -C /lib/modules/5.4.0-54-generic/build
>> M=/project/openvpn/ovpn-dco.git PWD=/project/openvpn/ovpn-dco.git
>> REVISION=2555e55 CONFIG_OVPN_DCO=m INSTALL_MOD_DIR=updates/      
>> modules                            
>> make[1]: Entering directory
>> '/usr/src/linux-headers-5.4.0-54-generic'                          
>>                                                                    
>>                                                
>>   CC [M]  /project/openvpn/ovpn-dco.git/drivers/net/ovpn-dco/main.o
>>                                                                    
>>                                                                    
>>        
>>   CC [M]  /project/openvpn/ovpn-dco.git/drivers/net/ovpn-dco/bind.o
>>                                                                    
>>                                                                    
>>        
>>   CC [M]
>>  /project/openvpn/ovpn-dco.git/drivers/net/ovpn-dco/crypto.o        
>>                                                                    
>>                                                                  
>>   CC [M]  /project/openvpn/ovpn-dco.git/drivers/net/ovpn-dco/ovpn.o
>>                                                                    
>>                                                                    
>>        
>>   CC [M]  /project/openvpn/ovpn-dco.git/drivers/net/ovpn-dco/peer.o
>>                                                                    
>>                                                                    
>>        
>> In file included from ./include/linux/bitmap.h:9,                  
>>                                                                    
>>                                                                    
>>        
>>                  from ./include/linux/cpumask.h:12,                
>>                                                                    
>>                                                                    
>>        
>>                  from ./arch/x86/include/asm/cpumask.h:5,          
>>                                                                    
>>                                                                    
>>        
>>                  from ./arch/x86/include/asm/msr.h:11,              
>>                                                                    
>>                                                                    
>>      
>>                  from ./arch/x86/include/asm/processor.h:2

Re: [Openvpn-devel] [ovpn-dco]

2021-01-13 Thread Antonio Quartulli
On 13/01/2021 10:12, Antonio Quartulli wrote:
> And I believe it is widely known that an IPv6 address is 12 bytes, not 8..

Sorry, I meant 16, not 12, but you get the point :-)

-- 
Antonio Quartulli


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


Re: [Openvpn-devel] [ovpn-dco]

2021-01-13 Thread Antonio Quartulli
ead_info.h:53,      
>                                                                    
>                                                                    
>      
>                  from ./include/linux/thread_info.h:38,            
>                                                                    
>                                                                    
>        
>                  from ./arch/x86/include/asm/preempt.h:7,          
>                                                                    
>                                                                    
>        
>                  from ./include/linux/preempt.h:78,                
>                                                                    
>                                                                    
>        
>                  from ./include/linux/spinlock.h:51,                
>                                                                    
>                                                                    
>      
>                  from ./include/linux/seqlock.h:36,                
>                                                                    
>                                                                    
>        
>                  from ./include/linux/time.h:6,                    
>                                                                    
>                                                                    
>        
>                  from ./include/linux/ktime.h:24,                  
>                                                                    
>                                                                    
>        
>                  from ./include/linux/timer.h:6,                    
>                                                                    
>                                                                    
>      
>                  from ./include/linux/netdevice.h:24,              
>                                                                    
>                                                                    
>        
>                  from
> /project/openvpn/ovpn-dco.git/linux-compat.h:20,                    
>                                                                    
>                                                    
>                  from :                              
>                                                                    
>                                                                    
>        
> In function ‘memcmp’,                                              
>                                                                    
>                                                                    
>        
>     inlined from ‘ovpn_peer_lookup_transp_addr’ at
> /project/openvpn/ovpn-dco.git/drivers/net/ovpn-dco/peer.c:457:8:    
>                                                                    
>                        
> ./include/linux/string.h:440:4: error: call to ‘__read_overflow’
> declared with attribute error: detected read beyond size of object
> passed as 1st parameter                                            
>            
>   440 |    __read_overflow();
>       |    ^
> make[3]: *** [scripts/Makefile.build:275:
> /project/openvpn/ovpn-dco.git/drivers/net/ovpn-dco/peer.o] Error 1
> make[2]: *** [scripts/Makefile.build:522:
> /project/openvpn/ovpn-dco.git/drivers/net/ovpn-dco] Error 2
> make[1]: *** [Makefile:1757: /project/openvpn/ovpn-dco.git] Error 2
> make[1]: Leaving directory '/usr/src/linux-headers-5.4.0-54-generic'
> make: *** [Makefile:46: all] Error 2
> 
> 
> Function memcmp
> in /usr/src/linux-headers-5.4.0-54-generic/include/linux/string.h.
> 434 __FORTIFY_INLINE int memcmp(const void *p, const void *q,
> __kernel_size_t size)
> 435 {
> 436         size_t p_size = __builtin_object_size(p, 0);
> 437         size_t q_size = __builtin_object_size(q, 0);
> 438         if (__builtin_constant_p(size)) {
> 439                 if (p_size < size)
> 440                         __read_overflow();
> 441                 if (q_size < size)
> 442                         __read_overflow2();
> 443         }
> 444         if (p_size < size || q_size < size)
> 445                 fortify_panic(__func__);
> 446         return __underlying_memcmp(p, q, size);
> 447 }
> 
> Seems that p_size is 8 in above memcmp function because if I change
> your code as below , then it's OK(9 causes same error).
> 456                 case AF_INET6:
> 457                         if (memcmp((void *)>sin6_addr,
> (void *)>sa.in6.sin6_addr,
> 458                                 //   sizeof(struct in6_addr)))
> 459                                         8))
> 460                                 break;
> 
> Tony
> 

-- 
Antonio Quartulli


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


Re: [Openvpn-devel] [PATCH] Zero initialise msghdr prior to calling sendmesg

2021-01-12 Thread Antonio Quartulli
Hi,


On 05/01/2021 14:17, Arne Schwabe wrote:
> This ensure that all unused fields in msg are zero.
> 
> Spotted by Coverity:
> 
> Using uninitialized value "msg". Field "msg.msg_flags" is uninitialized
> when calling "sendmsg".

No signed-off-by ?

> ---
>  src/openvpn/manage.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/src/openvpn/manage.c b/src/openvpn/manage.c
> index a4f99c9a..103ccadc 100644
> --- a/src/openvpn/manage.c
> +++ b/src/openvpn/manage.c
> @@ -2092,7 +2092,7 @@ man_io_error(struct management *man, const char *prefix)
>  static ssize_t
>  man_send_with_fd(int fd, void *ptr, size_t nbytes, int flags, int sendfd)
>  {
> -struct msghdr msg;
> +struct msghdr msg = {0};

In the rest of the code we have spaces around the 0, like "{ 0 }"
I suggest using the same style.

>  struct iovec iov[1];
>  
>  union {
> @@ -2124,7 +2124,7 @@ man_send_with_fd(int fd, void *ptr, size_t nbytes, int 
> flags, int sendfd)
>  static ssize_t
>  man_recv_with_fd(int fd, void *ptr, size_t nbytes, int flags, int *recvfd)
>  {
> -struct msghdr msghdr;
> +struct msghdr msghdr = {0};

same as above.

>  struct iovec iov[1];
>  ssize_t n;
>  
> 

Other than that it's Feature-ACK.
We should always fully initialize objects that we pass around.

Cheers,

-- 
Antonio Quartulli


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


Re: [Openvpn-devel] [ovpn-dco] AES-CCM available for testing

2020-12-07 Thread Antonio Quartulli
Hi Jan Just, Tony,

On 07/12/2020 10:10, Jan Just Keijser wrote:
> Thank you very much for adding this so quickly; it won't help Tony He
> though, as he is stuck using a rather old AL314 + R9000 chip which does
> not support CCM or GCM. I just checked the driver code and indeed there
> is no HW support for GCM.  They *do* support some AEAD algorithms:
> 
>   authenc-hmac-sha256-cbc-aes
>   authenc-hmac-sha384-cbc-aes
> 
> which are listed as the (true) AEAD equivalent of AES+SHA ; the question
> is : how hard would it be to add support for this (and would it be worth
> it?)

I would ask the same question to the vendor: how hard would it be to
support AES-GCM in the current HW engine?

Any info about that?
They are the best recipient for such feature request I think.

As far as I understood the HW engine is also open source, so actually
anybody with the right motivation could take up that task.


Forcing ourselves to sticking to legacy algorithms is not the right
move, imho (especially when there are solutions - see above).
To answer your question: my feeling is that working on it is not worth
the benefit.


Regards,


-- 
Antonio Quartulli


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


[Openvpn-devel] [ovpn-dco] AES-CCM available for testing

2020-12-06 Thread Antonio Quartulli
Hi all,

Some people have expressed interest in ovpn-dco supporting AES-CBC.

However, since ovpn-dco is currently using the AEAD kernel crypto API
only, introducing support for CBC mode would require quite some
refactoring and we do not really want to do that (the community believes
that as of now AEAD ciphers should always be preferred moving forward).

In a previous discussion on this mailing list, it was highlighted that
AES-CCM is nothing else than AES-CBC in disguise as AEAD cipher.

(for the curious: it is AES "Counter with CBC-MAC", known as CCM and
described in RFC3610).

For this reason I decided to give AES-CCM a try and I implemented in it
the "aes-ccm" branch of the ovpn-dco repo.

I am not sure if we're going to merge it to master yet, but for now it
would be interesting to gather feedback from those interested in this
cipher.

Please note that OpenVPN3 does not yet support this cipher, therefore
the only way to test AES-CCM in ovpn-dco is to use the ovpn-cli tool
provided in the tests/ folder.


To do so, just specify "aes-ccm" as algorithm when setting a new key.


Cheers,


-- 
Antonio Quartulli


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


Re: [Openvpn-devel] [PATCH v2] Fix line number reporting on config file errors after segments

2020-12-06 Thread Antonio Quartulli
Hi,

On 06/12/2020 13:57, Gert Doering wrote:
>  segments neglected to increment the "current line number
> in config file" variable (line_num), so after the first ,
> errors reported have the wrong line number.
> 
> Fix by introducing an extra argument to read_inline_file() function:
> "so many lines in the inline block", and changing the return values of
> the "check_inline*()" functions to "int", changing this from "false/true"
> to "0 = no inline, 1...N = inline with  lines".
> 
> On calling add_options() this is implicitly converted back to bool.
> 
> v2: use int return value, not extra call-by-reference parameter
> 
> Trac: #1325
> ---
>  src/openvpn/options.c | 33 +
>  1 file changed, 17 insertions(+), 16 deletions(-)
> 
> diff --git a/src/openvpn/options.c b/src/openvpn/options.c
> index 21f8d494..d8a86560 100644
> --- a/src/openvpn/options.c
> +++ b/src/openvpn/options.c
> @@ -4659,7 +4659,8 @@ in_src_get(const struct in_src *is, char *line, const 
> int size)
>  }
>  
>  static char *
> -read_inline_file(struct in_src *is, const char *close_tag, struct gc_arena 
> *gc)
> +read_inline_file(struct in_src *is, const char *close_tag,
> + int *num_lines, struct gc_arena *gc)
>  {
>  char line[OPTION_LINE_SIZE];
>  struct buffer buf = alloc_buf(8*OPTION_LINE_SIZE);
> @@ -4668,6 +4669,7 @@ read_inline_file(struct in_src *is, const char 
> *close_tag, struct gc_arena *gc)
>  
>  while (in_src_get(is, line, sizeof(line)))
>  {
> +(*num_lines)++;
>  char *line_ptr = line;
>  /* Remove leading spaces */
>  while (isspace(*line_ptr))
> @@ -4701,10 +4703,10 @@ read_inline_file(struct in_src *is, const char 
> *close_tag, struct gc_arena *gc)
>  return ret;
>  }
>  
> -static bool
> +static int
>  check_inline_file(struct in_src *is, char *p[], struct gc_arena *gc)
>  {
> -bool is_inline = false;
> +int num_inline_lines = 0;
>  
>  if (p[0] && !p[1])
>  {
> @@ -4717,16 +4719,15 @@ check_inline_file(struct in_src *is, char *p[], 
> struct gc_arena *gc)
>  p[0] = string_alloc(arg + 1, gc);
>  close_tag = alloc_buf(strlen(p[0]) + 4);
>  buf_printf(_tag, "", p[0]);
> -p[1] = read_inline_file(is, BSTR(_tag), gc);
> +p[1] = read_inline_file(is, BSTR(_tag), _inline_lines, 
> gc);
>  p[2] = NULL;
>  free_buf(_tag);
> -is_inline = true;
>  }
>  }
> -return is_inline;
> +return num_inline_lines;
>  }
>  
> -static bool
> +static int
>  check_inline_file_via_fp(FILE *fp, char *p[], struct gc_arena *gc)
>  {
>  struct in_src is;
> @@ -4735,7 +4736,7 @@ check_inline_file_via_fp(FILE *fp, char *p[], struct 
> gc_arena *gc)
>  return check_inline_file(, p, gc);
>  }
>  
> -static bool
> +static int
>  check_inline_file_via_buf(struct buffer *multiline, char *p[],
>struct gc_arena *gc)
>  {
> @@ -4806,13 +4807,13 @@ read_config_file(struct options *options,
>  }
>  if (parse_line(line + offset, p, SIZE(p)-1, file, line_num, 
> msglevel, >gc))
>  {
> -bool is_inline;
> -
>  bypass_doubledash([0]);
> -is_inline = check_inline_file_via_fp(fp, p, 
> >gc);
> -add_option(options, p, is_inline, file, line_num, level,
> +int lines_inline =
> +check_inline_file_via_fp(fp, p, >gc);

This is quite ugly  - maybe we could just keep it on one line (as we
recently agreed that we can go beyond 80 chars if useful)?


> +add_option(options, p, lines_inline, file, line_num, 
> level,
> msglevel, permission_mask, option_types_found,
> es);
> +line_num += lines_inline;
>  }
>  }
>  if (fp != stdin)
> @@ -4855,12 +4856,12 @@ read_config_string(const char *prefix,
>  ++line_num;
>  if (parse_line(line, p, SIZE(p)-1, prefix, line_num, msglevel, 
> >gc))
>  {
> -bool is_inline;
> -
>  bypass_doubledash([0]);
> -is_inline = check_inline_file_via_buf(, p, 
> >gc);
> -add_option(options, p, is_inline, prefix, line_num, 0, msglevel,
> +    int lines_inline =
> +check_inline_file_via_buf(, p, >gc);
> +add_opt

Re: [Openvpn-devel] [PATCH 1/2 v2] tls-crypt-v2: fix server memory leak

2020-12-03 Thread Antonio Quartulli
Hi,

On 03/12/2020 19:22, Steffan Karger wrote:
> tls-crypt-v2 was developed in parallel with the changes that allowed to
> use tls-auth/tls-crypt in connection blocks. The tls-crypt-v2 patch set
> was never updated to the new reality after commit 5817b49b, causing a
> memory leak of about 600 bytes for each connecting client.
> 
> It would be nicer to not reload the tls-crypt-v2 server key for each
> connecting client, but that requires more refactoring (and thus more time
> to get right). So for now just plug the leak by free'ing the memory when
> we close a client connection.
> 
> To test this easily, compile openvpn with -fsanity=address, run a server
> with tls-crypt-v2, connect a client, stop the server.
> 
> Signed-off-by: Steffan Karger 


Acked-by: Antonio Quartulli 


-- 
Antonio Quartulli


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


Re: [Openvpn-devel] [PATCH 1/2] tls-crypt-v2: fix server memory leak

2020-12-03 Thread Antonio Quartulli
Hi Steffan,

On 03/12/2020 16:49, Steffan Karger wrote:
> diff --git a/src/openvpn/init.c b/src/openvpn/init.c
> index 27a4170d..5cde8a4b 100644
> --- a/src/openvpn/init.c
> +++ b/src/openvpn/init.c
> @@ -3619,6 +3619,7 @@ do_close_free_key_schedule(struct context *c, bool 
> free_ssl_ctx)
>   * always free the tls_auth/crypt key. If persist_key is true, the key 
> will
>   * be reloaded from memory (pre-cached)
>   */
> +free_key_ctx(>c1.ks.tls_crypt_v2_server_key);
>  free_key_ctx_bi(>c1.ks.tls_wrap_key);
>  CLEAR(c->c1.ks.tls_wrap_key);
>  buf_clear(>c1.ks.tls_crypt_v2_wkc);

A few lines below we call key_schedule_free() (under certain conditions)
which also performs:

free_key_ctx(>tls_crypt_v2_server_key);


I believe it is safe to call free_key_ctx() twice on the same object,
but wouldn't it be better to have it called once only along the same
code path?

Regards,


-- 
Antonio Quartulli


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


Re: [Openvpn-devel] [ovpn-dco] Is cbc-hmac supported?

2020-11-25 Thread Antonio Quartulli
Hi Tony,

On 26/11/2020 01:46, Tony He wrote:
>>OpenSSL directly talks to the crypto engine via a proprietary interface
>>that the FW/driver exposes to userspace. The *data* flow does not cross
>>the linux kernel crypto API
> 
> No, OpenSSL doesn't directly talk to the  crypto engine via a
> proprietary interface that the FW/driver exposes to userspace.
> "cryptodev engine" is NOT the "HW engine" chip vendor provides. It's a
> common interface and its source is not from
> chip vendor. Please refer to:
> https://github.com/cryptodev-linux/cryptodev-linux
> <https://github.com/cryptodev-linux/cryptodev-linux>
> https://openwrt.org/docs/techref/hardware/cryptographic.hardware.accelerators
> <https://openwrt.org/docs/techref/hardware/cryptographic.hardware.accelerators>

Thanks for clarifying!
I thought you were talking about the crypto engine offload provided by
OpenSSL via vendor library.

So, if I understand this correctly, what you are saying is that the
vendor provides kernel patches in order to add HW support to the kernel
crypto API for an architecture that is normally not supported by the
upstream kernel. Then cryptodev is used to allow userspace to use the
kernel API.

This said, I am sorry, but I am not sure we should continue this
discussion any further, because as of now we have no plan to introduce
yet another crypto family in ovpn-dco.

One of the goal with ovpn-dco is to leave behind the legacy from
openvpn2 in userspace and focus on those features we believe to be
"state of the art". This is why we decided to only support AEAD with
only AES-GCM and CHACHA20POLY1305, DATA_V2 only, etc.

Focus is on keeping the code simple and ensure it can be accepted
upstream in the Linux kernel quickly.

It was an hard decision to make, but the whole group decided to take
this direction. People that want to use different
configurations/settings will still be able to do so by using openvpn2 in
userspace, as it happened until now.


Cheers,


> 
> Tony 
>  
> 
> Antonio Quartulli  于2020年11月26日周四 上午12:19写道:
> 
> Hi Tony,
> 
> > OpenVPN-> openssl->crypodev engine->cryptodev-linux->Linux kernel
> crypto API->HW engine crypto API-> HW engine driver-> HW engine
> 
> Now I understand better what you have in mind.
> 
> To the best of my knowledge, this is not how it works.
> 
> OpenSSL directly talks to the crypto engine via a proprietary interface
> that the FW/driver exposes to userspace. The *data* flow does not cross
> the linux kernel crypto API.
> 
> Moist of the time this special interfaces are made "to work with openssl
> only", so I am not even sure how the kernel API could use it.
> 
> Do you have any pointer saying otherwise?
> 
> 
> -- 
> Antonio Quartulli
> 

-- 
Antonio Quartulli


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


Re: [Openvpn-devel] [ovpn-dco] question about the comment about AEAD nonce

2020-11-24 Thread Antonio Quartulli
Hi Tony,

The graphic is wrong. Some of the text that you can find in the code
comes from old internal documentation and it hasn't always been updated.

To clarify, AES-GCM (and ChaCha20Poly1305) accepts a 12 bytes nonce that
OpenVPN creates by concatenating the 4 bytes packet ID (sent over the
wire) with 8 bytes nonce-tail generated from the key material.

So all numbers in the doc are correct - just the hex string is wrong.

It is now fixed. Thanks for reporting.

Please note that the documentation in the code is still work in
progress, so some other inconsistency may jump out every now and then.

Cheers,

On 25/11/2020 03:33, Tony He wrote:
> Hi Antonio,
> 
> I'm reading the source code to study this module driven by intertest.
> I'm new to crypto stuffs. In pktid.h:
> 
> /* When the OpenVPN protocol is run in AEAD mode, use
>  * the OpenVPN packet ID as the AEAD nonce:
>  *
>  *    0005 521c3b01 4308c041 83ba3099
>  *    [seq # ] [nonce_tail                                    ]
>  *    [               12-byte full IV                            ] ->
> NONCE_SIZE
>  *    [4-bytes                                                       ->
> NONCE_WIRE_SIZE
>  *    on wire]
>  */
> 
> /* AEAD nonce size */
> #define NONCE_SIZE 12
> 
> Is " 0005 521c3b01 4308c041 83ba3099" wrong? Its size is 16 bytes,
> but you also comment
> "12-byte full IV" after two lines of it. 
> 
> Tony

-- 
Antonio Quartulli


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


Re: [Openvpn-devel] [ovpn-dco] Kernel NULL point derefence

2020-11-24 Thread Antonio Quartulli
Hi Tony,

Thanks a lot for all your tests.
The faulty commit is:

commit ba109be633fd802b856d6a125f47e2d0ff7ad749
Author: Antonio Quartulli 
Date:   Sun Nov 22 16:13:17 2020 +0100

ovpn-dco: avoid potential out of bound access in aead_decrypt()


I have just pushed a fix to master to address the bug.
Could you please give it a go?

Thanks a lot!

On 24/11/2020 08:38, Tony He wrote:
> Hi Antonio,
> 
> Did more test. Just FYI.
> 
> ba109be633f bad.
> 6eb6292a9d3 ?
> 0989291e816 good
> 
> Tony
> 
> Tony He mailto:huangy...@gmail.com>> 于2020年11月
> 24日周二 上午9:19写道:
> 
> Hi Antonio,
> 
> I'm using the latest commit 4b104be to test and encountered
> following issue. I saw multi times in both peers. I never
> encountered this issue  before commit c56b9d0. Can you reproduce?
> 
> [  708.790419] ovpn_dco: module verification failed: signature
> and/or required key missing - tainting kernel                      
>                                                                    
>            
> [  708.790885] OpenVPN data channel offload (ovpn-dco) 4b104be-dirty
> -- (C) 2020 OpenVPN, Inc.                                          
>                                                                    
>      
> [  899.304454] BUG: kernel NULL pointer dereference, address:
> 0008                                                    
>                                                                    
>            
> [  899.305245] #PF: supervisor read access in kernel mode          
>                                                                    
>                                                                    
>        
> [  899.306044] #PF: error_code(0x) - not-present page          
>                                                                    
>                                                                    
>        
> [  899.306825] PGD 0 P4D 0                                          
>                                                                    
>                                                                    
>      
> [  899.307597] Oops:  [#1] SMP PTI                              
>                                                                    
>                                                                    
>      
> [  899.308335] CPU: 1 PID: 34 Comm: kworker/1:1 Tainted: G          
> OE     5.4.0-54-generic #60-Ubuntu                                  
>                                                                    
>      
> [  899.309922] Hardware name: innotek GmbH VirtualBox/VirtualBox,
> BIOS VirtualBox 12/01/2006                                          
>                                                                    
>        
> [  899.310887] Workqueue: ovpn-crypto-wq-tun0 ovpn_decrypt_work
> [ovpn_dco]                                                          
>                                                                    
>          
> [  899.311762] RIP: 0010:gcmaes_crypt_by_sg.constprop.0+0x244/0x6c0
> [aesni_intel]                                                      
>                                                                    
>        
> [  899.312518] Code: ac f8 48 83 f8 01 19 c0 f7 d0 83 e0 b6 eb 87 4c
> 8b 74 24 40 48 8d 7c 24 60 49 8b 76 40 41 8b 56 30 e8 10 eb ac f8 49
> 8b 76 48 <44> 8b 60 08 49 89 c5 49 39 76 40 0f 84 7d 02 00 00 41 8b
> 56 30
>  48                                                                
>                                                                    
>                                                                    
>        
> [  899.315985] RSP: 0018:9ed680127800 EFLAGS: 00010246          
>                                                                    
>                                                                    
>      
> [  899.316843] RAX:  RBX: 0030 RCX:
> e78440adf700                                                    
>                                                                    
>          
> [  899.317489] RDX: 0008 RSI: 9ed680127bb0 RDI:
> 9ed680127bb0                                                    
>                                                                    
>          
> [  899.318143] RBP: 9ed680127aa0 R08: 9ed680127ab

Re: [Openvpn-devel] [ovpn-dco] performance issue

2020-11-20 Thread Antonio Quartulli
25.88
> ksoftirqd/1                                                            
>                                                                    
>    1923 root      20   0       0      0      0 I   1.0   0.0   0:06.81
> kworker/1:3-ovpn-crypto-wq-tun0                                        
>                                                                    
>    1924 root      20   0       0      0      0 I   0.7   0.0   0:00.39
> kworker/0:1-ata_sff                 *
> *
> 
> 
> tony-vm-2004% mpstat -P ALL 2                                          
>                                                                        
>                                                                    
> Linux 5.4.0-54-generic (tony-vm-2004)   11/20/20        _x86_64_      
>  (2 CPU)                                                                
>                                                                        
>                                                                        
>                                                                        
>                                                        
> 01:56:07     CPU    %usr   %nice    %sys %iowait    %irq   %soft  %steal
>  %guest  %gnice   %idle
> 01:56:09     all    0.53    0.00   28.04    0.00    0.00   28.31    0.00
>    0.00    0.00   43.12
> 01:56:09       0    1.12    0.00    6.18    0.00    0.00    1.12    0.00
>    0.00    0.00   91.57
> 01:56:09       1    0.00    0.00   47.50    0.00    0.00   52.50    0.00
>    0.00    0.00    0.00
> 
> 01:56:09     CPU    %usr   %nice    %sys %iowait    %irq   %soft  %steal
>  %guest  %gnice   %idle
> 01:56:11     all    0.53    0.00   25.66    0.26    0.00   32.28    0.00
>    0.00    0.00   41.27
> 01:56:11       0    1.12    0.00    8.43    0.56    0.00    2.81    0.00
>    0.00    0.00   87.08
> 01:56:11       1    0.00    0.00   41.00    0.00    0.00   58.50    0.00
>    0.00    0.00    0.50
> 
> 
> Tony
> 
> 
> ___
> Openvpn-devel mailing list
> Openvpn-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/openvpn-devel
> 

-- 
Antonio Quartulli


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


Re: [Openvpn-devel] ovpn-dco: nestns-test.sh - fix the issue that veth is not created successfully

2020-11-18 Thread Antonio Quartulli
Hi Tony,

On 19/11/2020 02:26, Tony He wrote:
> Hi Antonio,
>
> To confirm, I installed a Ubuntu 20.04 VM and saw it supports these two
> formates.
>
Thanks a lot for checking - this is what I imagined too.
The ip tool has introduced several shortcuts over time to make it more
user friendly.

Nonetheless, your patch has been applied so that our script can work on
older distros too.

Best Regards,



-- 
Antonio Quartulli


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


Re: [Openvpn-devel] ovpn-dco: nestns-test.sh - fix the issue that veth is not created successfully

2020-11-18 Thread Antonio Quartulli
Hi Tony,

On 18/11/2020 15:54, Tony He wrote:
> 
> Hi Antonio,
> 
> Have you encountered this issue? Please help to review.
> 

No, we have never encountered this issue.

My feeling is that you are using userspace tools that are older than
what we normally use.

I see you are using Ubuntu 18.04 and that might be the reason.

In any case, your patch is small and does not hurt :-)

Thanks


-- 
Antonio Quartulli


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


Re: [Openvpn-devel] [PATCH] ovpn-dco: nestns-test.sh - fix the issue that veth is not created successfully.

2020-11-18 Thread Antonio Quartulli
Hi,

On 18/11/2020 15:54, Tony He wrote:
> According to user manual of "ip link", the command should be
> "ip link add veth0 type veth peer name veth1"
> 
> instead of
> "ip link add veth0 type veth peer veth1"
> 
> With this fix, after running "./netns-test.sh", we can do ping test by
> executing "ip netns exec peer0 ping 5.5.5.2"
> 
> Tested in Ubuntu 18.04 PC.
> 
> Signed-off-by: Tony He 

Applied to master in revision c56b9d024570 with slightly amended commit
message.

Cheers,

-- 
Antonio Quartulli


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


Re: [Openvpn-devel] [ovpn-dco] It seems not to compile ovpn-cli successfully

2020-11-18 Thread Antonio Quartulli
Hi Tony,

Thanks a lot for your interest in ovpn-dco!

It's amazing to see other people putting effort in using and testing our
new kernel module :-)

See my replies below:

On 18/11/2020 03:31,  wrote:
> /usr/include/libnl3/netlink/handlers.h:51:15: warning: ??struct nlmsgerr??
> declared inside parameter list will not be visible outside of this
> definition or declaration?0?2 ?0?2 ?0?2 ?0?2 ?0?2 ?0?2 ?0?2 ?0?2 ?0?2 ?0?2 
> ?0?2 ?0?2 ?0?2 ?0?2 ?0?2 ?0?2 ?0?2 ?0?2 ?0?2 ?0?2 ?0?2 ?0?2 ?0?2?0?2
> ?0?2 ?0?2 ?0?2 ?0?2 struct nlmsgerr *nlerr, void *arg);
> ?0?2 ?0?2 ?0?2 ?0?2 ?0?2 ?0?2 ?0?2 ?0?2^~~~
> /usr/include/libnl3/netlink/handlers.h:50:43: warning: ??struct
> sockaddr_nl?? declared inside parameter list will not be visible outside
> of this definition or declaration
> ?0?2typedef int (*nl_recvmsg_err_cb_t)(struct sockaddr_nl *nla,
> ?0?2 ?0?2 ?0?2 ?0?2 ?0?2 ?0?2 ?0?2 ?0?2 ?0?2 ?0?2 ?0?2 ?0?2 ?0?2 ?0?2 ?0?2 
> ?0?2 ?0?2 ?0?2 ?0?2 ?0?2 ?0?2 ?0?2^~~
> /usr/include/libnl3/netlink/handlers.h:135:18: warning: ??struct
> sockaddr_nl?? declared inside parameter list will not be visible outside
> of this definition or declaration
> ?0?2 ?0?2 ?0?2 ?0?2 ?0?2 ?0?2struct sockaddr_nl *,
> ?0?2 ?0?2 ?0?2 ?0?2 ?0?2 ?0?2 ?0?2 ?0?2 ?0?2 ^~~
> ovpn-cli.c: In function ??ovpn_nl_msg_send??:
> ovpn-cli.c:232:38: warning: passing argument 3 of ??nl_cb_err?? from
> incompatible pointer type [-Wincompatible-pointer-types]
> ?0?2 nl_cb_err(ctx->nl_cb, NL_CB_CUSTOM, ovpn_nl_cb_error, );
> ?0?2 ?0?2 ?0?2 ?0?2 ?0?2 ?0?2 ?0?2 ?0?2 ?0?2 ?0?2 ?0?2 ?0?2 ?0?2 ?0?2 ?0?2 
> ?0?2 ?0?2 ?0?2 ?0?2 ^~~~
> In file included from /usr/include/libnl3/netlink/socket.h:16:0,
> ?0?2 ?0?2 ?0?2 ?0?2 ?0?2 ?0?2 ?0?2 ?0?2 ?0?2from ovpn-cli.c:17:
> /usr/include/libnl3/netlink/handlers.h:127:13: note: expected
> ??nl_recvmsg_err_cb_t {aka int (*)(struct sockaddr_nl *, struct nlmsgerr
> *, void *)}?? but argument is of type ??int (*)(struct sockaddr_nl *,
> struct nlmsgerr *, void *)??
> ?0?2extern int?0?2 nl_cb_err(struct nl_cb *, enum nl_cb_kind, 
> nl_recvmsg_err_cb_t,
> ?0?2 ?0?2 ?0?2 ?0?2 ?0?2 ?0?2 ?0?2^

We haven't seen this issue so far, probably because we are using a more
recent libnl version (we test with libnl3.4 and libnl3.5).

I added a potential fix in latest master.


> ovpn-cli.c: In function ??ovpn_read_cipher??:
> ovpn-cli.c:355:17: error: ??OVPN_CIPHER_ALG_CHACHA20POLY1305?? undeclared
> (first use in this function); did you mean
> ??OVPN_CIPHER_ALG_CHACHA20_POLY1305???
> ?0?2 ?0?2ctx->cipher = OVPN_CIPHER_ALG_CHACHA20POLY1305;
> ?0?2 ?0?2 ?0?2 ?0?2 ?0?2 ?0?2 ?0?2 ?0?2 ?0?2^~~~
> ?0?2 ?0?2 ?0?2 ?0?2 ?0?2 ?0?2 ?0?2 ?0?2 ?0?2OVPN_CIPHER_ALG_CHACHA20_POLY1305

Ouch, this is my fault. It is fixed in the latest master now.


Can you please give latest master another try and let us know?


Thanks!


-- 
Antonio Quartulli


OpenPGP_0x48F0CCB68F59D14C.asc
Description: application/pgp-keys


OpenPGP_signature
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] networking_iproute2: fix memory leak in net_iface_mtu_set()

2020-10-09 Thread Antonio Quartulli
Hi,

On 09/10/2020 15:46, Steffan Karger wrote:
> ASAN yelled at me that someone forgot to call argv_free(). Fix that.
> 
> Signed-off-by: Steffan Karger 

I bet I know that someone!

Thanks a lot for fixing this.

Acked-by: Antonio Quartulli 

(this is for 2.5)

-- 
Antonio Quartulli


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


Re: [Openvpn-devel] [PATCH] Fix redirecting of IPv4 default gateway if connecting over IPv6.

2020-10-04 Thread Antonio Quartulli
Hi,

On 02/10/2020 19:57, Gert Doering wrote:
> Commit aa34684972eb0 fixed a long-standing bug in setting the
> "route-list" flag RTSA_REMOTE_HOST for IPv4 ("we have a well-defined
> remote_host == VPN server IP address") even if connecting over IPv6.
> 
> Unfortunately the logic in redirect_default_route_to_vpn() was also
> wrong, and refused cooperation if that flag is not set, triggering
> the message
> "NOTE: unable to redirect IPv4 default gateway -- Cannot
>  obtain current remote host address"
> 
> Correct operation: if RTSA_REMOTE_HOST is not set, or remote_host
> is IPV4_INVALID_ADDR (= 255.255.255.255), do not try to install a
> host route for continued connectivity to the VPN server - which is
> not needed when connecting over IPv6.  But the actual *routes*
> (/0 or 2 x /1) can be installed just fine.
> 
> There is a second bug here, which hits if there is no IPv4 gateway
> at all.  In that case, the same function triggers the message
> "NOTE: unable to redirect IPv4 default gateway -- Cannot
>  read current default gateway from system"
> 
> This is caused by using "IPV4_INVALID_ADDR" as a flag for "do we
> know the remote_host?" - which worked before, but after the commit
> said above, the "remote_host" field is not well-defined unless
> RTSA_REMOTE_HOST is set.  So, change the condition to check that.
> 
> Reported-By: François Kooman 
> Reported-By: Thomas Schäfer 
> Trac: #1332
> 
> Signed-off-by: Gert Doering 

The bug becomes obvious after reading commit aa34684972eb0
This fix is basically cleaning up some conditions which did not adapt to
the new logic.

Stared at the code and tested on my system with and without an IPv4
default route.

Signed-off-by: Antonio Quartulli 


-- 
Antonio Quartulli


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


Re: [Openvpn-devel] [PATCH v6 1/2] Selectively reformat too long lines

2020-09-24 Thread Antonio Quartulli
Hi,

On 20/09/2020 22:57, Vladislav Grishenko wrote:
> @@ -3170,7 +3179,7 @@ static const struct proto_names proto_names[] = {
>  {"udp6","UDPv6", AF_INET6, PROTO_UDP},
>  {"tcp6-server","TCPv6_SERVER", AF_INET6, PROTO_TCP_SERVER},
>  {"tcp6-client","TCPv6_CLIENT", AF_INET6, PROTO_TCP_CLIENT},
> -{"tcp6","TCPv6", AF_INET6, PROTO_TCP},
> +{"tcp6",   "TCPv6", AF_INET6, PROTO_TCP},

What are you actually fixing here? Adding a tab?
I feel there should be a space after each ',', but that is probably out
of the scope of this patch?


-- 
Antonio Quartulli


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


[Openvpn-devel] Introducing the OpenVPN Data Channel Offload Linux kernel module (ovpn-dco)

2020-09-22 Thread Antonio Quartulli
Dear all,

OpenVPN Inc.[1] and me are happy to announce that today we have released
to the public our work-in-progress project called "ovpn-dco".

ovpn-dco stands for "OpenVPN Data Channel Offload".
It is a Linux kernel module and it implements all required
functionalities to handle the OpenVPN data channel.

In other words, when using ovpn-dco, the OpenVPN software will not send
data traffic back and forth between user and kernel space (for
encryption/decryption and routing), but operations on payloads will take
place in the Linux kernel.

Moving the data channel to kernel space is expected to finally remove
that bottleneck that has long prevented OpenVPN to scale better.
This is just the first step towards a feature-complete kernel based
OpenVPN driver and we will continue to work on improving performance and
 implementing more features.

The project is under heavy development, therefore APIs, data structures
and anything else may drastically change in the following months.

Our goal is to bring ovpn-dco up-to-the-standard with the rest of the
Linux kernel and then submit it for review and inclusion to the netdev
community.

Clients currently able to leverage on the ovpn-dco kernel module are the
OpenVPN3 reference client[2] and the OpenVPN3 Linux client[3] will
include ovpn-dco support in the next release. Support for OpenVPN2 is
planned.

ovpn-dco is released under GPLv2 and everybody is welcome to run, crash
and patch this kernel module.

Code is hosted on both GitHub and GitLab:
* https://gitlab.com/openvpn/ovpn-dco
* https://github.com/OpenVPN/ovpn-dco

Please refer to the README file for more details about how to build and
test ovpn-dco.
Patch submission can be done through this mailing list, as explained in
the README as well.

https://gitlab.com/openvpn/ovpn-dco/-/blob/master/README


The idea of implementing an OpenVPN kernel module has been around for
years; finally what were once words have become lines of code.
I hope more enthusiasts will join the project and help pushing the
development forward.

A big thanks goes to OpenVPN Inc. for having provided all required
resources to make this happen.


Looking forward to see patches coming through!
Happy hacking!


[1] https://www.openvpn.net
[2]
https://github.com/openvpn/openvpn3#building-the-openvpn-3-client-on-linux
[3] https://community.openvpn.net/openvpn/wiki/OpenVPN3Linux

-- 
Antonio Quartulli
OpenVPN Inc.


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


Re: [Openvpn-devel] [PATCH] Fix line number reporting on config file errors after segments

2020-09-21 Thread Antonio Quartulli
Hi,

On 21/09/2020 09:50, Gert Doering wrote:
> Hi,
> 
> On Mon, Sep 21, 2020 at 09:22:38AM +0200, Antonio Quartulli wrote:
>> Sorry for not chiming in earlier, but honestly I believe your other
>> option would be "cleaner". The other option being "return int instead of
>> bool, where the returned value is the number of lines of the inline'd
>> material.
>>
>> If 0, we know there was no inline'd material, thus we can treat that
>> result as "not inline'd".
> 
> We could do that, indeed.  I was torn between both variants - this one
> is a bit more intrusive (because it adds a new argument to so many
> functions), but on the other hand, the bool stays a bool and is not
> magically overloaded - which would make the call to add_option() 
> either "even longer", or if you rely on magic "we just pass the integer
> to a bool-expecting function" much harder to understand.
> 
>> Anyway, this is just my opinion. If you believe this is a better
>> approach, I am fine with it too.
> 
> I wasn't sure.  In the end, this one didn't feel so bad, so this is
> what we have.
> 
> Maybe I'll do the other one as well, for fun, and to compare :-)
> 
>>> @@ -4692,7 +4696,7 @@ check_inline_file(struct in_src *is, char *p[], 
>>> struct gc_arena *gc)
>>>  p[0] = string_alloc(arg + 1, gc);
>>>  close_tag = alloc_buf(strlen(p[0]) + 4);
>>>  buf_printf(_tag, "", p[0]);
>>> -p[1] = read_inline_file(is, BSTR(_tag), gc);
>>> +p[1] = read_inline_file(is, BSTR(_tag), num_lines, gc);
>>
>> am I wrong or here we should add 1 to num_lines to include the opening
>> tag that was parsed *before* entering read_inline_file()?
> 
> The opening tag is parsed as "regular" line in read_config_file(), and as 
> such, the "++line_num" in there applies.
> 
> (I have tested this :-) - nothing more embarrassing than a patch to fix
> line numbers actually still printing wrong line numbers)
> 

Ok, then for me this patch is good to go.

FTR on IRC we discussed the possibility of using the "other approach".
Gert may give it a try and decide if he wants to send that version to
the ml as well.

In any case, this patch gets my blessing:

Acked-by: Antonio Quartulli 


-- 
Antonio Quartulli


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


Re: [Openvpn-devel] [PATCH] Fix line number reporting on config file errors after segments

2020-09-21 Thread Antonio Quartulli
Hi,

On 20/09/2020 11:09, Gert Doering wrote:
>  segments neglected to increment the "current line number
> in config file" variable (line_num), so after the first ,
> errors reported have the wrong line number.
> 
> Fix by introducing an extra argument to the check_inline*() /
> read_inline_file() call chain: "so many lines in the inline block".
> 
> Trac: #1325
> 
> Signed-off-by: Gert Doering 
> ---
>  src/openvpn/options.c | 29 -
>  1 file changed, 20 insertions(+), 9 deletions(-)
> 
> diff --git a/src/openvpn/options.c b/src/openvpn/options.c
> index f370a2bb..af517bab 100644
> --- a/src/openvpn/options.c
> +++ b/src/openvpn/options.c
> @@ -4634,7 +4634,8 @@ in_src_get(const struct in_src *is, char *line, const 
> int size)
>  }
>  
>  static char *
> -read_inline_file(struct in_src *is, const char *close_tag, struct gc_arena 
> *gc)
> +read_inline_file(struct in_src *is, const char *close_tag,
> + int *num_lines, struct gc_arena *gc)
>  {
>  char line[OPTION_LINE_SIZE];
>  struct buffer buf = alloc_buf(8*OPTION_LINE_SIZE);
> @@ -4643,6 +4644,7 @@ read_inline_file(struct in_src *is, const char 
> *close_tag, struct gc_arena *gc)
>  
>  while (in_src_get(is, line, sizeof(line)))
>  {
> +(*num_lines)++;
>  char *line_ptr = line;
>  /* Remove leading spaces */
>  while (isspace(*line_ptr))
> @@ -4677,9 +4679,11 @@ read_inline_file(struct in_src *is, const char 
> *close_tag, struct gc_arena *gc)
>  }
>  
>  static bool
> -check_inline_file(struct in_src *is, char *p[], struct gc_arena *gc)
> +check_inline_file(struct in_src *is, char *p[],
> +  int *num_lines, struct gc_arena *gc)

Sorry for not chiming in earlier, but honestly I believe your other
option would be "cleaner". The other option being "return int instead of
bool, where the returned value is the number of lines of the inline'd
material.

If 0, we know there was no inline'd material, thus we can treat that
result as "not inline'd".

Anyway, this is just my opinion. If you believe this is a better
approach, I am fine with it too.

>  {
>  bool is_inline = false;
> +*num_lines = 0;
>  
>  if (p[0] && !p[1])
>  {
> @@ -4692,7 +4696,7 @@ check_inline_file(struct in_src *is, char *p[], struct 
> gc_arena *gc)
>  p[0] = string_alloc(arg + 1, gc);
>  close_tag = alloc_buf(strlen(p[0]) + 4);
>  buf_printf(_tag, "", p[0]);
> -p[1] = read_inline_file(is, BSTR(_tag), gc);
> +p[1] = read_inline_file(is, BSTR(_tag), num_lines, gc);

am I wrong or here we should add 1 to num_lines to include the opening
tag that was parsed *before* entering read_inline_file()?

Regards,




-- 
Antonio Quartulli


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


Re: [Openvpn-devel] [PATCH] Fix combination of --dev tap and --topology subnet across multiple platforms.

2020-09-18 Thread Antonio Quartulli
Hi,

On 14/09/2020 09:08, Gert Doering wrote:
> --topology should have no effect in tap mode (tap is always "subnet"),
> but due to the way options are checked, setting "topology subnet" caught
> an improper branch on all non-linux and non-win32 platforms.
> 
> Easily tested by adding "--topology subnet" to a "--dev tap" t_client
> test.
> 
> Tested, verified, and fixed on FreeBSD 13.3, NetBSD 8.1, OpenBSD 6.5,
> OpenIndiana 2019 (Solaris) and MacOS X Mojave.
> 
> This is a forward-port of commit 6c13e24e5709 - the original intent
> for "master" was to restructure tun.c in a larger way and clean up
> these if() blocks more nicely... which has not happened yet, so this
> patch is basically applying exactly the same changes to context that
> has changed too much for git to be able to do this automatically.
> 
> Trac: #1085
> 
> Signed-off-by: Gert Doering 

This change is very well contained and straightforward.
It just does what it says and makes the if-branches more clear.

Acked-by: Antonio Quartulli 



-- 
Antonio Quartulli


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


Re: [Openvpn-devel] [PATCH v2] If IPv6 pool specification sets pool start to ::0 address, increment.

2020-09-17 Thread Antonio Quartulli
Hi,

On 17/09/2020 10:59, Gert Doering wrote:
> The first IPv6 address in a subnet is not usable (IPv6 anycast address),
> but our pool code ignored this.
> 
> Instead of assigning an unusable address or erroring out, just log the
> fact, and increment the pool start to ::1
> 
> NOTE: this is a bit simplistic.  A pool that is larger than /96 and
> has non-0 bits in the "uppermost bits" will still get the increment
> as we only look at the lowermost 32 bits.
> 
> NOTE2: if the pool is specified with "--server-ipv6 $base/$bits", this
> is a non-issue, as the address for the pool start will be incremented
> anyway.
> 
> v2: make comment more explicit about "we're only talking about the
> host part here" and "base sees only only 32 bit of the host part"
> 
> Reported-by: NicolaF_ in Trac
> Trac: #1282
> 
> Signed-off-by: Gert Doering 

Acked-by: Antonio Quartulli 

-- 
Antonio Quartulli


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


Re: [Openvpn-devel] [PATCH] If IPv6 pool specification sets pool start to ::0 address, increment.

2020-09-17 Thread Antonio Quartulli
Hi,

On 17/09/2020 09:01, Gert Doering wrote:
> We look at "base", which is only the host part, but "at most 32 bits of
> the host part".
> 
> (This is *your* code...!)

(self-shaming dance mode=ON)

Riiight, then drop this comment. The patch looks good, except for the
comment that needs more verbosity.


> 
> gert
> 

-- 
Antonio Quartulli


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


Re: [Openvpn-devel] [PATCH] If IPv6 pool specification sets pool start to ::0 address, increment.

2020-09-17 Thread Antonio Quartulli
Hi,

On 11/09/2020 13:59, Gert Doering wrote:
> The first IPv6 address in a subnet is not usable (IPv6 anycast address),
> but our pool code ignored this.
> 
> Instead of assigning an unusable address or erroring out, just log the
> fact, and increment the pool start to ::1
> 
> NOTE: this is a bit simplistic.  A pool that is larger than /96 and
> has non-0 bits in the "uppermost bits" will still get the increment
> as we only look at the lowermost 32 bits.
> 
> NOTE2: if the pool is specified with "--server-ipv6 $base/$bits", this
> is a non-issue, as the address for the pool start will be incremented
> anyway.
> 
> Reported-by: NicolaF_ in Trac
> Trac: #1282
> 
> Signed-off-by: Gert Doering 
> ---
>  doc/man-sections/server-options.rst |  3 ++-
>  src/openvpn/pool.c  | 15 +++
>  2 files changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/doc/man-sections/server-options.rst 
> b/doc/man-sections/server-options.rst
> index 2009953c..569a 100644
> --- a/doc/man-sections/server-options.rst
> +++ b/doc/man-sections/server-options.rst
> @@ -204,7 +204,8 @@ fast hardware. SSL/TLS authentication must be used in 
> this mode.
>   ifconfig-ipv6-pool ipv6addr/bits
>  
>The pool starts at ``ipv6addr`` and matches the offset determined from
> -  the start of the IPv4 pool.
> +  the start of the IPv4 pool.  If the host part of the given IPv6
> +  address is ``0``, the pool starts at ``ipv6addr`` +1.
>  
>  --ifconfig-pool-persist args
>Persist/unpersist ifconfig-pool data to ``file``, at ``seconds``
> diff --git a/src/openvpn/pool.c b/src/openvpn/pool.c
> index 1f74ac57..2814ff46 100644
> --- a/src/openvpn/pool.c
> +++ b/src/openvpn/pool.c
> @@ -224,6 +224,21 @@ ifconfig_pool_init(const bool ipv4_pool, enum pool_type 
> type, in_addr_t start,
>  }
>  
>  pool->ipv6.base = ipv6_base;
> +
> +/* if a pool starts at ::0, that first IPv6 address is not usable

can we reword a bit this comment? I.e.: "if the starting address of a
pool has the host part all zero, that first "

The "starts at ::0" confused me as if we were targeting pools starting
at [::].


> + * first clients (subnet anycast address).  Start with 1, then.
> + * NOTE: this will also fire for something like
> + *ifconfig-ipv6-pool 2001:db8:0:1:1234::0/64
> + * as we only look at the rightmost 32 bits.  So be it...

wouldn't this test miserably fail when the host part is smaller than 32?
like for a 2001:db8:0:1:1234::0/124?


Regards,

> + */
> +if (base == 0)
> +{
> +msg(D_IFCONFIG_POOL, "IFCONFIG POOL IPv6: incrementing pool 
> start "
> + "to avoid ::0 assignment");
> +    base++;
> +pool->ipv6.base.s6_addr[15]++;
> +}
> +
>  pool_ipv6_size = ipv6_netbits >= 112
>? (1 << (128 - ipv6_netbits)) - base
>: IFCONFIG_POOL_MAX;
> 

-- 
Antonio Quartulli


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


Re: [Openvpn-devel] [PATCH v3] socks.c: fix alen for DOMAIN type addresses, bump up buffer sizes

2020-09-14 Thread Antonio Quartulli
Hi,

On 14/09/2020 11:04, Antonio Quartulli wrote:
> Hi,
> 
> On 09/09/2020 14:22, Gert Doering wrote:
>> When a SOCKS5 server sends back a reply, it encodes an "address",
>> which can be IPv4 (4 bytes), IPv6 (16 bytes) or "a domain name",
>> which has a lenght (1 byte) and "a string of length " - so
>> when copying bytes, we need to hande "length +1" bytes.
>>
>> Our code totally doesn't use this variant of addresses on reception,
>> but since this has been pointed out by "tpw_rules" in Trac, fix it,
>> so if/when someone works on this again, the foundation is correct.
>>
>> While at it, increase buffer size used for sending to handle domain
>> names longer than 122 characters (length was already checked, so a
>> longer name would not overflow but just "not work").
>>
>> v2: increase buf[] len in recv_socks_reply() from 22 to 270 so it
>> is large enough to actually copy a domain name
>>
>> v3: increase buf[] len in establish_socks_proxy_passthru() from 128 to
>> 270, to handle long domain names in queries
>>
>> Reported-By: tpw_rules in Trac
>> Trac: #848
>>
>> Signed-off-by: Gert Doering 
> 
> After a quick discussion on IRC I am fine with this patch, assuming the
> whitespace is added after the '+' operator.
> 
> Further refactoring of this code will be carried on in later patches.

forgot to add:

Acked-by: Antonio Quartulli 

-- 
Antonio Quartulli


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


Re: [Openvpn-devel] [PATCH v3] socks.c: fix alen for DOMAIN type addresses, bump up buffer sizes

2020-09-14 Thread Antonio Quartulli
Hi,

On 09/09/2020 14:22, Gert Doering wrote:
> When a SOCKS5 server sends back a reply, it encodes an "address",
> which can be IPv4 (4 bytes), IPv6 (16 bytes) or "a domain name",
> which has a lenght (1 byte) and "a string of length " - so
> when copying bytes, we need to hande "length +1" bytes.
> 
> Our code totally doesn't use this variant of addresses on reception,
> but since this has been pointed out by "tpw_rules" in Trac, fix it,
> so if/when someone works on this again, the foundation is correct.
> 
> While at it, increase buffer size used for sending to handle domain
> names longer than 122 characters (length was already checked, so a
> longer name would not overflow but just "not work").
> 
> v2: increase buf[] len in recv_socks_reply() from 22 to 270 so it
> is large enough to actually copy a domain name
> 
> v3: increase buf[] len in establish_socks_proxy_passthru() from 128 to
> 270, to handle long domain names in queries
> 
> Reported-By: tpw_rules in Trac
> Trac: #848
> 
> Signed-off-by: Gert Doering 

After a quick discussion on IRC I am fine with this patch, assuming the
whitespace is added after the '+' operator.

Further refactoring of this code will be carried on in later patches.

Regards,

-- 
Antonio Quartulli


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


Re: [Openvpn-devel] [PATCH] If IPv6 pool specification sets pool start to ::0 address, increment.

2020-09-14 Thread Antonio Quartulli
Hi,

On 11/09/2020 13:59, Gert Doering wrote:
> index 1f74ac57..2814ff46 100644
> --- a/src/openvpn/pool.c
> +++ b/src/openvpn/pool.c
> @@ -224,6 +224,21 @@ ifconfig_pool_init(const bool ipv4_pool, enum pool_type 
> type, in_addr_t start,
>  }
>  
>  pool->ipv6.base = ipv6_base;
> +
> +/* if a pool starts at ::0, that first IPv6 address is not usable
> + * first clients (subnet anycast address).  Start with 1, then.
> + * NOTE: this will also fire for something like
> + *ifconfig-ipv6-pool 2001:db8:0:1:1234::0/64
> + * as we only look at the rightmost 32 bits.  So be it...
> + */
> +if (base == 0)

why not memcmp'ing ipv6.base with in6addr_any (defined in netinet/in.h)?
This way you get rid of the first NOTE (unless this header is linux
specific..).

Regards,

-- 
Antonio Quartulli


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


Re: [Openvpn-devel] [PATCH v3] socks.c: fix alen for DOMAIN type addresses, bump up buffer sizes

2020-09-14 Thread Antonio Quartulli
Hi,

On 09/09/2020 14:22, Gert Doering wrote:
> diff --git a/src/openvpn/socks.c b/src/openvpn/socks.c
> index 57f0cee2..d43d84a8 100644
> --- a/src/openvpn/socks.c
> +++ b/src/openvpn/socks.c
> @@ -312,7 +312,7 @@ recv_socks_reply(socket_descriptor_t sd,
>  char atyp = '\0';
>  int alen = 0;
>  int len = 0;
> -char buf[22];
> +char buf[270];   /* 4 + alen(max 256) + 2 */
>  const int timeout_sec = 5;
>  
>  if (addr != NULL)
> @@ -381,7 +381,10 @@ recv_socks_reply(socket_descriptor_t sd,
>  break;
>  
>  case '\x03':/* DOMAINNAME */
> -alen = (unsigned char) c;
> +/* RFC 1928, section 5: 1 byte length,  bytes name,
> + * so the total "address length" is (length+1)
> + */
> +alen = (unsigned char) c +1;

since you are touching this line...how about making it straight?
1) why casting to unsigned char? "alen" is int.
2) please add a space after the '+' operator.

Regards,


-- 
Antonio Quartulli


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


Re: [Openvpn-devel] [PATCH] Fix description of --client-disconnect calling convention in manpage.

2020-09-11 Thread Antonio Quartulli
Hi,

On 09/09/2020 14:29, Gert Doering wrote:
> The man page claimed that --client-disconnect "is passed the same
> pathname as the corresponding --client-connect command", which is
> not what the code does.  Fix.
> 
> Reported-By: hvenev in Trac
> Trac: #884
> 
> Signed-off-by: Gert Doering 

Checked the code and I agree with Gert fix to the documentation.
No additional argument is passed.

Acked-by: Antonio Quartulli 


-- 
Antonio Quartulli


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


Re: [Openvpn-devel] [PATCH] Replace 'echo -n' with 'printf' in tests/t_lpback.sh

2020-09-11 Thread Antonio Quartulli
Hi,

On 09/09/2020 15:00, Gert Doering wrote:
> "echo -n" is inherently less portable than printf, so the tests look
> ugly on (at least) OpenSolaris/Illumos on AIX.
> 
> Add a blank at the end of the tls-crypt-v2 messages, so it has the
> same look as the cipher messages ("... OK").
> 
> Reported-by: mnowak on Trac
> Trac: #1196
> 
> Signed-off-by: Gert Doering 

Change is not invasive and makes sense.

Gert is juggling way more platforms than I do, so if he believes this
helps portability, I am with him.

Acked-by: Antonio Quartulli 


-- 
Antonio Quartulli


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


Re: [Openvpn-devel] [PATCH v3] Fix best gateway selection over netlink

2020-09-10 Thread Antonio Quartulli
Hi,

On 08/09/2020 14:36, Vladislav Grishenko wrote:
> Netlink route request with NLM_F_DUMP flag set means to return
> all entries matching criteria passed in message content -
> matching supplied family & dst address in our case.
> So, gateway from the first ipv4 route was always used.
> 
> On kernels earlier than 2.6.38 default routes are the last ones,
> so arbitrary host/net route w/o gateway is likely be returned as
> first, causing gateway to be invalid or empty.
> After refactoring in 2.6.38 kernel default routes are on top, so
> the problem with older kernels was hidden.
> 
> Fix this behavior by selecting first 0.0.0.0/0 if dst was not set
> or empty. For IPv6, no behavior is changed - request ::/128 route,
> so just clarify the sizes via netlink route api.
> 
> Tested on 5.4.0, 4.1.51, 2.6.36 and 2.6.22 kernels.
> 
> Signed-off-by: Vladislav Grishenko 

Thanks for taking care of this issue and for digging into the sitnl code.

The change is really contained and and easy to review.
Tested a bit and it works as expected.

Acked-by: Antonio Quartulli 

-- 
Antonio Quartulli




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


Re: [Openvpn-devel] [PATCH v3] Fix best gateway selection over netlink

2020-09-08 Thread Antonio Quartulli
Hi,

On 08/09/2020 15:08, David Sommerseth wrote:
> On 08/09/2020 14:36, Vladislav Grishenko wrote:
>> On kernels earlier than 2.6.38 default routes are the last ones,
>> so arbitrary host/net route w/o gateway is likely be returned as
>> first, causing gateway to be invalid or empty.
>> After refactoring in 2.6.38 kernel default routes are on top, so
>> the problem with older kernels was hidden.
> 
> I haven't paid too much attention here, but I don't think I've seen this point
> being brought up.  But do we really care about such old kernels at all?
> 
> AFAIK, RHEL-6 (which goes EOL in November this year and which is not planned
> to be supported in OpenVPN 2.5+) is the only distro carrying such an old
> kernel release (2.6.32 baseline).  Even an internal OpenWRT 19.07 box of mine
> (which should be upgraded, I know!) ships with 4.14.  Unless I'm completely
> clueless (which is a possibility), 2.4 and 2.6 kernels are mostly interesting
> for boards with 4MB flash memory.  And I would suspect such boards with that
> little flash memory to belong to that past.  (And OpenVPN 2.4 is perfectly
> fine too for some time forward anyhow, which should work just fine).

Well, this is an actual flaw in the sitnl code.
It now works, but it may break at some point (it is apparently relying
on a self-made assumption which may not hold true in the future).

For this reason sitnl *needs* fixing.

On the other hand, helping out people that are stuck on old platforms to
upgrade to modern (aka more secure) OpenVPN versions is also not a bad idea.

Since the fix is relatively small, I'd consider it for inclusion.

I'll try to review it ASAP so that it can be considered for going into
the next beta (2.5 compiles with sitnl by default - this is how this
issue was exposed)


Regards,

-- 
Antonio Quartulli


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


Re: [Openvpn-devel] Kernel Acceleration Module

2020-08-31 Thread Antonio Quartulli
Hi Tony,

On 31/08/2020 08:17,  wrote:
> Dear Devs,
> 
> I read that a kernel acceleration module is WIP
> from?0?2https://openvpn.net/openvpn-hackathon-2019/. It's very exciting
> because performance will have much improvement.
> If you don't mind, would you share us some lastest news because it seems
> that no more news is found after Google? Thanks a lot!?0?2
> 

Thanks a lot for your interest.

There has been quite some work on that front pushed by OpenVPN Inc.
We will be sharing more in the next weeks.

Stay tuned :-)

Regards,

-- 
Antonio Quartulli


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


[Openvpn-devel] [PATCH] travis: don't run t_net.sh test

2020-08-10 Thread Antonio Quartulli
Not all travis instances are fit for running t_net.sh test due to
various configurations knob that we have no access to.

Prevent failures by not running t_net.sh on travis at all.
The t_net.sh is executed by other test rigs which we have more control
over.

The test is skipped by specifying RUN_SUDO=false which will make any
pre-test fail, forcing the Makefile to skip that particular test.

Signed-off-by: Antonio Quartulli 
---

Tested on travis:
https://travis-ci.org/github/OpenVPN/openvpn/builds/716605942

 .travis.yml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/.travis.yml b/.travis.yml
index 9e374f55..2d379c70 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -7,6 +7,7 @@ language: c
 env:
   global:
 - JOBS=3
+- RUN_SUDO="false"
 - PREFIX="${HOME}/opt"
 - TAP_WINDOWS_VERSION=9.23.3
 - LZO_VERSION=2.10
-- 
2.28.0



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


[Openvpn-devel] [PATCH v2] t_net.sh: drop hard dependency on t_client.rc

2020-07-21 Thread Antonio Quartulli
Right now t_net.sh depends on t_client.rc in order to source the
RUN_SUDO variable only.
However, t_client.rc is something that a few people only have configured
and thus this would result in t_net.sh almost never executed even if it
just could.

Drop dependency on t_client.rc by falling back to RUN_SUDO=sudo when the
file is missing and no RUN_SUDO is passed via env.

While at it, reword the error message to better match the current logic
flow.

Signed-off-by: Antonio Quartulli 
---

Changes from v1:
* default to sudo when no RUN_SUDO is set externally
* change warning message

 tests/t_net.sh | 26 +++---
 1 file changed, 11 insertions(+), 15 deletions(-)

diff --git a/tests/t_net.sh b/tests/t_net.sh
index c67c3df2..eef3c5d1 100755
--- a/tests/t_net.sh
+++ b/tests/t_net.sh
@@ -76,10 +76,6 @@ if [ -r "${top_builddir}"/t_client.rc ]; then
 . "${top_builddir}"/t_client.rc
 elif [ -r "${srcdir}"/t_client.rc ]; then
 . "${srcdir}"/t_client.rc
-else
-echo "$0: cannot find 't_client.rc' in build dir ('${top_builddir}')" >&2
-echo "$0: or source directory ('${srcdir}'). SKIPPING TEST." >&2
-exit 77
 fi
 
 if [ ! -x "$openvpn" ]; then
@@ -117,18 +113,18 @@ else
 
 if [ -z "$RUN_SUDO" ]
 then
-echo "$0: this test must run be as root, or RUN_SUDO=... " >&2
-echo "  must be set correctly in 't_client.rc'. SKIP." >&2
-exit 77
+echo "$0: no RUN_SUDO=... in t_client.rc or environment, defaulting to 
'sudo'." >&2
+echo "  if that does not work, set RUN_SUDO= correctly for your 
system." >&2
+RUN_SUDO="sudo"
+fi
+
+# check that we can run the unit-test binary with sudo
+   if $RUN_SUDO $UNIT_TEST test
+then
+   echo "$0: $RUN_SUDO $UNIT_TEST succeeded, good."
 else
-# check that we can run the unit-test binary with sudo
-   if $RUN_SUDO $UNIT_TEST test
-then
-   echo "$0: $RUN_SUDO $UNIT_TEST succeeded, good."
-else
-   echo "$0: $RUN_SUDO $UNIT_TEST failed, cannot go on. SKIP." >&2
-   exit 77
-fi
+   echo "$0: $RUN_SUDO $UNIT_TEST failed, cannot go on. SKIP." >&2
+   exit 77
 fi
 fi
 
-- 
2.27.0



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


Re: [Openvpn-devel] [PATCH 1/9 v3] Indicate that a client is in pull mode in IV_PROTO

2020-07-21 Thread Antonio Quartulli
Hi,

On 21/07/2020 18:38, Arne Schwabe wrote:
> This allows us to skip waiting for the first PUSH_REQUEST message from
> the client to send the response.
> 
> This changes the interpretation of IV_PROTO from a scalar to a bitfield
> Since we only have IV_PROTO=2 defined so far and will support DATA_V2
> this should not make any problem. This avoid adding another IV_xxx variable
> that takes valuable space in the protocol frame.
> 
> Signed-off-by: Arne Schwabe 
> 
> Patch V2: Use bitmask for IV_PROTO_DATA_V2 and add more documentation.
> 
> Patch V3: Rewrite IV_PROTO paragraph in man page, incoperate spelling fixes
>   by tincanteksup.
> 
> Signed-off-by: Arne Schwabe 


Thanks a lot for all the fixes. Looks good to me now.

-- 
Antonio Quartulli


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


Re: [Openvpn-devel] [PATCH] t_net.sh: drop hard dependency on t_client.rc

2020-07-21 Thread Antonio Quartulli
Hi,

On 21/07/2020 18:39, Gert Doering wrote:
> In here, print & set
> 
>   if [ -z "$RUN_SUDO" ]
>   then
>  +echo "$0: no RUN_SUDO=... in t_client.rc or environment, defaulting 
> to 'sudo'." >&2
>  +echo "  if that does not work, set RUN_SUDO= correctly for your 
> system." >&2
>  +RUN_SUDO=sudo"
>   fi
> 
> done - less code, message conveyed if needed.
>   


hmhmhmh makes sense. v2 incoming!

-- 
Antonio Quartulli


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


Re: [Openvpn-devel] [PATCH v6 4/9] Implement tls-groups option to specify eliptic curves/groups

2020-07-21 Thread Antonio Quartulli
Hi,

On 21/07/2020 17:49, Arne Schwabe wrote:
> By default OpenSSL 1.1+ only allows signatures and ecdh/ecdhx from the
> default list of X25519:secp256r1:X448:secp521r1:secp384r1. In
> TLS1.3 key exchange is independent from the signature/key of the
> certificates, so allowing all groups per default is not a sensible
> choice anymore and instead a shorter list is reasonable.
> 
> However, when using certificates with exotic curves that are not on
> the group list, the signatures of these certificates will no longer
> be accepted.
> 
> The tls-groups  option allows to modify the group list to account
> for these corner cases.
> 
> Patch V2: Uses local gc_arena instead of malloc/free, reword commit
>   message. Fix other typos/clarify messages
> 
> Patch V3: Style fixes, adjust code to changes from mbed tls session
>   fix
> 
> Patch V5: Fix compilation with OpenSSL 1.0.2
> 
> Patch V6: Redo the 'while((token = strsep(_groups, ":"))' change
>   that accidently got lost.
> 
> Signed-off-by: Arne Schwabe 


Much better now.

Acked-by: Antonio Quartulli 




-- 
Antonio Quartulli


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


Re: [Openvpn-devel] [PATCH v5 4/9] Implement tls-groups option to specify eliptic curves/groups

2020-07-21 Thread Antonio Quartulli



On 21/07/2020 15:46, Antonio Quartulli wrote:
> Aren't we calling strsep() twice in a row now?
> Once in the while() condition and once at the end of the cycle?
> 
> I think Arne agreed on the issue on IRC, but maybe forgot to fix the patch?
> 
> 

However, please note that now the patch compiles on my gitlab-ci test bench.

-- 
Antonio Quartulli


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


Re: [Openvpn-devel] [PATCH v5 4/9] Implement tls-groups option to specify eliptic curves/groups

2020-07-21 Thread Antonio Quartulli
Hi,

I think a comment in my previous review was overlooked.

On 17/07/2020 15:47, Arne Schwabe wrote:
> @@ -343,6 +348,42 @@ tls_ctx_set_cert_profile(struct tls_root_ctx *ctx, const 
> char *profile)
>  }
>  }
>  
> +void
> +tls_ctx_set_tls_groups(struct tls_root_ctx *ctx, const char *groups)
> +{
> +ASSERT(ctx);
> +struct gc_arena gc = gc_new();
> +
> +/* Get number of groups and allocate an array in ctx */
> +int groups_count = get_num_elements(groups, ':');
> +ALLOC_ARRAY_CLEAR(ctx->groups, mbedtls_ecp_group_id, groups_count + 1)
> +
> +/* Parse allowed ciphers, getting IDs */
> +int i = 0;
> +char *tmp_groups = string_alloc(groups, );
> +
> +const char *token;
> +while ((token = strsep(_groups, ":")))
> +{
> +const mbedtls_ecp_curve_info *ci =
> +mbedtls_ecp_curve_info_from_name(token);
> +if (!ci)
> +{
> +msg(M_WARN, "Warning unknown curve/group specified: %s", token);
> +}
> +else
> +{
> +ctx->groups[i] = ci->grp_id;
> +i++;
> +}
> +token = strsep(_groups, ":");

Aren't we calling strsep() twice in a row now?
Once in the while() condition and once at the end of the cycle?

I think Arne agreed on the issue on IRC, but maybe forgot to fix the patch?



Regards,


-- 
Antonio Quartulli


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


Re: [Openvpn-devel] [PATCH v2 1/9] Indicate that a client is in pull mode in IV_PROTO

2020-07-20 Thread Antonio Quartulli
Hi,

On 20/07/2020 11:17, Arne Schwabe wrote:
> This allows us to skip waiting for the first PUSH_REQUEST message from
> the client to send the response.
> 
> This changes the interpretation of IV_PROTO from a scalar to a bitfield
> Since we only have IV_PROTO=2 defined so far and will support DATA_V2
> this should not make any problem. This avoid adding another IV_xxx variable
> that takes valuable space in the protocol frame.
> 
> Signed-off-by: Arne Schwabe 
> 
> Patch V2: Use bitmask for IV_PROTO_DATA_V2 and add more documentation.
> 
> Signed-off-by: Arne Schwabe 
> ---
>  doc/man-sections/server-options.rst |  6 +-
>  src/openvpn/multi.c | 12 ++--
>  src/openvpn/ssl.c   | 15 +--
>  src/openvpn/ssl.h   | 16 
>  4 files changed, 44 insertions(+), 5 deletions(-)
> 
> diff --git a/doc/man-sections/server-options.rst 
> b/doc/man-sections/server-options.rst
> index c8e9fc61..e24cb135 100644
> --- a/doc/man-sections/server-options.rst
> +++ b/doc/man-sections/server-options.rst
> @@ -464,9 +464,13 @@ fast hardware. SSL/TLS authentication must be used in 
> this mode.
>:code:`IV_LZ4=1`
>  If the client supports LZ4 compressions.
>  
> -  :code:`IV_PROTO=2`
> +  :code:`IV_PROTO=2` (bit 2)
>  If the client supports peer-id floating mechanism
>  
> +  :code:`IV_PROTO=4` (bit 3)
> +When the client expects a push-reply and the server may
> +send this reply without waiting for a push-request first.
> +


Maybe it's just me, but documenting a bitfield this way is a bit
counter-intuitive. From the doc it seems to me that IV_PROTO can either
have value "2" or value "4", not a combination of the two.

Maybe it's better to have something like:

IV_PROTO
- bit 2: 
- bit 3: .

So it's clear that both bits can be used when needed?
An alternative would be to explicitly mention that both values/bits can
be used at the same time.

Opinions?



cheers,


>:code:`IV_NCP=2`
>  Negotiable ciphers, client supports ``--cipher`` pushed by
>  the server, a value of 2 or greater indicates client supports
> diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
> index d9456f34..cb950db5 100644
> --- a/src/openvpn/multi.c
> +++ b/src/openvpn/multi.c
> @@ -1792,10 +1792,18 @@ multi_client_set_protocol_options(struct context *c)
>  {
>  int proto = 0;
>  int r = sscanf(optstr, "IV_PROTO=%d", );
> -if ((r == 1) && (proto >= 2))
> +if (r == 1)
>  {
> -tls_multi->use_peer_id = true;
> +if (proto & IV_PROTO_DATA_V2)
> +{
> +tls_multi->use_peer_id = true;
> +}
> +if (proto & IV_PROTO_REQUEST_PUSH)
> +{
> +c->c2.push_request_received = true;
> +}
>  }
> +
>  }
>  
>  /* Select cipher if client supports Negotiable Crypto Parameters */
> diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
> index 54a23011..04d78a81 100644
> --- a/src/openvpn/ssl.c
> +++ b/src/openvpn/ssl.c
> @@ -2299,8 +2299,19 @@ push_peer_info(struct buffer *buf, struct tls_session 
> *session)
>  buf_printf(, "IV_PLAT=win\n");
>  #endif
>  
> -/* support for P_DATA_V2 */
> -buf_printf(, "IV_PROTO=2\n");
> +/* support for P_DATA_V2*/
> +int iv_proto = IV_PROTO_DATA_V2;
> +
> +/* support for receiving push_reply before sending
> + * push request, also signal that the client wants
> + * to get push-reply messages without without requiring a round
> + * trip for a push request message*/
> +if(session->opt->pull)
> +{
> +iv_proto |= IV_PROTO_REQUEST_PUSH;
> +}
> +
> +buf_printf(, "IV_PROTO=%d\n", iv_proto);
>  
>  /* support for Negotiable Crypto Parameters */
>  if (session->opt->ncp_enabled
> diff --git a/src/openvpn/ssl.h b/src/openvpn/ssl.h
> index 58a9b0d4..81f8810e 100644
> --- a/src/openvpn/ssl.h
> +++ b/src/openvpn/ssl.h
> @@ -99,6 +99,22 @@
>  /* Maximum length of OCC options string passed as part of auth handshake */
>  #define TLS_OPTIONS_LEN 512
>  
> +/* Definitions of the bits in the IV_PROTO bitfield
> + *
> + * In older OpenVPN versions this used as comparision
> + * IV_PROTO >= 2 to determine if DATA_V2 is supported.
> + * Therefore any client announcing any of the flags must
> + * also announce IV_PROTO_DATA_V2. We also treat bit 0
> + * as reserved for this reason */
> +

[Openvpn-devel] [PATCH] options: don't leak inline'd key material in logfile

2020-07-17 Thread Antonio Quartulli
With the conversion of the introduction of a bool variable to signal
when a certain string is a filename or the actual (inline'd) key
material, the SHOW_STR() macro is now leaking the inline'd material to
the log file.

This happens because SHOW_STR will just print the content of the passed
argument without any check. With the new logic this should not happen
anymore.

A new macro SHOW_STR_INLINE() is therefore introduced which will check
the appropriate bool member before deciding to print the actual string
content or not.

Trac: #1304
Reported-by: Richard Bonhomme 
Signed-off-by: Antonio Quartulli 
---
 src/openvpn/options.c | 26 +++---
 1 file changed, 15 insertions(+), 11 deletions(-)

diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index b6b8d769..8e9d845a 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -973,6 +973,10 @@ pull_filter_type_name(int type)
 
 #define SHOW_PARM(name, value, format) msg(D_SHOW_PARMS, "  " #name " = " 
format, (value))
 #define SHOW_STR(var)   SHOW_PARM(var, (o->var ? o->var : "[UNDEF]"), 
"'%s'")
+#define SHOW_STR_INLINE(var)SHOW_PARM(var, \
+  o->var ## _inline ? "[INLINE]" : \
+  (o->var ? o->var : "[UNDEF]"), \
+  "'%s'")
 #define SHOW_INT(var)   SHOW_PARM(var, o->var, "%d")
 #define SHOW_UINT(var)  SHOW_PARM(var, o->var, "%u")
 #define SHOW_UNSIGNED(var)  SHOW_PARM(var, o->var, "0x%08x")
@@ -1322,7 +1326,7 @@ show_p2mp_parms(const struct options *o)
 SHOW_BOOL(auth_user_pass_verify_script_via_file);
 SHOW_BOOL(auth_token_generate);
 SHOW_INT(auth_token_lifetime);
-SHOW_STR(auth_token_secret_file);
+SHOW_STR_INLINE(auth_token_secret_file);
 #if PORT_SHARE
 SHOW_STR(port_share_host);
 SHOW_STR(port_share_port);
@@ -1494,11 +1498,11 @@ show_connection_entry(const struct connection_entry *o)
 SHOW_INT(explicit_exit_notification);
 #endif
 
-SHOW_STR(tls_auth_file);
+SHOW_STR_INLINE(tls_auth_file);
 SHOW_PARM(key_direction, keydirection2ascii(o->key_direction, false, true),
   "%s");
-SHOW_STR(tls_crypt_file);
-SHOW_STR(tls_crypt_v2_file);
+SHOW_STR_INLINE(tls_crypt_file);
+SHOW_STR_INLINE(tls_crypt_v2_file);
 }
 
 
@@ -1697,7 +1701,7 @@ show_settings(const struct options *o)
 }
 #endif
 
-SHOW_STR(shared_secret_file);
+SHOW_STR_INLINE(shared_secret_file);
 SHOW_PARM(key_direction, keydirection2ascii(o->key_direction, false, 
true), "%s");
 SHOW_STR(ciphername);
 SHOW_BOOL(ncp_enabled);
@@ -1722,7 +1726,7 @@ show_settings(const struct options *o)
 SHOW_BOOL(tls_server);
 SHOW_BOOL(tls_client);
 SHOW_INT(key_method);
-SHOW_STR(ca_file);
+SHOW_STR_INLINE(ca_file);
 SHOW_STR(ca_path);
 SHOW_STR(dh_file);
 #ifdef ENABLE_MANAGEMENT
@@ -1732,8 +1736,8 @@ show_settings(const struct options *o)
 }
 else
 #endif
-SHOW_STR(cert_file);
-SHOW_STR(extra_certs_file);
+SHOW_STR_INLINE(cert_file);
+SHOW_STR_INLINE(extra_certs_file);
 
 #ifdef ENABLE_MANAGEMENT
 if ((o->management_flags & MF_EXTERNAL_KEY))
@@ -1742,9 +1746,9 @@ show_settings(const struct options *o)
 }
 else
 #endif
-SHOW_STR(priv_key_file);
+SHOW_STR_INLINE(priv_key_file);
 #ifndef ENABLE_CRYPTO_MBEDTLS
-SHOW_STR(pkcs12_file);
+SHOW_STR_INLINE(pkcs12_file);
 #endif
 #ifdef ENABLE_CRYPTOAPI
 SHOW_STR(cryptoapi_cert);
@@ -1756,7 +1760,7 @@ show_settings(const struct options *o)
 SHOW_STR(tls_export_cert);
 SHOW_INT(verify_x509_type);
 SHOW_STR(verify_x509_name);
-SHOW_STR(crl_file);
+SHOW_STR_INLINE(crl_file);
 SHOW_INT(ns_cert_type);
 {
 int i;
-- 
2.27.0



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


[Openvpn-devel] [PATCH] t_net.sh: drop hard dependency on t_client.rc

2020-07-17 Thread Antonio Quartulli
Right now t_net.sh depends on t_client.rc in order to source the
RUN_SUDO variable only.
However, t_client.rc is something that a few people only have configured
and thus this would result in t_net.sh almost never executed even if it
just could.

Drop dependency on t_client.rc by falling back to RUN_SUDO=sudo when the
file is missing.

The assignment is made as conditional so that a user can still override
RUN_SUDO by speciying an alternate string on the command line.

While at it, reword the error message to better match the current logic
flow.

Signed-off-by: Antonio Quartulli 
---
 tests/t_net.sh | 7 ++-
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/tests/t_net.sh b/tests/t_net.sh
index c67c3df2..63732db9 100755
--- a/tests/t_net.sh
+++ b/tests/t_net.sh
@@ -77,9 +77,7 @@ if [ -r "${top_builddir}"/t_client.rc ]; then
 elif [ -r "${srcdir}"/t_client.rc ]; then
 . "${srcdir}"/t_client.rc
 else
-echo "$0: cannot find 't_client.rc' in build dir ('${top_builddir}')" >&2
-echo "$0: or source directory ('${srcdir}'). SKIPPING TEST." >&2
-exit 77
+RUN_SUDO="${RUN_SUDO:-sudo}"
 fi
 
 if [ ! -x "$openvpn" ]; then
@@ -117,8 +115,7 @@ else
 
 if [ -z "$RUN_SUDO" ]
 then
-echo "$0: this test must run be as root, or RUN_SUDO=... " >&2
-echo "  must be set correctly in 't_client.rc'. SKIP." >&2
+echo "$0: using t_client.rc, but RUN_SUDO=... is not defined 
correctly. SKIP. " >&2
 exit 77
 else
 # check that we can run the unit-test binary with sudo
-- 
2.27.0



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


Re: [Openvpn-devel] [PATCH 1/9] Indicate that a client is in pull mode in IV_PROTO

2020-07-17 Thread Antonio Quartulli
Hi,

On 17/07/2020 15:47, Arne Schwabe wrote:
> This allows us to skip waiting for the first PUSH_REQUEST message from
> the client to send the response.
> 
> Signed-off-by: Arne Schwabe 
> ---
>  src/openvpn/multi.c | 12 ++--
>  src/openvpn/ssl.c   | 15 +--
>  src/openvpn/ssl.h   |  7 +++
>  3 files changed, 30 insertions(+), 4 deletions(-)
> 
> diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
> index 0095c871..88ba9db2 100644
> --- a/src/openvpn/multi.c
> +++ b/src/openvpn/multi.c
> @@ -1795,10 +1795,18 @@ multi_client_set_protocol_options(struct context *c)
>  {
>  int proto = 0;
>  int r = sscanf(optstr, "IV_PROTO=%d", );
> -if ((r == 1) && (proto >= 2))
> +if (r == 1)
>  {
> -tls_multi->use_peer_id = true;
> +if (proto >= 2)


I thought it was agreed (but I may be wrong) to substitute this check
with a bitwise AND, since this variable is now treated as a bitfield.

Regards,




> +{
> +tls_multi->use_peer_id = true;
> +}
> +if (proto & IV_PROTO_REQUEST_PUSH)
> +{
> +c->c2.push_request_received = true;
> +}
>  }
> +
>  }
>  
>  /* Select cipher if client supports Negotiable Crypto Parameters */
> diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
> index 54a23011..04d78a81 100644
> --- a/src/openvpn/ssl.c
> +++ b/src/openvpn/ssl.c
> @@ -2299,8 +2299,19 @@ push_peer_info(struct buffer *buf, struct tls_session 
> *session)
>  buf_printf(, "IV_PLAT=win\n");
>  #endif
>  
> -/* support for P_DATA_V2 */
> -buf_printf(, "IV_PROTO=2\n");
> +/* support for P_DATA_V2*/
> +int iv_proto = IV_PROTO_DATA_V2;
> +
> +/* support for receiving push_reply before sending
> + * push request, also signal that the client wants
> + * to get push-reply messages without without requiring a round
> + * trip for a push request message*/
> +if(session->opt->pull)
> +{
> +iv_proto |= IV_PROTO_REQUEST_PUSH;
> +}
> +
> +buf_printf(, "IV_PROTO=%d\n", iv_proto);
>  
>  /* support for Negotiable Crypto Parameters */
>  if (session->opt->ncp_enabled
> diff --git a/src/openvpn/ssl.h b/src/openvpn/ssl.h
> index 58a9b0d4..86fb6853 100644
> --- a/src/openvpn/ssl.h
> +++ b/src/openvpn/ssl.h
> @@ -99,6 +99,13 @@
>  /* Maximum length of OCC options string passed as part of auth handshake */
>  #define TLS_OPTIONS_LEN 512
>  
> +/* Definitions of the bits in the IV_PROTO bitfield */
> +#define IV_PROTO_DATA_V2(1<<1)  /**< Support P_DATA_V2 */
> +#define IV_PROTO_REQUEST_PUSH   (1<<2)  /**< Assume client will send a push
> +      * request and server does not need
> +  * to wait for a push-request to 
> send
> +  * a push-reply */
> +
>  /* Default field in X509 to be username */
>  #define X509_USERNAME_FIELD_DEFAULT "CN"
>  
> 

-- 
Antonio Quartulli


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


Re: [Openvpn-devel] [PATCH v7 2/6] client-connect: Add deferred support to the client-connect script handler

2020-07-17 Thread Antonio Quartulli



On 16/07/2020 15:43, Arne Schwabe wrote:
> From: Fabian Knittel 
> 
> This patch introduces the concept of a return value file for the 
> client-connect
> handlers. (This is very similar to the auth value file used during deferred
> authentication.)  The file name is stored in the client_connect_state struct.
> 
> In addition, the patch also allows the storage of the client config file name
> in struct client_connect_state.
> 
> Both changes are used by the client-connect script handler to support deferred
> client-connection handling.  The deferred return value file 
> (deferred_ret_file)
> is passed to the actual script via the environment.  If the script succeeds 
> and
> writes the value for deferral into the deferred_ret_file, the handler knows to
> indicate deferral.  Later on, the deferred handler checks whether the value of
> the deferred_ret_file has been updated to success or failure.
> 
> Signed-off-by: Fabian Knittel 
> Signed-off-by: Arne Schwabe 
> ---
>  src/openvpn/multi.c | 226 +---
>  src/openvpn/multi.h |  12 +++
>  2 files changed, 225 insertions(+), 13 deletions(-)
> 
> diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
> index 9128798d..e26daeea 100644
> --- a/src/openvpn/multi.c
> +++ b/src/openvpn/multi.c
> @@ -1854,6 +1854,168 @@ multi_client_set_protocol_options(struct context *c)
>  }
>  }
>  
> +/**
> + * Delete the temporary file for the return value of client connect
> + * It also removes it from it from client_connect_defer_state and
> + * environment
> + */
> +static void
> +ccs_delete_deferred_ret_file(struct multi_instance *mi)
> +{
> +struct client_connect_defer_state *ccs = 
> &(mi->client_connect_defer_state);
> +if (ccs->deferred_ret_file)
> +{
> +setenv_del(mi->context.c2.es, "client_connect_deferred_file");
> +if (!platform_unlink(ccs->deferred_ret_file))
> +{
> +msg(D_MULTI_ERRORS, "MULTI: problem deleting temporary file: %s",
> +ccs->deferred_ret_file);
> +}
> +free(ccs->deferred_ret_file);
> +ccs->deferred_ret_file = NULL;
> +}

As discussed on IRC, previous patches (already applied) changed the
style of many of our functions from:


if (x)
{
 do something
}



to:



if (!x)
{
  return
}


Now this patch is unfortunately introducing a set of functions all
implemented using the old pattern.

I suggest them to be converted before we merge this patch.

I don't really have other comments about this patch, therefore it may go
in once we get the code style right.


Regards,

-- 
Antonio Quartulli


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


Re: [Openvpn-devel] [PATCH v7 1/6] client-connect: Add CC_RET_DEFERRED and cope with deferred client-connect

2020-07-16 Thread Antonio Quartulli
Hi,

On 16/07/2020 15:43, Arne Schwabe wrote:
> This patch moves the state, that was previously tracked within the
> multi_connection_established() function, into struct client_connect_state.  
> The
> multi_connection_established() function can now be exited and re-entered as
> many times as necessary - without losing the client-connect handling state.
> 
> The patch also adds the new return value CC_RET_DEFERRED which indicates that
> the handler couldn't complete immediately, and needs to be called later.  At
> that point multi_connection_established() will exit without indicating
> completion.
> 
> Each client-connect handler now has an (optional) additional call-back:  The
> call-back for handling the deferred case.  If the main call-back returns
> CC_RET_DEFERRED, the next call to the handler will be through the deferred
> call-back.
> 
> Signed-off-by: Fabian Knittel 
> 
> Patch V3: Use a static struct in multi_instance instead of using
>   malloc/free and use two states (deffered with and without
>   result) instead of one to eliminate the counter that was
>   only tested for > 0.
> 
> Patch V5: Use new states in context_auth instead of the extra state
>   that the patch series previously used.
> 
> Patch V6: Restructure code to make it a bit more readable, rebase on
>   master.
> 
> Patch V7: move defferred bool into client connect handler calls, switch
>   to switch case
> 
> Signed-off-by: Arne Schwabe 


Haven't done a full test, this is why we have "Gert and his rig"[tm],
but the code looks good and I can't spot anything that may trigger my
personal alarm.

This version is much cleaner that what it looked like before. Thanks
Arne for reworking the patch once again.

Acked-by: Antonio Quartulli 

-- 
Antonio Quartulli


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


Re: [Openvpn-devel] [PATCH v6 8/14] client-connect: Add CC_RET_DEFERRED and cope with deferred client-connect

2020-07-15 Thread Antonio Quartulli
Hi,

On 15/07/2020 16:16, Arne Schwabe wrote:
[CUT]

> -multi_client_connect_early_setup(m, mi);
> +handler = _connect_handlers[defer_state->cur_handler_index];
>  
> -for (int i = 0; cc_succeeded && handlers[i]; i++)
> +while (cc_succeeded && handler->main != NULL)
>  {
> -ret = handlers[i](m, mi, _types_found);
> -cc_succeeded = cc_check_return(_succeeded_count, ret);
> +enum client_connect_return ret;
> +if (from_deferred)
> +{
> +ret = handler->deferred(m, mi, 
> &(defer_state->option_types_found));
> +from_deferred = false;
> +}
> +else
> +{
> +ret = handler->main(m, mi, &(defer_state->option_types_found));
> +}
> +
> +if (ret == CC_RET_SUCCEEDED)
> +{
> +/*
> + * Remember that we already had at least one handler
> + * returning a result should go to into deferred state
> + */
> +mi->context.c2.context_auth = CAS_PENDING_DEFERRED_PARTIAL;
> +}
> +else if (ret == CC_RET_SKIPPED)
> +{
> +/*
> + * Move on with the next handler without modifying any
> + * other state
> + */
> +}
> +else if (ret == CC_RET_DEFERRED)
> +{
> +/*
> + * we already set client_connect_status to DEFERRED_RESULT or
> + * DEFERRED_NO_RESULT and increased index. We just return
> + * from the function as having client_connect_status
> + */
> +return;
> +}
> +else if (ret == CC_RET_FAILED)
> +{
> +/*
> + * One handler failed. We abort the chain and set the final
> + * result to failed
> + */
> +cc_succeeded = false;
> +}
> +else
> +{
> +ASSERT(0);
> +    }
> +
> +    handler = _connect_handlers[defer_state->cur_handler_index];
> +(defer_state->cur_handler_index)++;

shouldn't these 2 lines above be inverted?
(these parenthesis)


Cheers,





-- 
Antonio Quartulli


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


Re: [Openvpn-devel] [PATCH] reformat multi_client_generate_tls_keys according to uncrustify

2020-07-15 Thread Antonio Quartulli
Hi,

On 15/07/2020 16:14, Arne Schwabe wrote:
> The refactor accidently used a wrong code style template and
> ended up using 2 instead of 4 as indent.
> 
> Signed-off-by: Arne Schwabe 

Acked-by: Antonio Quartulli 

-- 
Antonio Quartulli


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


Re: [Openvpn-devel] [PATCH v5 10/14] client-connect: Move adding inotify watch into its own function

2020-07-15 Thread Antonio Quartulli
Hi,

On 11/07/2020 11:36, Arne Schwabe wrote:
> This make the code a bit better readable and also prepares resuing
> the function for client-connect return files
> 
> Signed-off-by: Arne Schwabe 


This patch looks good and does what it says. No functional change is
implemented, but it's all about moving a chunk of code in its own
function and indenting it better than how it was.

Acked-by: Antonio Quartulli 

-- 
Antonio Quartulli


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


Re: [Openvpn-devel] [PATCH v5 09/14] client-connect: Add deferred support to the client-connect script handler

2020-07-15 Thread Antonio Quartulli
  if (ret != CC_RET_DEFERRED)
>  {
> -msg(D_MULTI_ERRORS, "MULTI: problem deleting temporary file: %s",
> -dc_file);
> +ccs_delete_config_file(mi);
> +ccs_delete_deferred_ret_file(mi);
>  }
> -cleanup:
>  argv_free();
>  gc_free();
>  }
>  return ret;
>  }
>  
> +static enum client_connect_return
> +multi_client_connect_script_deferred(struct multi_context *m,
> + struct multi_instance *mi,
> + unsigned int *option_types_found)
> +{
> +ASSERT(mi);
> +ASSERT(option_types_found);
> +struct client_connect_defer_state *ccs = 
> &(mi->client_connect_defer_state);

parenthesis are not needed

> +enum client_connect_return ret = CC_RET_SKIPPED;
> +
> +ret = ccs_test_deferred_ret_file(mi);
> +
> +if (ret == CC_RET_SKIPPED)
> +{
> +/*
> + * Skipped and deferred are equivalent in this context.
> + * skipped means that the called program has not yet
> + * written a return status implicitly needing more time
> + * while deferred is the explicit notifcation that it

typ0: notification

> + * needs more time
> + */
> +ret = CC_RET_DEFERRED;

The comment above made me wonder..What happens if he client-connect
script will never write to file due to a bug or a crash or anything?



> +}
> +
> +if (ret != CC_RET_DEFERRED)
> +{
> +ccs_delete_deferred_ret_file(mi);
> +multi_client_connect_post(m, mi, ccs->config_file,
> +  option_types_found);
> +ccs_delete_config_file(mi);
> +}
> +return ret;
> +}
> +
>  /**
>   * Generates the data channel keys
>   */
> @@ -2251,7 +2451,7 @@ static const struct client_connect_handlers 
> client_connect_handlers[] = {
>  },
>  {
>  .main = multi_client_connect_call_script,
> -.deferred = multi_client_connect_fail
> +.deferred = multi_client_connect_script_deferred
>  },
>  {
>  .main = multi_client_connect_mda,
> diff --git a/src/openvpn/multi.h b/src/openvpn/multi.h
> index 11da0209..3ebf6b9f 100644
> --- a/src/openvpn/multi.h
> +++ b/src/openvpn/multi.h
> @@ -73,6 +73,18 @@ struct client_connect_defer_state
>  /* Remember which option classes where processed for delayed option
>   * handling. */
>  unsigned int option_types_found;
> +
> +/**
> + * The temporrary file name that contains the return status of the

typ0: temporary

> + * client-connect script if it exits with defer as status
> + */
> +char *deferred_ret_file;
> +
> +/**
> + * The temporary file name that contains the config directives
> + * returned by the client-connect script
> + */
> +char *config_file;
>  };
>  
>  /**
> 


The patch looks good. My only concern is reported above and is not
really about this patch, but about the way the deferred handler is designed.

-- 
Antonio Quartulli


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


Re: [Openvpn-devel] [PATCH v5 08/14] client-connect: Add CC_RET_DEFERRED and cope with deferred client-connect

2020-07-14 Thread Antonio Quartulli
   if (mi->context.c2.context_auth == CAS_PENDING
> +if (is_cas_pending(mi->context.c2.context_auth)
>  && CONNECTION_ESTABLISHED(>context))
>  {
>  multi_connection_established(m, mi);
> @@ -3559,7 +3642,7 @@ management_client_auth(void *arg,
>  {
>  if (auth)
>  {
> -if (mi->context.c2.context_auth == CAS_PENDING)
> +if (is_cas_pending(mi->context.c2.context_auth))
>  {
>  set_cc_config(mi, cc_config);
>  cc_config_owned = false;
> @@ -3571,7 +3654,7 @@ management_client_auth(void *arg,
>  {
>  msg(D_MULTI_LOW, "MULTI: connection rejected: %s, 
> CLI:%s", reason, np(client_reason));
>  }
> -if (mi->context.c2.context_auth != CAS_PENDING)
> +if (!is_cas_pending(mi->context.c2.context_auth))
>  {
>  send_auth_failed(>context, client_reason); /* 
> mid-session reauth failed */
>  multi_schedule_context_wakeup(m, mi);
> diff --git a/src/openvpn/multi.h b/src/openvpn/multi.h
> index 1d30dcc6..11da0209 100644
> --- a/src/openvpn/multi.h
> +++ b/src/openvpn/multi.h
> @@ -62,6 +62,18 @@ struct deferred_signal_schedule_entry
>  struct timeval wakeup;
>  };
>  
> +/**
> + * Detached client connection state.  This is the state that is tracked while
> + * the client connect hooks are executed.
> + */
> +struct client_connect_defer_state
> +{
> +/* Index of currently executed handler.  */
> +int cur_handler_index;
> +/* Remember which option classes where processed for delayed option
> + * handling. */
> +unsigned int option_types_found;
> +};
>  
>  /**
>   * Server-mode state structure for one single VPN tunnel.
> @@ -108,7 +120,7 @@ struct multi_instance {
>  
>  struct context context; /**< The context structure storing state
>   *   for this VPN tunnel. */
> -
> +struct client_connect_defer_state client_connect_defer_state;
>  #ifdef ENABLE_ASYNC_PUSH
>  int inotify_watch; /* watch descriptor for acf */
>  #endif
> @@ -195,6 +207,7 @@ enum client_connect_return
>  {
>  CC_RET_FAILED,
>  CC_RET_SUCCEEDED,
> +CC_RET_DEFERRED,
>  CC_RET_SKIPPED
>  };
>  
> diff --git a/src/openvpn/openvpn.h b/src/openvpn/openvpn.h
> index 7c469b01..ccc7f118 100644
> --- a/src/openvpn/openvpn.h
> +++ b/src/openvpn/openvpn.h
> @@ -217,6 +217,8 @@ struct context_1
>  enum client_connect_status {
>  CAS_SUCCEEDED=0,
>  CAS_PENDING,
> +CAS_PENDING_DEFERRED,
> +CAS_PENDING_DEFERRED_PARTIAL,   /**< at least handler succeeded, no 
> result yet*/
>  CAS_FAILED,
>  CAS_PARTIAL,/**< Variant of CAS_FAILED: at least one
>   * client-connect script/plugin succeeded
> @@ -225,6 +227,13 @@ enum client_connect_status {
>   */
>  };
>  
> +static inline bool
> +is_cas_pending(enum client_connect_status cas)
> +{
> +return cas == CAS_PENDING || cas == CAS_PENDING_DEFERRED
> +   || cas == CAS_PENDING_DEFERRED_PARTIAL;
> +}
> +
>  /**
>   * Level 2 %context containing state that is reset on both \c SIGHUP and
>   * \c SIGUSR1 restarts.
> 


Other than my stylistic comments the patch looks good and does what it
says. IMHO the code is not the prettiest ever, but it gets difficult to
suggest something different without an overhaul of the existing code.

Acked-by: Antonio Quartulli 

-- 
Antonio Quartulli


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


Re: [Openvpn-devel] [PATCH v5 07/14] client-connect: Change cas_context from int to enum

2020-07-14 Thread Antonio Quartulli
Hi,

On 11/07/2020 11:36, Arne Schwabe wrote:
> This deviates from Fabian's original patch that relied on the now
> removed connection_established bool as pointer being NULL or non NULL as
> implicit third state and makeing connection_established as a substate of
> (cas_context == CAS_PENDING)
> 
> Signed-off-by: Arne Schwabe 
> 
> Patch V5: extend cas_context with two new states instead adding an
>   extra mini state machine.
> 
> Signed-off-by: Arne Schwabe 
> ---
>  src/openvpn/multi.c   |  2 +-
>  src/openvpn/multi.h   |  1 +
>  src/openvpn/openvpn.h | 24 +---
>  3 files changed, 19 insertions(+), 8 deletions(-)
> 
> diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
> index 83848fdc..f9b8af80 100644
> --- a/src/openvpn/multi.c
> +++ b/src/openvpn/multi.c
> @@ -2163,7 +2163,7 @@ multi_client_connect_early_setup(struct multi_context 
> *m,
>   * Try to source a dynamic config file from the
>   * --client-config-dir directory.
>   */
> -enum client_connect_return
> +static enum client_connect_return

I don't think this change belongs to this patch  ?

>  multi_client_connect_source_ccd(struct multi_context *m,
>  struct multi_instance *mi,
>  unsigned int *option_types_found)
> diff --git a/src/openvpn/multi.h b/src/openvpn/multi.h
> index 4fb4d0b6..1d30dcc6 100644
> --- a/src/openvpn/multi.h
> +++ b/src/openvpn/multi.h
> @@ -62,6 +62,7 @@ struct deferred_signal_schedule_entry
>  struct timeval wakeup;
>  };
>  
> +
>  /**
>   * Server-mode state structure for one single VPN tunnel.
>   *
> diff --git a/src/openvpn/openvpn.h b/src/openvpn/openvpn.h
> index a1308852..7c469b01 100644
> --- a/src/openvpn/openvpn.h
> +++ b/src/openvpn/openvpn.h
> @@ -210,6 +210,21 @@ struct context_1
>  #endif
>  };
>  
> +
> +/* client authentication state, CAS_SUCCEEDED must be 0 since
> + * non multi code path still checks this variable but does not initialise it
> + * so the code depends on zero initialisation */
> +enum client_connect_status {
> +CAS_SUCCEEDED=0,

Please add spaces around '='

> +CAS_PENDING,
> +CAS_FAILED,
> +CAS_PARTIAL,/**< Variant of CAS_FAILED: at least one
> + * client-connect script/plugin succeeded
> + * while a later one in the chain failed
> + * (we still need cleanup compared to FAILED)
> + */
> +};
> +
>  /**
>   * Level 2 %context containing state that is reset on both \c SIGHUP and
>   * \c SIGUSR1 restarts.
> @@ -444,13 +459,8 @@ struct context_2
>  int push_ifconfig_ipv6_netbits;
>  struct in6_addr push_ifconfig_ipv6_remote;
>  
> -/* client authentication state, CAS_SUCCEEDED must be 0 */
> -#define CAS_SUCCEEDED 0
> -#define CAS_PENDING   1
> -#define CAS_FAILED2
> -#define CAS_PARTIAL   3  /* at least one client-connect script/plugin
> -  * succeeded while a later one in the chain failed 
> */
> -int context_auth;
> +
> +enum client_connect_status context_auth;
>  
>  struct event_timeout push_request_interval;
>  int n_sent_push_requests;
> 


The rest looks good and makes sense. Using enum is always better as the
compiler (and the reader) has extra information about how a variable can
be used.

I believe the first chunk with the +static should be moved somewhere
else...but other than that and the missing-spaces comment:

Acked-by: Antonio Quartulli 

-- 
Antonio Quartulli


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


Re: [Openvpn-devel] [PATCH v5 06/14] client-connect: Refactor client-connect handling to calling a bunch of hooks in a loop

2020-07-14 Thread Antonio Quartulli
Hi,

On 11/07/2020 11:36, Arne Schwabe wrote:
> From: Fabian Knittel 
> 
> This patch changes the calling of the client-connect functions into an array
> of hooks and a block of code that calls them in a loop.
> 
> Signed-off-by: Fabian Knittel 
> Signed-off-by: Arne Schwabe 
> 
> Patch V5: Rebase on master.
> 
> Signed-off-by: Arne Schwabe 
> ---
>  src/openvpn/multi.c | 43 +--
>  1 file changed, 17 insertions(+), 26 deletions(-)
> 
> diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
> index 9bb52ef7..83848fdc 100644
> --- a/src/openvpn/multi.c
> +++ b/src/openvpn/multi.c
> @@ -2241,6 +2241,10 @@ cc_check_return(int *cc_succeeded_count,
>  }
>  }
>  
> +typedef enum client_connect_return (*multi_client_connect_handler)
> +(struct multi_context *m, struct multi_instance *mi,
> + unsigned int *option_types_found);
> +

how about something like this:

2519 typedef enum client_connect_return
2520 (*multi_client_connect_handler)(struct multi_context *m,
2521 struct multi_instance *mi,
2522 unsigned int *option_types_found);
2523

I find it easier to read (just my opinion)


>  /*
>   * Called as soon as the SSL/TLS connection is authenticated.
>   *
> @@ -2268,7 +2272,17 @@ multi_connection_established(struct multi_context *m, 
> struct multi_instance *mi)
>  {
>  return;
>  }
> -unsigned int option_types_found = 0;
> +
> +multi_client_connect_handler handlers[] = {
> +multi_client_connect_source_ccd,
> +multi_client_connect_call_plugin_v1,
> +multi_client_connect_call_plugin_v2,
> +multi_client_connect_call_script,
> +multi_client_connect_mda,
> +NULL
> +};
> +
> +unsigned int option_types_found = 0;

something is wrong in the indentation of the chunk above: too many spaces.

>  
>  int cc_succeeded = true; /* client connect script status */
>  int cc_succeeded_count = 0;
> @@ -2276,32 +2290,9 @@ multi_connection_established(struct multi_context *m, 
> struct multi_instance *mi)
>  
>  multi_client_connect_early_setup(m, mi);
>  
> -ret = multi_client_connect_source_ccd(m, mi, _types_found);
> -cc_succeeded = cc_check_return(_succeeded_count, ret);
> -
> -if (cc_succeeded)
> -{
> -ret = multi_client_connect_call_plugin_v1(m, mi, 
> _types_found);
> -cc_succeeded = cc_check_return(_succeeded_count, ret);
> -}
> -
> -if (cc_succeeded)
> -{
> -ret = multi_client_connect_call_plugin_v2(m, mi, 
> _types_found);
> -cc_succeeded = cc_check_return(_succeeded_count, ret);
> -}
> -
> -
> -if (cc_succeeded)
> +for (int i = 0; cc_succeeded && handlers[i]; i++)
>  {
> -ret = multi_client_connect_call_script(m, mi, _types_found);
> -cc_succeeded = cc_check_return(_succeeded_count, ret);
> -}
> -
> -if (cc_succeeded)
> -{
> -
> -ret = multi_client_connect_mda(m, mi, _types_found);
> +ret = handlers[i](m, mi, _types_found);
>  cc_succeeded = cc_check_return(_succeeded_count, ret);
>  }
>  
> 


Except for the indentation issue, the rest looks good. This patch simply
makes the handlers invocation more generic and part of a loop.

Acked-by: Antonio Quartulli 

-- 
Antonio Quartulli


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


Re: [Openvpn-devel] [PATCH v5 05/14] client-connect: Refactor to use return values instead of modifying a passed-in flag

2020-07-14 Thread Antonio Quartulli
n ret;
>  
>  multi_client_connect_early_setup(m, mi);
>  
> -multi_client_connect_source_ccd(m, mi, _types_found);
> +ret = multi_client_connect_source_ccd(m, mi, _types_found);
> +cc_succeeded = cc_check_return(_succeeded_count, ret);
>  
> -multi_client_connect_call_plugin_v1(m, mi, _types_found,
> -_succeeded,
> -_succeeded_count);
> +if (cc_succeeded)
> +{
> +ret = multi_client_connect_call_plugin_v1(m, mi, 
> _types_found);
> +cc_succeeded = cc_check_return(_succeeded_count, ret);
> +}
> +
> +if (cc_succeeded)
> +{
> +ret = multi_client_connect_call_plugin_v2(m, mi, 
> _types_found);
> +cc_succeeded = cc_check_return(_succeeded_count, ret);
> +}
>  
> -multi_client_connect_call_plugin_v2(m, mi, _types_found,
> -_succeeded,
> -_succeeded_count);
>  
> -/*
> - * Check for client-connect script left by management interface client
> - */
>  if (cc_succeeded)
>  {
> -multi_client_connect_call_script(m, mi, _types_found,
> - _succeeded,
> - _succeeded_count);
> +ret = multi_client_connect_call_script(m, mi, _types_found);
> +cc_succeeded = cc_check_return(_succeeded_count, ret);
>  }
>  
> -#ifdef MANAGEMENT_DEF_AUTH
> -if (cc_succeeded && mi->cc_config)
> +if (cc_succeeded)
>  {
> -multi_client_connect_mda(m, mi, _types_found);
> -++cc_succeeded_count;
> +

remove this empty line above

> +ret = multi_client_connect_mda(m, mi, _types_found);
> +cc_succeeded = cc_check_return(_succeeded_count, ret);
>  }
> -#endif
>  
>  /*
>   * Check for "disable" directive in client-config-dir file
> @@ -2282,13 +2312,11 @@ multi_connection_established(struct multi_context *m, 
> struct multi_instance *mi)
>  if (mi->context.options.disable)
>  {
>  msg(D_MULTI_ERRORS, "MULTI: client has been rejected due to "
> -" 'disable' directive");
> +"'disable' directive");
>  cc_succeeded = false;
>  cc_succeeded_count = 0;
>  }
>  
> -
> -
>  if (cc_succeeded)
>  {
>  multi_client_connect_late_setup(m, mi, option_types_found);
> @@ -2300,7 +2328,6 @@ multi_connection_established(struct multi_context *m, 
> struct multi_instance *mi)
>  cc_succeeded_count ? CAS_PARTIAL : CAS_FAILED;
>  }
>  
> -
>  /* increment number of current authenticated clients */
>  ++m->n_clients;
>  update_mstat_n_clients(m->n_clients);
> diff --git a/src/openvpn/multi.h b/src/openvpn/multi.h
> index c51107f4..4fb4d0b6 100644
> --- a/src/openvpn/multi.h
> +++ b/src/openvpn/multi.h
> @@ -187,6 +187,16 @@ struct multi_context {
>  struct deferred_signal_schedule_entry deferred_shutdown_signal;
>  };
>  
> +/**
> + * Return values used by the client connect call-back functions.
> + */
> +enum client_connect_return
> +{
> +CC_RET_FAILED,
> +CC_RET_SUCCEEDED,
> +CC_RET_SKIPPED
> +};
> +
>  /*
>   * Host route
>   */
> 


Apart from the few style comments I posted above, this patch looks good.

Using the return value instead of modifying arguments passed via pointer
looks much saner and easier to read, imho, so more reasons to go this way.


Acked-by: Antonio Quartulli 


-- 
Antonio Quartulli


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


Re: [Openvpn-devel] [PATCH v5 04/14] client-connect: Move multi_client_connect_setenv into early_setup

2020-07-14 Thread Antonio Quartulli
Hi,

On 11/07/2020 11:36, Arne Schwabe wrote:
> From: Fabian Knittel 
> 
> This patch moves multi_client_connect_setenv into
> multi_client_connect_early_setup and makes sure that every client-connect
> handling function updates the virtual address selection.
> 
> Background: This unifies how the client-connect handling functions work.
> 
> Signed-off-by: Fabian Knittel 
> Signed-off-by: Arne Schwabe 
> 
> Patch V5: Rebase on master
> 
> Signed-off-by: Arne Schwabe 
> ---
>  src/openvpn/multi.c | 22 +-
>  1 file changed, 13 insertions(+), 9 deletions(-)
> 
> diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
> index 35e0bd10..539ebfc0 100644
> --- a/src/openvpn/multi.c
> +++ b/src/openvpn/multi.c
> @@ -2150,6 +2150,12 @@ multi_client_connect_early_setup(struct multi_context 
> *m,
>  
>  /* reset pool handle to null */
>  mi->vaddr_handle = -1;
> +
> +/* do --client-connect setenvs */
> +multi_select_virtual_addr(m, mi);
> +
> +multi_client_connect_setenv(m, mi);
> +
>  }
>  
>  /**
> @@ -2195,6 +2201,13 @@ multi_client_connect_source_ccd(struct multi_context 
> *m,
>CLIENT_CONNECT_OPT_MASK,
>option_types_found,
>mi->context.c2.es);
> +/*
> + * Select a virtual address from either --ifconfig-push in
> + * --client-config-dir file or --ifconfig-pool.
> + */
> +multi_select_virtual_addr(m, mi);
> +
> +multi_client_connect_setenv(m, mi);
>  }
>  gc_free();
>  }


With this patch applied we are moving these 2 lines to:
- multi_client_connect_source_ccd()
- multi_client_connect_early_setup()

In multi_connection_established() we call the two aforementioned
functions one after the other. This means we are really executing the
select_virtual_addr() twice in a row. Am I wrong?

Maybe this gets fixed in a later patch, but are we sure we are not
breaking anything? If this double call gets rearranged in a later
patch...maybe it's better to merge these 2 patches? Bisect may become
quite ugly otherwise.

Cheers,


> @@ -2236,15 +2249,6 @@ multi_connection_established(struct multi_context *m, 
> struct multi_instance *mi)
>  
>  multi_client_connect_source_ccd(m, mi, _types_found);
>  
> -/*
> - * Select a virtual address from either --ifconfig-push in
> - * --client-config-dir file or --ifconfig-pool.
> - */
> -multi_select_virtual_addr(m, mi);
> -
> -/* do --client-connect setenvs */
> -multi_client_connect_setenv(m, mi);
> -
>  multi_client_connect_call_plugin_v1(m, mi, _types_found,
>  _succeeded,
>  _succeeded_count);
> 

-- 
Antonio Quartulli


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


Re: [Openvpn-devel] [PATCH v5 03/14] client-connect: Refactor multi_client_connect_source_ccd

2020-07-14 Thread Antonio Quartulli
Hi,

On 11/07/2020 11:36, Arne Schwabe wrote:
> From: Fabian Knittel 
> 
> Refactor multi_client_connect_source_ccd(), so that options_server_import() 
> (or
> the success path in general) is only entered in one place within the function.
> 
> Signed-off-by: Fabian Knittel 
> 
> Patch V5: Simplify the logic even further to make more easy to understand.
> 
> Signed-off-by: Arne Schwabe 
> ---

Shouldn't this patch also bear Fabian's signature?

Anyway, other than the stylistic point made by Gert this patch looks
good. It's mostly creating a new interface without really introducing
any radical change.

Acked-by: Antonio Quartulli 


-- 
Antonio Quartulli


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


Re: [Openvpn-devel] [PATCH v5 02/14] client-connect: Split multi_connection_established into separate functions

2020-07-14 Thread Antonio Quartulli
Hi,

On 11/07/2020 11:36, Arne Schwabe wrote:
> From: Fabian Knittel 
> 
> This patch splits up the multi_connection_established() function.  Each new
> helper function does a specific job.  Functions that do a similar job receive 
> a
> similar calling interface.
> 
> The patch tries not to reindent code, so that the real changes are as clearly
> visible as possible.  (A follow-up patch will only do indentation changes.)
> 
> Signed-off-by: Fabian Knittel 
> 
> PATCH v3: Since the code has changed enough from the time the original patch
> to the current master, the splitting has been redone from the current code.
> Also some style and minor code changes have been added doing this patch.
> This elimininates and the big reformatting done before eliminates the follow
> up patch with indentation changes.
> 
> The original patch already replaces some instances of option_permission_mask
> with CLIENT_CONNECT_OPT_MASK. The V3 version does this more consistenly.
> 
> Patch v4: Move config -> mi->cc_config into its own commit
> 
> Patch v5: Clean up some minor issues, add one missing check on
> temporary file deletion, rebase on latest master.
> 
> Signed-off-by: Arne Schwabe 

Thanks for taking care of my concerns.

The patch looks good and does what it says. Unfortunately the code
itself is already pretty convoluted, so this refactoring patch looks
convoluted as well...but after looking through it the change is minimal.


Acked-by: Antonio Quartulli 


-- 
Antonio Quartulli


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


Re: [Openvpn-devel] [PATCH v5 03/14] client-connect: Refactor multi_client_connect_source_ccd

2020-07-13 Thread Antonio Quartulli
Hi,

On 13/07/2020 13:29, Gert Doering wrote:
> instead, maybe this?
> 
>> +const char *ccd_client = 
>> + platform_gen_path(mi->context.options.client_config_dir,
>> +   tls_common_name(mi->context.c2.tls_multi,
>> +   false), );
> 

I also like the above.

> or maybe we want to extract "tls_common_name(..., false)" into a temp
> variable here? 
> 
>> +const char *cn = tls_common_name(mi->context.c2.tls_multi, false);
>> +const char *ccd_client = 
>> + platform_gen_path(mi->context.options.client_config_dir, 
>   cn, );
> 

Imho this is not prettier than the version above :D so I'd personally go
with the version above instead of this.


Cheers,


-- 
Antonio Quartulli


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


Re: [Openvpn-devel] [PATCH 8/8] Code cleanup: remove superflous variable

2020-07-09 Thread Antonio Quartulli
Hi,

On 09/07/2020 12:16, Arne Schwabe wrote:
> Signed-off-by: Arne Schwabe 
> ---
>  src/openvpn/ssl.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
> index 4ee4c245..54a23011 100644
> --- a/src/openvpn/ssl.c
> +++ b/src/openvpn/ssl.c
> @@ -1231,11 +1231,10 @@ lame_duck_must_die(const struct tls_session *session, 
> interval_t *wakeup)
>  const struct key_state *lame = >key[KS_LAME_DUCK];
>  if (lame->state >= S_INITIAL)
>  {
> -const time_t local_now = now;
>  ASSERT(lame->must_die); /* a lame duck key must always have an 
> expiration */
> -if (local_now < lame->must_die)
> +if (now < lame->must_die)
>  {
> -compute_earliest_wakeup(wakeup, lame->must_die - local_now);
> +compute_earliest_wakeup(wakeup, lame->must_die - now);

The only reason for having this local variable is the case where this
code would run concurrently with a thread that could update "now" behind
our back.

Since openvpn runs in a single thread, such scenario is not possible at all.

For this reason this patch makes sense and it removes one more bit that
was originally introduced in the attempt of implementing multi threading.

Acked-by: Antonio Quartulli 

>  return false;
>  }
>  else
> 

-- 
Antonio Quartulli


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


Re: [Openvpn-devel] [PATCH 3/8] Extract process_incoming_push_reply from process_incoming_push_msg

2020-07-09 Thread Antonio Quartulli
c->c2.es))
> -{
> -push_update_digest(c->c2.pulled_options_state, _orig,
> -   >options);
> -switch (c->options.push_continuation)
> -{
> -case 0:
> -case 1:
> -md_ctx_final(c->c2.pulled_options_state, 
> c->c2.pulled_options_digest.digest);
> -md_ctx_cleanup(c->c2.pulled_options_state);
> -md_ctx_free(c->c2.pulled_options_state);
> -c->c2.pulled_options_state = NULL;
> -c->c2.pulled_options_digest_init_done = false;
> -ret = PUSH_MSG_REPLY;
> -break;
> -
> -case 2:
> -ret = PUSH_MSG_CONTINUATION;
> -break;
> -}
> -    }
> -    }
> -else if (ch == '\0')
> -{
> -ret = PUSH_MSG_REPLY;
> -}
> -/* show_settings (>options); */
> +return process_incoming_push_reply(c, permission_mask,
> +   option_types_found, );
> +}
> +else
> +{
> +return PUSH_MSG_ERROR;
>  }
> -return ret;
>  }
>  
>  
> 

The rest looks good!
Tested on the client side and the PUSH_REPLY was still properly
processed as expected,


Assuming that const gets fixed:

Acked-by: Antonio Quartulli 

-- 
Antonio Quartulli


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


Re: [Openvpn-devel] [PATCH 2/8] Make key_state->authenticated more state machine like

2020-07-09 Thread Antonio Quartulli
Hi,

thanks for bearing with me and for adding the requested comment. I
believe the status enum is now easier to grasp and to extend.

Code was basically already reviewed ad this revision did not bring any
functional change with it. "It still looks good".

Tested only on the client side and it did not break anything visible (as
expected).

Acked-by: Antonio Quartulli 

On 09/07/2020 12:15, Arne Schwabe wrote:
> This order the states from unauthenticated to authenticated and also
> changes the comparison for KS_AUTH_FALSE from != to >
> 
> It also add comments and documents part using the state machine
> better.
> 
> Remove a now obsolete comment and two obsolete ifdefs. While
> keeping the ifdef in ssl_verify would save a few bytes of code,
> this is too minor to justify keeping the ifdef
> 
> Signed-off-by: Arne Schwabe 
> ---
>  src/openvpn/multi.c  | 12 +---
>  src/openvpn/ssl.c|  7 ---
>  src/openvpn/ssl_common.h | 22 --
>  src/openvpn/ssl_verify.c | 18 +++---
>  4 files changed, 36 insertions(+), 23 deletions(-)
> 
> diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
> index f1ced9b7..f1332c8d 100644
> --- a/src/openvpn/multi.c
> +++ b/src/openvpn/multi.c
> @@ -2352,12 +2352,12 @@ multi_process_post(struct multi_context *m, struct 
> multi_instance *mi, const uns
>  if (!IS_SIG(>context) && ((flags & MPP_PRE_SELECT) || ((flags & 
> MPP_CONDITIONAL_PRE_SELECT) && !ANY_OUT(>context
>  {
>  #if defined(ENABLE_ASYNC_PUSH) && defined(ENABLE_DEF_AUTH)
> -bool was_authenticated = false;
> +bool was_unauthenticated = true;
>  struct key_state *ks = NULL;
>  if (mi->context.c2.tls_multi)
>  {
>  ks = 
> >context.c2.tls_multi->session[TM_ACTIVE].key[KS_PRIMARY];
> -was_authenticated = ks->authenticated;
> +was_unauthenticated = (ks->authenticated == KS_AUTH_FALSE);
>  }
>  #endif
>  
> @@ -2366,7 +2366,13 @@ multi_process_post(struct multi_context *m, struct 
> multi_instance *mi, const uns
>  pre_select(>context);
>  
>  #if defined(ENABLE_ASYNC_PUSH) && defined(ENABLE_DEF_AUTH)
> -if (ks && ks->auth_control_file && ks->auth_deferred && 
> !was_authenticated)
> +/*
> + * if we see the state transition from unauthenticated to deferred
> + * and a auth_control_file, we assume it got just added and add
> + * inotify watch to that file
> + */
> +if (ks && ks->auth_control_file && was_unauthenticated
> +&& (ks->authenticated == KS_AUTH_DEFERRED))
>  {
>  /* watch acf file */
>  long watch_descriptor = inotify_add_watch(m->top.c2.inotify_fd, 
> ks->auth_control_file, IN_CLOSE_WRITE | IN_ONESHOT);
> diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
> index 9df7552d..f3fe0ecf 100644
> --- a/src/openvpn/ssl.c
> +++ b/src/openvpn/ssl.c
> @@ -2466,7 +2466,7 @@ key_method_2_write(struct buffer *buf, struct 
> tls_session *session)
>  if (session->opt->server && !(session->opt->ncp_enabled
>&& session->opt->mode == MODE_SERVER && 
> ks->key_id <= 0))
>  {
> -if (ks->authenticated != KS_AUTH_FALSE)
> +if (ks->authenticated > KS_AUTH_FALSE)
>  {
>  if (!tls_session_generate_data_channel_keys(session))
>  {
> @@ -2659,7 +2659,7 @@ key_method_2_read(struct buffer *buf, struct tls_multi 
> *multi, struct tls_sessio
>  secure_memzero(up, sizeof(*up));
>  
>  /* Perform final authentication checks */
> -if (ks->authenticated != KS_AUTH_FALSE)
> +if (ks->authenticated > KS_AUTH_FALSE)
>  {
>  verify_final_auth_checks(multi, session);
>  }
> @@ -2684,7 +2684,7 @@ key_method_2_read(struct buffer *buf, struct tls_multi 
> *multi, struct tls_sessio
>   * Call OPENVPN_PLUGIN_TLS_FINAL plugin if defined, for final
>   * veto opportunity over authentication decision.
>   */
> -if ((ks->authenticated != KS_AUTH_FALSE)
> +if ((ks->authenticated > KS_AUTH_FALSE)
>  && plugin_defined(session->opt->plugins, OPENVPN_PLUGIN_TLS_FINAL))
>  {
>  key_state_export_keying_material(>ks_ssl, session);
> @@ -2715,6 +2715,7 @@ key_method_2_read(struct buffer *buf, struct tls_multi 
> *multi, struct tls_sessio
>  return true;
>  
>  error:
> +ks->authenticated = KS_AUTH_FALSE;
>  sec

Re: [Openvpn-devel] [PATCH 1/8] Deprecate ncp-disable and add improved ncp to Changes.rst

2020-07-09 Thread Antonio Quartulli
Hi,

On 09/07/2020 12:15, Arne Schwabe wrote:
> Signed-off-by: Arne Schwabe 
> ---
>  Changes.rst   | 18 ++
>  src/openvpn/options.c |  5 -
>  2 files changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/Changes.rst b/Changes.rst
> index 00dd6ed8..2752d29b 100644
> --- a/Changes.rst
> +++ b/Changes.rst
> @@ -13,6 +13,24 @@ ChaCha20-Poly1305 cipher support
>  Added support for using the ChaCha20-Poly1305 cipher in the OpenVPN data
>  channel.
>  
> +Improved Data channel cipher negotiation
> +OpenVPN clients will now signal all supported cipher from the
^^^ cipherS

> +``ncp-ciphers`` option to the server via ``IV_CIPHERS``. OpenVPN
> +servers will select the first common cipher from the ``ncp-ciphers``
> +list instead of blindly pushing the first cipher of the list. This
> +allows to use a configuration like
> +``ncp-ciphers ChaCha20-Poly1305:AES-256-GCM`` on the server that
> +prefers ChaCha20-Poly1305 but uses it only if the client supports it.
> +
> +Deprecated features
> +---
> +For an up-to-date list of all deprecated options, see this wiki page:
> +https://community.openvpn.net/openvpn/wiki/DeprecatedOptions
> +
> +- ``ncp-disable`` has been deprecated
> +With the improved and matured data channel cipher negioation, the use

 ^^^ negotiation

> +of ``ncp-disable`` should not be necessary anymore.
> +
>  
>  Overview of changes in 2.4
>  ==
> diff --git a/src/openvpn/options.c b/src/openvpn/options.c
> index a72b677a..75871b46 100644
> --- a/src/openvpn/options.c
> +++ b/src/openvpn/options.c
> @@ -545,7 +545,7 @@ static const char usage_message[] =
>  "  (default=%s).\n"
>  "  Set alg=none to disable encryption.\n"
>  "--ncp-ciphers list : List of ciphers that are allowed to be 
> negotiated.\n"
> -"--ncp-disable   : Disable cipher negotiation.\n"
> +"--ncp-disable   : (DEPRECATED) Disable cipher negotiation.\n"
>  "--prng alg [nsl] : For PRNG, use digest algorithm alg, and\n"
>  "   nonce_secret_len=nsl.  Set alg=none to disable 
> PRNG.\n"
>  #ifdef HAVE_EVP_CIPHER_CTX_SET_KEY_LENGTH
> @@ -7904,6 +7904,9 @@ add_option(struct options *options,
>  {
>  VERIFY_PERMISSION(OPT_P_GENERAL|OPT_P_INSTANCE);
>  options->ncp_enabled = false;
> +msg(M_WARN, "DEPRECATED OPTION: ncp-disable. Disabling dynamic "
> +"cipher negioating is a depracted debug feature that 
> will "
   ^^^ negotiation^^^ deprecated

> +"be removed in OpenVPN 2.6");
>  }
>  else if (streq(p[0], "prng") && p[1] && !p[3])
>  {
> 


I only have one doubt about how visible the message is:

2020-07-09 16:05:08 DEPRECATED OPTION: ncp-disable. Disabling dynamic
cipher negioating is a depracted debug feature that will be removed in
OpenVPN 2.6

appears at the top of the log (first line actually) and does not seem
easy to spot.

How about adding some * or  to make it pop out of the
screen?

I personally did not see it immediately, even though I knew it was
there! (note: it may just be me)

-- 
Antonio Quartulli


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


Re: [Openvpn-devel] [PATCH v2] Make key_state->authenticated more state machine like

2020-07-08 Thread Antonio Quartulli
 status of the key-state object
 * during its life-cycle. Based on the step taken by the server code,
 * the status will move forward by one (or more) values. This allows
 * us to make more flexible status checks by using the ">" or "<"
 * operator
 */

>  enum ks_auth_state {
> -  KS_AUTH_FALSE,
> -  KS_AUTH_TRUE,
> -  KS_AUTH_DEFERRED
> +  KS_AUTH_FALSE,  /**< Key state is not authenticated  */
> +  KS_AUTH_DEFERRED,   /**< Key state authentication is being 
> deferred,
> +* by async auth */
> +  KS_AUTH_TRUE/**< Key state is authenticated. TLS and 
> user/pass
> +* succeeded. This include AUTH_PENDING/OOB
> +* authentication as those hold the
> +* connection artifically in KS_AUTH_DEFERRED
> +*/
>  };
>  
>  /**
> @@ -194,8 +199,6 @@ struct key_state
>  enum ks_auth_state authenticated;
>  time_t auth_deferred_expire;
>  
> -#ifdef ENABLE_DEF_AUTH
> -/* If auth_deferred is true, authentication is being deferred */
>  #ifdef MANAGEMENT_DEF_AUTH
>  unsigned int mda_key_id;
>  unsigned int mda_status;
> @@ -205,7 +208,6 @@ struct key_state
>  time_t acf_last_mod;
>  char *auth_control_file;
>  #endif
> -#endif
>  };
>  
>  /** Control channel wrapping (--tls-auth/--tls-crypt) context */
> diff --git a/src/openvpn/ssl_verify.c b/src/openvpn/ssl_verify.c
> index e28f1f3a..990fba99 100644
> --- a/src/openvpn/ssl_verify.c
> +++ b/src/openvpn/ssl_verify.c
> @@ -950,7 +950,7 @@ tls_authentication_status(struct tls_multi *multi, const 
> int latency)
>  if (DECRYPT_KEY_ENABLED(multi, ks))
>  {
>  active = true;
> -if (ks->authenticated != KS_AUTH_FALSE)
> +if (ks->authenticated > KS_AUTH_FALSE)
>  {
>  #ifdef ENABLE_DEF_AUTH
>  unsigned int s1 = ACF_DISABLED;
> @@ -1249,6 +1249,9 @@ verify_user_pass_management(struct tls_session *session,
>  
>  /*
>   * Main username/password verification entry point
> + *
> + * Will set session->ks[KS_PRIMARY].authenticated according to
> + * result of the username/password verifcation
>   */
>  void
>  verify_user_pass(struct user_pass *up, struct tls_multi *multi,
> @@ -1414,17 +1417,10 @@ verify_user_pass(struct user_pass *up, struct 
> tls_multi *multi,
>   */
>  send_push_reply_auth_token(multi);
>  }
> -#ifdef ENABLE_DEF_AUTH
>  msg(D_HANDSHAKE, "TLS: Username/Password authentication %s for 
> username '%s' %s",
>  (ks->authenticated == KS_AUTH_DEFERRED) ? "deferred" : 
> "succeeded",
>  up->username,
>  (session->opt->ssl_flags & SSLF_USERNAME_AS_COMMON_NAME) ? "[CN 
> SET]" : "");
> -#else
> -msg(D_HANDSHAKE, "TLS: Username/Password authentication %s for 
> username '%s' %s",
> -"succeeded",
> -up->username,
> -(session->opt->ssl_flags & SSLF_USERNAME_AS_COMMON_NAME) ? "[CN 
> SET]" : "");
> -#endif
>  }
>  else
>  {
> @@ -1445,7 +1441,7 @@ verify_final_auth_checks(struct tls_multi *multi, 
> struct tls_session *session)
>  }
>  
>  /* Don't allow the CN to change once it's been locked */
> -if (ks->authenticated != KS_AUTH_FALSE && multi->locked_cn)
> +if (ks->authenticated > KS_AUTH_FALSE && multi->locked_cn)
>  {
>  const char *cn = session->common_name;
>  if (cn && strcmp(cn, multi->locked_cn))
> @@ -1461,7 +1457,7 @@ verify_final_auth_checks(struct tls_multi *multi, 
> struct tls_session *session)
>  }
>  
>  /* Don't allow the cert hashes to change once they have been locked */
> -if (ks->authenticated != KS_AUTH_FALSE && multi->locked_cert_hash_set)
> +if (ks->authenticated > KS_AUTH_FALSE && multi->locked_cert_hash_set)
>  {
>  const struct cert_hash_set *chs = session->cert_hash_set;
>  if (chs && !cert_hash_compare(chs, multi->locked_cert_hash_set))
> @@ -1475,7 +1471,7 @@ verify_final_auth_checks(struct tls_multi *multi, 
> struct tls_session *session)
>  }
>  
>  /* verify --client-config-dir based authentication */
> -if (ks->authenticated != KS_AUTH_FALSE && 
> session->opt->client_config_dir_exclusive)
> +if (ks->authenticated > KS_AUTH_FALSE && 
> session->opt->client_config_dir_exclusive)
>  {
>  struct gc_arena gc = gc_new();
>  
> 

The rest looks good!

I don't have a super strong opinion about that comment I proposed, but I
think it can help to better understand how the state enum is used and
how to modify it in the future.

I'd prefer to have it, but in any case:

Acked-by: Antonio Quartulli 


-- 
Antonio Quartulli


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


Re: [Openvpn-devel] [PATCH 3/3] Make key_state->authenticated more state machine like

2020-07-07 Thread Antonio Quartulli
   up->username,
> -(session->opt->ssl_flags & SSLF_USERNAME_AS_COMMON_NAME) ? "[CN 
> SET]" : "");
> -#endif
>  }
>  else
>  {
> @@ -1445,7 +1438,7 @@ verify_final_auth_checks(struct tls_multi *multi, 
> struct tls_session *session)
>  }
>  
>  /* Don't allow the CN to change once it's been locked */
> -if (ks->authenticated != KS_AUTH_FALSE && multi->locked_cn)
> +if (ks->authenticated > KS_AUTH_FALSE && multi->locked_cn)
>  {
>  const char *cn = session->common_name;
>  if (cn && strcmp(cn, multi->locked_cn))
> @@ -1461,7 +1454,7 @@ verify_final_auth_checks(struct tls_multi *multi, 
> struct tls_session *session)
>  }
>  
>  /* Don't allow the cert hashes to change once they have been locked */
> -if (ks->authenticated != KS_AUTH_FALSE && multi->locked_cert_hash_set)
> +if (ks->authenticated > KS_AUTH_FALSE && multi->locked_cert_hash_set)
>  {
>  const struct cert_hash_set *chs = session->cert_hash_set;
>  if (chs && !cert_hash_compare(chs, multi->locked_cert_hash_set))
> @@ -1475,7 +1468,7 @@ verify_final_auth_checks(struct tls_multi *multi, 
> struct tls_session *session)
>  }
>  
>  /* verify --client-config-dir based authentication */
> -if (ks->authenticated != KS_AUTH_FALSE && 
> session->opt->client_config_dir_exclusive)
> +if (ks->authenticated > KS_AUTH_FALSE && 
> session->opt->client_config_dir_exclusive)
>  {
>  struct gc_arena gc = gc_new();
>  
> 

-- 
Antonio Quartulli


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


Re: [Openvpn-devel] [PATCH 1/3] Simplify multi_connection_established.

2020-07-07 Thread Antonio Quartulli
Hi,

On 07/07/2020 14:16, Arne Schwabe wrote:
> Instead of having the whole function as
> 
> if (x) { func }
> 
> do
> 
>if (!x) func

I guess this commit message needs some love? You probably meant:

Instead of..

if (x) { func }

do

if (!x) return;
func


At least, this is what the patch does :-)

> 
> Due to the whitespace changes in the function body this patch looks
> very strange. Ignoring whitespace makes the diff look sane.
> 
> Signed-off-by: Arne Schwabe 

I am super-pro this style.

It makes the whole function easier to read and saves a lot of horizontal
spaces by killing one level of indentation.

Reviewing this patch with "git show -w" makes it super obvious.

No functional change included, the patch is just reverting the top
condition and returning right away.

Stared at the code and compile-tested. (but please fix the commit
message before merging)

Acked-by: Antonio Quartulli 


-- 
Antonio Quartulli


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


Re: [Openvpn-devel] [PATCH] Make openvpn --version exit with exit code 0

2020-07-07 Thread Antonio Quartulli
Hi,

On 07/07/2020 16:26, Steffan Karger wrote:
> For some reason, openvpn --version has since the beginning of time
> returned exit code 1. A quick sample among common unix utilities confirms
> that the rest of the world agrees with me that 0 makes more sense. Let's
> make openvpn --version exit with exit code 0 too.
> 
> Signed-off-by: Steffan Karger 

I also agree with Steffan - if --version was passed as argument than
"success" behaviour is to print the related text and exit with success.

Like he said, this is what most other unix command line tools do.

I quickly tested the patch and I can confirm it does what it says :)

Acked-by: Antonio Quartulli 



-- 
Antonio Quartulli


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


Re: [Openvpn-devel] [PATCH v2] Remove --writepid file on program exit.

2020-07-07 Thread Antonio Quartulli
Hi,

On 07/07/2020 10:42, Gert Doering wrote:
> For whatever reason, we never removed the pid file on program exit.
> 
> Not only this is unclean, but it also makes testing for "I want this
> test case to FAIL" in t_client.sh more annoying to code for "is the
> OpenVPN process still around?"...
> 
> Do not unlink the file if chroot() is active (might be outside the
> chroot arena - testing for realpath etc. is left for someone else).
> 
> Signed-off-by: Gert Doering 
> 
> --
> v2: make this work on M_FATAL exit, by unlinking from openvpn_exit() in
> error.h - this requires moving write_pid() to init.c so module hierarchy
> is maintained and introducing a static variable to save the PID file
> name (otherwise it is no longer available when the top level GC is gone).
> ---
>  src/openvpn/error.c   |  1 +
>  src/openvpn/init.c| 42 ++
>  src/openvpn/init.h|  3 +++
>  src/openvpn/openvpn.c | 24 +---
>  4 files changed, 47 insertions(+), 23 deletions(-)
> 
> diff --git a/src/openvpn/error.c b/src/openvpn/error.c
> index ad4f0ef4..d6247fec 100644
> --- a/src/openvpn/error.c
> +++ b/src/openvpn/error.c
> @@ -743,6 +743,7 @@ openvpn_exit(const int status)
>  #ifdef _WIN32
>  uninit_win32();
>  #endif
> +remove_pid_file();
>  
>  close_syslog();
>  
> diff --git a/src/openvpn/init.c b/src/openvpn/init.c
> index dd1747f3..cb850bc0 100644
> --- a/src/openvpn/init.c
> +++ b/src/openvpn/init.c
> @@ -58,6 +58,7 @@
>  
>  
>  static struct context *static_context; /* GLOBAL */
> +static const char *saved_pid_file_name; /* GLOBAL */
>  
>  /*
>   * Crypto initialization flags
> @@ -4687,6 +4688,47 @@ close_context(struct context *c, int sig, unsigned int 
> flags)
>  }
>  }
>  
> +/* Write our PID to a file */
> +void
> +write_pid_file(const char *filename, const char *chroot_dir)
> +{
> +if (filename)
> +{
> +unsigned int pid = 0;
> +FILE *fp = platform_fopen(filename, "w");
> +if (!fp)
> +{
> +msg(M_ERR, "Open error on pid file %s", filename);
> +return;
> +}
> +
> +pid = platform_getpid();
> +fprintf(fp, "%u\n", pid);
> +if (fclose(fp))
> +{
> +msg(M_ERR, "Close error on pid file %s", filename);
> +}
> +
> +/* remember file name so it can be deleted "out of context" later */
> + /* (the chroot case is more complex and not handled today) */


This comment is indented using a tab, instead of the proper amount of
spaces.


> +if (!chroot_dir)
> +{
> +saved_pid_file_name = strdup(filename);
> +}
> +}
> +}
> +
> +/* remove PID file on exit, called from openvpn_exit() */
> +void
> +remove_pid_file(void)
> +{
> +if (saved_pid_file_name)
> +{
> +platform_unlink(saved_pid_file_name);
> +}
> +}
> +
> +
>  /*
>   * Do a loopback test
>   * on the crypto subsystem.
> diff --git a/src/openvpn/init.h b/src/openvpn/init.h
> index 0e6258f0..a2fdccd3 100644
> --- a/src/openvpn/init.h
> +++ b/src/openvpn/init.h
> @@ -143,4 +143,7 @@ void open_plugins(struct context *c, const bool 
> import_options, int init_point);
>  
>  void tun_abort(void);
>  
> +void write_pid_file(const char *filename, const char *chroot_dir);
> +void remove_pid_file(void);
> +
>  #endif /* ifndef INIT_H */
> diff --git a/src/openvpn/openvpn.c b/src/openvpn/openvpn.c
> index dc7001dc..857c5faa 100644
> --- a/src/openvpn/openvpn.c
> +++ b/src/openvpn/openvpn.c
> @@ -46,28 +46,6 @@ process_signal_p2p(struct context *c)
>  return process_signal(c);
>  }
>  
> -/* Write our PID to a file */
> -static void
> -write_pid(const char *filename)
> -{
> -if (filename)
> -{
> -unsigned int pid = 0;
> -FILE *fp = platform_fopen(filename, "w");
> -if (!fp)
> -{
> -msg(M_ERR, "Open error on pid file %s", filename);
> -}
> -
> -pid = platform_getpid();
> -fprintf(fp, "%u\n", pid);
> -if (fclose(fp))
> -{
> -msg(M_ERR, "Close error on pid file %s", filename);
> -}
> -}
> -}
> -
>  
>  /**/
>  /**
> @@ -274,7 +252,7 @@ openvpn_main(int argc, char *argv[])
>  if (c.first_time)
>  {
>  c.did_we_daemonize = possi

Re: [Openvpn-devel] [PATCH 2/2] merge key_state->authenticated and key_state->auth_deferred

2020-07-06 Thread Antonio Quartulli
;username))
>  {
> -ks->authenticated = true;
> +ks->authenticated = KS_AUTH_TRUE;
>  #ifdef PLUGIN_DEF_AUTH
>  if (s1 == OPENVPN_PLUGIN_FUNC_DEFERRED)
>  {
> -ks->auth_deferred = true;
> +ks->authenticated = KS_AUTH_DEFERRED;
>  }
>  #endif
>  #ifdef MANAGEMENT_DEF_AUTH
>  if (man_def_auth != KMDA_UNDEF)
>  {
> -ks->auth_deferred = true;
> +ks->authenticated = KS_AUTH_DEFERRED;
>  }
>  #endif
>  if ((session->opt->ssl_flags & SSLF_USERNAME_AS_COMMON_NAME))
> @@ -1416,7 +1416,7 @@ verify_user_pass(struct user_pass *up, struct tls_multi 
> *multi,
>  }
>  #ifdef ENABLE_DEF_AUTH
>  msg(D_HANDSHAKE, "TLS: Username/Password authentication %s for 
> username '%s' %s",
> -ks->auth_deferred ? "deferred" : "succeeded",
> +(ks->authenticated == KS_AUTH_DEFERRED) ? "deferred" : 
> "succeeded",
>  up->username,
>  (session->opt->ssl_flags & SSLF_USERNAME_AS_COMMON_NAME) ? "[CN 
> SET]" : "");
>  #else
> @@ -1428,6 +1428,7 @@ verify_user_pass(struct user_pass *up, struct tls_multi 
> *multi,
>  }
>  else
>  {
> +ks->authenticated = KS_AUTH_FALSE;

Previously there was no code here doing anything like this.
Is this line actually fixing a bug? Or why is being added?

Since this patch is just about merging the two members, I'd stick with
it and avoid adding other changes that may introduce their own issue, no?

I'd personally keep this line for its own patch, coming with a proper
explanation as to why that is needed.

>  msg(D_TLS_ERRORS, "TLS Auth Error: Auth Username/Password 
> verification failed for peer");
>  }
>  }
> @@ -1444,7 +1445,7 @@ verify_final_auth_checks(struct tls_multi *multi, 
> struct tls_session *session)
>  }
>  
>  /* Don't allow the CN to change once it's been locked */
> -if (ks->authenticated && multi->locked_cn)
> +if (ks->authenticated != KS_AUTH_FALSE && multi->locked_cn)
>  {
>  const char *cn = session->common_name;
>  if (cn && strcmp(cn, multi->locked_cn))
> @@ -1460,7 +1461,7 @@ verify_final_auth_checks(struct tls_multi *multi, 
> struct tls_session *session)
>  }
>  
>  /* Don't allow the cert hashes to change once they have been locked */
> -if (ks->authenticated && multi->locked_cert_hash_set)
> +if (ks->authenticated != KS_AUTH_FALSE && multi->locked_cert_hash_set)
>  {
>  const struct cert_hash_set *chs = session->cert_hash_set;
>  if (chs && !cert_hash_compare(chs, multi->locked_cert_hash_set))
> @@ -1474,7 +1475,7 @@ verify_final_auth_checks(struct tls_multi *multi, 
> struct tls_session *session)
>  }
>  
>  /* verify --client-config-dir based authentication */
> -if (ks->authenticated && session->opt->client_config_dir_exclusive)
> +if (ks->authenticated != KS_AUTH_FALSE && 
> session->opt->client_config_dir_exclusive)
>  {
>  struct gc_arena gc = gc_new();
>  
> @@ -1483,7 +1484,7 @@ verify_final_auth_checks(struct tls_multi *multi, 
> struct tls_session *session)
>   cn, );
>  if (!cn || !strcmp(cn, CCD_DEFAULT) || !platform_test_file(path))
>  {
> -ks->authenticated = false;
> +ks->authenticated = KS_AUTH_FALSE;
>  wipe_auth_token(multi);
>  msg(D_TLS_ERRORS, "TLS Auth Error: --client-config-dir 
> authentication failed for common name '%s' file='%s'",
>  session->common_name,
> 


The rest looks good, but can't ACK as it is.

Regards,

-- 
Antonio Quartulli


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


Re: [Openvpn-devel] [PATCH] t_client.sh: correctly report all failed instances in summary

2020-07-03 Thread Antonio Quartulli
Hi,

On 26/06/2020 10:27, Gert Doering wrote:
> t_client.sh reports a summary at the end:
> 
>   Test sets succeeded: none.
>   Test sets failed: 1 2 3 4 5.
> 
> for tests that are skipped due to the pre-test ping check ("vpn target
> IP must not ping before VPN ist started") the script forgot to add
> the instance number to the summary line.  Fixed.
> 
> Signed-off-by: Gert Doering 

Simple enough and pretty clear. This is the only FAIL case where we set
the exit code, print a message, but don't append the test to the
SUMMARY_FAIL.

This is fixed now.

I can't easily come up with a t_client instance that verifies this
failure, but I trust Gert to have done so after having spotted the issue.

Acked-by: Antonio Quartulli 



-- 
Antonio Quartulli


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


Re: [Openvpn-devel] [PATCH] Remove did_open_context, defined and connection_established_flag

2020-07-03 Thread Antonio Quartulli
Compile tested and checked logically by going through the various places
where these members were used (except for defined which is not used
anywhere :-)).

It all looks good and makes sense!


Glad to see that we are simplifying these jungle of state variables..

On 03/07/2020 11:55, Arne Schwabe wrote:
> multi_instance->defined is not used anywhere.
> 
> did_open_context is always set to true when a context is created in
> multi_create_instance, so checking it for true is always true.
> 
> context_auth is also always set to CAS_PENDING in multi_create_instance.
> 
> connection_established_flag is only set to true if context_auth
> is changed from CAS_PENDING to one another state, so we can also check
> for cas_context != CAS_PENDING.
> 
> Signed-off-by: Arne Schwabe 

Acked-by: Antonio Quartulli 


-- 
Antonio Quartulli


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


[Openvpn-devel] [PATCH] multi.c: use mi->cc_config instead of config variable

2020-07-01 Thread Antonio Quartulli
Commit ("Remove parameter config from multi_client_connect_mda") has
removed the config variable in favour of mi->cc_config, however one
occurence was not changed.

Fix it now by properly using mi->cc_config.

Signed-off-by: Antonio Quartulli 
---
 src/openvpn/multi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
index 19261c37..0e9e0db3 100644
--- a/src/openvpn/multi.c
+++ b/src/openvpn/multi.c
@@ -1729,7 +1729,7 @@ multi_client_connect_mda(struct multi_context *m,
 {
 struct buffer_entry *be;
 
-for (be = config->head; be != NULL; be = be->next)
+for (be = mi->cc_config->head; be != NULL; be = be->next)
 {
 const char *opt = BSTR(>buf);
 options_string_import(>context.options,
-- 
2.27.0



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


Re: [Openvpn-devel] [PATCH] Remove parameter config from multi_client_connect_mda

2020-07-01 Thread Antonio Quartulli
Hi,

On 01/07/2020 14:22, Arne Schwabe wrote:
> config is always used as mi->cc_config and we pass mi,
> so directly use mi->cc_config
> 
> Signed-off-by: Arne Schwabe 

Acked-by: Antonio Quartulli 



-- 
Antonio Quartulli


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


Re: [Openvpn-devel] [PATCH v4 3/3] Implement tls-groups option to specify eliptic curves/groups

2020-06-25 Thread Antonio Quartulli
Hi,

on my GitLab CI build test, the compilation failed with the following
message, while compiling against openssl-1.1:

 /usr/bin/ld: ssl_openssl.o: in function `tls_ctx_set_tls_groups':
/builds/ordex986/openvpn/src/openvpn/ssl_openssl.c:611: undefined
reference to `SSL_CTX_set1_groups'
collect2: error: ld returned 1 exit status


Any clue?

On 23/06/2020 11:21, Antonio Quartulli wrote:
> Hi,
> 
> On 22/06/2020 16:02, Arne Schwabe wrote:
> 
> [CUT]
> 
>> @@ -343,6 +348,42 @@ tls_ctx_set_cert_profile(struct tls_root_ctx *ctx, 
>> const char *profile)
>>  }
>>  }
>>  
>> +void
>> +tls_ctx_set_tls_groups(struct tls_root_ctx *ctx, const char *groups)
>> +{
>> +ASSERT(ctx);
>> +struct gc_arena gc = gc_new();
>> +
>> +/* Get number of groups and allocate an array in ctx */
>> +int groups_count = get_num_elements(groups, ':');
>> +ALLOC_ARRAY_CLEAR(ctx->groups, mbedtls_ecp_group_id, groups_count + 1)
>> +
>> +/* Parse allowed ciphers, getting IDs */
>> +int i = 0;
>> +char *tmp_groups = string_alloc(groups, );
>> +
>> +const char *token;
>> +while ((token = strsep(_groups, ":")))
>> +{
>> +const mbedtls_ecp_curve_info *ci =
>> +mbedtls_ecp_curve_info_from_name(token);
>> +if (!ci)
>> +{
>> +msg(M_WARN, "Warning unknown curve/group specified: %s", token);
>> +}
>> +else
>> +{
>> +ctx->groups[i] = ci->grp_id;
>> +i++;
>> +}
>> +token = strsep(_groups, ":");
> 
> The line above should be removed, otherwise end up doing strsep() twice
> in a row.
> 
> 
>> +}
>> +ctx->groups[i] = MBEDTLS_ECP_DP_NONE;
>> +
>> +gc_free();
>> +}
>> +
>> +
>>  void
>>  tls_ctx_check_cert_time(const struct tls_root_ctx *ctx)
>>  {
>> @@ -1043,6 +1084,11 @@ key_state_ssl_init(struct key_state_ssl *ks_ssl,
>>  mbedtls_ssl_conf_ciphersuites(ks_ssl->ssl_config, 
>> ssl_ctx->allowed_ciphers);
>>  }
>>  
>> +if (ssl_ctx->groups)
>> +{
>> +mbedtls_ssl_conf_curves(ks_ssl->ssl_config, ssl_ctx->groups);
>> +}
>> +
>>  /* Disable record splitting (for now).  OpenVPN assumes records are sent
>>   * unfragmented, and changing that will require thorough review and
>>   * testing.  Since OpenVPN is not susceptible to BEAST, we can just
>> diff --git a/src/openvpn/ssl_mbedtls.h b/src/openvpn/ssl_mbedtls.h
>> index 92381f1a..0525134f 100644
>> --- a/src/openvpn/ssl_mbedtls.h
>> +++ b/src/openvpn/ssl_mbedtls.h
>> @@ -105,6 +105,7 @@ struct tls_root_ctx {
>>  #endif
>>  struct external_context external_key; /**< External key context */
>>  int *allowed_ciphers;   /**< List of allowed ciphers for this 
>> connection */
>> +mbedtls_ecp_group_id *groups; /**< List of allowed groups for this 
>> connection */
>>  mbedtls_x509_crt_profile cert_profile; /**< Allowed certificate types */
>>  };
>>  
>> diff --git a/src/openvpn/ssl_openssl.c b/src/openvpn/ssl_openssl.c
>> index a489053b..da7d252a 100644
>> --- a/src/openvpn/ssl_openssl.c
>> +++ b/src/openvpn/ssl_openssl.c
>> @@ -565,6 +565,57 @@ tls_ctx_set_cert_profile(struct tls_root_ctx *ctx, 
>> const char *profile)
>>  #endif /* ifdef HAVE_SSL_CTX_SET_SECURITY_LEVEL */
>>  }
>>  
>> +void
>> +tls_ctx_set_tls_groups(struct tls_root_ctx *ctx, const char *groups)
>> +{
>> +ASSERT(ctx);
>> +struct gc_arena gc = gc_new();
>> +/* This method could be as easy as
>> + *  SSL_CTX_set1_groups_list(ctx->ctx, groups)
>> + * but OpenSSL does not like the name secp256r1 for prime256v1
>> + * This is one of the important curves.
>> + * To support the same name for OpenSSL and mbedTLS, we do
>> + * this dance.
>> + */
>> +
>> +int groups_count = get_num_elements(groups, ':');
>> +
>> +int *glist;
>> +/* Allocate an array for them */
>> +ALLOC_ARRAY_CLEAR_GC(glist, int, groups_count, );
>> +
>> +/* Parse allowed ciphers, getting IDs */
>> +int glistlen = 0;
>> +char *tmp_groups = string_alloc(groups, );
>> +
>> +const char *token;
>> +while ((token = strsep(_groups, ":")))
>> +{
>> +if (streq(token, "secp256r1"))
>> +{
>> +token = "prime256v1";
>> +}
>> +int nid = OBJ_sn2nid(token);
>> +
>> +if (nid == 0)
> 
> From a style perspective, I think it looks better to add an empty line
> *before* "int nid =..." and remove the one after.
> 
> This way we also follow the same pattern:
> 
> x = a();
> if (x ..)
> {
> }
> 
> as other parts of the code.
> 
> 
> 
> 
> The rest looks good to me.
> 
> Regards,
> 

-- 
Antonio Quartulli


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


Re: [Openvpn-devel] [PATCH v4 3/3] Implement tls-groups option to specify eliptic curves/groups

2020-06-23 Thread Antonio Quartulli
Hi,

On 22/06/2020 16:02, Arne Schwabe wrote:

[CUT]

> @@ -343,6 +348,42 @@ tls_ctx_set_cert_profile(struct tls_root_ctx *ctx, const 
> char *profile)
>  }
>  }
>  
> +void
> +tls_ctx_set_tls_groups(struct tls_root_ctx *ctx, const char *groups)
> +{
> +ASSERT(ctx);
> +struct gc_arena gc = gc_new();
> +
> +/* Get number of groups and allocate an array in ctx */
> +int groups_count = get_num_elements(groups, ':');
> +ALLOC_ARRAY_CLEAR(ctx->groups, mbedtls_ecp_group_id, groups_count + 1)
> +
> +/* Parse allowed ciphers, getting IDs */
> +int i = 0;
> +char *tmp_groups = string_alloc(groups, );
> +
> +const char *token;
> +while ((token = strsep(_groups, ":")))
> +{
> +const mbedtls_ecp_curve_info *ci =
> +mbedtls_ecp_curve_info_from_name(token);
> +if (!ci)
> +{
> +msg(M_WARN, "Warning unknown curve/group specified: %s", token);
> +}
> +else
> +{
> +ctx->groups[i] = ci->grp_id;
> +i++;
> +}
> +token = strsep(_groups, ":");

The line above should be removed, otherwise end up doing strsep() twice
in a row.


> +}
> +ctx->groups[i] = MBEDTLS_ECP_DP_NONE;
> +
> +gc_free();
> +}
> +
> +
>  void
>  tls_ctx_check_cert_time(const struct tls_root_ctx *ctx)
>  {
> @@ -1043,6 +1084,11 @@ key_state_ssl_init(struct key_state_ssl *ks_ssl,
>  mbedtls_ssl_conf_ciphersuites(ks_ssl->ssl_config, 
> ssl_ctx->allowed_ciphers);
>  }
>  
> +if (ssl_ctx->groups)
> +{
> +mbedtls_ssl_conf_curves(ks_ssl->ssl_config, ssl_ctx->groups);
> +}
> +
>  /* Disable record splitting (for now).  OpenVPN assumes records are sent
>   * unfragmented, and changing that will require thorough review and
>   * testing.  Since OpenVPN is not susceptible to BEAST, we can just
> diff --git a/src/openvpn/ssl_mbedtls.h b/src/openvpn/ssl_mbedtls.h
> index 92381f1a..0525134f 100644
> --- a/src/openvpn/ssl_mbedtls.h
> +++ b/src/openvpn/ssl_mbedtls.h
> @@ -105,6 +105,7 @@ struct tls_root_ctx {
>  #endif
>  struct external_context external_key; /**< External key context */
>  int *allowed_ciphers;   /**< List of allowed ciphers for this 
> connection */
> +mbedtls_ecp_group_id *groups; /**< List of allowed groups for this 
> connection */
>  mbedtls_x509_crt_profile cert_profile; /**< Allowed certificate types */
>  };
>  
> diff --git a/src/openvpn/ssl_openssl.c b/src/openvpn/ssl_openssl.c
> index a489053b..da7d252a 100644
> --- a/src/openvpn/ssl_openssl.c
> +++ b/src/openvpn/ssl_openssl.c
> @@ -565,6 +565,57 @@ tls_ctx_set_cert_profile(struct tls_root_ctx *ctx, const 
> char *profile)
>  #endif /* ifdef HAVE_SSL_CTX_SET_SECURITY_LEVEL */
>  }
>  
> +void
> +tls_ctx_set_tls_groups(struct tls_root_ctx *ctx, const char *groups)
> +{
> +ASSERT(ctx);
> +struct gc_arena gc = gc_new();
> +/* This method could be as easy as
> + *  SSL_CTX_set1_groups_list(ctx->ctx, groups)
> + * but OpenSSL does not like the name secp256r1 for prime256v1
> + * This is one of the important curves.
> + * To support the same name for OpenSSL and mbedTLS, we do
> + * this dance.
> + */
> +
> +int groups_count = get_num_elements(groups, ':');
> +
> +int *glist;
> +/* Allocate an array for them */
> +ALLOC_ARRAY_CLEAR_GC(glist, int, groups_count, );
> +
> +/* Parse allowed ciphers, getting IDs */
> +int glistlen = 0;
> +char *tmp_groups = string_alloc(groups, );
> +
> +const char *token;
> +while ((token = strsep(_groups, ":")))
> +{
> +if (streq(token, "secp256r1"))
> +{
> +token = "prime256v1";
> +}
> +int nid = OBJ_sn2nid(token);
> +
> +if (nid == 0)

>From a style perspective, I think it looks better to add an empty line
*before* "int nid =..." and remove the one after.

This way we also follow the same pattern:

x = a();
if (x ..)
{
}

as other parts of the code.




The rest looks good to me.

Regards,

-- 
Antonio Quartulli


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


[Openvpn-devel] [PATCH] pool: remove useless 'options.h' include

2020-06-10 Thread Antonio Quartulli
Commit 6a8cd033 ("pool: add support for ifconfig-pool-persist with IPv6
only") has accidentally introduced an include for 'options.h', which
revealed to not be useful at all. Remove it.

Reported-by: Gert Doering 
Signed-off-by: Antonio Quartulli 
---
 src/openvpn/pool.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/src/openvpn/pool.c b/src/openvpn/pool.c
index 68fa4782..370f6af7 100644
--- a/src/openvpn/pool.c
+++ b/src/openvpn/pool.c
@@ -34,7 +34,6 @@
 #include "error.h"
 #include "socket.h"
 #include "otime.h"
-#include "options.h"
 
 #include "memdbg.h"
 
-- 
2.27.0



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


[Openvpn-devel] [PATCH] multi: skip IPv4 logic in multi_select_virtual_addr() if no pool is configured

2020-06-10 Thread Antonio Quartulli
When no IPv4 pool is configured (but we have an IPv6 pool
only), the multi_select_virtual_addr() function will spit
a warning when allocating an address for a new client.
This happens because the code will check for some IPv4
bits and will see that they are missing.

However, these bits are not really important, because in
this use case we don't want to configure any IPv4 address
at all.

For this reason it is safe to wrap this entire logic in
an if-block that just does not execute when no IPv4 pool
is configured.

This avoids the warning and will also avoid any other
hidden side effect.

Reported-by: Gert Doering 
Signed-off-by: Antonio Quartulli 
---
 src/openvpn/multi.c | 50 -
 1 file changed, 27 insertions(+), 23 deletions(-)

diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
index 2fbbe9ec..99472f14 100644
--- a/src/openvpn/multi.c
+++ b/src/openvpn/multi.c
@@ -1504,36 +1504,40 @@ multi_select_virtual_addr(struct multi_context *m, 
struct multi_instance *mi)
   ? print_in6_addr( remote_ipv6, 0,  )
   : "(Not enabled)") );
 
-/* set push_ifconfig_remote_netmask from pool ifconfig address(es) 
*/
-mi->context.c2.push_ifconfig_local = remote;
-if (tunnel_type == DEV_TYPE_TAP || (tunnel_type == DEV_TYPE_TUN && 
tunnel_topology == TOP_SUBNET))
+if (mi->context.options.ifconfig_pool_defined)
 {
-mi->context.c2.push_ifconfig_remote_netmask = 
mi->context.options.ifconfig_pool_netmask;
-if (!mi->context.c2.push_ifconfig_remote_netmask)
+/* set push_ifconfig_remote_netmask from pool ifconfig 
address(es) */
+mi->context.c2.push_ifconfig_local = remote;
+if (tunnel_type == DEV_TYPE_TAP || (tunnel_type == 
DEV_TYPE_TUN && tunnel_topology == TOP_SUBNET))
 {
-mi->context.c2.push_ifconfig_remote_netmask = 
mi->context.c1.tuntap->remote_netmask;
+mi->context.c2.push_ifconfig_remote_netmask = 
mi->context.options.ifconfig_pool_netmask;
+if (!mi->context.c2.push_ifconfig_remote_netmask)
+{
+mi->context.c2.push_ifconfig_remote_netmask = 
mi->context.c1.tuntap->remote_netmask;
+}
 }
-}
-else if (tunnel_type == DEV_TYPE_TUN)
-{
-if (tunnel_topology == TOP_P2P)
+else if (tunnel_type == DEV_TYPE_TUN)
 {
-mi->context.c2.push_ifconfig_remote_netmask = 
mi->context.c1.tuntap->local;
+if (tunnel_topology == TOP_P2P)
+{
+mi->context.c2.push_ifconfig_remote_netmask = 
mi->context.c1.tuntap->local;
+}
+else if (tunnel_topology == TOP_NET30)
+{
+mi->context.c2.push_ifconfig_remote_netmask = local;
+}
 }
-else if (tunnel_topology == TOP_NET30)
+
+if (mi->context.c2.push_ifconfig_remote_netmask)
 {
-mi->context.c2.push_ifconfig_remote_netmask = local;
+mi->context.c2.push_ifconfig_defined = true;
+}
+else
+{
+msg(D_MULTI_ERRORS,
+"MULTI: no --ifconfig-pool netmask parameter is 
available to push to %s",
+multi_instance_string(mi, false, ));
 }
-}
-
-if (mi->context.c2.push_ifconfig_remote_netmask)
-{
-mi->context.c2.push_ifconfig_defined = true;
-}
-else
-{
-msg(D_MULTI_ERRORS, "MULTI: no --ifconfig-pool netmask 
parameter is available to push to %s",
-multi_instance_string(mi, false, ));
 }
 
 if (mi->context.options.ifconfig_ipv6_pool_defined)
-- 
2.27.0



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


Re: [Openvpn-devel] [PATCH] Simplify pool size handling, fix possible array overrun on pool reading.

2020-06-09 Thread Antonio Quartulli
e_to_handle(const struct 
> ifconfig_pool *pool,
> | in_addr->s6_addr[15];
>  
>  ret = addr - base;
> -if (ret < 0 || ret >= pool->ipv6.size)
> +if (ret < 0 || ret >= pool->size)
>  {
>  ret = -1;
>  }
> @@ -449,7 +435,7 @@ ifconfig_pool_handle_to_ip_base(const struct 
> ifconfig_pool *pool, ifconfig_pool_
>  {
>  in_addr_t ret = 0;
>  
> -if (pool->ipv4.enabled && hand >= 0 && hand < pool->ipv4.size)
> +if (pool->ipv4.enabled && hand >= 0 && hand < pool->size)
>  {
>  switch (pool->ipv4.type)
>  {
> @@ -479,7 +465,7 @@ ifconfig_pool_handle_to_ipv6_base(const struct 
> ifconfig_pool *pool, ifconfig_poo
>  struct in6_addr ret = IN6ADDR_ANY_INIT;
>  
>  /* IPv6 pools are always INDIV (--linear) */
> -if (pool->ipv6.enabled && hand >= 0 && hand < pool->ipv6.size)
> +if (pool->ipv6.enabled && hand >= 0 && hand < pool->size)
>  {
>  ret = add_in6_addr( pool->ipv6.base, hand );
>  }
> @@ -504,9 +490,9 @@ ifconfig_pool_list(const struct ifconfig_pool *pool, 
> struct status_output *out)
>  if (pool && out)
>  {
>  struct gc_arena gc = gc_new();
> -int i, pool_size = ifconfig_pool_size(pool);
> +int i;
>  
> -for (i = 0; i < pool_size; ++i)
> +for (i = 0; i < pool->size; ++i)
>  {
>  const struct ifconfig_pool_entry *e = >list[i];
>  struct in6_addr ip6;
> @@ -720,7 +706,7 @@ ifconfig_pool_read(struct ifconfig_pool_persist *persist, 
> struct ifconfig_pool *
>   */
>  if (h >= 0)
>  {
> -msg(M_INFO, "succeeded -> ifconfig_pool_set()");
> +msg(M_INFO, "succeeded -> ifconfig_pool_set(hand=%d)",h);
>  ifconfig_pool_set(pool, cn_buf, h, persist->fixed);
>  }
>  }
> diff --git a/src/openvpn/pool.h b/src/openvpn/pool.h
> index 6af04645..b06424c9 100644
> --- a/src/openvpn/pool.h
> +++ b/src/openvpn/pool.h
> @@ -55,13 +55,12 @@ struct ifconfig_pool
>  bool enabled;
>  enum pool_type type;
>  in_addr_t base;
> -int size;
>  } ipv4;
>  struct {
>  bool enabled;
>  struct in6_addr base;
> -unsigned int size;
>  } ipv6;
> +int size;
>  struct ifconfig_pool_entry *list;
>  };
>  
> 


The rest looks good, thanks!

I am ACKing this patch under the assumption that those little changes
are applied before merging:

Acked-by: Antonio Quartulli 


-- 
Antonio Quartulli


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


[Openvpn-devel] [PATCH v6] ipv6-pool: get rid of size constraint

2020-06-08 Thread Antonio Quartulli
Signed-off-by: Antonio Quartulli 

---
Changes from v5:
- restyle base addr computation to avoid odd line wrapping

Changes from v4:
- make the base computation depending on the size of the pool:
- large pools will still start at +0x1000 (backward compatible)
- smaller pools will start at +2

 src/openvpn/helper.c  | 13 +
 src/openvpn/options.c | 13 +
 src/openvpn/pool.c| 12 
 3 files changed, 30 insertions(+), 8 deletions(-)

diff --git a/src/openvpn/helper.c b/src/openvpn/helper.c
index 277e6972..fbfc287f 100644
--- a/src/openvpn/helper.c
+++ b/src/openvpn/helper.c
@@ -198,12 +198,17 @@ helper_client_server(struct options *o)
 print_in6_addr( add_in6_addr( o->server_network_ipv6, 2), 0, 
>gc );
 o->ifconfig_ipv6_netbits = o->server_netbits_ipv6;
 
-/* pool starts at "base address + 0x1000" - leave enough room */
-ASSERT( o->server_netbits_ipv6 <= 112 );/* want 16 bits */
+/* basic sanity check */
+ASSERT(o->server_netbits_ipv6 >= 64 && o->server_netbits_ipv6 <= 124);
 
 o->ifconfig_ipv6_pool_defined = true;
-o->ifconfig_ipv6_pool_base =
-add_in6_addr( o->server_network_ipv6, 0x1000 );
+/* For large enough pools we keep the original behaviour of adding
+ * 0x1000 when computing the base.
+ *
+ * Smaller pools can't get that far, therefore we just increase by 2
+ */
+o->ifconfig_ipv6_pool_base = add_in6_addr(o->server_network_ipv6,
+  o->server_netbits_ipv6 < 112 
? 0x1000 : 2);
 o->ifconfig_ipv6_pool_netbits = o->server_netbits_ipv6;
 
 push_option( o, "tun-ipv6", M_USAGE );
diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index 018f6f18..9d3a8dfe 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -6718,9 +6718,12 @@ add_option(struct options *options,
 msg(msglevel, "error parsing --server-ipv6 parameter");
 goto err;
 }
-if (netbits < 64 || netbits > 112)
+if (netbits < 64 || netbits > 124)
 {
-msg( msglevel, "--server-ipv6 settings: only /64../112 supported 
right now (not /%d)", netbits );
+msg(msglevel,
+"--server-ipv6 settings: network must be between /64 and /124 
(not /%d)",
+netbits);
+
 goto err;
 }
 options->server_ipv6_defined = true;
@@ -6840,9 +6843,11 @@ add_option(struct options *options,
 msg(msglevel, "error parsing --ifconfig-ipv6-pool parameters");
 goto err;
 }
-if (netbits < 64 || netbits > 112)
+if (netbits < 64 || netbits > 124)
 {
-msg( msglevel, "--ifconfig-ipv6-pool settings: only /64../112 
supported right now (not /%d)", netbits );
+msg(msglevel,
+"--ifconfig-ipv6-pool settings: network must be between /64 
and /124 (not /%d)",
+netbits);
 goto err;
 }
 
diff --git a/src/openvpn/pool.c b/src/openvpn/pool.c
index 4e303712..29667623 100644
--- a/src/openvpn/pool.c
+++ b/src/openvpn/pool.c
@@ -207,6 +207,12 @@ ifconfig_pool_init(const bool ipv4_pool, enum pool_type 
type, in_addr_t start,
 ASSERT(0);
 }
 
+if (pool->ipv4.size < 2)
+{
+msg(M_FATAL, "IPv4 pool size is too small (%d), must be at least 
2",
+pool->ipv4.size);
+}
+
 msg(D_IFCONFIG_POOL, "IFCONFIG POOL IPv4: base=%s size=%d",
 print_in_addr_t(pool->ipv4.base, 0, ), pool->ipv4.size);
 }
@@ -245,6 +251,12 @@ ifconfig_pool_init(const bool ipv4_pool, enum pool_type 
type, in_addr_t start,
   ? (1 << (128 - ipv6_netbits)) - base
   : IFCONFIG_POOL_MAX;
 
+if (pool->ipv6.size < 2)
+{
+msg(M_FATAL, "IPv6 pool size is too small (%d), must be at least 
2",
+pool->ipv6.size);
+}
+
 msg(D_IFCONFIG_POOL, "IFCONFIG POOL IPv6: base=%s size=%d netbits=%d",
 print_in6_addr(pool->ipv6.base, 0, ), pool->ipv6.size,
 ipv6_netbits);
-- 
2.27.0



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


[Openvpn-devel] [PATCH v5] ipv6-pool: get rid of size constraint

2020-06-08 Thread Antonio Quartulli
Signed-off-by: Antonio Quartulli 

---
Changes from v4:
- make the base computation depending on the size of the pool:
- large pools will still start at +0x1000 (backward compatible)
- smaller pools will start at +2

 src/openvpn/helper.c  | 21 +
 src/openvpn/options.c | 13 +
 src/openvpn/pool.c| 12 
 3 files changed, 38 insertions(+), 8 deletions(-)

diff --git a/src/openvpn/helper.c b/src/openvpn/helper.c
index 277e6972..e353d5b7 100644
--- a/src/openvpn/helper.c
+++ b/src/openvpn/helper.c
@@ -198,12 +198,25 @@ helper_client_server(struct options *o)
 print_in6_addr( add_in6_addr( o->server_network_ipv6, 2), 0, 
>gc );
 o->ifconfig_ipv6_netbits = o->server_netbits_ipv6;
 
-/* pool starts at "base address + 0x1000" - leave enough room */
-ASSERT( o->server_netbits_ipv6 <= 112 );/* want 16 bits */
+/* basic sanity check */
+ASSERT(o->server_netbits_ipv6 >= 64 && o->server_netbits_ipv6 <= 124);
 
 o->ifconfig_ipv6_pool_defined = true;
-o->ifconfig_ipv6_pool_base =
-add_in6_addr( o->server_network_ipv6, 0x1000 );
+/* For large enough pools we keep the original behaviour of adding
+ * 0x1000 when computing the base.
+ *
+ * Smaller pools can't get that far, therefore we just increase by 2
+ */
+if (o->server_netbits_ipv6 < 112)
+{
+o->ifconfig_ipv6_pool_base = add_in6_addr(o->server_network_ipv6,
+  0x1000);
+}
+else
+{
+o->ifconfig_ipv6_pool_base = add_in6_addr(o->server_network_ipv6,
+  2);
+}
 o->ifconfig_ipv6_pool_netbits = o->server_netbits_ipv6;
 
 push_option( o, "tun-ipv6", M_USAGE );
diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index 018f6f18..9d3a8dfe 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -6718,9 +6718,12 @@ add_option(struct options *options,
 msg(msglevel, "error parsing --server-ipv6 parameter");
 goto err;
 }
-if (netbits < 64 || netbits > 112)
+if (netbits < 64 || netbits > 124)
 {
-msg( msglevel, "--server-ipv6 settings: only /64../112 supported 
right now (not /%d)", netbits );
+msg(msglevel,
+"--server-ipv6 settings: network must be between /64 and /124 
(not /%d)",
+netbits);
+
 goto err;
 }
 options->server_ipv6_defined = true;
@@ -6840,9 +6843,11 @@ add_option(struct options *options,
 msg(msglevel, "error parsing --ifconfig-ipv6-pool parameters");
 goto err;
 }
-if (netbits < 64 || netbits > 112)
+if (netbits < 64 || netbits > 124)
 {
-msg( msglevel, "--ifconfig-ipv6-pool settings: only /64../112 
supported right now (not /%d)", netbits );
+msg(msglevel,
+"--ifconfig-ipv6-pool settings: network must be between /64 
and /124 (not /%d)",
+netbits);
 goto err;
 }
 
diff --git a/src/openvpn/pool.c b/src/openvpn/pool.c
index 4e303712..29667623 100644
--- a/src/openvpn/pool.c
+++ b/src/openvpn/pool.c
@@ -207,6 +207,12 @@ ifconfig_pool_init(const bool ipv4_pool, enum pool_type 
type, in_addr_t start,
 ASSERT(0);
 }
 
+if (pool->ipv4.size < 2)
+{
+msg(M_FATAL, "IPv4 pool size is too small (%d), must be at least 
2",
+pool->ipv4.size);
+}
+
 msg(D_IFCONFIG_POOL, "IFCONFIG POOL IPv4: base=%s size=%d",
 print_in_addr_t(pool->ipv4.base, 0, ), pool->ipv4.size);
 }
@@ -245,6 +251,12 @@ ifconfig_pool_init(const bool ipv4_pool, enum pool_type 
type, in_addr_t start,
   ? (1 << (128 - ipv6_netbits)) - base
   : IFCONFIG_POOL_MAX;
 
+if (pool->ipv6.size < 2)
+{
+msg(M_FATAL, "IPv6 pool size is too small (%d), must be at least 
2",
+pool->ipv6.size);
+}
+
 msg(D_IFCONFIG_POOL, "IFCONFIG POOL IPv6: base=%s size=%d netbits=%d",
 print_in6_addr(pool->ipv6.base, 0, ), pool->ipv6.size,
 ipv6_netbits);
-- 
2.27.0



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


Re: [Openvpn-devel] [PATCH v4 7/7] ipv6-pool: get rid of size constraint

2020-06-08 Thread Antonio Quartulli
Hi,

On 07/06/2020 12:59, Gert Doering wrote:
> My idea would be to make this conditional on the pool size - if the size
> is /64.../111, make it +0x1000 ("keep existing behaviour for large-enough
> pools"), if it's /112.../124, make it +2
> 
> Thoughts?

I think it makes sense, especially to preserve the current behaviour on
already deployed setups.

New patch incoming!


-- 
Antonio Quartulli


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


[Openvpn-devel] [PATCH v5] options: enable IPv4 redirection logic only if really required

2020-06-08 Thread Antonio Quartulli
From: Antonio Quartulli 

If no IPv4 redirection flag is set, do not enable the IPv4
redirection logic at all so that it won't bother adding any
useless IPv4 route.

Trac: #208
Signed-off-by: Antonio Quartulli 

---
Changes from v4:
- add warning about undefined behaviour when specifying
  redirect-gateway/private at the same time
- fix behaviour of redirect-private

Changes from v3:
- move error message modification to previous patch

Changes from v2:
- patchset rebased on top of pre-ipv6-only patchset
---
 src/openvpn/options.c | 15 +--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index 7556e7ee..018f6f18 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -6542,6 +6542,18 @@ add_option(struct options *options,
 int j;
 VERIFY_PERMISSION(OPT_P_ROUTE);
 rol_check_alloc(options);
+
+if (options->routes->flags & RG_ENABLE)
+{
+msg(M_WARN,
+"WARNING: You have specified redirect-gateway and "
+"redirect-private at the same time (or the same option "
+"multiple times). This is not well supported and may lead to "
+"unexpected results");
+}
+
+options->routes->flags |= RG_ENABLE;
+
 if (streq(p[0], "redirect-gateway"))
 {
 options->routes->flags |= RG_REROUTE_GW;
@@ -6579,7 +6591,7 @@ add_option(struct options *options,
 }
 else if (streq(p[j], "!ipv4"))
 {
-options->routes->flags &= ~RG_REROUTE_GW;
+options->routes->flags &= ~(RG_REROUTE_GW | RG_ENABLE);
 }
 else
 {
@@ -6591,7 +6603,6 @@ add_option(struct options *options,
 /* we need this here to handle pushed --redirect-gateway */
 remap_redirect_gateway_flags(options);
 #endif
-options->routes->flags |= RG_ENABLE;
 }
 else if (streq(p[0], "block-ipv6") && !p[1])
 {
-- 
2.27.0



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


[Openvpn-devel] [PATCH v5] pool: add support for ifconfig-pool-persist with IPv6 only

2020-06-06 Thread Antonio Quartulli
From: Antonio Quartulli 

Without altering the pool logic, this patch enables using
a persistent IP pool also when the server is configured
with IPv6 only.

Trac: #208
Signed-off-by: Antonio Quartulli 

---
Changes from v4:
- prevent persist-pool parser from bailing out when only IPv4 addresses
  are used
- simplify ifconfig_pool_set() function
- re-arrange ifconfig_pool_read() function to better deal with all cases
  (i.e. only ipv6 or invalid ipv4 but not ipv6..etc..)

Changes from v3:
- patchset rebased on top of pre-ipv6-only patchset
---
 src/openvpn/options.c |   7 +-
 src/openvpn/pool.c| 208 --
 2 files changed, 165 insertions(+), 50 deletions(-)

diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index 4f03a27e..327207bd 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -2346,9 +2346,12 @@ options_postprocess_verify_ce(const struct options 
*options, const struct connec
 {
 msg(M_USAGE, "--up-delay cannot be used with --mode server");
 }
-if (!options->ifconfig_pool_defined && 
options->ifconfig_pool_persist_filename)
+if (!options->ifconfig_pool_defined
+&& !options->ifconfig_ipv6_pool_defined
+&& options->ifconfig_pool_persist_filename)
 {
-msg(M_USAGE, "--ifconfig-pool-persist must be used with 
--ifconfig-pool");
+msg(M_USAGE,
+"--ifconfig-pool-persist must be used with --ifconfig-pool or 
--ifconfig-ipv6-pool");
 }
 if (options->ifconfig_ipv6_pool_defined && 
!options->ifconfig_ipv6_local)
 {
diff --git a/src/openvpn/pool.c b/src/openvpn/pool.c
index a7981b8d..29667623 100644
--- a/src/openvpn/pool.c
+++ b/src/openvpn/pool.c
@@ -34,6 +34,7 @@
 #include "error.h"
 #include "socket.h"
 #include "otime.h"
+#include "options.h"
 
 #include "memdbg.h"
 
@@ -403,12 +404,52 @@ ifconfig_pool_ip_base_to_handle(const struct 
ifconfig_pool *pool, const in_addr_
 return ret;
 }
 
+static ifconfig_pool_handle
+ifconfig_pool_ipv6_base_to_handle(const struct ifconfig_pool *pool,
+  const struct in6_addr *in_addr)
+{
+ifconfig_pool_handle ret;
+uint32_t base, addr;
+
+/* IPv6 pool is always IFCONFIG_POOL_INDIV.
+ *
+ * We assume the offset can't be larger than 2^32-1, therefore we compute
+ * the difference only among the last 4 bytes like if they were two 32bit
+ * long integers. The rest of the address must match.
+ */
+for (int i = 0; i < (12); i++)
+{
+if (pool->ipv6.base.s6_addr[i] != in_addr->s6_addr[i])
+{
+return -1;
+}
+}
+
+base = (pool->ipv6.base.s6_addr[12] << 24)
+   | (pool->ipv6.base.s6_addr[13] << 16)
+   | (pool->ipv6.base.s6_addr[14] << 8)
+   | pool->ipv6.base.s6_addr[15];
+
+addr = (in_addr->s6_addr[12] << 24)
+   | (in_addr->s6_addr[13] << 16)
+   | (in_addr->s6_addr[14] << 8)
+   | in_addr->s6_addr[15];
+
+ret = addr - base;
+if (ret < 0 || ret >= pool->ipv6.size)
+{
+ret = -1;
+}
+
+return ret;
+}
+
 static in_addr_t
 ifconfig_pool_handle_to_ip_base(const struct ifconfig_pool *pool, 
ifconfig_pool_handle hand)
 {
 in_addr_t ret = 0;
 
-if (hand >= 0 && hand < pool->ipv4.size)
+if (pool->ipv4.enabled && hand >= 0 && hand < pool->ipv4.size)
 {
 switch (pool->ipv4.type)
 {
@@ -435,10 +476,10 @@ ifconfig_pool_handle_to_ip_base(const struct 
ifconfig_pool *pool, ifconfig_pool_
 static struct in6_addr
 ifconfig_pool_handle_to_ipv6_base(const struct ifconfig_pool *pool, 
ifconfig_pool_handle hand)
 {
-struct in6_addr ret = in6addr_any;
+struct in6_addr ret = IN6ADDR_ANY_INIT;
 
 /* IPv6 pools are always INDIV (--linear) */
-if (hand >= 0 && hand < pool->ipv6.size)
+if (pool->ipv6.enabled && hand >= 0 && hand < pool->ipv6.size)
 {
 ret = add_in6_addr( pool->ipv6.base, hand );
 }
@@ -446,18 +487,15 @@ ifconfig_pool_handle_to_ipv6_base(const struct 
ifconfig_pool *pool, ifconfig_poo
 }
 
 static void
-ifconfig_pool_set(struct ifconfig_pool *pool, const char *cn, const in_addr_t 
addr, const bool fixed)
+ifconfig_pool_set(struct ifconfig_pool *pool, const char *cn,
+  ifconfig_pool_handle h, const bool fixed)
 {
-ifconfig_pool_handle h = ifconfig_pool_ip_base_to_handle(pool, addr);
-if (h >= 0)
-{
-struct ifconfig_pool_entry *e = >list[h];
-ifconfig_pool_entry_free(e, true);
-e->in_use = false;
-e->common_name = string_alloc(cn, NULL);
-e->last

Re: [Openvpn-devel] [PATCH v2 3/3] Implement tls-groups option to specify eliptic curves/groups

2020-06-04 Thread Antonio Quartulli
ASSERT(ctx);
> +struct gc_arena gc = gc_new();
> +
> +/* Get number of groups and allocate an array in ctx */
> +int groups_count = get_num_elements(groups, ':');
> +ALLOC_ARRAY_CLEAR(ctx->groups, mbedtls_ecp_group_id, groups_count + 1)
> +
> +/* Parse allowed ciphers, getting IDs */
> +int i = 0;
> +char *tmp_groups = string_alloc(groups, );
> +
> +const char *token = strsep(_groups, ":");
> +while (token)

Couldn't we avoid having the assignment here and at the end of the loop
by doing:

const char *token;
while ((token = strsep(_groups, ":"))

?

> +{
> +const mbedtls_ecp_curve_info *ci =
> +mbedtls_ecp_curve_info_from_name(token);
> +if (!ci)
> +{
> +msg(M_WARN, "Warning unknown curve/group specified: %s", token);
> +}
> +else
> +{
> +ctx->groups[i] = ci->grp_id;
> +i++;
> +}
> +token = strsep(_groups, ":");
> +}
> +ctx->groups[i] = MBEDTLS_ECP_DP_NONE;
> +
> +gc_free();
> +}
> +
> +
>  void
>  tls_ctx_check_cert_time(const struct tls_root_ctx *ctx)
>  {
> @@ -1043,6 +1084,11 @@ key_state_ssl_init(struct key_state_ssl *ks_ssl,
>  mbedtls_ssl_conf_ciphersuites(ks_ssl->ssl_config, 
> ssl_ctx->allowed_ciphers);
>  }
>  
> +if (ssl_ctx->groups)
> +{
> +mbedtls_ssl_conf_curves(_ssl->ssl_config, ssl_ctx->groups);

the '&' should not be there I guess.

/usr/include/mbedtls/ssl.h:2925:51: note: expected ‘mbedtls_ssl_config
*’ but argument is of type ‘mbedtls_ssl_config **’


> +}
> +
>  /* Disable record splitting (for now).  OpenVPN assumes records are sent
>   * unfragmented, and changing that will require thorough review and
>   * testing.  Since OpenVPN is not susceptible to BEAST, we can just
> diff --git a/src/openvpn/ssl_mbedtls.h b/src/openvpn/ssl_mbedtls.h
> index 92381f1a..1dc28313 100644
> --- a/src/openvpn/ssl_mbedtls.h
> +++ b/src/openvpn/ssl_mbedtls.h
> @@ -105,6 +105,7 @@ struct tls_root_ctx {
>  #endif
>  struct external_context external_key; /**< External key context */
>  int *allowed_ciphers;   /**< List of allowed ciphers for this 
> connection */
> +mbedtls_ecp_group_id *groups;/**< List of allowed groups 
> for this connection */

Maybe you can reduce the space before the comment? :-D

>  mbedtls_x509_crt_profile cert_profile; /**< Allowed certificate types */
>  };
>  
> diff --git a/src/openvpn/ssl_openssl.c b/src/openvpn/ssl_openssl.c
> index d7bd6aa2..06971d63 100644
> --- a/src/openvpn/ssl_openssl.c
> +++ b/src/openvpn/ssl_openssl.c
> @@ -557,6 +557,58 @@ tls_ctx_set_cert_profile(struct tls_root_ctx *ctx, const 
> char *profile)
>  #endif /* ifdef HAVE_SSL_CTX_SET_SECURITY_LEVEL */
>  }
>  
> +void
> +tls_ctx_set_tls_groups(struct tls_root_ctx *ctx, const char *groups)
> +{
> +ASSERT(ctx);
> +struct gc_arena gc = gc_new();
> +/* This method could be as easy as
> + *  SSL_CTX_set1_groups_list(ctx->ctx, groups)
> + * but OpenSSL does not like the name secp256r1 for prime256v1
> + * This is one of the important curves.
> + * To support the same name for OpenSSL and mbedTLS, we do
> + * this dance.
> + */
> +
> +int groups_count = get_num_elements(groups, ':');
> +
> +int *glist;
> +/* Allocate an array for them */
> +ALLOC_ARRAY_CLEAR_GC(glist, int, groups_count, );
> +
> +/* Parse allowed ciphers, getting IDs */
> +int glistlen = 0;
> +char *tmp_groups = string_alloc(groups, );
> +
> +const char *token = strsep(_groups, ":");
> +while (token)

same suggestion as above

> +{
> +if (streq(token, "secp256r1"))
> +{
> +token = "prime256v1";
> +}
> +int nid = OBJ_sn2nid(token);
> +
> +if (nid == 0)
> +{
> +msg(M_WARN, "Warning unknown curve/group specified: %s", token);
> +}
> +else
> +{
> +glist[glistlen] = nid;
> +glistlen++;
> +}
> +token = strsep(_groups, ":");
> +}
> +
> +if (!SSL_CTX_set1_groups(ctx->ctx, glist, glistlen))
> +{
> +crypto_msg(M_FATAL, "Failed to set allowed TLS group list: %s",
> +   groups);
> +}
> +gc_free();
> +}
> +
>  void
>  tls_ctx_check_cert_time(const struct tls_root_ctx *ctx)
>  {
> @@ -2179,6 +2231,8 @@ show_available_tls_ciphers_list(const char *cipher_list,
>  void
>  show_available_curves(void)
>  {
> +printf("Consider using openssl ecparam -list_curves as\n"
> +   "alternative to running this command.");

How about putting single quotes around the command? At first I thought
that "as" was also part of the openssl options :-D


>  #ifndef OPENSSL_NO_EC
>  EC_builtin_curve *curves = NULL;
>  size_t crv_len = 0;
> @@ -2188,7 +2242,7 @@ show_available_curves(void)
>  ALLOC_ARRAY(curves, EC_builtin_curve, crv_len);
>  if (EC_get_builtin_curves(curves, crv_len))
>  {
> -printf("Available Elliptic curves:\n");
> +printf("\nAvailable Elliptic curves/groups:\n");
>  for (n = 0; n < crv_len; n++)
>  {
>  const char *sname;
> 


The rest looks good.

I applied this patch on top of master with git am -3 because there are
some trivial conflicts to fix.


Cheers,

-- 
Antonio Quartulli


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


[Openvpn-devel] [PATCH v5] pool: allow to configure an IPv6-only ifconfig-pool

2020-06-01 Thread Antonio Quartulli
From: Antonio Quartulli 

With this change a server is allowed to allocate an
IPv6-only pool. This is required to make it capable
of managing an IPv6-only tunnel.

Trac: #208
Signed-off-by: Antonio Quartulli 

---
Changes from v4:
- make 'IFCONFIG POOL' message symmetric across IPv4 and IPv6
- avoid crash when ifconfig_pool_size() is invoked with NULL poll
  (will create a trivial conflict with "ipv6-pool: get rid of size constraint")

Changes from v3:
- properly compute pool size taking into account the actual base address
---
 src/openvpn/multi.c |  10 ++-
 src/openvpn/pool.c  | 181 
 src/openvpn/pool.h  |   8 +-
 3 files changed, 146 insertions(+), 53 deletions(-)

diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
index 7f61350d..2fbbe9ec 100644
--- a/src/openvpn/multi.c
+++ b/src/openvpn/multi.c
@@ -388,7 +388,8 @@ multi_init(struct multi_context *m, struct context *t, bool 
tcp_mode, int thread
  * differently based on whether a tun or tap style
  * tunnel.
  */
-if (t->options.ifconfig_pool_defined)
+if (t->options.ifconfig_pool_defined
+|| t->options.ifconfig_ipv6_pool_defined)
 {
 int pool_type = IFCONFIG_POOL_INDIV;
 
@@ -397,7 +398,8 @@ multi_init(struct multi_context *m, struct context *t, bool 
tcp_mode, int thread
 pool_type = IFCONFIG_POOL_30NET;
 }
 
-m->ifconfig_pool = ifconfig_pool_init(pool_type,
+m->ifconfig_pool = ifconfig_pool_init(t->options.ifconfig_pool_defined,
+  pool_type,
   t->options.ifconfig_pool_start,
   t->options.ifconfig_pool_end,
   t->options.duplicate_cn,
@@ -1495,7 +1497,9 @@ multi_select_virtual_addr(struct multi_context *m, struct 
multi_instance *mi)
 const int tunnel_topology = TUNNEL_TOPOLOGY(mi->context.c1.tuntap);
 
 msg( M_INFO, "MULTI_sva: pool returned IPv4=%s, IPv6=%s",
- print_in_addr_t( remote, 0,  ),
+ (mi->context.options.ifconfig_pool_defined
+  ? print_in_addr_t(remote, 0, )
+  : "(Not enabled)"),
  (mi->context.options.ifconfig_ipv6_pool_defined
   ? print_in6_addr( remote_ipv6, 0,  )
   : "(Not enabled)") );
diff --git a/src/openvpn/pool.c b/src/openvpn/pool.c
index 6dd72bb9..6e35fe04 100644
--- a/src/openvpn/pool.c
+++ b/src/openvpn/pool.c
@@ -58,6 +58,29 @@ ifconfig_pool_entry_free(struct ifconfig_pool_entry *ipe, 
bool hard)
 }
 }
 
+static const int
+ifconfig_pool_size(const struct ifconfig_pool *pool)
+{
+int min = INT_MAX;
+
+if (!pool || (!pool->ipv4.enabled && !pool->ipv6.enabled))
+{
+return 0;
+}
+
+if (pool->ipv4.enabled)
+{
+min = pool->ipv4.size;
+}
+
+if (pool->ipv6.enabled && pool->ipv6.size < min)
+{
+min = pool->ipv6.size;
+}
+
+return min;
+}
+
 static int
 ifconfig_pool_find(struct ifconfig_pool *pool, const char *common_name)
 {
@@ -65,8 +88,9 @@ ifconfig_pool_find(struct ifconfig_pool *pool, const char 
*common_name)
 time_t earliest_release = 0;
 int previous_usage = -1;
 int new_usage = -1;
+int pool_size = ifconfig_pool_size(pool);
 
-for (i = 0; i < pool->ipv4.size; ++i)
+for (i = 0; i < pool_size; ++i)
 {
 struct ifconfig_pool_entry *ipe = >list[i];
 if (!ipe->in_use)
@@ -147,34 +171,43 @@ ifconfig_pool_verify_range(const int msglevel, const 
in_addr_t start, const in_a
 }
 
 struct ifconfig_pool *
-ifconfig_pool_init(enum pool_type type, in_addr_t start, in_addr_t end,
-   const bool duplicate_cn,
+ifconfig_pool_init(const bool ipv4_pool, enum pool_type type, in_addr_t start,
+   in_addr_t end, const bool duplicate_cn,
const bool ipv6_pool, const struct in6_addr ipv6_base,
const int ipv6_netbits )
 {
 struct gc_arena gc = gc_new();
 struct ifconfig_pool *pool = NULL;
+int pool_size = -1;
 
 ASSERT(start <= end && end - start < IFCONFIG_POOL_MAX);
 ALLOC_OBJ_CLEAR(pool, struct ifconfig_pool);
 
-pool->ipv4.type = type;
 pool->duplicate_cn = duplicate_cn;
 
-switch (pool->ipv4.type)
+pool->ipv4.enabled = ipv4_pool;
+
+if (pool->ipv4.enabled)
 {
-case IFCONFIG_POOL_30NET:
-pool->ipv4.base = start & ~3;
-pool->ipv4.size = (((end | 3) + 1) - pool->ipv4.base) >> 2;
-break;
+pool->ipv4.type = type;
+switch (pool->ipv4.type)
+{
+case IFCONFIG_POOL_30NET:
+pool->ipv4.base = start & ~3;
+

  1   2   3   4   5   6   7   >