Re: [Openvpn-devel] OpenVPN3 thread safety

[Arne Schwabe]

Am 12.11.2023 um 14:16 schrieb Savely Krasovsky:

Hello!

I am trying to use OpenVPN3 with Golang SWIG binding. It works pretty nice, but 
I have random segmentation faults without obvious reason. My current guess is 
that Golang calls OpenVPN3 from various threads and library is not ready for 
that sometime. Am I right? Documentation doesn't say a lot about threading.

If this is a case, I have two possible solutions:
  1. Use OPENVPN_ENABLE_BIGMUTEX build flag (I am not sure I need that).
  2. Move all calls to OpenVPN3 into single goroutine and use 
runtime.LockOSThread().

Could you please clarify, should I get segfaults in that case or not and the 
problems is unrelated.



Yes. The client is singlethreaded and expects to be called only from a 
single thread. You can have multiple clients in multiple threads but all 
our own software uses just a single thread for the client itself.



Arne



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

Re: [Openvpn-devel] OpenVPN3 thread safety

[Savely Krasovsky]

Thanks for the reply!

After few days I have found a major bug: a close and free client before stop() 
successfully executes. After fixing this I didn't get any segfaults anymore. 
Program survives very extensive tests. But after your reply I am in question 
again... Sure, openvpn3 has single thread after Connect(), but Go still could 
call Stop() or any other method from another thread. And still I didn't get any 
issues at the moment. Should I rewrite it anyway?

Пн, 20.11.2023 в 9:55 Arne Schwabe писал(а):
> Am 12.11.2023 um 14:16 schrieb Savely Krasovsky:
>> Hello!
>>
>> I am trying to use OpenVPN3 with Golang SWIG binding. It works pretty nice, 
>> but I have random segmentation faults without obvious reason. My current 
>> guess is that Golang calls OpenVPN3 from various threads and library is not 
>> ready for that sometime. Am I right? Documentation doesn't say a lot about 
>> threading.
>>
>> If this is a case, I have two possible solutions:
>>   1. Use OPENVPN_ENABLE_BIGMUTEX build flag (I am not sure I need that).
>>   2. Move all calls to OpenVPN3 into single goroutine and use 
>> runtime.LockOSThread().
>>
>> Could you please clarify, should I get segfaults in that case or not and the 
>> problems is unrelated.
>
>
> Yes. The client is singlethreaded and expects to be called only from a 
> single thread. You can have multiple clients in multiple threads but all 
> our own software uses just a single thread for the client itself.
>
>
> Arne

-- 
Best regards,
Savely Krasovsky


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

Re: [Openvpn-devel] OpenVPN3 thread safety

[Antonio Quartulli]

Hi,

On 20/11/2023 10:11, Savely Krasovsky wrote:

Thanks for the reply!

After few days I have found a major bug: a close and free client before stop() 
successfully executes. After fixing this I didn't get any segfaults anymore. 
Program survives very extensive tests. But after your reply I am in question 
again... Sure, openvpn3 has single thread after Connect(), but Go still could 
call Stop() or any other method from another thread. And still I didn't get any 
issues at the moment. Should I rewrite it anyway?




Calling from different threads is not an issue.
Invoking multiple methods at the same time is.

If you call openvpn3 code from multiple threads, it may be better for 
you to implement your own locking system "outside" of the library which 
makes sure no more than one caller is running openvpn3 code at a given 
moment.


Cheers,


Пн, 20.11.2023 в 9:55 Arne Schwabe писал(а):

Am 12.11.2023 um 14:16 schrieb Savely Krasovsky:

Hello!

I am trying to use OpenVPN3 with Golang SWIG binding. It works pretty nice, but 
I have random segmentation faults without obvious reason. My current guess is 
that Golang calls OpenVPN3 from various threads and library is not ready for 
that sometime. Am I right? Documentation doesn't say a lot about threading.

If this is a case, I have two possible solutions:
   1. Use OPENVPN_ENABLE_BIGMUTEX build flag (I am not sure I need that).
   2. Move all calls to OpenVPN3 into single goroutine and use 
runtime.LockOSThread().

Could you please clarify, should I get segfaults in that case or not and the 
problems is unrelated.



Yes. The client is singlethreaded and expects to be called only from a
single thread. You can have multiple clients in multiple threads but all
our own software uses just a single thread for the client itself.


Arne




--
Antonio Quartulli


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

Re: [Openvpn-devel] OpenVPN3 thread safety

[Savely Krasovsky]

That's exactly what I did! Works perfectly, thanks for calming me down.

Пн, 20.11.2023 в 10:37 Antonio Quartulli писал(а):
> Hi,
>
> On 20/11/2023 10:11, Savely Krasovsky wrote:
>> Thanks for the reply!
>> 
>> After few days I have found a major bug: a close and free client before 
>> stop() successfully executes. After fixing this I didn't get any segfaults 
>> anymore. Program survives very extensive tests. But after your reply I am in 
>> question again... Sure, openvpn3 has single thread after Connect(), but Go 
>> still could call Stop() or any other method from another thread. And still I 
>> didn't get any issues at the moment. Should I rewrite it anyway?
>> 
>
>
> Calling from different threads is not an issue.
> Invoking multiple methods at the same time is.
>
> If you call openvpn3 code from multiple threads, it may be better for 
> you to implement your own locking system "outside" of the library which 
> makes sure no more than one caller is running openvpn3 code at a given 
> moment.
>
> Cheers,
>
>> Пн, 20.11.2023 в 9:55 Arne Schwabe писал(а):
>>> Am 12.11.2023 um 14:16 schrieb Savely Krasovsky:
 Hello!

 I am trying to use OpenVPN3 with Golang SWIG binding. It works pretty 
 nice, but I have random segmentation faults without obvious reason. My 
 current guess is that Golang calls OpenVPN3 from various threads and 
 library is not ready for that sometime. Am I right? Documentation doesn't 
 say a lot about threading.

 If this is a case, I have two possible solutions:
1. Use OPENVPN_ENABLE_BIGMUTEX build flag (I am not sure I need that).
2. Move all calls to OpenVPN3 into single goroutine and use 
 runtime.LockOSThread().

 Could you please clarify, should I get segfaults in that case or not and 
 the problems is unrelated.
>>>
>>>
>>> Yes. The client is singlethreaded and expects to be called only from a
>>> single thread. You can have multiple clients in multiple threads but all
>>> our own software uses just a single thread for the client itself.
>>>
>>>
>>> Arne
>> 
>
> -- 
> Antonio Quartulli

-- 
Best regards,
Savely Krasovsky


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

[Openvpn-devel] [S] Change in openvpn[master]: protocol_dump: tls-crypt support

[plaisthos (Code Review)]

Attention is currently required from: flichtenheld.

Hello flichtenheld,

I'd like you to do a code review.
Please visit

http://gerrit.openvpn.net/c/openvpn/+/442?usp=email

to review the following change.


Change subject: protocol_dump: tls-crypt support
..

protocol_dump: tls-crypt support

Change-Id: Ie25aa287f3534090c1d93fc3bb69727dd20fc6fe
---
M src/openvpn/openvpn.h
M src/openvpn/ssl.c
M src/openvpn/ssl.h
3 files changed, 29 insertions(+), 1 deletion(-)



  git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/42/442/1

diff --git a/src/openvpn/openvpn.h b/src/openvpn/openvpn.h
index 5b2be63..dabc5be 100644
--- a/src/openvpn/openvpn.h
+++ b/src/openvpn/openvpn.h
@@ -541,7 +541,8 @@
 #define PROTO_DUMP(buf, gc) protocol_dump((buf), \
   PROTO_DUMP_FLAGS   \
   |(c->c2.tls_multi ? PD_TLS : 0)   \
-  |(c->options.tls_auth_file ? 
md_kt_size(c->c1.ks.key_type.digest) : 0), \
+  |(c->options.tls_auth_file ? 
md_kt_size(c->c1.ks.key_type.digest) : 0) \
+  |(c->options.tls_crypt_file || 
c->options.tls_crypt_v2_file ? PD_TLS_CRYPT : 0), \
   gc)

 /* this represents "disabled peer-id" */
diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
index b4cd8f5..400230c 100644
--- a/src/openvpn/ssl.c
+++ b/src/openvpn/ssl.c
@@ -4272,6 +4272,32 @@
 }
 buf_printf(&out, " pid=%s", packet_id_net_print(&pin, (flags & 
PD_VERBOSE), gc));
 }
+/*
+ * packet_id + tls-crypt hmac
+ */
+if (flags & PD_TLS_CRYPT)
+{
+struct packet_id_net pin;
+uint8_t tls_crypt_hmac[TLS_CRYPT_TAG_SIZE];
+
+if (!packet_id_read(&pin, &buf, true))
+{
+goto done;
+}
+buf_printf(&out, " pid=%s", packet_id_net_print(&pin, (flags & 
PD_VERBOSE), gc));
+if (!buf_read(&buf, tls_crypt_hmac, TLS_CRYPT_TAG_SIZE))
+{
+goto done;
+}
+if (flags & PD_VERBOSE)
+{
+buf_printf(&out, " tls_crypt_hmac=%s", format_hex(tls_crypt_hmac, 
TLS_CRYPT_TAG_SIZE, 0, gc));
+}
+/*
+ * Remainder is encrypted and optional wKc
+ */
+goto done;
+}

 /*
  * ACK list
diff --git a/src/openvpn/ssl.h b/src/openvpn/ssl.h
index 3c40fbe..e842746 100644
--- a/src/openvpn/ssl.h
+++ b/src/openvpn/ssl.h
@@ -525,6 +525,7 @@
 #define PD_SHOW_DATA   (1<<8)
 #define PD_TLS (1<<9)
 #define PD_VERBOSE (1<<10)
+#define PD_TLS_CRYPT   (1<<11)

 const char *protocol_dump(struct buffer *buffer,
   unsigned int flags,

--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/442?usp=email
To unsubscribe, or for help writing mail filters, visit 
http://gerrit.openvpn.net/settings

Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: Ie25aa287f3534090c1d93fc3bb69727dd20fc6fe
Gerrit-Change-Number: 442
Gerrit-PatchSet: 1
Gerrit-Owner: plaisthos 
Gerrit-Reviewer: flichtenheld 
Gerrit-CC: openvpn-devel 
Gerrit-Attention: flichtenheld 
Gerrit-MessageType: newchange
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Re: [Openvpn-devel] [PATCH] protocol_dump: support tls-crypt

[Arne Schwabe]

Am 30.10.23 um 11:58 schrieb Reynir:

Dear list,

Please find attached a patch to add support for tls-crypt packets in 
protocol_dump. Currently, protocol_dump will print garbage for tls-crypt 
packets.


This patch makes protocol_dump print the clear text parts of the packet 
such as the auth tag and replay packet id. It does not try to print the 
wKc for HARD_RESET_CLIENT_V3 or CONTROL_WKC_V1 packets. A previous 
iteration, not submitted to the list, printed ENCRYPTED placeholders for 
ack list and DATA, but I decided to cut down on the noise instead.


This is my first patch submitted to openvpn so please bear with me.



Code looks good and works fine here.

Acked-By: Arne Schwabe 



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

[Openvpn-devel] [S] Change in openvpn[master]: Remove unused defined from configure and cmake config

[plaisthos (Code Review)]

Attention is currently required from: flichtenheld.

Hello flichtenheld,

I'd like you to do a code review.
Please visit

http://gerrit.openvpn.net/c/openvpn/+/443?usp=email

to review the following change.


Change subject: Remove unused defined from configure and cmake config
..

Remove unused defined from configure and cmake config

Change-Id: Ifd0376b36d4050dc22bc93b8fcf7ed29faef0021
---
M config.h.cmake.in
M configure.ac
2 files changed, 1 insertion(+), 34 deletions(-)



  git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/43/443/1

diff --git a/config.h.cmake.in b/config.h.cmake.in
index 19b79bc..25273ac 100644
--- a/config.h.cmake.in
+++ b/config.h.cmake.in
@@ -62,9 +62,6 @@
 /* Enable --x509-username-field feature */
 #cmakedefine ENABLE_X509ALTUSERNAME

-/* Compiler supports anonymous unions */
-#define HAVE_ANONYMOUS_UNION_SUPPORT
-
 /* Define to 1 if you have the  header file. */
 #cmakedefine HAVE_ARPA_INET_H 1

@@ -139,9 +136,6 @@
 /* Define to 1 if you have the `getpwnam' function. */
 #cmakedefine HAVE_GETPWNAM

-/* Define to 1 if you have the `getrlimit' function. */
-#undef HAVE_GETRLIMIT
-
 /* Define to 1 if you have the `getsockname' function. */
 #cmakedefine HAVE_GETSOCKNAME

@@ -235,8 +229,6 @@
 /* Define to 1 if you have the  header file. */
 #cmakedefine HAVE_PWD_H

-/* Define to 1 if you have the `readv' function. */
-#undef HAVE_READV

 /* Define to 1 if you have the `recvmsg' function. */
 #cmakedefine HAVE_RECVMSG
@@ -383,9 +375,6 @@
 /* Define to 1 if you have the  header file. */
 #undef HAVE_VFORK_H

-/* Define to 1 if you have the `vsnprintf' function. */
-#undef HAVE_VSNPRINTF
-
 /* we always assume a recent mbed TLS version */
 #define HAVE_MBEDTLS_PSA_CRYPTO_H 1
 #define HAVE_MBEDTLS_SSL_TLS_PRF 1
diff --git a/configure.ac b/configure.ac
index 84eaad6..055b2a7 100644
--- a/configure.ac
+++ b/configure.ac
@@ -561,28 +561,6 @@
,
[[${SOCKET_INCLUDES}]]
 )
-AC_MSG_CHECKING([anonymous union support])
-AC_COMPILE_IFELSE(
-   [AC_LANG_PROGRAM(
-   [[
-   struct mystruct {
- union {
-   int m1;
-   char m2;
- };
-   };
-   ]],
-   [[
-   struct mystruct s;
-   s.m1 = 1; s.m2 = 2;
-   ]]
-   )],
-   [
-   AC_MSG_RESULT([yes])
-   AC_DEFINE([HAVE_ANONYMOUS_UNION_SUPPORT], [], [Compiler 
supports anonymous unions])
-   ],
-   [AC_MSG_RESULT([no])]
-)

 saved_LDFLAGS="$LDFLAGS"
 LDFLAGS="$LDFLAGS -Wl,--wrap=exit"
@@ -655,7 +633,7 @@
 AC_CHECK_FUNCS([ \
daemon chroot getpwnam setuid nice system dup dup2 \
syslog openlog mlockall getrlimit getgrnam setgid \
-   setgroups flock readv writev time gettimeofday \
+   setgroups flock time gettimeofday \
setsid chdir \
chsize ftruncate execve getpeereid basename dirname access \
epoll_create strsep \

--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/443?usp=email
To unsubscribe, or for help writing mail filters, visit 
http://gerrit.openvpn.net/settings

Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: Ifd0376b36d4050dc22bc93b8fcf7ed29faef0021
Gerrit-Change-Number: 443
Gerrit-PatchSet: 1
Gerrit-Owner: plaisthos 
Gerrit-Reviewer: flichtenheld 
Gerrit-CC: openvpn-devel 
Gerrit-Attention: flichtenheld 
Gerrit-MessageType: newchange
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

[Openvpn-devel] [S] Change in openvpn[master]: Allow specifying custom mbed TLS directories with CMake

[plaisthos (Code Review)]

Attention is currently required from: flichtenheld, plaisthos.

Hello flichtenheld,

I'd like you to reexamine a change. Please visit

http://gerrit.openvpn.net/c/openvpn/+/377?usp=email

to look at the new patch set (#2).

The following approvals got outdated and were removed:
Code-Review+1 by flichtenheld


Change subject: Allow specifying custom mbed TLS directories with CMake
..

Allow specifying custom mbed TLS directories with CMake

When installing mbed TLS 2.x and 3.x in parallel, it is useful to point
cmake to the version that should be used.

Change-Id: I7fd9e730e87210d2b7d090c8f9c7c6734bd7374e
---
M CMakeLists.txt
1 file changed, 10 insertions(+), 1 deletion(-)


  git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/77/377/2

diff --git a/CMakeLists.txt b/CMakeLists.txt
index d21c9bd..fe9b596 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -32,6 +32,8 @@
 endif ()

 option(MBED "BUILD with mbed" OFF)
+set(MBED_INCLUDE_PATH "" CACHE STRING "Path to mbed TLS include directory")
+set(MBED_LIBRARY_PATH "" CACHE STRING "Path to mbed library directory")
 option(WOLFSSL "BUILD with wolfSSL" OFF)
 option(ENABLE_LZ4 "BUILD with lz4" ON)
 option(ENABLE_LZO "BUILD with lzo" ON)
@@ -233,7 +235,14 @@

 function(add_library_deps target)
 if (${MBED})
-target_link_libraries(${target} -lmbedtls -lmbedx509 -lmbedcrypto)
+if (NOT (MBED_INCLUDE_PATH STREQUAL "") )
+target_include_directories(${target} PRIVATE ${MBED_INCLUDE_PATH})
+endif ()
+if(NOT (MBED_LIBRARY_PATH STREQUAL ""))
+target_link_directories(${target} PRIVATE ${MBED_LIBRARY_PATH})
+endif ()
+
+target_link_libraries(${target} PRIVATE -lmbedtls -lmbedx509 
-lmbedcrypto)
 elseif (${WOLFSSL})
 pkg_search_module(wolfssl wolfssl REQUIRED)
 target_link_libraries(${target} PUBLIC ${wolfssl_LINK_LIBRARIES})

--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/377?usp=email
To unsubscribe, or for help writing mail filters, visit 
http://gerrit.openvpn.net/settings

Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I7fd9e730e87210d2b7d090c8f9c7c6734bd7374e
Gerrit-Change-Number: 377
Gerrit-PatchSet: 2
Gerrit-Owner: plaisthos 
Gerrit-Reviewer: flichtenheld 
Gerrit-Reviewer: plaisthos 
Gerrit-CC: MaxF 
Gerrit-CC: openvpn-devel 
Gerrit-Attention: plaisthos 
Gerrit-Attention: flichtenheld 
Gerrit-MessageType: newpatchset
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

[Openvpn-devel] [XS] Change in openvpn[master]: Add check for nice in cmake config

[plaisthos (Code Review)]

Attention is currently required from: flichtenheld.

Hello flichtenheld,

I'd like you to do a code review.
Please visit

http://gerrit.openvpn.net/c/openvpn/+/444?usp=email

to review the following change.


Change subject: Add check for nice in cmake config
..

Add check for nice in cmake config

Change-Id: I2cc8f9b82079acca250db5871ffd9fad2997d1a8
---
M CMakeLists.txt
M config.h.cmake.in
2 files changed, 2 insertions(+), 1 deletion(-)



  git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/44/444/1

diff --git a/CMakeLists.txt b/CMakeLists.txt
index fe9b596..f82ee96 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -146,6 +146,7 @@
 check_symbol_exists(fork unistd.h HAVE_FORK)
 check_symbol_exists(execve unistd.h HAVE_EXECVE)
 check_symbol_exists(ftruncate unistd.h HAVE_FTRUNCATE)
+check_symbol_exists(nice unistd.h HAVE_NICE)
 check_symbol_exists(setgid unistd.h HAVE_SETGID)
 check_symbol_exists(setuid unistd.h HAVE_SETUID)
 check_symbol_exists(setsid unistd.h HAVE_SETSID)
diff --git a/config.h.cmake.in b/config.h.cmake.in
index 25273ac..c1f5848 100644
--- a/config.h.cmake.in
+++ b/config.h.cmake.in
@@ -209,7 +209,7 @@
 #cmakedefine HAVE_NET_TUN_IF_TUN_H

 /* Define to 1 if you have the `nice' function. */
-#undef HAVE_NICE
+#cmakedefine HAVE_NICE

 /* Define to 1 if you have the `openlog' function. */
 #cmakedefine HAVE_OPENLOG

--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/444?usp=email
To unsubscribe, or for help writing mail filters, visit 
http://gerrit.openvpn.net/settings

Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I2cc8f9b82079acca250db5871ffd9fad2997d1a8
Gerrit-Change-Number: 444
Gerrit-PatchSet: 1
Gerrit-Owner: plaisthos 
Gerrit-Reviewer: flichtenheld 
Gerrit-CC: openvpn-devel 
Gerrit-Attention: flichtenheld 
Gerrit-MessageType: newchange
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

[Openvpn-devel] [S] Change in openvpn[master]: protocol_dump: tls-crypt support

[plaisthos (Code Review)]

Attention is currently required from: flichtenheld.

plaisthos has posted comments on this change. ( 
http://gerrit.openvpn.net/c/openvpn/+/442?usp=email )

Change subject: protocol_dump: tls-crypt support
..


Patch Set 1: Code-Review+2


--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/442?usp=email
To unsubscribe, or for help writing mail filters, visit 
http://gerrit.openvpn.net/settings

Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: Ie25aa287f3534090c1d93fc3bb69727dd20fc6fe
Gerrit-Change-Number: 442
Gerrit-PatchSet: 1
Gerrit-Owner: plaisthos 
Gerrit-Reviewer: flichtenheld 
Gerrit-Reviewer: plaisthos 
Gerrit-CC: openvpn-devel 
Gerrit-Attention: flichtenheld 
Gerrit-Comment-Date: Mon, 20 Nov 2023 10:24:18 +
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

[Openvpn-devel] [M] Change in openvpn[master]: Various fixes for -Wconversion errors

[plaisthos (Code Review)]

Attention is currently required from: flichtenheld.

plaisthos has posted comments on this change. ( 
http://gerrit.openvpn.net/c/openvpn/+/267?usp=email )

Change subject: Various fixes for -Wconversion errors
..


Patch Set 4: Code-Review+2


--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/267?usp=email
To unsubscribe, or for help writing mail filters, visit 
http://gerrit.openvpn.net/settings

Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I6818b153bdeb1eed65870af99b0531e95807fe0f
Gerrit-Change-Number: 267
Gerrit-PatchSet: 4
Gerrit-Owner: flichtenheld 
Gerrit-Reviewer: plaisthos 
Gerrit-CC: openvpn-devel 
Gerrit-Attention: flichtenheld 
Gerrit-Comment-Date: Mon, 20 Nov 2023 10:30:56 +
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

[Openvpn-devel] [M] Change in openvpn[master]: tun: use is_tun_p2p more consistently

[plaisthos (Code Review)]

Attention is currently required from: flichtenheld.

plaisthos has posted comments on this change. ( 
http://gerrit.openvpn.net/c/openvpn/+/380?usp=email )

Change subject: tun: use is_tun_p2p more consistently
..


Patch Set 1: Code-Review+2


--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/380?usp=email
To unsubscribe, or for help writing mail filters, visit 
http://gerrit.openvpn.net/settings

Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: Ice8b95f953c3f7e71657a78ea12b02a08c60aa67
Gerrit-Change-Number: 380
Gerrit-PatchSet: 1
Gerrit-Owner: flichtenheld 
Gerrit-Reviewer: plaisthos 
Gerrit-CC: openvpn-devel 
Gerrit-Attention: flichtenheld 
Gerrit-Comment-Date: Mon, 20 Nov 2023 10:49:14 +
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

[Openvpn-devel] [M] Change in openvpn[master]: Remove compat versionhelpers.h and remove cmake/configure check for it

[plaisthos (Code Review)]

Attention is currently required from: flichtenheld.

Hello flichtenheld,

I'd like you to do a code review.
Please visit

http://gerrit.openvpn.net/c/openvpn/+/445?usp=email

to review the following change.


Change subject: Remove compat versionhelpers.h and remove cmake/configure check 
for it
..

Remove compat versionhelpers.h and remove cmake/configure check for it

The cmake file defined that file to be never present in contrast to the
old msvc-config.h that always had it present. Also interactive.c includes
versionhelpers.h without the check, so we always assume it to be present
anyway. Remove also the comapt implementation taken from mingw

Change-Id: I9c85ccab6d51064ebff2c391740ba8c2d044ed1a
---
M CMakeLists.txt
M config.h.cmake.in
M configure.ac
M src/compat/Makefile.am
D src/compat/compat-versionhelpers.h
M src/openvpn/win32.c
M src/openvpnserv/interactive.c
7 files changed, 1 insertion(+), 131 deletions(-)



  git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/45/445/1

diff --git a/CMakeLists.txt b/CMakeLists.txt
index d21c9bd..fa6d623 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -308,7 +308,6 @@
 src/compat/compat-dirname.c
 src/compat/compat-gettimeofday.c
 src/compat/compat-strsep.c
-src/compat/compat-versionhelpers.h
 src/openvpn/argv.c
 src/openvpn/argv.h
 src/openvpn/base64.c
diff --git a/config.h.cmake.in b/config.h.cmake.in
index 19b79bc..8edaff4 100644
--- a/config.h.cmake.in
+++ b/config.h.cmake.in
@@ -374,9 +374,6 @@
 /* Define to 1 if you have the  header file. */
 #undef HAVE_VALGRIND_MEMCHECK_H

-/* Define to 1 if you have the  header file. */
-#undef HAVE_VERSIONHELPERS_H
-
 /* Define to 1 if you have the `vfork' function. */
 #undef HAVE_VFORK

diff --git a/configure.ac b/configure.ac
index 84eaad6..94c6654 100644
--- a/configure.ac
+++ b/configure.ac
@@ -455,7 +455,6 @@
unistd.h dlfcn.h \
netinet/in.h \
netinet/tcp.h arpa/inet.h netdb.h \
-   versionhelpers.h \
 ])
 AC_CHECK_HEADERS([ \
sys/time.h sys/ioctl.h sys/stat.h \
diff --git a/src/compat/Makefile.am b/src/compat/Makefile.am
index f5de451..5298dd8 100644
--- a/src/compat/Makefile.am
+++ b/src/compat/Makefile.am
@@ -20,5 +20,4 @@
compat-basename.c \
compat-gettimeofday.c \
compat-daemon.c \
-   compat-strsep.c \
-   compat-versionhelpers.h
+   compat-strsep.c
\ No newline at end of file
diff --git a/src/compat/compat-versionhelpers.h 
b/src/compat/compat-versionhelpers.h
deleted file mode 100644
index b071602..000
--- a/src/compat/compat-versionhelpers.h
+++ /dev/null
@@ -1,116 +0,0 @@
-/**
- * This file is part of the mingw-w64 runtime package.
- * No warranty is given; refer to the file DISCLAIMER within this package.
- */
-
-#ifndef _INC_VERSIONHELPERS
-#define _INC_VERSIONHELPERS
-
-#include 
-
-#if WINAPI_FAMILY_PARTITION(WINAPI_PARTITION_DESKTOP) && !defined(__WIDL__)
-
-#ifdef __cplusplus
-#define VERSIONHELPERAPI inline bool
-#else
-#define VERSIONHELPERAPI FORCEINLINE BOOL
-#endif
-
-#define _WIN32_WINNT_WINBLUE0x0603
-
-#ifndef _WIN32_WINNT_WINTHRESHOLD
-#define _WIN32_WINNT_WINTHRESHOLD0x0A00 /* Windows 10 */
-#endif
-
-VERSIONHELPERAPI
-IsWindowsVersionOrGreater(WORD major, WORD minor, WORD servpack)
-{
-OSVERSIONINFOEXW vi = {sizeof(vi), major, minor, 0, 0, {0}, servpack};
-return VerifyVersionInfoW(&vi, 
VER_MAJORVERSION|VER_MINORVERSION|VER_SERVICEPACKMAJOR,
-  
VerSetConditionMask(VerSetConditionMask(VerSetConditionMask(0,
-   
   VER_MAJORVERSION, VER_GREATER_EQUAL),
-  
VER_MINORVERSION, VER_GREATER_EQUAL),
-  VER_SERVICEPACKMAJOR, 
VER_GREATER_EQUAL));
-}
-
-VERSIONHELPERAPI
-IsWindowsXPOrGreater(void)
-{
-return IsWindowsVersionOrGreater(HIBYTE(_WIN32_WINNT_WINXP), 
LOBYTE(_WIN32_WINNT_WINXP), 0);
-}
-
-VERSIONHELPERAPI
-IsWindowsXPSP1OrGreater(void)
-{
-return IsWindowsVersionOrGreater(HIBYTE(_WIN32_WINNT_WINXP), 
LOBYTE(_WIN32_WINNT_WINXP), 1);
-}
-
-VERSIONHELPERAPI
-IsWindowsXPSP2OrGreater(void)
-{
-return IsWindowsVersionOrGreater(HIBYTE(_WIN32_WINNT_WINXP), 
LOBYTE(_WIN32_WINNT_WINXP), 2);
-}
-
-VERSIONHELPERAPI
-IsWindowsXPSP3OrGreater(void)
-{
-return IsWindowsVersionOrGreater(HIBYTE(_WIN32_WINNT_WINXP), 
LOBYTE(_WIN32_WINNT_WINXP), 3);
-}
-
-VERSIONHELPERAPI
-IsWindowsVistaOrGreater(void)
-{
-return IsWindowsVersionOrGreater(HIBYTE(_WIN32_WINNT_VISTA), 
LOBYTE(_WIN32_WINNT_VISTA), 0);
-}
-
-VERSIONHELPERAPI
-IsWindowsVistaSP1OrGreater(void)
-{
-return IsWindowsVersionOrGreater(HIBYTE(_WIN32_WINNT_VISTA), 
LOBYTE(_WIN32_WINNT_VISTA), 1);
-}
-
-VERSIONHELPERAPI
-IsWindowsVistaSP2OrGreater(void)
-{
-return IsWindowsVersionOrGreater(HIBYTE(

[Openvpn-devel] [L] Change in openvpn[master]: Remove support for NTLM v1 proxy authentication

[plaisthos (Code Review)]

Attention is currently required from: flichtenheld.

plaisthos has posted comments on this change. ( 
http://gerrit.openvpn.net/c/openvpn/+/379?usp=email )

Change subject: Remove support for NTLM v1 proxy authentication
..


Patch Set 2: Code-Review+2


--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/379?usp=email
To unsubscribe, or for help writing mail filters, visit 
http://gerrit.openvpn.net/settings

Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I0dcb2dac4136f194da7050a8ea8495e9faba9dd9
Gerrit-Change-Number: 379
Gerrit-PatchSet: 2
Gerrit-Owner: flichtenheld 
Gerrit-Reviewer: plaisthos 
Gerrit-CC: openvpn-devel 
Gerrit-Attention: flichtenheld 
Gerrit-Comment-Date: Mon, 20 Nov 2023 10:53:04 +
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

[Openvpn-devel] [XL] Change in openvpn[master]: sample-keys: renew for the next 10 years

[plaisthos (Code Review)]

Attention is currently required from: flichtenheld.

plaisthos has posted comments on this change. ( 
http://gerrit.openvpn.net/c/openvpn/+/422?usp=email )

Change subject: sample-keys: renew for the next 10 years
..


Patch Set 1: Code-Review+2


--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/422?usp=email
To unsubscribe, or for help writing mail filters, visit 
http://gerrit.openvpn.net/settings

Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: Ie264ec1ec61fd71e8cc87987be3e2adc2735c201
Gerrit-Change-Number: 422
Gerrit-PatchSet: 1
Gerrit-Owner: flichtenheld 
Gerrit-Reviewer: plaisthos 
Gerrit-CC: openvpn-devel 
Gerrit-Attention: flichtenheld 
Gerrit-Comment-Date: Mon, 20 Nov 2023 10:53:39 +
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

[Openvpn-devel] [M] Change in openvpn[master]: Remove dead list test code

[plaisthos (Code Review)]

Attention is currently required from: flichtenheld.

Hello flichtenheld,

I'd like you to do a code review.
Please visit

http://gerrit.openvpn.net/c/openvpn/+/446?usp=email

to review the following change.


Change subject: Remove dead list test code
..

Remove dead list test code

Change-Id: I7511bc43cd6a0bcb89476f27d5822ab4a78d0d21
---
M src/openvpn/init.c
M src/openvpn/list.c
M src/openvpn/list.h
3 files changed, 0 insertions(+), 191 deletions(-)



  git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/46/446/1

diff --git a/src/openvpn/init.c b/src/openvpn/init.c
index 8c707a4..5618c69 100644
--- a/src/openvpn/init.c
+++ b/src/openvpn/init.c
@@ -865,11 +865,6 @@
 return false;
 #endif

-#ifdef LIST_TEST
-list_test();
-return false;
-#endif
-
 #ifdef IFCONFIG_POOL_TEST
 ifconfig_pool_test(0x0A010004, 0x0A0100FF);
 return false;
diff --git a/src/openvpn/list.c b/src/openvpn/list.c
index 480f39d..dc4b1df 100644
--- a/src/openvpn/list.c
+++ b/src/openvpn/list.c
@@ -326,185 +326,6 @@
 }


-#ifdef LIST_TEST
-
-/*
- * Test the hash code by implementing a simple
- * word frequency algorithm.
- */
-
-struct word
-{
-const char *word;
-int n;
-};
-
-static uint32_t
-word_hash_function(const void *key, uint32_t iv)
-{
-const char *str = (const char *) key;
-const int len = strlen(str);
-return hash_func((const uint8_t *)str, len, iv);
-}
-
-static bool
-word_compare_function(const void *key1, const void *key2)
-{
-return strcmp((const char *)key1, (const char *)key2) == 0;
-}
-
-static void
-print_nhash(struct hash *hash)
-{
-struct hash_iterator hi;
-struct hash_element *he;
-int count = 0;
-
-hash_iterator_init(hash, &hi, true);
-
-while ((he = hash_iterator_next(&hi)))
-{
-printf("%d ", (int) he->value);
-++count;
-}
-printf("\n");
-
-hash_iterator_free(&hi);
-ASSERT(count == hash_n_elements(hash));
-}
-
-static void
-rmhash(struct hash *hash, const char *word)
-{
-hash_remove(hash, word);
-}
-
-void
-list_test(void)
-{
-openvpn_thread_init();
-
-{
-struct gc_arena gc = gc_new();
-struct hash *hash = hash_init(1, get_random(), word_hash_function, 
word_compare_function);
-struct hash *nhash = hash_init(256, get_random(), word_hash_function, 
word_compare_function);
-
-printf("hash_init n_buckets=%d mask=0x%08x\n", hash->n_buckets, 
hash->mask);
-
-/* parse words from stdin */
-while (true)
-{
-char buf[256];
-char wordbuf[256];
-int wbi;
-int bi;
-char c;
-
-if (!fgets(buf, sizeof(buf), stdin))
-{
-break;
-}
-
-bi = wbi = 0;
-do
-{
-c = buf[bi++];
-if (isalnum(c) || c == '_')
-{
-ASSERT(wbi < (int) sizeof(wordbuf));
-wordbuf[wbi++] = c;
-}
-else
-{
-if (wbi)
-{
-struct word *w;
-ASSERT(wbi < (int) sizeof(wordbuf));
-wordbuf[wbi++] = '\0';
-
-/* word is parsed from stdin */
-
-/* does it already exist in table? */
-w = (struct word *) hash_lookup(hash, wordbuf);
-
-if (w)
-{
-/* yes, increment count */
-++w->n;
-}
-else
-{
-/* no, make a new object */
-ALLOC_OBJ_GC(w, struct word, &gc);
-w->word = string_alloc(wordbuf, &gc);
-w->n = 1;
-ASSERT(hash_add(hash, w->word, w, false));
-ASSERT(hash_add(nhash, w->word, (void *) 
((random() & 0x0F) + 1), false));
-}
-}
-wbi = 0;
-}
-} while (c);
-}
-
-#if 1
-/* remove some words from the table */
-{
-rmhash(hash, "true");
-rmhash(hash, "false");
-}
-#endif
-
-/* output contents of hash table */
-{
-int base;
-int inc = 0;
-int count = 0;
-
-for (base = 0; base < hash_n_buckets(hash); base += inc)
-{
-struct hash_iterator hi;
-struct hash_element *he;
-inc = (get_random() % 3) + 1;
-hash_iterator_init_range(hash, &hi, true, base, base + inc);
-
-while ((he = hash_iterator_next(&hi)))
-{
-stru

[Openvpn-devel] [M] Change in openvpn[master]: Print SSL peer signature information in handshake debug details

[plaisthos (Code Review)]

Attention is currently required from: plaisthos.

Hello flichtenheld,

I'd like you to reexamine a change. Please visit

http://gerrit.openvpn.net/c/openvpn/+/365?usp=email

to look at the new patch set (#8).


Change subject: Print SSL peer signature information in handshake debug details
..

Print SSL peer signature information in handshake debug details

This is more SSL debug information that most people do not really need
or care about. OpenSSL's own s_client also logs them:

Peer signing digest: SHA256
Peer signature type: ECDSA

The complete message looks like this:

   Control Channel: TLSv1.3, cipher TLSv1.3 TLS_AES_256_GCM_SHA384, peer 
certificate: 2048 bits RSA, signature: RSA-SHA256, server temp key: 253 bits 
X25519, peer signing digest/type: SHA256 RSASSA-PSS

or when forcing a specific group via tls-groups X448 with a ECDSA server:

   Control Channel: TLSv1.3, cipher TLSv1.3 TLS_AES_256_GCM_SHA384, peer 
certificate: 384 bits ECsecp384r1, signature: ecdsa-with-SHA256, server temp 
key: 448 bits X448, peer signing digest/type: SHA384 ECDSA

Change-Id: Ib5fc0c4b8f164596681ac5ad73002068ec6de1e5
---
M src/openvpn/ssl_openssl.c
1 file changed, 80 insertions(+), 2 deletions(-)


  git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/65/365/8

diff --git a/src/openvpn/ssl_openssl.c b/src/openvpn/ssl_openssl.c
index 23e7623..10e3c22 100644
--- a/src/openvpn/ssl_openssl.c
+++ b/src/openvpn/ssl_openssl.c
@@ -2165,6 +2165,82 @@
 EVP_PKEY_free(pkey);
 }

+#if !defined(LIBRESSL_VERSION_NUMBER)  && OPENSSL_VERSION_NUMBER >= 0x101fL
+/**
+ * Translate an OpenSSL NID into a more human readable name
+ * @param nid
+ * @return
+ */
+static const char *
+get_sigtype(int nid)
+{
+/* Fix a few OpenSSL names to be better understandable */
+switch (nid)
+{
+case EVP_PKEY_RSA:
+/* will otherwise say rsaEncryption */
+return "RSA";
+
+case EVP_PKEY_DSA:
+/* dsaEncryption otherwise */
+return "DSA";
+
+case EVP_PKEY_EC:
+/* will say id-ecPublicKey */
+return "ECDSA";
+
+case -1:
+return "(error getting name)";
+
+default:
+return OBJ_nid2sn(nid);
+}
+}
+#endif /* ifndef LIBRESSL_VERSION_NUMBER */
+
+/**
+ * Get the type of the signature that is used by the peer during the
+ * TLS handshake
+ */
+static void
+print_peer_signature(SSL *ssl, char *buf, size_t buflen)
+{
+int peer_sig_nid = NID_undef, peer_sig_type_nid = NID_undef;
+const char *peer_sig = "unknown";
+const char *peer_sig_type = "unknown type";
+
+/* Even though these methods use the deprecated NIDs instead of using
+ * string as new OpenSSL APIs do, there seem to be no API that replaces
+ * it yet */
+#if !defined(LIBRESSL_VERSION_NUMBER) || LIBRESSL_VERSION_NUMBER > 0x3050400fL
+if (SSL_get_peer_signature_nid(ssl, &peer_sig_nid)
+&& peer_sig_nid != NID_undef)
+{
+peer_sig = OBJ_nid2sn(peer_sig_nid);
+}
+#endif
+
+#if !defined(LIBRESSL_VERSION_NUMBER) && OPENSSL_VERSION_NUMBER >= 0x101fL
+/* LibreSSL 3.7.x and 3.8.0 weirdly implement this function but fail on
+ * linking with an unresolved symbol */
+if (SSL_get_peer_signature_type_nid(ssl, &peer_sig_type_nid)
+&& peer_sig_type_nid != NID_undef)
+{
+peer_sig_type = get_sigtype(peer_sig_type_nid);
+}
+#endif
+
+if (peer_sig_nid == NID_undef && peer_sig_type_nid == NID_undef)
+{
+return;
+}
+
+openvpn_snprintf(buf, buflen, ", peer signing digest/type: %s %s",
+ peer_sig, peer_sig_type);
+}
+
+
+
 /* **
  *
  * Information functions
@@ -2179,8 +2255,9 @@
 char s1[256];
 char s2[256];
 char s3[256];
+char s4[256];

-s1[0] = s2[0] = s3[0] = 0;
+s1[0] = s2[0] = s3[0] = s4[0] = 0;
 ciph = SSL_get_current_cipher(ks_ssl->ssl);
 openvpn_snprintf(s1, sizeof(s1), "%s %s, cipher %s %s",
  prefix,
@@ -2195,8 +2272,9 @@
 X509_free(cert);
 }
 print_server_tempkey(ks_ssl->ssl, s3, sizeof(s3));
+print_peer_signature(ks_ssl->ssl, s4, sizeof(s4));

-msg(D_HANDSHAKE, "%s%s%s", s1, s2, s3);
+msg(D_HANDSHAKE, "%s%s%s%s", s1, s2, s3, s4);
 }

 void

--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/365?usp=email
To unsubscribe, or for help writing mail filters, visit 
http://gerrit.openvpn.net/settings

Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: Ib5fc0c4b8f164596681ac5ad73002068ec6de1e5
Gerrit-Change-Number: 365
Gerrit-PatchSet: 8
Gerrit-Owner: plaisthos 
Gerrit-Reviewer: flichtenheld 
Gerrit-CC: openvpn-devel 
Gerrit-Attention: plaisthos 
Gerrit-MessageType: newpatchset
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/l

[Openvpn-devel] [XS] Change in openvpn[master]: Deprecate tls-exit option

[plaisthos (Code Review)]

Attention is currently required from: flichtenheld.

Hello flichtenheld,

I'd like you to do a code review.
Please visit

http://gerrit.openvpn.net/c/openvpn/+/447?usp=email

to review the following change.


Change subject: Deprecate tls-exit option
..

Deprecate tls-exit option

This option is questionable and I cannot see any reason to actually use it.

Change-Id: I93afff2372c4150d6bddc8c07fd4ebc8bfb0cc3e
---
M Changes.rst
M doc/man-sections/tls-options.rst
M src/openvpn/options.c
3 files changed, 7 insertions(+), 2 deletions(-)



  git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/47/447/1

diff --git a/Changes.rst b/Changes.rst
index 3676dce..922f78d 100644
--- a/Changes.rst
+++ b/Changes.rst
@@ -10,6 +10,10 @@
 ``--allow-deprecated-insecure-static-crypto`` but will be removed in
 OpenVPN 2.8.

+```tls-exit``` has been deprecated since it is unclear what the use case
+for this option is. If you have a valid use case, please reach out since
+the will otherwise be removed in the future.
+
 Overview of changes in 2.6
 ==

diff --git a/doc/man-sections/tls-options.rst b/doc/man-sections/tls-options.rst
index 908a42a..da5f362 100644
--- a/doc/man-sections/tls-options.rst
+++ b/doc/man-sections/tls-options.rst
@@ -553,7 +553,7 @@
   code.

 --tls-exit
-  Exit on TLS negotiation failure.
+  **DEPRECATED** Exit on TLS negotiation failure.

 --tls-export-cert directory
   Store the certificates the clients use upon connection to this
diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index 2594b66..5eb1a45 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -608,7 +608,7 @@
 "--tran-window n : Transition window -- old key can live this many 
seconds\n"
 "  after new key renegotiation begins (default=%d).\n"
 "--single-session: Allow only one session (reset state on restart).\n"
-"--tls-exit  : Exit on TLS negotiation failure.\n"
+"--tls-exit  : (DEPRECATED) Exit on TLS negotiation failure.\n"
 "--tls-auth f [d]: Add an additional layer of authentication on top of the 
TLS\n"
 "  control channel to protect against attacks on the TLS 
stack\n"
 "  and DoS attacks.\n"
@@ -8960,6 +8960,7 @@
 }
 else if (streq(p[0], "tls-exit") && !p[1])
 {
+msg(M_WARN, "DEPRECATED OPTION: The option --tls-exit is deprecated.");
 VERIFY_PERMISSION(OPT_P_GENERAL);
 options->tls_exit = true;
 }

--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/447?usp=email
To unsubscribe, or for help writing mail filters, visit 
http://gerrit.openvpn.net/settings

Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I93afff2372c4150d6bddc8c07fd4ebc8bfb0cc3e
Gerrit-Change-Number: 447
Gerrit-PatchSet: 1
Gerrit-Owner: plaisthos 
Gerrit-Reviewer: flichtenheld 
Gerrit-CC: openvpn-devel 
Gerrit-Attention: flichtenheld 
Gerrit-MessageType: newchange
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

[Openvpn-devel] [S] Change in openvpn[master]: Remove unused defined from configure and cmake config

[flichtenheld (Code Review)]

Attention is currently required from: plaisthos.

flichtenheld has posted comments on this change. ( 
http://gerrit.openvpn.net/c/openvpn/+/443?usp=email )

Change subject: Remove unused defined from configure and cmake config
..


Patch Set 1: Code-Review-2

(2 comments)

Commit Message:

http://gerrit.openvpn.net/c/openvpn/+/443/comment/1abfd006_4c063df8 :
PS1, Line 7: Remove unused defined from configure and cmake config
"defines"


File configure.ac:

http://gerrit.openvpn.net/c/openvpn/+/443/comment/3f2d6cca_2005d138 :
PS1, Line 582:  AC_DEFINE([HAVE_ANONYMOUS_UNION_SUPPORT], [], [Compiler 
supports anonymous unions])
This is used in mroute.h. Need to remove that usage.



--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/443?usp=email
To unsubscribe, or for help writing mail filters, visit 
http://gerrit.openvpn.net/settings

Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: Ifd0376b36d4050dc22bc93b8fcf7ed29faef0021
Gerrit-Change-Number: 443
Gerrit-PatchSet: 1
Gerrit-Owner: plaisthos 
Gerrit-Reviewer: flichtenheld 
Gerrit-CC: openvpn-devel 
Gerrit-Attention: plaisthos 
Gerrit-Comment-Date: Mon, 20 Nov 2023 11:20:30 +
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

[Openvpn-devel] [XS] Change in openvpn[master]: Add check for nice in cmake config

[flichtenheld (Code Review)]

Attention is currently required from: plaisthos.

flichtenheld has posted comments on this change. ( 
http://gerrit.openvpn.net/c/openvpn/+/444?usp=email )

Change subject: Add check for nice in cmake config
..


Patch Set 1: Code-Review+2


--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/444?usp=email
To unsubscribe, or for help writing mail filters, visit 
http://gerrit.openvpn.net/settings

Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I2cc8f9b82079acca250db5871ffd9fad2997d1a8
Gerrit-Change-Number: 444
Gerrit-PatchSet: 1
Gerrit-Owner: plaisthos 
Gerrit-Reviewer: flichtenheld 
Gerrit-CC: openvpn-devel 
Gerrit-Attention: plaisthos 
Gerrit-Comment-Date: Mon, 20 Nov 2023 11:22:20 +
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

[Openvpn-devel] [M] Change in openvpn[master]: Remove compat versionhelpers.h and remove cmake/configure check for it

[flichtenheld (Code Review)]

Attention is currently required from: plaisthos.

flichtenheld has posted comments on this change. ( 
http://gerrit.openvpn.net/c/openvpn/+/445?usp=email )

Change subject: Remove compat versionhelpers.h and remove cmake/configure check 
for it
..


Patch Set 1: Code-Review-1

(2 comments)

Commit Message:

http://gerrit.openvpn.net/c/openvpn/+/445/comment/ac1dbdba_a1bab3b8 :
PS1, Line 10: old msvc-config.h that always had it present. Also interactive.c 
includes
interactive.c does not include it without the check?


Patchset:

PS1:
Code looks good, but commit message seems wrong?



--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/445?usp=email
To unsubscribe, or for help writing mail filters, visit 
http://gerrit.openvpn.net/settings

Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I9c85ccab6d51064ebff2c391740ba8c2d044ed1a
Gerrit-Change-Number: 445
Gerrit-PatchSet: 1
Gerrit-Owner: plaisthos 
Gerrit-Reviewer: flichtenheld 
Gerrit-CC: openvpn-devel 
Gerrit-Attention: plaisthos 
Gerrit-Comment-Date: Mon, 20 Nov 2023 11:27:24 +
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

[Openvpn-devel] [M] Change in openvpn[master]: Allow the TLS session to send out TLS alerts

[plaisthos (Code Review)]

Attention is currently required from: flichtenheld.

Hello flichtenheld,

I'd like you to do a code review.
Please visit

http://gerrit.openvpn.net/c/openvpn/+/449?usp=email

to review the following change.


Change subject: Allow the TLS session to send out TLS alerts
..

Allow the TLS session to send out TLS alerts

Previous OpenVPN versions shut down the TLS control channel immediately
when encountering an error. This also meant that we would not send out
TLS alerts to notify a client about potential problems like mismatching
TLS versions or having no common cipher.

This commit adds a new key_state S_ERROR_PRE which still allows to
send out the remaining TLS packets of the control session which are
typically the alert message and then going to S_ERROR. We do not
wait for retries. So this is more a one-shot notify but that is
acceptable in this situation.

Sending out alerts  is a slight compromise in security as alerts give
out a bit of information that otherwise is not given
out. But since all other consumers TLS implementation are already doing this
and TLS implementation (nowadays) are very careful not to leak (sensitive)
information by alerts and since the user experience is much better with
alerts, this compromise is worth it.

Change-Id: I0ad48915004ddee587e97c8ed190ba8ee989e48d
---
M Changes.rst
M src/openvpn/ssl.c
M src/openvpn/ssl_backend.h
M src/openvpn/ssl_common.h
M src/openvpn/ssl_mbedtls.c
M src/openvpn/ssl_openssl.c
6 files changed, 98 insertions(+), 18 deletions(-)



  git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/49/449/1

diff --git a/Changes.rst b/Changes.rst
index 3676dce..4c8be39 100644
--- a/Changes.rst
+++ b/Changes.rst
@@ -1,5 +1,16 @@
 Overview of changes in 2.7
 ==
+New features
+
+TLS alerts
+OpenVPN 2.7 will send out TLS alerts to peer informing them if the TLS
+session shuts down or when the TLS implementation informs the peer about
+an error in the TLS session (e.g. mismatching TLS versions). This improves
+the user experience as the client shows an error instead of running into
+a timeout when the server just stop responding completely.
+
+Deprecated features
+---
 ``secret`` support has been removed by default.
 static key mode (non-TLS) is no longer considered "good and secure enough"
 for today's requirements.  Use TLS mode instead.  If deploying a PKI CA
diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
index b4cd8f5..16d3d22 100644
--- a/src/openvpn/ssl.c
+++ b/src/openvpn/ssl.c
@@ -853,6 +853,9 @@
 case S_ERROR:
 return "S_ERROR";

+case S_ERROR_PRE:
+return "S_ERROR_PRE";
+
 case S_GENERATED_KEYS:
 return "S_GENERATED_KEYS";

@@ -2839,6 +2842,35 @@
 return true;
 }

+/**
+ * Shut down an SSL session, so an SSL close notify is sent if there no other
+ * SSL notify.
+ * @param ks
+ */
+void
+do_ssl_shutdown(struct key_state *ks)
+{
+}
+
+
+static bool
+check_outgoing_ciphertext(struct key_state *ks, struct tls_session *session,
+  bool *state_change)
+{
+/* Outgoing Ciphertext to reliable buffer */
+if (ks->state >= S_PRE_START)
+{
+struct buffer *buf = 
reliable_get_buf_output_sequenced(ks->send_reliable);
+if (buf)
+{
+if (!write_outgoing_tls_ciphertext(session, state_change))
+{
+return false;
+}
+}
+}
+return true;
+}

 static bool
 tls_process_state(struct tls_multi *multi,
@@ -2912,6 +2944,16 @@
 return false;
 }

+if (ks->state == S_ERROR_PRE)
+{
+/* When we end up here, we had one last chance to send an outstanding
+ * packet that contained an alert. We do not ensure that this packet
+ * has been successfully delivered  (ie wait for the ACK etc)
+ * but rather stop processing now */
+ks->state = S_ERROR;
+return false;
+}
+
 /* Write incoming ciphertext to TLS object */
 struct reliable_entry *entry = 
reliable_get_entry_sequenced(ks->rec_reliable);
 if (entry)
@@ -2992,29 +3034,31 @@
 dmsg(D_TLS_DEBUG, "Outgoing Plaintext -> TLS");
 }
 }
-
-/* Outgoing Ciphertext to reliable buffer */
-if (ks->state >= S_START)
+if (!check_outgoing_ciphertext(ks, session, &state_change))
 {
-buf = reliable_get_buf_output_sequenced(ks->send_reliable);
-if (buf)
-{
-if (!write_outgoing_tls_ciphertext(session, &state_change))
-{
-goto error;
-}
-}
+goto error;
 }

 return state_change;
 error:
 tls_clear_error();
-ks->state = S_ERROR;
+
+/* Shut down the TLS session but do a last read from the TLS
+ * object to be able to read potential TLS alerts */
+do_ssl_shutdown(ks);
+check_o

[Openvpn-devel] [XS] Change in openvpn[master]: Log SSL alerts more prominently

[plaisthos (Code Review)]

Attention is currently required from: flichtenheld.

Hello flichtenheld,

I'd like you to do a code review.
Please visit

http://gerrit.openvpn.net/c/openvpn/+/448?usp=email

to review the following change.


Change subject: Log SSL alerts more prominently
..

Log SSL alerts more prominently

When we receive an SSL alert from a server we currently only log a
very cryptic OpenSSL error message:

   OpenSSL: error:0A00042E:SSL routines::tlsv1 alert protocol version:SSL alert 
number 70

This also enables logging the much more readable SSL error message:

   Received fatal SSL alert: protocol version

which previously needed --verb 8 to be displayed (now verb 3). Also rework the
message to be better readable.

Change-Id: I6bdab3028c9bd679c31d4177a746a3ea505dcbbf
Signed-off-by: Arne Schwabe 
---
M src/openvpn/ssl_openssl.c
1 file changed, 2 insertions(+), 2 deletions(-)



  git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/48/448/1

diff --git a/src/openvpn/ssl_openssl.c b/src/openvpn/ssl_openssl.c
index 23e7623..82872bf 100644
--- a/src/openvpn/ssl_openssl.c
+++ b/src/openvpn/ssl_openssl.c
@@ -196,8 +196,8 @@
 }
 else if (where & SSL_CB_ALERT)
 {
-dmsg(D_HANDSHAKE_VERBOSE, "SSL alert (%s): %s: %s",
- where & SSL_CB_READ ? "read" : "write",
+dmsg(D_TLS_DEBUG_LOW, "%s %s SSL alert: %s",
+ where & SSL_CB_READ ? "Received" : "Sent",
  SSL_alert_type_string_long(ret),
  SSL_alert_desc_string_long(ret));
 }

--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/448?usp=email
To unsubscribe, or for help writing mail filters, visit 
http://gerrit.openvpn.net/settings

Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I6bdab3028c9bd679c31d4177a746a3ea505dcbbf
Gerrit-Change-Number: 448
Gerrit-PatchSet: 1
Gerrit-Owner: plaisthos 
Gerrit-Reviewer: flichtenheld 
Gerrit-CC: openvpn-devel 
Gerrit-Attention: flichtenheld 
Gerrit-MessageType: newchange
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

[Openvpn-devel] [S] Change in openvpn[master]: Introduce report_command_status helper function

[plaisthos (Code Review)]

Attention is currently required from: flichtenheld.

Hello flichtenheld,

I'd like you to do a code review.
Please visit

http://gerrit.openvpn.net/c/openvpn/+/450?usp=email

to review the following change.


Change subject: Introduce report_command_status helper function
..

Introduce report_command_status helper function

Instead of repeating near identical code several times
in manage.c, use a small helper function instead.

Change-Id: I91f739f5cb43386b2ce767cf3603a76e6b93e216
---
M src/openvpn/manage.c
1 file changed, 21 insertions(+), 24 deletions(-)



  git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/50/450/1

diff --git a/src/openvpn/manage.c b/src/openvpn/manage.c
index feb62b2..3cf392a 100644
--- a/src/openvpn/manage.c
+++ b/src/openvpn/manage.c
@@ -284,6 +284,24 @@
 #endif
 }

+
+/**
+ * Small function to report the success or failure of a command to
+ * the management interface
+ */
+static void
+report_command_status(const bool status, const char *command)
+{
+if (status)
+{
+msg(M_CLIENT, "SUCCESS: %s command succeeded", command);
+}
+else
+{
+msg(M_CLIENT, "ERROR: %s command failed", command);
+}
+}
+
 static void
 man_delete_unix_socket(struct management *man)
 {
@@ -974,14 +992,7 @@
 NULL,
 man->connection.in_extra);
 man->connection.in_extra = NULL;
-if (status)
-{
-msg(M_CLIENT, "SUCCESS: client-auth command succeeded");
-}
-else
-{
-msg(M_CLIENT, "ERROR: client-auth command failed");
-}
+report_command_status(status, "client_auth");
 }
 else
 {
@@ -1260,14 +1271,7 @@
 if (man->persist.callback.proxy_cmd)
 {
 const bool status = 
(*man->persist.callback.proxy_cmd)(man->persist.callback.arg, p);
-if (status)
-{
-msg(M_CLIENT, "SUCCESS: proxy command succeeded");
-}
-else
-{
-msg(M_CLIENT, "ERROR: proxy command failed");
-}
+report_command_status(status, "proxy");
 }
 else
 {
@@ -1281,14 +1285,7 @@
 if (man->persist.callback.remote_cmd)
 {
 const bool status = 
(*man->persist.callback.remote_cmd)(man->persist.callback.arg, p);
-if (status)
-{
-msg(M_CLIENT, "SUCCESS: remote command succeeded");
-}
-else
-{
-msg(M_CLIENT, "ERROR: remote command failed");
-}
+report_command_status(status, "remote");
 }
 else
 {

--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/450?usp=email
To unsubscribe, or for help writing mail filters, visit 
http://gerrit.openvpn.net/settings

Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I91f739f5cb43386b2ce767cf3603a76e6b93e216
Gerrit-Change-Number: 450
Gerrit-PatchSet: 1
Gerrit-Owner: plaisthos 
Gerrit-Reviewer: flichtenheld 
Gerrit-CC: openvpn-devel 
Gerrit-Attention: flichtenheld 
Gerrit-MessageType: newchange
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

[Openvpn-devel] [XS] Change in openvpn[master]: Remove unused function prototype crypto_adjust_frame_parameters

[plaisthos (Code Review)]

Attention is currently required from: flichtenheld.

Hello flichtenheld,

I'd like you to do a code review.
Please visit

http://gerrit.openvpn.net/c/openvpn/+/451?usp=email

to review the following change.


Change subject: Remove unused function prototype crypto_adjust_frame_parameters
..

Remove unused function prototype crypto_adjust_frame_parameters

Change-Id: I1141eb7740d8900ed4af0ff5ff52aa3659df99aa
---
M src/openvpn/crypto.h
1 file changed, 0 insertions(+), 6 deletions(-)



  git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/51/451/1

diff --git a/src/openvpn/crypto.h b/src/openvpn/crypto.h
index c5fd253..9255d38 100644
--- a/src/openvpn/crypto.h
+++ b/src/openvpn/crypto.h
@@ -429,12 +429,6 @@
  struct gc_arena *gc);


-/** Calculate crypto overhead and adjust frame to account for that */
-void crypto_adjust_frame_parameters(struct frame *frame,
-const struct key_type *kt,
-bool packet_id,
-bool packet_id_long_form);
-
 /** Calculate the maximum overhead that our encryption has
  * on a packet. This does not include needed additional buffer size
  *

--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/451?usp=email
To unsubscribe, or for help writing mail filters, visit 
http://gerrit.openvpn.net/settings

Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I1141eb7740d8900ed4af0ff5ff52aa3659df99aa
Gerrit-Change-Number: 451
Gerrit-PatchSet: 1
Gerrit-Owner: plaisthos 
Gerrit-Reviewer: flichtenheld 
Gerrit-CC: openvpn-devel 
Gerrit-Attention: flichtenheld 
Gerrit-MessageType: newchange
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

[Openvpn-devel] [XS] Change in openvpn[master]: Deprecate tls-exit option

[flichtenheld (Code Review)]

Attention is currently required from: plaisthos.

flichtenheld has posted comments on this change. ( 
http://gerrit.openvpn.net/c/openvpn/+/447?usp=email )

Change subject: Deprecate tls-exit option
..


Patch Set 1: Code-Review-1

(3 comments)

Commit Message:

http://gerrit.openvpn.net/c/openvpn/+/447/comment/9a2eea6d_bac3bdc0 :
PS1, Line 9: This option is questionable and I cannot see any reason to 
actually use it.
t_cltsrv.sh uses it, so I think when claiming it is questionable you should 
remove that usage first.


Patchset:

PS1:
I don't think we should continue using the option when claiming we know of no 
use-case for it.


File Changes.rst:

http://gerrit.openvpn.net/c/openvpn/+/447/comment/a41e900e_2065f27a :
PS1, Line 15: the will otherwise be removed in the future.
"the option will"



--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/447?usp=email
To unsubscribe, or for help writing mail filters, visit 
http://gerrit.openvpn.net/settings

Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I93afff2372c4150d6bddc8c07fd4ebc8bfb0cc3e
Gerrit-Change-Number: 447
Gerrit-PatchSet: 1
Gerrit-Owner: plaisthos 
Gerrit-Reviewer: flichtenheld 
Gerrit-CC: openvpn-devel 
Gerrit-Attention: plaisthos 
Gerrit-Comment-Date: Mon, 20 Nov 2023 11:40:25 +
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

[Openvpn-devel] [XS] Change in openvpn[master]: Log SSL alerts more prominently

[flichtenheld (Code Review)]

Attention is currently required from: plaisthos.

flichtenheld has posted comments on this change. ( 
http://gerrit.openvpn.net/c/openvpn/+/448?usp=email )

Change subject: Log SSL alerts more prominently
..


Patch Set 1: Code-Review+2


--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/448?usp=email
To unsubscribe, or for help writing mail filters, visit 
http://gerrit.openvpn.net/settings

Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I6bdab3028c9bd679c31d4177a746a3ea505dcbbf
Gerrit-Change-Number: 448
Gerrit-PatchSet: 1
Gerrit-Owner: plaisthos 
Gerrit-Reviewer: flichtenheld 
Gerrit-CC: openvpn-devel 
Gerrit-Attention: plaisthos 
Gerrit-Comment-Date: Mon, 20 Nov 2023 11:43:14 +
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

[Openvpn-devel] [XS] Change in openvpn[master]: Add check for nice in cmake config

[plaisthos (Code Review)]

Attention is currently required from: plaisthos.

Hello flichtenheld,

I'd like you to reexamine a change. Please visit

http://gerrit.openvpn.net/c/openvpn/+/444?usp=email

to look at the new patch set (#2).

The change is no longer submittable: checks~ChecksSubmitRule is unsatisfied now.


Change subject: Add check for nice in cmake config
..

Add check for nice in cmake config

Change-Id: I2cc8f9b82079acca250db5871ffd9fad2997d1a8
---
M CMakeLists.txt
M config.h.cmake.in
2 files changed, 2 insertions(+), 1 deletion(-)


  git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/44/444/2

diff --git a/CMakeLists.txt b/CMakeLists.txt
index fe9b596..f82ee96 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -146,6 +146,7 @@
 check_symbol_exists(fork unistd.h HAVE_FORK)
 check_symbol_exists(execve unistd.h HAVE_EXECVE)
 check_symbol_exists(ftruncate unistd.h HAVE_FTRUNCATE)
+check_symbol_exists(nice unistd.h HAVE_NICE)
 check_symbol_exists(setgid unistd.h HAVE_SETGID)
 check_symbol_exists(setuid unistd.h HAVE_SETUID)
 check_symbol_exists(setsid unistd.h HAVE_SETSID)
diff --git a/config.h.cmake.in b/config.h.cmake.in
index 25273ac..c1f5848 100644
--- a/config.h.cmake.in
+++ b/config.h.cmake.in
@@ -209,7 +209,7 @@
 #cmakedefine HAVE_NET_TUN_IF_TUN_H

 /* Define to 1 if you have the `nice' function. */
-#undef HAVE_NICE
+#cmakedefine HAVE_NICE

 /* Define to 1 if you have the `openlog' function. */
 #cmakedefine HAVE_OPENLOG

--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/444?usp=email
To unsubscribe, or for help writing mail filters, visit 
http://gerrit.openvpn.net/settings

Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I2cc8f9b82079acca250db5871ffd9fad2997d1a8
Gerrit-Change-Number: 444
Gerrit-PatchSet: 2
Gerrit-Owner: plaisthos 
Gerrit-Reviewer: flichtenheld 
Gerrit-CC: openvpn-devel 
Gerrit-Attention: plaisthos 
Gerrit-MessageType: newpatchset
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

[Openvpn-devel] [S] Change in openvpn[master]: Remove unused/uneeded defines from configure and cmake config

[plaisthos (Code Review)]

Attention is currently required from: plaisthos.

Hello flichtenheld,

I'd like you to reexamine a change. Please visit

http://gerrit.openvpn.net/c/openvpn/+/443?usp=email

to look at the new patch set (#2).


Change subject: Remove unused/uneeded defines from configure and cmake config
..

Remove unused/uneeded defines from configure and cmake config

Anonymous unions/structs are technically a custom GNU C99 feature but
was already widely supported by other compilers. With C11 this feature
has become a standard feature so all compilers nowadays support it.

Change-Id: Ifd0376b36d4050dc22bc93b8fcf7ed29faef0021
---
M config.h.cmake.in
M configure.ac
M src/openvpn/mroute.h
3 files changed, 2 insertions(+), 45 deletions(-)


  git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/43/443/2

diff --git a/config.h.cmake.in b/config.h.cmake.in
index 19b79bc..25273ac 100644
--- a/config.h.cmake.in
+++ b/config.h.cmake.in
@@ -62,9 +62,6 @@
 /* Enable --x509-username-field feature */
 #cmakedefine ENABLE_X509ALTUSERNAME

-/* Compiler supports anonymous unions */
-#define HAVE_ANONYMOUS_UNION_SUPPORT
-
 /* Define to 1 if you have the  header file. */
 #cmakedefine HAVE_ARPA_INET_H 1

@@ -139,9 +136,6 @@
 /* Define to 1 if you have the `getpwnam' function. */
 #cmakedefine HAVE_GETPWNAM

-/* Define to 1 if you have the `getrlimit' function. */
-#undef HAVE_GETRLIMIT
-
 /* Define to 1 if you have the `getsockname' function. */
 #cmakedefine HAVE_GETSOCKNAME

@@ -235,8 +229,6 @@
 /* Define to 1 if you have the  header file. */
 #cmakedefine HAVE_PWD_H

-/* Define to 1 if you have the `readv' function. */
-#undef HAVE_READV

 /* Define to 1 if you have the `recvmsg' function. */
 #cmakedefine HAVE_RECVMSG
@@ -383,9 +375,6 @@
 /* Define to 1 if you have the  header file. */
 #undef HAVE_VFORK_H

-/* Define to 1 if you have the `vsnprintf' function. */
-#undef HAVE_VSNPRINTF
-
 /* we always assume a recent mbed TLS version */
 #define HAVE_MBEDTLS_PSA_CRYPTO_H 1
 #define HAVE_MBEDTLS_SSL_TLS_PRF 1
diff --git a/configure.ac b/configure.ac
index 84eaad6..055b2a7 100644
--- a/configure.ac
+++ b/configure.ac
@@ -561,28 +561,6 @@
,
[[${SOCKET_INCLUDES}]]
 )
-AC_MSG_CHECKING([anonymous union support])
-AC_COMPILE_IFELSE(
-   [AC_LANG_PROGRAM(
-   [[
-   struct mystruct {
- union {
-   int m1;
-   char m2;
- };
-   };
-   ]],
-   [[
-   struct mystruct s;
-   s.m1 = 1; s.m2 = 2;
-   ]]
-   )],
-   [
-   AC_MSG_RESULT([yes])
-   AC_DEFINE([HAVE_ANONYMOUS_UNION_SUPPORT], [], [Compiler 
supports anonymous unions])
-   ],
-   [AC_MSG_RESULT([no])]
-)

 saved_LDFLAGS="$LDFLAGS"
 LDFLAGS="$LDFLAGS -Wl,--wrap=exit"
@@ -655,7 +633,7 @@
 AC_CHECK_FUNCS([ \
daemon chroot getpwnam setuid nice system dup dup2 \
syslog openlog mlockall getrlimit getgrnam setgid \
-   setgroups flock readv writev time gettimeofday \
+   setgroups flock time gettimeofday \
setsid chdir \
chsize ftruncate execve getpeereid basename dirname access \
epoll_create strsep \
diff --git a/src/openvpn/mroute.h b/src/openvpn/mroute.h
index a06e872..7cfa18e 100644
--- a/src/openvpn/mroute.h
+++ b/src/openvpn/mroute.h
@@ -96,17 +96,7 @@
 uint8_t prefix[12];
 in_addr_t addr; /* _network order_ IPv4 address */
 } v4mappedv6;
-}
-#ifndef HAVE_ANONYMOUS_UNION_SUPPORT
-/* Wrappers to support compilers that do not grok anonymous unions */
-mroute_union
-#define raw_addr mroute_union.raw_addr
-#define ether mroute_union.ether
-#define v4 mroute_union.v4
-#define v6 mroute_union.v6
-#define v4mappedv6 mroute_union.v4mappedv6
-#endif
-;
+};
 };

 /* Double-check that struct packing works as expected */

--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/443?usp=email
To unsubscribe, or for help writing mail filters, visit 
http://gerrit.openvpn.net/settings

Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: Ifd0376b36d4050dc22bc93b8fcf7ed29faef0021
Gerrit-Change-Number: 443
Gerrit-PatchSet: 2
Gerrit-Owner: plaisthos 
Gerrit-Reviewer: flichtenheld 
Gerrit-CC: openvpn-devel 
Gerrit-Attention: plaisthos 
Gerrit-MessageType: newpatchset
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

[Openvpn-devel] [S] Change in openvpn[master]: Remove unused/uneeded defines from configure and cmake config

[plaisthos (Code Review)]

Attention is currently required from: flichtenheld.

plaisthos has posted comments on this change. ( 
http://gerrit.openvpn.net/c/openvpn/+/443?usp=email )

Change subject: Remove unused/uneeded defines from configure and cmake config
..


Patch Set 2:

(2 comments)

Commit Message:

http://gerrit.openvpn.net/c/openvpn/+/443/comment/1929ebea_270a8e08 :
PS1, Line 7: Remove unused defined from configure and cmake config
> "defines"
Done


File configure.ac:

http://gerrit.openvpn.net/c/openvpn/+/443/comment/4c0f0d83_98a4fc60 :
PS1, Line 582:  AC_DEFINE([HAVE_ANONYMOUS_UNION_SUPPORT], [], [Compiler 
supports anonymous unions])
> This is used in mroute.h. Need to remove that usage.
Done



--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/443?usp=email
To unsubscribe, or for help writing mail filters, visit 
http://gerrit.openvpn.net/settings

Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: Ifd0376b36d4050dc22bc93b8fcf7ed29faef0021
Gerrit-Change-Number: 443
Gerrit-PatchSet: 2
Gerrit-Owner: plaisthos 
Gerrit-Reviewer: flichtenheld 
Gerrit-CC: openvpn-devel 
Gerrit-Attention: flichtenheld 
Gerrit-Comment-Date: Mon, 20 Nov 2023 11:44:55 +
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: flichtenheld 
Gerrit-MessageType: comment
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

[Openvpn-devel] [XS] Change in openvpn[master]: Deprecate tls-exit option

[plaisthos (Code Review)]

Attention is currently required from: flichtenheld.

plaisthos has posted comments on this change. ( 
http://gerrit.openvpn.net/c/openvpn/+/447?usp=email )

Change subject: Deprecate tls-exit option
..


Patch Set 1:

(1 comment)

Patchset:

PS1:
> I don't think we should continue using the option when claiming we know of no 
> use-case for it.
So it is more a testing option? If we want to keep it for that we could 
document it in the man page that it is meant for testing and not for real world 
use.



--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/447?usp=email
To unsubscribe, or for help writing mail filters, visit 
http://gerrit.openvpn.net/settings

Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I93afff2372c4150d6bddc8c07fd4ebc8bfb0cc3e
Gerrit-Change-Number: 447
Gerrit-PatchSet: 1
Gerrit-Owner: plaisthos 
Gerrit-Reviewer: flichtenheld 
Gerrit-CC: openvpn-devel 
Gerrit-Attention: flichtenheld 
Gerrit-Comment-Date: Mon, 20 Nov 2023 11:49:01 +
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: flichtenheld 
Gerrit-MessageType: comment
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

[Openvpn-devel] [M] Change in openvpn[master]: Remove compat versionhelpers.h and remove cmake/configure check for it

[plaisthos (Code Review)]

Attention is currently required from: flichtenheld.

plaisthos has posted comments on this change. ( 
http://gerrit.openvpn.net/c/openvpn/+/445?usp=email )

Change subject: Remove compat versionhelpers.h and remove cmake/configure check 
for it
..


Patch Set 1:

(1 comment)

Commit Message:

http://gerrit.openvpn.net/c/openvpn/+/445/comment/b2ce1fb4_43a25ec3 :
PS1, Line 10: old msvc-config.h that always had it present. Also interactive.c 
includes
> interactive. […]
Yeah. interactive.c is just

#include 

#include 

#include "openvpn-msg.h"



--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/445?usp=email
To unsubscribe, or for help writing mail filters, visit 
http://gerrit.openvpn.net/settings

Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I9c85ccab6d51064ebff2c391740ba8c2d044ed1a
Gerrit-Change-Number: 445
Gerrit-PatchSet: 1
Gerrit-Owner: plaisthos 
Gerrit-Reviewer: flichtenheld 
Gerrit-CC: openvpn-devel 
Gerrit-Attention: flichtenheld 
Gerrit-Comment-Date: Mon, 20 Nov 2023 11:49:42 +
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: flichtenheld 
Gerrit-MessageType: comment
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

[Openvpn-devel] [M] Change in openvpn[master]: Remove compat versionhelpers.h and remove cmake/configure check for it

[plaisthos (Code Review)]

Attention is currently required from: flichtenheld.

plaisthos has posted comments on this change. ( 
http://gerrit.openvpn.net/c/openvpn/+/445?usp=email )

Change subject: Remove compat versionhelpers.h and remove cmake/configure check 
for it
..


Patch Set 1:

(1 comment)

Commit Message:

http://gerrit.openvpn.net/c/openvpn/+/445/comment/ad40222e_e26eea37 :
PS1, Line 10: old msvc-config.h that always had it present. Also interactive.c 
includes
> Yeah. interactive.c is just […]
argh. SOrry. I messed that up thinking that I have not modified that that was 
not part of the patch. sorry. Will rewrite the commit message.



--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/445?usp=email
To unsubscribe, or for help writing mail filters, visit 
http://gerrit.openvpn.net/settings

Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I9c85ccab6d51064ebff2c391740ba8c2d044ed1a
Gerrit-Change-Number: 445
Gerrit-PatchSet: 1
Gerrit-Owner: plaisthos 
Gerrit-Reviewer: flichtenheld 
Gerrit-CC: openvpn-devel 
Gerrit-Attention: flichtenheld 
Gerrit-Comment-Date: Mon, 20 Nov 2023 11:50:33 +
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: plaisthos 
Comment-In-Reply-To: flichtenheld 
Gerrit-MessageType: comment
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

[Openvpn-devel] [M] Change in openvpn[master]: Remove compat versionhelpers.h and remove cmake/configure check for it

[plaisthos (Code Review)]

Attention is currently required from: flichtenheld.

Hello flichtenheld, 

I'd like you to reexamine a change. Please visit

http://gerrit.openvpn.net/c/openvpn/+/445?usp=email

to look at the new patch set (#2).

The following approvals got outdated and were removed:
Code-Review-1 by flichtenheld


Change subject: Remove compat versionhelpers.h and remove cmake/configure check 
for it
..

Remove compat versionhelpers.h and remove cmake/configure check for it

The cmake file defined that file to be never present in contrast to the
old msvc-config.h that always had it present.
Remove also the comapt implementation taken from mingw. All our current
build environment already have that header in place.

Change-Id: I9c85ccab6d51064ebff2c391740ba8c2d044ed1a
---
M CMakeLists.txt
M config.h.cmake.in
M configure.ac
M src/compat/Makefile.am
D src/compat/compat-versionhelpers.h
M src/openvpn/win32.c
M src/openvpnserv/interactive.c
7 files changed, 1 insertion(+), 131 deletions(-)


  git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/45/445/2

diff --git a/CMakeLists.txt b/CMakeLists.txt
index d21c9bd..fa6d623 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -308,7 +308,6 @@
 src/compat/compat-dirname.c
 src/compat/compat-gettimeofday.c
 src/compat/compat-strsep.c
-src/compat/compat-versionhelpers.h
 src/openvpn/argv.c
 src/openvpn/argv.h
 src/openvpn/base64.c
diff --git a/config.h.cmake.in b/config.h.cmake.in
index 19b79bc..8edaff4 100644
--- a/config.h.cmake.in
+++ b/config.h.cmake.in
@@ -374,9 +374,6 @@
 /* Define to 1 if you have the  header file. */
 #undef HAVE_VALGRIND_MEMCHECK_H

-/* Define to 1 if you have the  header file. */
-#undef HAVE_VERSIONHELPERS_H
-
 /* Define to 1 if you have the `vfork' function. */
 #undef HAVE_VFORK

diff --git a/configure.ac b/configure.ac
index 84eaad6..94c6654 100644
--- a/configure.ac
+++ b/configure.ac
@@ -455,7 +455,6 @@
unistd.h dlfcn.h \
netinet/in.h \
netinet/tcp.h arpa/inet.h netdb.h \
-   versionhelpers.h \
 ])
 AC_CHECK_HEADERS([ \
sys/time.h sys/ioctl.h sys/stat.h \
diff --git a/src/compat/Makefile.am b/src/compat/Makefile.am
index f5de451..5298dd8 100644
--- a/src/compat/Makefile.am
+++ b/src/compat/Makefile.am
@@ -20,5 +20,4 @@
compat-basename.c \
compat-gettimeofday.c \
compat-daemon.c \
-   compat-strsep.c \
-   compat-versionhelpers.h
+   compat-strsep.c
\ No newline at end of file
diff --git a/src/compat/compat-versionhelpers.h 
b/src/compat/compat-versionhelpers.h
deleted file mode 100644
index b071602..000
--- a/src/compat/compat-versionhelpers.h
+++ /dev/null
@@ -1,116 +0,0 @@
-/**
- * This file is part of the mingw-w64 runtime package.
- * No warranty is given; refer to the file DISCLAIMER within this package.
- */
-
-#ifndef _INC_VERSIONHELPERS
-#define _INC_VERSIONHELPERS
-
-#include 
-
-#if WINAPI_FAMILY_PARTITION(WINAPI_PARTITION_DESKTOP) && !defined(__WIDL__)
-
-#ifdef __cplusplus
-#define VERSIONHELPERAPI inline bool
-#else
-#define VERSIONHELPERAPI FORCEINLINE BOOL
-#endif
-
-#define _WIN32_WINNT_WINBLUE0x0603
-
-#ifndef _WIN32_WINNT_WINTHRESHOLD
-#define _WIN32_WINNT_WINTHRESHOLD0x0A00 /* Windows 10 */
-#endif
-
-VERSIONHELPERAPI
-IsWindowsVersionOrGreater(WORD major, WORD minor, WORD servpack)
-{
-OSVERSIONINFOEXW vi = {sizeof(vi), major, minor, 0, 0, {0}, servpack};
-return VerifyVersionInfoW(&vi, 
VER_MAJORVERSION|VER_MINORVERSION|VER_SERVICEPACKMAJOR,
-  
VerSetConditionMask(VerSetConditionMask(VerSetConditionMask(0,
-   
   VER_MAJORVERSION, VER_GREATER_EQUAL),
-  
VER_MINORVERSION, VER_GREATER_EQUAL),
-  VER_SERVICEPACKMAJOR, 
VER_GREATER_EQUAL));
-}
-
-VERSIONHELPERAPI
-IsWindowsXPOrGreater(void)
-{
-return IsWindowsVersionOrGreater(HIBYTE(_WIN32_WINNT_WINXP), 
LOBYTE(_WIN32_WINNT_WINXP), 0);
-}
-
-VERSIONHELPERAPI
-IsWindowsXPSP1OrGreater(void)
-{
-return IsWindowsVersionOrGreater(HIBYTE(_WIN32_WINNT_WINXP), 
LOBYTE(_WIN32_WINNT_WINXP), 1);
-}
-
-VERSIONHELPERAPI
-IsWindowsXPSP2OrGreater(void)
-{
-return IsWindowsVersionOrGreater(HIBYTE(_WIN32_WINNT_WINXP), 
LOBYTE(_WIN32_WINNT_WINXP), 2);
-}
-
-VERSIONHELPERAPI
-IsWindowsXPSP3OrGreater(void)
-{
-return IsWindowsVersionOrGreater(HIBYTE(_WIN32_WINNT_WINXP), 
LOBYTE(_WIN32_WINNT_WINXP), 3);
-}
-
-VERSIONHELPERAPI
-IsWindowsVistaOrGreater(void)
-{
-return IsWindowsVersionOrGreater(HIBYTE(_WIN32_WINNT_VISTA), 
LOBYTE(_WIN32_WINNT_VISTA), 0);
-}
-
-VERSIONHELPERAPI
-IsWindowsVistaSP1OrGreater(void)
-{
-return IsWindowsVersionOrGreater(HIBYTE(_WIN32_WINNT_VISTA), 
LOBYTE(_WIN32_WINNT_VISTA), 1);
-}
-
-VERSIONHELPERAPI
-IsWindowsVistaSP2OrGreater(vo

[Openvpn-devel] [M] Change in openvpn[master]: Allow the TLS session to send out TLS alerts

[flichtenheld (Code Review)]

Attention is currently required from: plaisthos.

flichtenheld has posted comments on this change. ( 
http://gerrit.openvpn.net/c/openvpn/+/449?usp=email )

Change subject: Allow the TLS session to send out TLS alerts
..


Patch Set 1: Code-Review-2

(9 comments)

Commit Message:

http://gerrit.openvpn.net/c/openvpn/+/449/comment/ce9fb57d_f01b1b19 :
PS1, Line 20: Sending out alerts  is a slight compromise in security as alerts 
give
superfluous space


http://gerrit.openvpn.net/c/openvpn/+/449/comment/8bf6026e_889913be :
PS1, Line 22: out. But since all other consumers TLS implementation are already 
doing this
"consumer TLS implementations"


http://gerrit.openvpn.net/c/openvpn/+/449/comment/a80886c0_b6d5627c :
PS1, Line 23: and TLS implementation (nowadays) are very careful not to leak 
(sensitive)
"implementations"


Patchset:

PS1:
Can't judge the state machine patch really. But the doubts about the shutdown 
function makes this a NAK either way.


File Changes.rst:

http://gerrit.openvpn.net/c/openvpn/+/449/comment/9c3faa2e_d4fd7ba3 :
PS1, Line 6: OpenVPN 2.7 will send out TLS alerts to peer informing them if 
the TLS
"peers"


http://gerrit.openvpn.net/c/openvpn/+/449/comment/fd295cc6_2def6a9b :
PS1, Line 10: a timeout when the server just stop responding completely.
"stops"


File src/openvpn/ssl.c:

http://gerrit.openvpn.net/c/openvpn/+/449/comment/38284760_a524bebe :
PS1, Line 2846:  * Shut down an SSL session, so an SSL close notify is sent if 
there no other
"there is"


http://gerrit.openvpn.net/c/openvpn/+/449/comment/8e4b3e48_338f6a18 :
PS1, Line 2851: do_ssl_shutdown(struct key_state *ks)
Why do we need an empty function? Was this supposed to call 
key_state_ssl_shutdown?


File src/openvpn/ssl_backend.h:

http://gerrit.openvpn.net/c/openvpn/+/449/comment/fa6d4de4_e2eda72e :
PS1, Line 376:  * a shutdown altert.
alert



--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/449?usp=email
To unsubscribe, or for help writing mail filters, visit 
http://gerrit.openvpn.net/settings

Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I0ad48915004ddee587e97c8ed190ba8ee989e48d
Gerrit-Change-Number: 449
Gerrit-PatchSet: 1
Gerrit-Owner: plaisthos 
Gerrit-Reviewer: flichtenheld 
Gerrit-CC: openvpn-devel 
Gerrit-Attention: plaisthos 
Gerrit-Comment-Date: Mon, 20 Nov 2023 11:55:56 +
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

[Openvpn-devel] [S] Change in openvpn[master]: Introduce report_command_status helper function

[flichtenheld (Code Review)]

Attention is currently required from: plaisthos.

flichtenheld has posted comments on this change. ( 
http://gerrit.openvpn.net/c/openvpn/+/450?usp=email )

Change subject: Introduce report_command_status helper function
..


Patch Set 1: Code-Review+2


--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/450?usp=email
To unsubscribe, or for help writing mail filters, visit 
http://gerrit.openvpn.net/settings

Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I91f739f5cb43386b2ce767cf3603a76e6b93e216
Gerrit-Change-Number: 450
Gerrit-PatchSet: 1
Gerrit-Owner: plaisthos 
Gerrit-Reviewer: flichtenheld 
Gerrit-CC: openvpn-devel 
Gerrit-Attention: plaisthos 
Gerrit-Comment-Date: Mon, 20 Nov 2023 11:57:47 +
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

[Openvpn-devel] [XS] Change in openvpn[master]: Remove unused function prototype crypto_adjust_frame_parameters

[flichtenheld (Code Review)]

Attention is currently required from: plaisthos.

flichtenheld has posted comments on this change. ( 
http://gerrit.openvpn.net/c/openvpn/+/451?usp=email )

Change subject: Remove unused function prototype crypto_adjust_frame_parameters
..


Patch Set 1: Code-Review+2


--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/451?usp=email
To unsubscribe, or for help writing mail filters, visit 
http://gerrit.openvpn.net/settings

Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I1141eb7740d8900ed4af0ff5ff52aa3659df99aa
Gerrit-Change-Number: 451
Gerrit-PatchSet: 1
Gerrit-Owner: plaisthos 
Gerrit-Reviewer: flichtenheld 
Gerrit-CC: openvpn-devel 
Gerrit-Attention: plaisthos 
Gerrit-Comment-Date: Mon, 20 Nov 2023 11:58:27 +
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

[Openvpn-devel] [XS] Change in openvpn[master]: Deprecate tls-exit option

[flichtenheld (Code Review)]

Attention is currently required from: plaisthos.

flichtenheld has posted comments on this change. ( 
http://gerrit.openvpn.net/c/openvpn/+/447?usp=email )

Change subject: Deprecate tls-exit option
..


Patch Set 1:

(1 comment)

Patchset:

PS1:
> So it is more a testing option? If we want to keep it for that we could 
> document it in the man page  […]
I don't know, the origins of this options are in the pre-git days, so a quick 
search didn't bring up anything useful. I'm saying we need to look into this 
before deciding to deprecate it.



--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/447?usp=email
To unsubscribe, or for help writing mail filters, visit 
http://gerrit.openvpn.net/settings

Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I93afff2372c4150d6bddc8c07fd4ebc8bfb0cc3e
Gerrit-Change-Number: 447
Gerrit-PatchSet: 1
Gerrit-Owner: plaisthos 
Gerrit-Reviewer: flichtenheld 
Gerrit-CC: openvpn-devel 
Gerrit-Attention: plaisthos 
Gerrit-Comment-Date: Mon, 20 Nov 2023 12:01:04 +
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: plaisthos 
Comment-In-Reply-To: flichtenheld 
Gerrit-MessageType: comment
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

[Openvpn-devel] [M] Change in openvpn[master]: Remove compat versionhelpers.h and remove cmake/configure check for it

[flichtenheld (Code Review)]

Attention is currently required from: plaisthos.

flichtenheld has posted comments on this change. ( 
http://gerrit.openvpn.net/c/openvpn/+/445?usp=email )

Change subject: Remove compat versionhelpers.h and remove cmake/configure check 
for it
..


Patch Set 2: Code-Review+2

(3 comments)

Commit Message:

http://gerrit.openvpn.net/c/openvpn/+/445/comment/6d2ffebd_108c2911 :
PS2, Line 11: Remove also the comapt implementation taken from mingw. All our 
current
"compat"


http://gerrit.openvpn.net/c/openvpn/+/445/comment/96f8a775_b2f3fbf2 :
PS2, Line 12: build environment already have that header in place.
"environments"


Patchset:

PS2:
Two little typos in the commit message. But code looks good.



--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/445?usp=email
To unsubscribe, or for help writing mail filters, visit 
http://gerrit.openvpn.net/settings

Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I9c85ccab6d51064ebff2c391740ba8c2d044ed1a
Gerrit-Change-Number: 445
Gerrit-PatchSet: 2
Gerrit-Owner: plaisthos 
Gerrit-Reviewer: flichtenheld 
Gerrit-CC: openvpn-devel 
Gerrit-Attention: plaisthos 
Gerrit-Comment-Date: Mon, 20 Nov 2023 12:06:34 +
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

[Openvpn-devel] [S] Change in openvpn[master]: Remove unused/uneeded defines from configure and cmake config

[flichtenheld (Code Review)]

Attention is currently required from: plaisthos.

flichtenheld has posted comments on this change. ( 
http://gerrit.openvpn.net/c/openvpn/+/443?usp=email )

Change subject: Remove unused/uneeded defines from configure and cmake config
..


Patch Set 2: Code-Review+2

(1 comment)

Commit Message:

http://gerrit.openvpn.net/c/openvpn/+/443/comment/91e569b6_1c969f5d :
PS2, Line 7: Remove unused/uneeded defines from configure and cmake config
"unneeded"



--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/443?usp=email
To unsubscribe, or for help writing mail filters, visit 
http://gerrit.openvpn.net/settings

Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: Ifd0376b36d4050dc22bc93b8fcf7ed29faef0021
Gerrit-Change-Number: 443
Gerrit-PatchSet: 2
Gerrit-Owner: plaisthos 
Gerrit-Reviewer: flichtenheld 
Gerrit-CC: openvpn-devel 
Gerrit-Attention: plaisthos 
Gerrit-Comment-Date: Mon, 20 Nov 2023 12:09:37 +
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

[Openvpn-devel] [M] Change in openvpn[master]: Allow the TLS session to send out TLS alerts

[plaisthos (Code Review)]

Attention is currently required from: flichtenheld.

plaisthos has posted comments on this change. ( 
http://gerrit.openvpn.net/c/openvpn/+/449?usp=email )

Change subject: Allow the TLS session to send out TLS alerts
..


Patch Set 1:

(8 comments)

Commit Message:

http://gerrit.openvpn.net/c/openvpn/+/449/comment/e66f6133_04b1cc5d :
PS1, Line 20: Sending out alerts  is a slight compromise in security as alerts 
give
> superfluous space
Done


http://gerrit.openvpn.net/c/openvpn/+/449/comment/d04c679a_cc1ae3ed :
PS1, Line 22: out. But since all other consumers TLS implementation are already 
doing this
> "consumer TLS implementations"
Done


http://gerrit.openvpn.net/c/openvpn/+/449/comment/8660b029_d3b05646 :
PS1, Line 23: and TLS implementation (nowadays) are very careful not to leak 
(sensitive)
> "implementations"
Done


File Changes.rst:

http://gerrit.openvpn.net/c/openvpn/+/449/comment/9f072ad8_eab0e86f :
PS1, Line 6: OpenVPN 2.7 will send out TLS alerts to peer informing them if 
the TLS
> "peers"
Done


http://gerrit.openvpn.net/c/openvpn/+/449/comment/568b8456_4669a9a3 :
PS1, Line 10: a timeout when the server just stop responding completely.
> "stops"
Done


File src/openvpn/ssl.c:

http://gerrit.openvpn.net/c/openvpn/+/449/comment/a39b7fdf_e1dbd71f :
PS1, Line 2846:  * Shut down an SSL session, so an SSL close notify is sent if 
there no other
> "there is"
Done


http://gerrit.openvpn.net/c/openvpn/+/449/comment/8e57006f_630e475d :
PS1, Line 2851: do_ssl_shutdown(struct key_state *ks)
> Why do we need an empty function? Was this supposed to call 
> key_state_ssl_shutdown?
Yes. That is accidentally leftover code from an earlier version. I decided on 
the key_state_ssl_shutdown name as that aligns more with similar functions in 
existing code


File src/openvpn/ssl_backend.h:

http://gerrit.openvpn.net/c/openvpn/+/449/comment/6cfd1f28_022bf406 :
PS1, Line 376:  * a shutdown altert.
> alert
Done



--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/449?usp=email
To unsubscribe, or for help writing mail filters, visit 
http://gerrit.openvpn.net/settings

Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I0ad48915004ddee587e97c8ed190ba8ee989e48d
Gerrit-Change-Number: 449
Gerrit-PatchSet: 1
Gerrit-Owner: plaisthos 
Gerrit-Reviewer: flichtenheld 
Gerrit-CC: openvpn-devel 
Gerrit-Attention: flichtenheld 
Gerrit-Comment-Date: Mon, 20 Nov 2023 13:02:18 +
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: flichtenheld 
Gerrit-MessageType: comment
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

[Openvpn-devel] [M] Change in openvpn[master]: Allow the TLS session to send out TLS alerts

[plaisthos (Code Review)]

Attention is currently required from: flichtenheld.

Hello flichtenheld, 

I'd like you to reexamine a change. Please visit

http://gerrit.openvpn.net/c/openvpn/+/449?usp=email

to look at the new patch set (#2).


Change subject: Allow the TLS session to send out TLS alerts
..

Allow the TLS session to send out TLS alerts

Previous OpenVPN versions shut down the TLS control channel immediately
when encountering an error. This also meant that we would not send out
TLS alerts to notify a client about potential problems like mismatching
TLS versions or having no common cipher.

This commit adds a new key_state S_ERROR_PRE which still allows to
send out the remaining TLS packets of the control session which are
typically the alert message and then going to S_ERROR. We do not
wait for retries. So this is more a one-shot notify but that is
acceptable in this situation.

Sending out alerts is a slight compromise in security as alerts give
out a bit of information that otherwise is not given
out. But since all other consumers TLS implementations are already doing this
and TLS implementations (nowadays) are very careful not to leak (sensitive)
information by alerts and since the user experience is much better with
alerts, this compromise is worth it.

Change-Id: I0ad48915004ddee587e97c8ed190ba8ee989e48d
---
M Changes.rst
M src/openvpn/ssl.c
M src/openvpn/ssl_backend.h
M src/openvpn/ssl_common.h
M src/openvpn/ssl_mbedtls.c
M src/openvpn/ssl_openssl.c
6 files changed, 87 insertions(+), 18 deletions(-)


  git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/49/449/2

diff --git a/Changes.rst b/Changes.rst
index 3676dce..dbef0d9 100644
--- a/Changes.rst
+++ b/Changes.rst
@@ -1,5 +1,16 @@
 Overview of changes in 2.7
 ==
+New features
+
+TLS alerts
+OpenVPN 2.7 will send out TLS alerts to peers informing them if the TLS
+session shuts down or when the TLS implementation informs the peer about
+an error in the TLS session (e.g. mismatching TLS versions). This improves
+the user experience as the client shows an error instead of running into
+a timeout when the server just stops responding completely.
+
+Deprecated features
+---
 ``secret`` support has been removed by default.
 static key mode (non-TLS) is no longer considered "good and secure enough"
 for today's requirements.  Use TLS mode instead.  If deploying a PKI CA
diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
index f46b661..8bc41ac 100644
--- a/src/openvpn/ssl.c
+++ b/src/openvpn/ssl.c
@@ -853,6 +853,9 @@
 case S_ERROR:
 return "S_ERROR";

+case S_ERROR_PRE:
+return "S_ERROR_PRE";
+
 case S_GENERATED_KEYS:
 return "S_GENERATED_KEYS";

@@ -2839,6 +2842,24 @@
 return true;
 }

+static bool
+check_outgoing_ciphertext(struct key_state *ks, struct tls_session *session,
+  bool *state_change)
+{
+/* Outgoing Ciphertext to reliable buffer */
+if (ks->state >= S_PRE_START)
+{
+struct buffer *buf = 
reliable_get_buf_output_sequenced(ks->send_reliable);
+if (buf)
+{
+if (!write_outgoing_tls_ciphertext(session, state_change))
+{
+return false;
+}
+}
+}
+return true;
+}

 static bool
 tls_process_state(struct tls_multi *multi,
@@ -2918,6 +2939,16 @@
 return false;
 }

+if (ks->state == S_ERROR_PRE)
+{
+/* When we end up here, we had one last chance to send an outstanding
+ * packet that contained an alert. We do not ensure that this packet
+ * has been successfully delivered  (ie wait for the ACK etc)
+ * but rather stop processing now */
+ks->state = S_ERROR;
+return false;
+}
+
 /* Write incoming ciphertext to TLS object */
 struct reliable_entry *entry = 
reliable_get_entry_sequenced(ks->rec_reliable);
 if (entry)
@@ -2998,29 +3029,31 @@
 dmsg(D_TLS_DEBUG, "Outgoing Plaintext -> TLS");
 }
 }
-
-/* Outgoing Ciphertext to reliable buffer */
-if (ks->state >= S_START)
+if (!check_outgoing_ciphertext(ks, session, &continue_tls_process))
 {
-buf = reliable_get_buf_output_sequenced(ks->send_reliable);
-if (buf)
-{
-if (!write_outgoing_tls_ciphertext(session, &state_change))
-{
-goto error;
-}
-}
+goto error;
 }

 return continue_tls_process;
 error:
 tls_clear_error();
-ks->state = S_ERROR;
+
+/* Shut down the TLS session but do a last read from the TLS
+ * object to be able to read potential TLS alerts */
+key_state_ssl_shutdown(&ks->ks_ssl);
+check_outgoing_ciphertext(ks, session, &continue_tls_process);
+
+/* Put ourselves in the pre error state that will only send out the
+  

[Openvpn-devel] [S] Change in openvpn[master]: Rename state_change to continue_tls_process

[plaisthos (Code Review)]

Attention is currently required from: flichtenheld.

Hello flichtenheld,

I'd like you to do a code review.
Please visit

http://gerrit.openvpn.net/c/openvpn/+/452?usp=email

to review the following change.


Change subject: Rename state_change to continue_tls_process
..

Rename state_change to continue_tls_process

The name state_change is more confusing than helpful as it not really
indicates if there was a state change but rather if processing should
be continued. There even some states that are definitively state changes
(setting to_link buffer) that require continue_tls_process to be set
to false.

Change-Id: Ib6d713f2eb08a4c39d97de3e1a4a832cedc09585
---
M src/openvpn/ssl.c
1 file changed, 21 insertions(+), 15 deletions(-)



  git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/52/452/1

diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
index b4cd8f5..f46b661 100644
--- a/src/openvpn/ssl.c
+++ b/src/openvpn/ssl.c
@@ -2848,13 +2848,19 @@
   struct link_socket_info *to_link_socket_info,
   interval_t *wakeup)
 {
-bool state_change = false;
+/* This variable indicates if we should call this method
+ * again to process more incoming/outgoing TLS state/data
+ * We want to repeat this until we either determined that there
+ * is nothing more to process or that further processing
+ * should only be done after the outer loop (sending packets etc.)
+ * has run once more */
+bool continue_tls_process = false;
 struct key_state *ks = &session->key[KS_PRIMARY];  /* primary key */

 /* Initial handshake */
 if (ks->state == S_INITIAL)
 {
-state_change = session_move_pre_start(session, ks, false);
+continue_tls_process = session_move_pre_start(session, ks, false);
 }

 /* Are we timed out on receive? */
@@ -2872,7 +2878,7 @@
 if (ks->state == S_PRE_START && reliable_empty(ks->send_reliable))
 {
 ks->state = S_START;
-state_change = true;
+continue_tls_process = true;

 /* New connection, remove any old X509 env variables */
 tls_x509_clear_env(session->opt->es);
@@ -2885,7 +2891,7 @@
 && reliable_empty(ks->send_reliable))
 {
 session_move_active(multi, session, to_link_socket_info, ks);
-state_change = true;
+continue_tls_process = true;
 }

 /* Reliable buffer to outgoing TCP/UDP (send up to CONTROL_SEND_ACK_MAX 
ACKs
@@ -2927,7 +2933,7 @@
 }
 else
 {
-if (!read_incoming_tls_ciphertext(&entry->buf, ks, &state_change))
+if (!read_incoming_tls_ciphertext(&entry->buf, ks, 
&continue_tls_process))
 {
 goto error;
 }
@@ -2938,7 +2944,7 @@
 struct buffer *buf = &ks->plaintext_read_buf;
 if (!buf->len)
 {
-if (!read_incoming_tls_plaintext(ks, buf, wakeup, &state_change))
+if (!read_incoming_tls_plaintext(ks, buf, wakeup, 
&continue_tls_process))
 {
 goto error;
 }
@@ -2954,7 +2960,7 @@
 goto error;
 }

-state_change = true;
+continue_tls_process = true;
 dmsg(D_TLS_DEBUG_MED, "STATE S_SENT_KEY");
 ks->state = S_SENT_KEY;
 }
@@ -2970,7 +2976,7 @@
 goto error;
 }

-state_change = true;
+continue_tls_process = true;
 dmsg(D_TLS_DEBUG_MED, "STATE S_GOT_KEY");
 ks->state = S_GOT_KEY;
 }
@@ -2988,7 +2994,7 @@
 }
 if (status == 1)
 {
-state_change = true;
+continue_tls_process = true;
 dmsg(D_TLS_DEBUG, "Outgoing Plaintext -> TLS");
 }
 }
@@ -3006,7 +3012,7 @@
 }
 }

-return state_change;
+return continue_tls_process;
 error:
 tls_clear_error();
 ks->state = S_ERROR;
@@ -3065,19 +3071,19 @@
 msg(D_TLS_DEBUG_LOW, "TLS: tls_process: killed expiring key");
 }

-bool state_change = true;
-while (state_change)
+bool continue_tls_process = true;
+while (continue_tls_process)
 {
 update_time();

 dmsg(D_TLS_DEBUG, "TLS: tls_process: chg=%d ks=%s lame=%s 
to_link->len=%d wakeup=%d",
- state_change,
+ continue_tls_process,
  state_name(ks->state),
  state_name(ks_lame->state),
  to_link->len,
  *wakeup);
-state_change = tls_process_state(multi, session, to_link, to_link_addr,
- to_link_socket_info, wakeup);
+continue_tls_process = tls_process_state(multi, session, to_link, 
to_link_addr,
+ to_link_socket_info, wakeup);

 if (ks->state == S_ERROR)
 {

--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/452?usp=email
To unsubscribe, or for help writing mail filters, visit 

[Openvpn-devel] [XS] Change in openvpn[master]: Log SSL alerts more prominently

[plaisthos (Code Review)]

Attention is currently required from: plaisthos.

Hello flichtenheld,

I'd like you to reexamine a change. Please visit

http://gerrit.openvpn.net/c/openvpn/+/448?usp=email

to look at the new patch set (#2).

The change is no longer submittable: checks~ChecksSubmitRule is unsatisfied now.


Change subject: Log SSL alerts more prominently
..

Log SSL alerts more prominently

When we receive an SSL alert from a server we currently only log a
very cryptic OpenSSL error message:

   OpenSSL: error:0A00042E:SSL routines::tlsv1 alert protocol version:SSL alert 
number 70

This also enables logging the much more readable SSL error message:

   Received fatal SSL alert: protocol version

which previously needed --verb 8 to be displayed (now verb 3). Also rework the
message to be better readable.

Change-Id: I6bdab3028c9bd679c31d4177a746a3ea505dcbbf
Signed-off-by: Arne Schwabe 
---
M src/openvpn/ssl_openssl.c
1 file changed, 2 insertions(+), 2 deletions(-)


  git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/48/448/2

diff --git a/src/openvpn/ssl_openssl.c b/src/openvpn/ssl_openssl.c
index 23e7623..82872bf 100644
--- a/src/openvpn/ssl_openssl.c
+++ b/src/openvpn/ssl_openssl.c
@@ -196,8 +196,8 @@
 }
 else if (where & SSL_CB_ALERT)
 {
-dmsg(D_HANDSHAKE_VERBOSE, "SSL alert (%s): %s: %s",
- where & SSL_CB_READ ? "read" : "write",
+dmsg(D_TLS_DEBUG_LOW, "%s %s SSL alert: %s",
+ where & SSL_CB_READ ? "Received" : "Sent",
  SSL_alert_type_string_long(ret),
  SSL_alert_desc_string_long(ret));
 }

--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/448?usp=email
To unsubscribe, or for help writing mail filters, visit 
http://gerrit.openvpn.net/settings

Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I6bdab3028c9bd679c31d4177a746a3ea505dcbbf
Gerrit-Change-Number: 448
Gerrit-PatchSet: 2
Gerrit-Owner: plaisthos 
Gerrit-Reviewer: flichtenheld 
Gerrit-CC: openvpn-devel 
Gerrit-Attention: plaisthos 
Gerrit-MessageType: newpatchset
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

[Openvpn-devel] [M] Change in openvpn[master]: Allow the TLS session to send out TLS alerts

[flichtenheld (Code Review)]

Attention is currently required from: plaisthos.

flichtenheld has posted comments on this change. ( 
http://gerrit.openvpn.net/c/openvpn/+/449?usp=email )

Change subject: Allow the TLS session to send out TLS alerts
..


Patch Set 2: Code-Review+1

(1 comment)

Patchset:

PS2:
My concerns have been addressed, but state machine needs further review.



--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/449?usp=email
To unsubscribe, or for help writing mail filters, visit 
http://gerrit.openvpn.net/settings

Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I0ad48915004ddee587e97c8ed190ba8ee989e48d
Gerrit-Change-Number: 449
Gerrit-PatchSet: 2
Gerrit-Owner: plaisthos 
Gerrit-Reviewer: flichtenheld 
Gerrit-CC: openvpn-devel 
Gerrit-Attention: plaisthos 
Gerrit-Comment-Date: Mon, 20 Nov 2023 13:08:11 +
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

[Openvpn-devel] [S] Change in openvpn[master]: Rename state_change to continue_tls_process

[flichtenheld (Code Review)]

Attention is currently required from: plaisthos.

flichtenheld has posted comments on this change. ( 
http://gerrit.openvpn.net/c/openvpn/+/452?usp=email )

Change subject: Rename state_change to continue_tls_process
..


Patch Set 1: Code-Review-1

(3 comments)

Patchset:

PS1:
Good change, but should recurse


File src/openvpn/ssl.c:

http://gerrit.openvpn.net/c/openvpn/+/452/comment/bdfcf50c_c5c3ee11 :
PS1, Line 2936: if (!read_incoming_tls_ciphertext(&entry->buf, ks, 
&continue_tls_process))
Should rename argument of read_incoming_tls_ciphertext as well


http://gerrit.openvpn.net/c/openvpn/+/452/comment/2662067f_27eb71a1 :
PS1, Line 2947: if (!read_incoming_tls_plaintext(ks, buf, wakeup, 
&continue_tls_process))
Should rename argument of read_incoming_tls_plaintext as well



--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/452?usp=email
To unsubscribe, or for help writing mail filters, visit 
http://gerrit.openvpn.net/settings

Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: Ib6d713f2eb08a4c39d97de3e1a4a832cedc09585
Gerrit-Change-Number: 452
Gerrit-PatchSet: 1
Gerrit-Owner: plaisthos 
Gerrit-Reviewer: flichtenheld 
Gerrit-CC: openvpn-devel 
Gerrit-Attention: plaisthos 
Gerrit-Comment-Date: Mon, 20 Nov 2023 13:12:13 +
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

[Openvpn-devel] [S] Change in openvpn[master]: Rename state_change to continue_tls_process

[plaisthos (Code Review)]

Attention is currently required from: flichtenheld, plaisthos.

Hello flichtenheld,

I'd like you to reexamine a change. Please visit

http://gerrit.openvpn.net/c/openvpn/+/452?usp=email

to look at the new patch set (#2).

The following approvals got outdated and were removed:
Code-Review-1 by flichtenheld


Change subject: Rename state_change to continue_tls_process
..

Rename state_change to continue_tls_process

The name state_change is more confusing than helpful as it not really
indicates if there was a state change but rather if processing should
be continued. There even some states that are definitively state changes
(setting to_link buffer) that require continue_tls_process to be set
to false.

Change-Id: Ib6d713f2eb08a4c39d97de3e1a4a832cedc09585
---
M src/openvpn/ssl.c
1 file changed, 27 insertions(+), 22 deletions(-)


  git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/52/452/2

diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
index b4cd8f5..f8e0c51 100644
--- a/src/openvpn/ssl.c
+++ b/src/openvpn/ssl.c
@@ -2694,7 +2694,7 @@
  */
 static bool
 read_incoming_tls_ciphertext(struct buffer *buf, struct key_state *ks,
- bool *state_change)
+ bool *continue_tls_process)
 {
 int status = 0;
 if (buf->len)
@@ -2714,7 +2714,7 @@
 if (status == 1)
 {
 reliable_mark_deleted(ks->rec_reliable, buf);
-*state_change = true;
+*continue_tls_process = true;
 dmsg(D_TLS_DEBUG, "Incoming Ciphertext -> TLS");
 }
 return true;
@@ -2730,7 +2730,7 @@

 static bool
 read_incoming_tls_plaintext(struct key_state *ks, struct buffer *buf,
-interval_t *wakeup, bool *state_change)
+interval_t *wakeup, bool *continue_tls_process)
 {
 ASSERT(buf_init(buf, 0));

@@ -2744,7 +2744,7 @@
 }
 if (status == 1)
 {
-*state_change = true;
+*continue_tls_process = true;
 dmsg(D_TLS_DEBUG, "TLS -> Incoming Plaintext");

 /* More data may be available, wake up again asap to check. */
@@ -2754,7 +2754,7 @@
 }

 static bool
-write_outgoing_tls_ciphertext(struct tls_session *session, bool *state_change)
+write_outgoing_tls_ciphertext(struct tls_session *session, bool 
*continue_tls_process)
 {
 struct key_state *ks = &session->key[KS_PRIMARY];

@@ -2830,7 +2830,7 @@

 reliable_mark_active_outgoing(ks->send_reliable, buf, opcode);
 INCR_GENERATED;
-*state_change = true;
+*continue_tls_process = true;
 }
 dmsg(D_TLS_DEBUG, "Outgoing Ciphertext -> Reliable");
 }
@@ -2839,7 +2839,6 @@
 return true;
 }

-
 static bool
 tls_process_state(struct tls_multi *multi,
   struct tls_session *session,
@@ -2848,13 +2847,19 @@
   struct link_socket_info *to_link_socket_info,
   interval_t *wakeup)
 {
-bool state_change = false;
+/* This variable indicates if we should call this method
+ * again to process more incoming/outgoing TLS state/data
+ * We want to repeat this until we either determined that there
+ * is nothing more to process or that further processing
+ * should only be done after the outer loop (sending packets etc.)
+ * has run once more */
+bool continue_tls_process = false;
 struct key_state *ks = &session->key[KS_PRIMARY];  /* primary key */

 /* Initial handshake */
 if (ks->state == S_INITIAL)
 {
-state_change = session_move_pre_start(session, ks, false);
+continue_tls_process = session_move_pre_start(session, ks, false);
 }
 
 /* Are we timed out on receive? */
@@ -2872,7 +2877,7 @@
 if (ks->state == S_PRE_START && reliable_empty(ks->send_reliable))
 {
 ks->state = S_START;
-state_change = true;
+continue_tls_process = true;

 /* New connection, remove any old X509 env variables */
 tls_x509_clear_env(session->opt->es);
@@ -2885,7 +2890,7 @@
 && reliable_empty(ks->send_reliable))
 {
 session_move_active(multi, session, to_link_socket_info, ks);
-state_change = true;
+continue_tls_process = true;
 }

 /* Reliable buffer to outgoing TCP/UDP (send up to CONTROL_SEND_ACK_MAX 
ACKs
@@ -2927,7 +2932,7 @@
 }
 else
 {
-if (!read_incoming_tls_ciphertext(&entry->buf, ks, &state_change))
+if (!read_incoming_tls_ciphertext(&entry->buf, ks, 
&continue_tls_process))
 {
 goto error;
 }
@@ -2938,7 +2943,7 @@
 struct buffer *buf = &ks->plaintext_read_buf;
 if (!buf->len)
 {
-if (!read_incoming_tls_plaintext(ks, buf, wakeup, &state_change))
+if (!read_incoming_tls_plaintext(ks, buf, wakeup, 
&continue_tls_process))
 {
 goto error;
 }

[Openvpn-devel] [M] Change in openvpn[master]: Allow the TLS session to send out TLS alerts

[plaisthos (Code Review)]

Attention is currently required from: flichtenheld, plaisthos.

Hello flichtenheld,

I'd like you to reexamine a change. Please visit

http://gerrit.openvpn.net/c/openvpn/+/449?usp=email

to look at the new patch set (#3).

The following approvals got outdated and were removed:
Code-Review+1 by flichtenheld


Change subject: Allow the TLS session to send out TLS alerts
..

Allow the TLS session to send out TLS alerts

Previous OpenVPN versions shut down the TLS control channel immediately
when encountering an error. This also meant that we would not send out
TLS alerts to notify a client about potential problems like mismatching
TLS versions or having no common cipher.

This commit adds a new key_state S_ERROR_PRE which still allows to
send out the remaining TLS packets of the control session which are
typically the alert message and then going to S_ERROR. We do not
wait for retries. So this is more a one-shot notify but that is
acceptable in this situation.

Sending out alerts is a slight compromise in security as alerts give
out a bit of information that otherwise is not given
out. But since all other consumers TLS implementations are already doing this
and TLS implementations (nowadays) are very careful not to leak (sensitive)
information by alerts and since the user experience is much better with
alerts, this compromise is worth it.

Change-Id: I0ad48915004ddee587e97c8ed190ba8ee989e48d
---
M Changes.rst
M src/openvpn/ssl.c
M src/openvpn/ssl_backend.h
M src/openvpn/ssl_common.h
M src/openvpn/ssl_mbedtls.c
M src/openvpn/ssl_openssl.c
6 files changed, 88 insertions(+), 18 deletions(-)


  git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/49/449/3

diff --git a/Changes.rst b/Changes.rst
index 3676dce..dbef0d9 100644
--- a/Changes.rst
+++ b/Changes.rst
@@ -1,5 +1,16 @@
 Overview of changes in 2.7
 ==
+New features
+
+TLS alerts
+OpenVPN 2.7 will send out TLS alerts to peers informing them if the TLS
+session shuts down or when the TLS implementation informs the peer about
+an error in the TLS session (e.g. mismatching TLS versions). This improves
+the user experience as the client shows an error instead of running into
+a timeout when the server just stops responding completely.
+
+Deprecated features
+---
 ``secret`` support has been removed by default.
 static key mode (non-TLS) is no longer considered "good and secure enough"
 for today's requirements.  Use TLS mode instead.  If deploying a PKI CA
diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
index f8e0c51..dbfa24f 100644
--- a/src/openvpn/ssl.c
+++ b/src/openvpn/ssl.c
@@ -853,6 +853,9 @@
 case S_ERROR:
 return "S_ERROR";

+case S_ERROR_PRE:
+return "S_ERROR_PRE";
+
 case S_GENERATED_KEYS:
 return "S_GENERATED_KEYS";

@@ -2840,6 +2843,25 @@
 }

 static bool
+check_outgoing_ciphertext(struct key_state *ks, struct tls_session *session,
+  bool *continue_tls_process)
+{
+/* Outgoing Ciphertext to reliable buffer */
+if (ks->state >= S_PRE_START)
+{
+struct buffer *buf = 
reliable_get_buf_output_sequenced(ks->send_reliable);
+if (buf)
+{
+if (!write_outgoing_tls_ciphertext(session, continue_tls_process))
+{
+return false;
+}
+}
+}
+return true;
+}
+
+static bool
 tls_process_state(struct tls_multi *multi,
   struct tls_session *session,
   struct buffer *to_link,
@@ -2917,6 +2939,16 @@
 return false;
 }

+if (ks->state == S_ERROR_PRE)
+{
+/* When we end up here, we had one last chance to send an outstanding
+ * packet that contained an alert. We do not ensure that this packet
+ * has been successfully delivered  (ie wait for the ACK etc)
+ * but rather stop processing now */
+ks->state = S_ERROR;
+return false;
+}
+
 /* Write incoming ciphertext to TLS object */
 struct reliable_entry *entry = 
reliable_get_entry_sequenced(ks->rec_reliable);
 if (entry)
@@ -2997,29 +3029,31 @@
 dmsg(D_TLS_DEBUG, "Outgoing Plaintext -> TLS");
 }
 }
-
-/* Outgoing Ciphertext to reliable buffer */
-if (ks->state >= S_START)
+if (!check_outgoing_ciphertext(ks, session, &continue_tls_process))
 {
-buf = reliable_get_buf_output_sequenced(ks->send_reliable);
-if (buf)
-{
-if (!write_outgoing_tls_ciphertext(session, &state_change))
-{
-goto error;
-}
-}
+goto error;
 }

 return continue_tls_process;
 error:
 tls_clear_error();
-ks->state = S_ERROR;
+
+/* Shut down the TLS session but do a last read from the TLS
+ * object to be able to read potential TLS alerts */
+ 

[Openvpn-devel] [M] Change in openvpn[master]: Allow the TLS session to send out TLS alerts

[plaisthos (Code Review)]

Attention is currently required from: flichtenheld, plaisthos.

Hello flichtenheld,

I'd like you to reexamine a change. Please visit

http://gerrit.openvpn.net/c/openvpn/+/449?usp=email

to look at the new patch set (#4).


Change subject: Allow the TLS session to send out TLS alerts
..

Allow the TLS session to send out TLS alerts

Previous OpenVPN versions shut down the TLS control channel immediately
when encountering an error. This also meant that we would not send out
TLS alerts to notify a client about potential problems like mismatching
TLS versions or having no common cipher.

This commit adds a new key_state S_ERROR_PRE which still allows to
send out the remaining TLS packets of the control session which are
typically the alert message and then going to S_ERROR. We do not
wait for retries. So this is more a one-shot notify but that is
acceptable in this situation.

Sending out alerts is a slight compromise in security as alerts give
out a bit of information that otherwise is not given
out. But since all other consumers TLS implementations are already doing this
and TLS implementations (nowadays) are very careful not to leak (sensitive)
information by alerts and since the user experience is much better with
alerts, this compromise is worth it.

Change-Id: I0ad48915004ddee587e97c8ed190ba8ee989e48d
---
M Changes.rst
M src/openvpn/ssl.c
M src/openvpn/ssl_backend.h
M src/openvpn/ssl_common.h
M src/openvpn/ssl_mbedtls.c
M src/openvpn/ssl_openssl.c
6 files changed, 89 insertions(+), 19 deletions(-)


  git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/49/449/4

diff --git a/Changes.rst b/Changes.rst
index 3676dce..dbef0d9 100644
--- a/Changes.rst
+++ b/Changes.rst
@@ -1,5 +1,16 @@
 Overview of changes in 2.7
 ==
+New features
+
+TLS alerts
+OpenVPN 2.7 will send out TLS alerts to peers informing them if the TLS
+session shuts down or when the TLS implementation informs the peer about
+an error in the TLS session (e.g. mismatching TLS versions). This improves
+the user experience as the client shows an error instead of running into
+a timeout when the server just stops responding completely.
+
+Deprecated features
+---
 ``secret`` support has been removed by default.
 static key mode (non-TLS) is no longer considered "good and secure enough"
 for today's requirements.  Use TLS mode instead.  If deploying a PKI CA
diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
index f8e0c51..fd3de87 100644
--- a/src/openvpn/ssl.c
+++ b/src/openvpn/ssl.c
@@ -853,6 +853,9 @@
 case S_ERROR:
 return "S_ERROR";

+case S_ERROR_PRE:
+return "S_ERROR_PRE";
+
 case S_GENERATED_KEYS:
 return "S_GENERATED_KEYS";

@@ -2840,6 +2843,25 @@
 }

 static bool
+check_outgoing_ciphertext(struct key_state *ks, struct tls_session *session,
+  bool *continue_tls_process)
+{
+/* Outgoing Ciphertext to reliable buffer */
+if (ks->state >= S_PRE_START)
+{
+struct buffer *buf = 
reliable_get_buf_output_sequenced(ks->send_reliable);
+if (buf)
+{
+if (!write_outgoing_tls_ciphertext(session, continue_tls_process))
+{
+return false;
+}
+}
+}
+return true;
+}
+
+static bool
 tls_process_state(struct tls_multi *multi,
   struct tls_session *session,
   struct buffer *to_link,
@@ -2863,7 +2885,7 @@
 }

 /* Are we timed out on receive? */
-if (now >= ks->must_negotiate && ks->state < S_ACTIVE)
+if (now >= ks->must_negotiate && ks->state >= S_UNDEF && ks->state < 
S_ACTIVE)
 {
 msg(D_TLS_ERRORS,
 "TLS Error: TLS key negotiation failed to occur within %d seconds 
(check your network connectivity)",
@@ -2917,6 +2939,16 @@
 return false;
 }

+if (ks->state == S_ERROR_PRE)
+{
+/* When we end up here, we had one last chance to send an outstanding
+ * packet that contained an alert. We do not ensure that this packet
+ * has been successfully delivered  (ie wait for the ACK etc)
+ * but rather stop processing now */
+ks->state = S_ERROR;
+return false;
+}
+
 /* Write incoming ciphertext to TLS object */
 struct reliable_entry *entry = 
reliable_get_entry_sequenced(ks->rec_reliable);
 if (entry)
@@ -2997,29 +3029,31 @@
 dmsg(D_TLS_DEBUG, "Outgoing Plaintext -> TLS");
 }
 }
-
-/* Outgoing Ciphertext to reliable buffer */
-if (ks->state >= S_START)
+if (!check_outgoing_ciphertext(ks, session, &continue_tls_process))
 {
-buf = reliable_get_buf_output_sequenced(ks->send_reliable);
-if (buf)
-{
-if (!write_outgoing_tls_ciphertext(session, &state_change))
-{
-goto err

[Openvpn-devel] [PATCH applied] Re: protocol_dump: tls-crypt support

[Gert Doering]

Your patch has been applied to the master and release/2.6 branch 
("printing garbage" is arguably a bug that needs fixing).

I have taken parts of the "mail mail" into the commit message, and
added the "signed-off-by:" that the OpenVPN project wants.

I haven't actually tested it beyond "does it compile" - it looks good,
and Arne is the current authority on tls-crypt anyway.

commit 227799b8345128dd3adf2029323457804209fe93 (master)
commit 0a39d1c1e2582330db09052bcf0e32bbf5bafde2 (release/2.6)
Author: Reynir Björnsson
Date:   Thu Oct 26 16:55:32 2023 +0200

 protocol_dump: tls-crypt support

 Signed-off-by: Reynir Björnsson 
 Acked-by: Arne Schwabe 
 Message-Id: <8237adde-2523-9e48-5cd4-070463887...@gmail.com>
 URL: 
https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg27310.html
 Signed-off-by: Gert Doering 


--
kind regards,

Gert Doering


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

[Openvpn-devel] [S] Change in openvpn[master]: Rename state_change to continue_tls_process

[flichtenheld (Code Review)]

Attention is currently required from: plaisthos.

flichtenheld has posted comments on this change. ( 
http://gerrit.openvpn.net/c/openvpn/+/452?usp=email )

Change subject: Rename state_change to continue_tls_process
..


Patch Set 2: Code-Review-2

(1 comment)

File src/openvpn/ssl.c:

http://gerrit.openvpn.net/c/openvpn/+/452/comment/17cb7023_b42d55a5 :
PS2, Line 3007: if (!write_outgoing_tls_ciphertext(session, 
&state_change))
Still says state_change, breaking the build



--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/452?usp=email
To unsubscribe, or for help writing mail filters, visit 
http://gerrit.openvpn.net/settings

Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: Ib6d713f2eb08a4c39d97de3e1a4a832cedc09585
Gerrit-Change-Number: 452
Gerrit-PatchSet: 2
Gerrit-Owner: plaisthos 
Gerrit-Reviewer: flichtenheld 
Gerrit-CC: openvpn-devel 
Gerrit-Attention: plaisthos 
Gerrit-Comment-Date: Mon, 20 Nov 2023 14:54:58 +
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

[Openvpn-devel] [S] Change in openvpn[master]: protocol_dump: tls-crypt support

[cron2 (Code Review)]

cron2 has abandoned this change. ( 
http://gerrit.openvpn.net/c/openvpn/+/442?usp=email )

Change subject: protocol_dump: tls-crypt support
..


Abandoned

merged upstream, without change id
--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/442?usp=email
To unsubscribe, or for help writing mail filters, visit 
http://gerrit.openvpn.net/settings

Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: Ie25aa287f3534090c1d93fc3bb69727dd20fc6fe
Gerrit-Change-Number: 442
Gerrit-PatchSet: 1
Gerrit-Owner: plaisthos 
Gerrit-Reviewer: flichtenheld 
Gerrit-Reviewer: plaisthos 
Gerrit-CC: openvpn-devel 
Gerrit-MessageType: abandon
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

[Openvpn-devel] [L] Change in openvpn[release/2.6]: Backport mbed TLS 3 support to OpenVPN 2.6

[MaxF (Code Review)]

Attention is currently required from: flichtenheld, plaisthos.

Hello plaisthos, flichtenheld,

I'd like you to do a code review.
Please visit

http://gerrit.openvpn.net/c/openvpn/+/404?usp=email

to review the following change.


Change subject: Backport mbed TLS 3 support to OpenVPN 2.6
..

Backport mbed TLS 3 support to OpenVPN 2.6

Based on commits
- ace7a4f1c271550bb8ad276663e045ab97a46f16
- f53f06316dbb804128fc5cbee1d8edb274ce81df
- efad93d049c318a3bd9ea5956c6ac8237b8d6d70
- b5faf1b2e90fd44c5137a2b8f3da98c7ae482fc1

Change-Id: Icb4ae73741dc84ef0ff7ef72721cc12b999f4d03
Signed-off-by: Max Fillinger 
---
M README.mbedtls
M config.h.cmake.in
M configure.ac
M src/openvpn/Makefile.am
M src/openvpn/crypto_mbedtls.c
A src/openvpn/mbedtls_compat.h
M src/openvpn/options.c
M src/openvpn/ssl_mbedtls.c
M src/openvpn/ssl_verify_mbedtls.c
9 files changed, 422 insertions(+), 115 deletions(-)



  git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/04/404/1

diff --git a/README.mbedtls b/README.mbedtls
index d3466fa..ed9d369 100644
--- a/README.mbedtls
+++ b/README.mbedtls
@@ -1,13 +1,13 @@
-This version of OpenVPN has mbed TLS support. To enable follow the following
-instructions:
+This version of OpenVPN has mbed TLS support. To enable, follow the
+instructions below:

-To Build and Install,
+To build and install,

./configure --with-crypto-library=mbedtls
make
make install

-This version depends on mbed TLS 2.0 (and requires at least 2.0.0).
+This version requires mbed TLS version >= 2.0.0 or >= 3.2.1.

 *

@@ -16,7 +16,8 @@
 As of mbed TLS 2.17, it can be licensed *only* under the Apache v2.0 license.
 That license is incompatible with OpenVPN's GPLv2.

-If you wish to distribute OpenVPN linked with mbed TLS, there are two options:
+We are currently in the process of resolving this problem, but for now, if you
+wish to distribute OpenVPN linked with mbed TLS, there are two options:

  * Ensure that your case falls under the system library exception in GPLv2, or

@@ -24,9 +25,6 @@
that may be licensed under GPLv2. Unfortunately, this version is
unsupported and won't receive any more updates.

-If nothing changes about the license situation, mbed TLS support may be
-deprecated in a future release of OpenVPN.
-
 *

 Due to limitations in the mbed TLS library, the following features are missing
@@ -42,3 +40,8 @@
  * X.509 subject line has a different format than the OpenSSL subject line
  * X.509 certificate export does not work
  * X.509 certificate tracking
+
+*
+
+Mbed TLS 3 has implemented (parts of) the TLS 1.3 protocol, but we have 
disabled
+support in OpenVPN because the TLS-Exporter function is not yet implemented.
diff --git a/config.h.cmake.in b/config.h.cmake.in
index 29006ce..e88cb77 100644
--- a/config.h.cmake.in
+++ b/config.h.cmake.in
@@ -387,7 +387,10 @@
 #undef HAVE_VSNPRINTF

 /* we always assume a recent mbed TLS version */
-#define HAVE_CTR_DRBG_UPDATE_RET 1
+#define HAVE_MBEDTLS_PSA_CRYPTO_H 1
+#define HAVE_MBEDTLS_SSL_TLS_PRF 1
+#define HAVE_MBEDTLS_SSL_SET_EXPORT_KEYS_CB 1
+#define HAVE_MBEDTLS_CTR_DRBG_UPDATE_RET 1

 /* Path to ifconfig tool */
 #define IFCONFIG_PATH "@IFCONFIG_PATH@"
diff --git a/configure.ac b/configure.ac
index 052fe3f..749c857 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1019,13 +1019,19 @@
 #include 
]],
[[
-#if MBEDTLS_VERSION_NUMBER < 0x0200 || MBEDTLS_VERSION_NUMBER >= 0x0300
+#if MBEDTLS_VERSION_NUMBER < 0x0200 || (MBEDTLS_VERSION_NUMBER >= 
0x0300 && MBEDTLS_VERSION_NUMBER < 0x03020100)
 #error invalid version
 #endif
]]
)],
[AC_MSG_RESULT([ok])],
-   [AC_MSG_ERROR([mbed TLS 2.y.z required])]
+   [AC_MSG_ERROR([mbed TLS version >= 2.0.0 or >= 3.2.1 required])]
+   )
+
+   AC_CHECK_HEADER(
+   psa/crypto.h,
+   [AC_DEFINE([HAVE_MBEDTLS_PSA_CRYPTO_H], [1], [yes])],
+   [AC_DEFINE([HAVE_MBEDTLS_PSA_CRYPTO_H], [0], [no])]
)

AC_CHECK_FUNCS(
@@ -1037,16 +1043,32 @@
[AC_MSG_ERROR([mbed TLS check for AEAD support failed])]
)

+   AC_CHECK_FUNC(
+   [mbedtls_ssl_tls_prf],
+   [AC_DEFINE([HAVE_MBEDTLS_SSL_TLS_PRF], [1], [yes])],
+   [AC_DEFINE([HAVE_MBEDTLS_SSL_TLS_PRF], [0], [no])]
+   )
+
have_export_keying_material="yes"
AC_CHECK_FUNC(
[mbedtls_ssl_conf_export_keys_ext_cb],
-   ,
-   [have_export_keying_material="no"]
+   [AC_DEFINE([HAVE_MBEDTLS_SSL_CONF_EXPORT_KEYS_EXT_CB], [1], 
[yes])],
+   [AC_D

[Openvpn-devel] [M] Change in openvpn[master]: Rename state_change to continue_tls_process

[plaisthos (Code Review)]

Attention is currently required from: flichtenheld.

plaisthos has posted comments on this change. ( 
http://gerrit.openvpn.net/c/openvpn/+/452?usp=email )

Change subject: Rename state_change to continue_tls_process
..


Patch Set 3:

(3 comments)

File src/openvpn/ssl.c:

http://gerrit.openvpn.net/c/openvpn/+/452/comment/4493eb6f_5624d2ca :
PS1, Line 2936: if (!read_incoming_tls_ciphertext(&entry->buf, ks, 
&continue_tls_process))
> Should rename argument of read_incoming_tls_ciphertext as well
Done


http://gerrit.openvpn.net/c/openvpn/+/452/comment/1e69d0fc_7323711d :
PS1, Line 2947: if (!read_incoming_tls_plaintext(ks, buf, wakeup, 
&continue_tls_process))
> Should rename argument of read_incoming_tls_plaintext as well
Done


File src/openvpn/ssl.c:

http://gerrit.openvpn.net/c/openvpn/+/452/comment/c4c12c6f_b4fe17bb :
PS2, Line 3007: if (!write_outgoing_tls_ciphertext(session, 
&state_change))
> Still says state_change, breaking the build
That is what you get for only testing the whole patch set and not each commit.



--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/452?usp=email
To unsubscribe, or for help writing mail filters, visit 
http://gerrit.openvpn.net/settings

Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: Ib6d713f2eb08a4c39d97de3e1a4a832cedc09585
Gerrit-Change-Number: 452
Gerrit-PatchSet: 3
Gerrit-Owner: plaisthos 
Gerrit-Reviewer: flichtenheld 
Gerrit-CC: openvpn-devel 
Gerrit-Attention: flichtenheld 
Gerrit-Comment-Date: Mon, 20 Nov 2023 17:15:44 +
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: flichtenheld 
Gerrit-MessageType: comment
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

[Openvpn-devel] [XS] Change in openvpn[master]: Log SSL alerts more prominently

[plaisthos (Code Review)]

Attention is currently required from: plaisthos.

Hello flichtenheld,

I'd like you to reexamine a change. Please visit

http://gerrit.openvpn.net/c/openvpn/+/448?usp=email

to look at the new patch set (#3).

The change is no longer submittable: checks~ChecksSubmitRule is unsatisfied now.


Change subject: Log SSL alerts more prominently
..

Log SSL alerts more prominently

When we receive an SSL alert from a server we currently only log a
very cryptic OpenSSL error message:

   OpenSSL: error:0A00042E:SSL routines::tlsv1 alert protocol version:SSL alert 
number 70

This also enables logging the much more readable SSL error message:

   Received fatal SSL alert: protocol version

which previously needed --verb 8 to be displayed (now verb 3). Also rework the
message to be better readable.

Change-Id: I6bdab3028c9bd679c31d4177a746a3ea505dcbbf
Signed-off-by: Arne Schwabe 
---
M src/openvpn/ssl_openssl.c
1 file changed, 2 insertions(+), 2 deletions(-)


  git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/48/448/3

diff --git a/src/openvpn/ssl_openssl.c b/src/openvpn/ssl_openssl.c
index 23e7623..82872bf 100644
--- a/src/openvpn/ssl_openssl.c
+++ b/src/openvpn/ssl_openssl.c
@@ -196,8 +196,8 @@
 }
 else if (where & SSL_CB_ALERT)
 {
-dmsg(D_HANDSHAKE_VERBOSE, "SSL alert (%s): %s: %s",
- where & SSL_CB_READ ? "read" : "write",
+dmsg(D_TLS_DEBUG_LOW, "%s %s SSL alert: %s",
+ where & SSL_CB_READ ? "Received" : "Sent",
  SSL_alert_type_string_long(ret),
  SSL_alert_desc_string_long(ret));
 }

--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/448?usp=email
To unsubscribe, or for help writing mail filters, visit 
http://gerrit.openvpn.net/settings

Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I6bdab3028c9bd679c31d4177a746a3ea505dcbbf
Gerrit-Change-Number: 448
Gerrit-PatchSet: 3
Gerrit-Owner: plaisthos 
Gerrit-Reviewer: flichtenheld 
Gerrit-CC: openvpn-devel 
Gerrit-Attention: plaisthos 
Gerrit-MessageType: newpatchset
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

[Openvpn-devel] [M] Change in openvpn[master]: Rename state_change to continue_tls_process

[plaisthos (Code Review)]

Attention is currently required from: plaisthos.

Hello flichtenheld,

I'd like you to reexamine a change. Please visit

http://gerrit.openvpn.net/c/openvpn/+/452?usp=email

to look at the new patch set (#3).


Change subject: Rename state_change to continue_tls_process
..

Rename state_change to continue_tls_process

The name state_change is more confusing than helpful as it not really
indicates if there was a state change but rather if processing should
be continued. There even some states that are definitively state changes
(setting to_link buffer) that require continue_tls_process to be set
to false.

Change-Id: Ib6d713f2eb08a4c39d97de3e1a4a832cedc09585
---
M src/openvpn/ssl.c
1 file changed, 28 insertions(+), 23 deletions(-)


  git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/52/452/3

diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
index 400230c..20f4956 100644
--- a/src/openvpn/ssl.c
+++ b/src/openvpn/ssl.c
@@ -2694,7 +2694,7 @@
  */
 static bool
 read_incoming_tls_ciphertext(struct buffer *buf, struct key_state *ks,
- bool *state_change)
+ bool *continue_tls_process)
 {
 int status = 0;
 if (buf->len)
@@ -2714,7 +2714,7 @@
 if (status == 1)
 {
 reliable_mark_deleted(ks->rec_reliable, buf);
-*state_change = true;
+*continue_tls_process = true;
 dmsg(D_TLS_DEBUG, "Incoming Ciphertext -> TLS");
 }
 return true;
@@ -2730,7 +2730,7 @@

 static bool
 read_incoming_tls_plaintext(struct key_state *ks, struct buffer *buf,
-interval_t *wakeup, bool *state_change)
+interval_t *wakeup, bool *continue_tls_process)
 {
 ASSERT(buf_init(buf, 0));

@@ -2744,7 +2744,7 @@
 }
 if (status == 1)
 {
-*state_change = true;
+*continue_tls_process = true;
 dmsg(D_TLS_DEBUG, "TLS -> Incoming Plaintext");

 /* More data may be available, wake up again asap to check. */
@@ -2754,7 +2754,7 @@
 }

 static bool
-write_outgoing_tls_ciphertext(struct tls_session *session, bool *state_change)
+write_outgoing_tls_ciphertext(struct tls_session *session, bool 
*continue_tls_process)
 {
 struct key_state *ks = &session->key[KS_PRIMARY];

@@ -2830,7 +2830,7 @@

 reliable_mark_active_outgoing(ks->send_reliable, buf, opcode);
 INCR_GENERATED;
-*state_change = true;
+*continue_tls_process = true;
 }
 dmsg(D_TLS_DEBUG, "Outgoing Ciphertext -> Reliable");
 }
@@ -2839,7 +2839,6 @@
 return true;
 }

-
 static bool
 tls_process_state(struct tls_multi *multi,
   struct tls_session *session,
@@ -2848,13 +2847,19 @@
   struct link_socket_info *to_link_socket_info,
   interval_t *wakeup)
 {
-bool state_change = false;
+/* This variable indicates if we should call this method
+ * again to process more incoming/outgoing TLS state/data
+ * We want to repeat this until we either determined that there
+ * is nothing more to process or that further processing
+ * should only be done after the outer loop (sending packets etc.)
+ * has run once more */
+bool continue_tls_process = false;
 struct key_state *ks = &session->key[KS_PRIMARY];  /* primary key */

 /* Initial handshake */
 if (ks->state == S_INITIAL)
 {
-state_change = session_move_pre_start(session, ks, false);
+continue_tls_process = session_move_pre_start(session, ks, false);
 }

 /* Are we timed out on receive? */
@@ -2872,7 +2877,7 @@
 if (ks->state == S_PRE_START && reliable_empty(ks->send_reliable))
 {
 ks->state = S_START;
-state_change = true;
+continue_tls_process = true;

 /* New connection, remove any old X509 env variables */
 tls_x509_clear_env(session->opt->es);
@@ -2885,7 +2890,7 @@
 && reliable_empty(ks->send_reliable))
 {
 session_move_active(multi, session, to_link_socket_info, ks);
-state_change = true;
+continue_tls_process = true;
 }

 /* Reliable buffer to outgoing TCP/UDP (send up to CONTROL_SEND_ACK_MAX 
ACKs
@@ -2927,7 +2932,7 @@
 }
 else
 {
-if (!read_incoming_tls_ciphertext(&entry->buf, ks, &state_change))
+if (!read_incoming_tls_ciphertext(&entry->buf, ks, 
&continue_tls_process))
 {
 goto error;
 }
@@ -2938,7 +2943,7 @@
 struct buffer *buf = &ks->plaintext_read_buf;
 if (!buf->len)
 {
-if (!read_incoming_tls_plaintext(ks, buf, wakeup, &state_change))
+if (!read_incoming_tls_plaintext(ks, buf, wakeup, 
&continue_tls_process))
 {
 goto error;
 }
@@ -2954,7 +2959,7 @@
 goto error;
 }

-state_change = true;
+co

[Openvpn-devel] [M] Change in openvpn[master]: Allow the TLS session to send out TLS alerts

[plaisthos (Code Review)]

Attention is currently required from: flichtenheld, plaisthos.

Hello flichtenheld,

I'd like you to reexamine a change. Please visit

http://gerrit.openvpn.net/c/openvpn/+/449?usp=email

to look at the new patch set (#5).


Change subject: Allow the TLS session to send out TLS alerts
..

Allow the TLS session to send out TLS alerts

Previous OpenVPN versions shut down the TLS control channel immediately
when encountering an error. This also meant that we would not send out
TLS alerts to notify a client about potential problems like mismatching
TLS versions or having no common cipher.

This commit adds a new key_state S_ERROR_PRE which still allows to
send out the remaining TLS packets of the control session which are
typically the alert message and then going to S_ERROR. We do not
wait for retries. So this is more a one-shot notify but that is
acceptable in this situation.

Sending out alerts is a slight compromise in security as alerts give
out a bit of information that otherwise is not given
out. But since all other consumers TLS implementations are already doing this
and TLS implementations (nowadays) are very careful not to leak (sensitive)
information by alerts and since the user experience is much better with
alerts, this compromise is worth it.

Change-Id: I0ad48915004ddee587e97c8ed190ba8ee989e48d
---
M Changes.rst
M src/openvpn/ssl.c
M src/openvpn/ssl_backend.h
M src/openvpn/ssl_common.h
M src/openvpn/ssl_mbedtls.c
M src/openvpn/ssl_openssl.c
6 files changed, 89 insertions(+), 19 deletions(-)


  git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/49/449/5

diff --git a/Changes.rst b/Changes.rst
index 3676dce..dbef0d9 100644
--- a/Changes.rst
+++ b/Changes.rst
@@ -1,5 +1,16 @@
 Overview of changes in 2.7
 ==
+New features
+
+TLS alerts
+OpenVPN 2.7 will send out TLS alerts to peers informing them if the TLS
+session shuts down or when the TLS implementation informs the peer about
+an error in the TLS session (e.g. mismatching TLS versions). This improves
+the user experience as the client shows an error instead of running into
+a timeout when the server just stops responding completely.
+
+Deprecated features
+---
 ``secret`` support has been removed by default.
 static key mode (non-TLS) is no longer considered "good and secure enough"
 for today's requirements.  Use TLS mode instead.  If deploying a PKI CA
diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
index 20f4956..f794a37 100644
--- a/src/openvpn/ssl.c
+++ b/src/openvpn/ssl.c
@@ -853,6 +853,9 @@
 case S_ERROR:
 return "S_ERROR";

+case S_ERROR_PRE:
+return "S_ERROR_PRE";
+
 case S_GENERATED_KEYS:
 return "S_GENERATED_KEYS";

@@ -2840,6 +2843,25 @@
 }

 static bool
+check_outgoing_ciphertext(struct key_state *ks, struct tls_session *session,
+  bool *continue_tls_process)
+{
+/* Outgoing Ciphertext to reliable buffer */
+if (ks->state >= S_PRE_START)
+{
+struct buffer *buf = 
reliable_get_buf_output_sequenced(ks->send_reliable);
+if (buf)
+{
+if (!write_outgoing_tls_ciphertext(session, continue_tls_process))
+{
+return false;
+}
+}
+}
+return true;
+}
+
+static bool
 tls_process_state(struct tls_multi *multi,
   struct tls_session *session,
   struct buffer *to_link,
@@ -2863,7 +2885,7 @@
 }

 /* Are we timed out on receive? */
-if (now >= ks->must_negotiate && ks->state < S_ACTIVE)
+if (now >= ks->must_negotiate && ks->state >= S_UNDEF && ks->state < 
S_ACTIVE)
 {
 msg(D_TLS_ERRORS,
 "TLS Error: TLS key negotiation failed to occur within %d seconds 
(check your network connectivity)",
@@ -2917,6 +2939,16 @@
 return false;
 }

+if (ks->state == S_ERROR_PRE)
+{
+/* When we end up here, we had one last chance to send an outstanding
+ * packet that contained an alert. We do not ensure that this packet
+ * has been successfully delivered  (ie wait for the ACK etc)
+ * but rather stop processing now */
+ks->state = S_ERROR;
+return false;
+}
+
 /* Write incoming ciphertext to TLS object */
 struct reliable_entry *entry = 
reliable_get_entry_sequenced(ks->rec_reliable);
 if (entry)
@@ -2997,29 +3029,31 @@
 dmsg(D_TLS_DEBUG, "Outgoing Plaintext -> TLS");
 }
 }
-
-/* Outgoing Ciphertext to reliable buffer */
-if (ks->state >= S_START)
+if (!check_outgoing_ciphertext(ks, session, &continue_tls_process))
 {
-buf = reliable_get_buf_output_sequenced(ks->send_reliable);
-if (buf)
-{
-if (!write_outgoing_tls_ciphertext(session, &continue_tls_process))
-{
-