openvpn-devel Mailing List for OpenVPN

On 30/11/18 17:27, Kristian McColm wrote: > 1500 is not safe on many mobile/cell networks as most have a smaller MTU like 1460 due to GTP headers used between the PGW and eNodeB. There is also reduced MTU when performing NAT64, i.e. packets translated from IPv4 to IPv6 become 20 bytes larger after translation due to additional IP header bytes. > > I believe it is most efficient to implement an application layer fragmentation feature, similar to IKEv2 fragmentation (RFC7383) so that the underlying network layer does not have to perform IP layer fragmentation which has known risks. that is pretty much what --fragment does... > Additionally, TCP MSS rewriting only works for TCP. Many VPNs use UDP which is not helped by MSS fix obviously. If supported by the client/server OS, path MTU discovery can be used to determine the MTU of a given connection. TCP MSS using --mssfix works inside the tunnel only, and only for TCP connections. It does not depend on the outside protocol (UDP or TCP). I fully agree that having PMTUD would be nice to have, but even that has its drawbacks... JM2CW, JJK > -----Original Message----- > From: Jan Just Keijser [mailto:ja...@ni...] > Sent: November 30, 2018 11:16 > To: Simon Matter <sim...@in...>; Lev Stipakov <lst...@gm...> > Cc: ope...@li... > Subject: Re: [Openvpn-devel] Summary of the community meeting (Wed, 28th Nov 2018) > > Hi Lev, Simon, > > On 30/11/18 07:10, Simon Matter wrote: >>> Hi Jan Just, >>> >>> (forgot to add openvpn-devel in previous mail) >>> >>> Some background information. >>> >>> In openvpn3 we decided not to implement fragments, because: >>> >>> - this is quite a big feature which has to be supported through the >>> whole stack (client, server, kernel module) >>> - we assume that it is not used by most of users >>> >>> However, for those who needs --fragment, we'll implement: >>> >>> - mssfix support, this should solve problems with tcp-based protocols >>> - sending icmp "packet too big" for other protocols, we assume that >>> they'll respect icmp response >>> >>> --fragment is/was very useful on a system where you do not / cannot >>> change >>>> the tun MTU size. Up to Windows XP, this was not possible without >>>> rebooting the OS. Since Vista, it *is* possible to change the MTU of >>>> an adapter on the fly (as explained in my trusty old cookbook, of >>>> course ;)) >>>> >>> As James wrote a while ago (13 years ago :) >>> >>> https://openvpn.net/archive/openvpn-users/2005-10/msg00354.html >>> >>>> A lot of experience gained during the OpenVPN 1.x releases showed >>>> that >>> it's best to fix the tunnel MTU at >>>> 1500 and vary the other parameters (and use --mssfix to prevent >>> fragmentation rather than a lower tunnel MTU). >>> >>> Don't know if still holds. Assuming that we can change tun-mtu on any >>> supported platforms, do you think that it is better to do _that_, >>> rather than have mssfix/icmp ptb workaround? >>> >>> >>>> With that, it would be possible to fix the link-mtu to 1500 (or >>>> whatever is set on the outbound interface) and then subtract the >>>> header size to get a meaningful tun-mtu size. >>>> >>> Do you think 1500 is a safe value? Arne just mentioned today that his >>> network interface MTU is 1500 and router's MTU is 1492 due to PPPoE, >>> and openvpn2 works because it assumes that mtu is 1450. >> I also think 1500 isn't safe because different types ob internet >> connections may reduce it, like the mentioned PPPoE. >> >> > this is why the --fragment feature is so handy - no need to change the mtu values on either end, yet you can fiddle with the fragmentation size to tune performance. > > I agree that in most home environments the MTU size should be lower - if you look at e.g. IPsec you will find that it sets the tunnel MTU size to > 1360 to ensure that it will "almost always" work. > Having said that, I'd like see a solution that will work in 99% of the cases *AND* that will give most users close-to-optimal performance.  If "fragment" is not implemented in OpenVPN 3 then what happens if the tun MTU is too high?  where does fragmentation occur? at the OS level? at the UDP level?  I'd prefer to leave any fragmenting to the underlying OS as much as possible - hence if I want to send packets of 4000 bytes than either they are fragmented at the tun level (i.e. before they are processed by OpenVPN) or at the transport level (i.e. OpenVPN spits out a 4000 byte packet over the UDP/TCP connection, which the OS then fragements).  The problem with fragmenting at the UDP level is that you need to keep track of the fragments in order to reassemble them - TCP does this for you. > > cheers, > > JJK > > > > _______________________________________________ > Openvpn-devel mailing list > Ope...@li... > https://lists.sourceforge.net/lists/listinfo/openvpn-devel 

1500 is not safe on many mobile/cell networks as most have a smaller MTU like 1460 due to GTP headers used between the PGW and eNodeB. There is also reduced MTU when performing NAT64, i.e. packets translated from IPv4 to IPv6 become 20 bytes larger after translation due to additional IP header bytes. I believe it is most efficient to implement an application layer fragmentation feature, similar to IKEv2 fragmentation (RFC7383) so that the underlying network layer does not have to perform IP layer fragmentation which has known risks. Additionally, TCP MSS rewriting only works for TCP. Many VPNs use UDP which is not helped by MSS fix obviously. If supported by the client/server OS, path MTU discovery can be used to determine the MTU of a given connection. -----Original Message----- From: Jan Just Keijser [mailto:ja...@ni...] Sent: November 30, 2018 11:16 To: Simon Matter <sim...@in...>; Lev Stipakov <lst...@gm...> Cc: ope...@li... Subject: Re: [Openvpn-devel] Summary of the community meeting (Wed, 28th Nov 2018) Hi Lev, Simon, On 30/11/18 07:10, Simon Matter wrote: >> Hi Jan Just, >> >> (forgot to add openvpn-devel in previous mail) >> >> Some background information. >> >> In openvpn3 we decided not to implement fragments, because: >> >> - this is quite a big feature which has to be supported through the >> whole stack (client, server, kernel module) >> - we assume that it is not used by most of users >> >> However, for those who needs --fragment, we'll implement: >> >> - mssfix support, this should solve problems with tcp-based protocols >> - sending icmp "packet too big" for other protocols, we assume that >> they'll respect icmp response >> >> --fragment is/was very useful on a system where you do not / cannot >> change >>> the tun MTU size. Up to Windows XP, this was not possible without >>> rebooting the OS. Since Vista, it *is* possible to change the MTU of >>> an adapter on the fly (as explained in my trusty old cookbook, of >>> course ;)) >>> >> As James wrote a while ago (13 years ago :) >> >> https://openvpn.net/archive/openvpn-users/2005-10/msg00354.html >> >>> A lot of experience gained during the OpenVPN 1.x releases showed >>> that >> it's best to fix the tunnel MTU at >>> 1500 and vary the other parameters (and use --mssfix to prevent >> fragmentation rather than a lower tunnel MTU). >> >> Don't know if still holds. Assuming that we can change tun-mtu on any >> supported platforms, do you think that it is better to do _that_, >> rather than have mssfix/icmp ptb workaround? >> >> >>> With that, it would be possible to fix the link-mtu to 1500 (or >>> whatever is set on the outbound interface) and then subtract the >>> header size to get a meaningful tun-mtu size. >>> >> Do you think 1500 is a safe value? Arne just mentioned today that his >> network interface MTU is 1500 and router's MTU is 1492 due to PPPoE, >> and openvpn2 works because it assumes that mtu is 1450. > I also think 1500 isn't safe because different types ob internet > connections may reduce it, like the mentioned PPPoE. > > this is why the --fragment feature is so handy - no need to change the mtu values on either end, yet you can fiddle with the fragmentation size to tune performance. I agree that in most home environments the MTU size should be lower - if you look at e.g. IPsec you will find that it sets the tunnel MTU size to 1360 to ensure that it will "almost always" work. Having said that, I'd like see a solution that will work in 99% of the cases *AND* that will give most users close-to-optimal performance.  If "fragment" is not implemented in OpenVPN 3 then what happens if the tun MTU is too high?  where does fragmentation occur? at the OS level? at the UDP level?  I'd prefer to leave any fragmenting to the underlying OS as much as possible - hence if I want to send packets of 4000 bytes than either they are fragmented at the tun level (i.e. before they are processed by OpenVPN) or at the transport level (i.e. OpenVPN spits out a 4000 byte packet over the UDP/TCP connection, which the OS then fragements).  The problem with fragmenting at the UDP level is that you need to keep track of the fragments in order to reassemble them - TCP does this for you. cheers, JJK _______________________________________________ Openvpn-devel mailing list Ope...@li... https://lists.sourceforge.net/lists/listinfo/openvpn-devel 

Hi Lev, Simon, On 30/11/18 07:10, Simon Matter wrote: >> Hi Jan Just, >> >> (forgot to add openvpn-devel in previous mail) >> >> Some background information. >> >> In openvpn3 we decided not to implement fragments, because: >> >> - this is quite a big feature which has to be supported through the whole >> stack (client, server, kernel module) >> - we assume that it is not used by most of users >> >> However, for those who needs --fragment, we'll implement: >> >> - mssfix support, this should solve problems with tcp-based protocols >> - sending icmp "packet too big" for other protocols, we assume that >> they'll respect icmp response >> >> --fragment is/was very useful on a system where you do not / cannot change >>> the tun MTU size. Up to Windows XP, this was not >>> possible without rebooting the OS. Since Vista, it *is* possible to >>> change >>> the MTU of an adapter on the fly (as explained in my >>> trusty old cookbook, of course ;)) >>> >> As James wrote a while ago (13 years ago :) >> >> https://openvpn.net/archive/openvpn-users/2005-10/msg00354.html >> >>> A lot of experience gained during the OpenVPN 1.x releases showed that >> it's best to fix the tunnel MTU at >>> 1500 and vary the other parameters (and use --mssfix to prevent >> fragmentation rather than a lower tunnel MTU). >> >> Don't know if still holds. Assuming that we can change tun-mtu on any >> supported platforms, do you think that it is better >> to do _that_, rather than have mssfix/icmp ptb workaround? >> >> >>> With that, it would be possible to fix the link-mtu to 1500 (or >>> whatever is set on the outbound interface) and then subtract the header >>> size to get a meaningful tun-mtu size. >>> >> Do you think 1500 is a safe value? Arne just mentioned today that his >> network interface MTU is 1500 and router's MTU is 1492 due to >> PPPoE, and openvpn2 works because it assumes that mtu is 1450. > I also think 1500 isn't safe because different types ob internet > connections may reduce it, like the mentioned PPPoE. > > this is why the --fragment feature is so handy - no need to change the mtu values on either end, yet you can fiddle with the fragmentation size to tune performance. I agree that in most home environments the MTU size should be lower - if you look at e.g. IPsec you will find that it sets the tunnel MTU size to 1360 to ensure that it will "almost always" work. Having said that, I'd like see a solution that will work in 99% of the cases *AND* that will give most users close-to-optimal performance.  If "fragment" is not implemented in OpenVPN 3 then what happens if the tun MTU is too high?  where does fragmentation occur? at the OS level? at the UDP level?  I'd prefer to leave any fragmenting to the underlying OS as much as possible - hence if I want to send packets of 4000 bytes than either they are fragmented at the tun level (i.e. before they are processed by OpenVPN) or at the transport level (i.e. OpenVPN spits out a 4000 byte packet over the UDP/TCP connection, which the OS then fragements).  The problem with fragmenting at the UDP level is that you need to keep track of the fragments in order to reassemble them - TCP does this for you. cheers, JJK 

Hi, On Mon, Oct 29, 2018 at 06:20:40PM +0100, Arne Schwabe wrote: > This can be used to redirect all IPv6 traffic to the tun interface, > effectively black holing the IPv6 traffic. Without ICMPv6 error > messages this will result in timeouts when the server does not send > error codes. block-ipv6 allows client side only blocking on all > platforms that OpenVPN supports IPv6. On Android it is only way to do > sensible IPv6 blocking on Android < 5.0 and broken devices (Samsung). Generally I agree with the need for this, so feature-ACK. Functionally, I've tested this on Linux, and it does what it says on the lid (I've only tested client, but if that works, I expect server to work as well). Style-wise, there is, unfortunately, still way to go - missing brackets, non-pretty alignment, etc. -> so please run uncrustify, then send us a v6. Besides the uncrustify things, please leave af_addr_size() in socket.h - its only callers are in socket.c, so the move to proto.h does not make any sense anymore (I assume you wanted to call it from ip_checksum(), but since that one does a local decision for 4/16 byte address size, moving the other function is no longer needed). Lastly, the context of the hunk in options.c has no #ifdef ENABLE_PUSH_PEER_INFO anymore, but "master" still has that - so please send the patch that removes this #ifdef to the list as well :-) So, "changes requested" :-) - thanks. As a side note, how did you test the "without ifconfig-ipv6" variant? When I tried this it refused to install the IPv6 routes... gert -- "If was one thing all people took for granted, was conviction that if you feed honest figures into a computer, honest figures come out. Never doubted it myself till I met a computer with a sense of humor." Robert A. Heinlein, The Moon is a Harsh Mistress Gert Doering - Munich, Germany ge...@gr... 

There are an amazing number of brackets that were either totally missing, or have snuck up on the "for(...){" line. Further, uncrustify wants "|" in multi-line logical expressions now at the beginning of the new line, and "PRIi64" now gets surrounding spaces. Added "sp_after_semi_for_empty=Add" to uncrustify.conf to leave a few for() statements alone that look better the way they are. Signed-off-by: Gert Doering <ge...@gr...> --- dev-tools/uncrustify.conf | 3 ++ src/openvpn/block_dns.h | 4 +-- src/openvpn/buffer.c | 4 +-- src/openvpn/buffer.h | 1 + src/openvpn/console.h | 2 +- src/openvpn/crypto.c | 10 +++++-- src/openvpn/crypto.h | 5 ++-- src/openvpn/crypto_backend.h | 2 +- src/openvpn/crypto_mbedtls.c | 7 +++-- src/openvpn/crypto_mbedtls.h | 3 +- src/openvpn/crypto_openssl.c | 37 +++++++++++++------------ src/openvpn/crypto_openssl.h | 3 +- src/openvpn/cryptoapi.c | 26 ++++++++++-------- src/openvpn/env_set.c | 2 +- src/openvpn/env_set.h | 6 ++-- src/openvpn/error.c | 2 +- src/openvpn/event.c | 4 +-- src/openvpn/forward.c | 4 +-- src/openvpn/forward.h | 2 ++ src/openvpn/init.c | 22 +++++++-------- src/openvpn/integer.h | 4 +-- src/openvpn/manage.c | 4 +-- src/openvpn/mss.c | 2 +- src/openvpn/multi.c | 2 +- src/openvpn/multi.h | 1 + src/openvpn/ntlm.c | 4 +-- src/openvpn/openssl_compat.h | 47 ++++++++++++++++---------------- src/openvpn/options.c | 10 +++++-- src/openvpn/otime.c | 4 +-- src/openvpn/packet_id.c | 12 ++++---- src/openvpn/packet_id.h | 2 +- src/openvpn/ping.h | 2 +- src/openvpn/plugin.c | 3 +- src/openvpn/proxy.c | 4 +-- src/openvpn/reliable.c | 4 +-- src/openvpn/route.c | 4 +-- src/openvpn/run_command.c | 6 ++-- src/openvpn/shaper.c | 2 +- src/openvpn/shaper.h | 2 +- src/openvpn/socket.c | 6 +++- src/openvpn/socket.h | 2 +- src/openvpn/ssl.c | 20 +++++++------- src/openvpn/ssl_common.h | 4 +-- src/openvpn/ssl_mbedtls.c | 32 +++++++++++----------- src/openvpn/ssl_mbedtls.h | 4 +-- src/openvpn/ssl_openssl.c | 32 +++++++++++----------- src/openvpn/ssl_verify.c | 42 ++++++++++++++-------------- src/openvpn/ssl_verify_backend.h | 1 + src/openvpn/ssl_verify_openssl.c | 7 +++-- src/openvpn/tls_crypt.c | 6 ++-- src/openvpn/tls_crypt.h | 6 ++-- src/openvpn/tun.c | 15 +++++----- src/openvpn/win32.c | 2 +- 53 files changed, 243 insertions(+), 204 deletions(-) diff --git a/dev-tools/uncrustify.conf b/dev-tools/uncrustify.conf index d8ea870e..25eb4cdd 100644 --- a/dev-tools/uncrustify.conf +++ b/dev-tools/uncrustify.conf @@ -60,6 +60,9 @@ mod_add_long_ifdef_else_comment=5 # Misc cleanup mod_remove_extra_semicolon=true +# leave blank at end of empty for() statements +sp_after_semi_for_empty=Add + # Use C-style comments (/* .. */) cmt_c_nl_end=true cmt_star_cont=true diff --git a/src/openvpn/block_dns.h b/src/openvpn/block_dns.h index 50b383f6..f9b1e5d0 100644 --- a/src/openvpn/block_dns.h +++ b/src/openvpn/block_dns.h @@ -65,5 +65,5 @@ DWORD set_interface_metric(const NET_IFINDEX index, const ADDRESS_FAMILY family, const ULONG metric); -#endif -#endif +#endif /* ifndef OPENVPN_BLOCK_DNS_H */ +#endif /* ifdef _WIN32 */ diff --git a/src/openvpn/buffer.c b/src/openvpn/buffer.c index 27c75271..2aae5c4c 100644 --- a/src/openvpn/buffer.c +++ b/src/openvpn/buffer.c @@ -312,7 +312,7 @@ openvpn_snprintf(char *str, size_t size, const char *format, ...) /* * openvpn_swprintf() is currently only used by Windows code paths - * and when enabled for all platforms it will currently break older + * and when enabled for all platforms it will currently break older * OpenBSD versions lacking vswprintf(3) support in their libc. */ @@ -474,7 +474,7 @@ x_gc_freespecial(struct gc_arena *a) } void -gc_addspecial(void *addr, void(free_function)(void *), struct gc_arena *a) +gc_addspecial(void *addr, void (free_function)(void *), struct gc_arena *a) { ASSERT(a); struct gc_entry_special *e; diff --git a/src/openvpn/buffer.h b/src/openvpn/buffer.h index d402d05b..a4fe6f9b 100644 --- a/src/openvpn/buffer.h +++ b/src/openvpn/buffer.h @@ -458,6 +458,7 @@ __attribute__ ((format(__printf__, 3, 4))) */ bool openvpn_swprintf(wchar_t *const str, const size_t size, const wchar_t *const format, ...); + /* * Unlike in openvpn_snprintf, we cannot use format attributes since * GCC doesn't support wprintf as archetype. diff --git a/src/openvpn/console.h b/src/openvpn/console.h index 5a70e5fd..0ffd6683 100644 --- a/src/openvpn/console.h +++ b/src/openvpn/console.h @@ -21,7 +21,7 @@ * You should have received a copy of the GNU General Public License along * with this program; if not, write to the Free Software Foundation, Inc., * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. -*/ + */ #ifndef CONSOLE_H #define CONSOLE_H diff --git a/src/openvpn/crypto.c b/src/openvpn/crypto.c index e9bf22b0..df6f36ca 100644 --- a/src/openvpn/crypto.c +++ b/src/openvpn/crypto.c @@ -920,10 +920,12 @@ key_is_zero(struct key *key, const struct key_type *kt) { int i; for (i = 0; i < kt->cipher_length; ++i) + { if (key->cipher[i]) { return false; } + } msg(D_CRYPT_ERRORS, "CRYPTO INFO: WARNING: zero key detected"); return true; } @@ -1270,7 +1272,9 @@ read_key_file(struct key2 *key2, const char *file, const unsigned int flags) { in = buffer_read_from_file(file, &gc); if (!buf_valid(&in)) + { msg(M_FATAL, "Read error on key file ('%s')", file); + } size = in.len; } @@ -1462,7 +1466,7 @@ write_key_file(const int nkeys, const char *filename) buf_printf(&out, "%s\n", static_key_foot); /* write key file, now formatted in out, to file */ - if(!buffer_write_file(filename, &out)) + if (!buffer_write_file(filename, &out)) { nbits = -1; } @@ -1692,7 +1696,9 @@ prng_reset_nonce(void) { int i; for (i = 0; i < size; ++i) + { nonce_data[i] = (uint8_t) i; + } } #endif } @@ -1773,7 +1779,7 @@ void print_cipher(const cipher_kt_t *cipher) { const char *var_key_size = cipher_kt_var_key_size(cipher) ? - " by default" : ""; + " by default" : ""; printf("%s (%d bit key%s, ", translate_cipher_name_to_openvpn(cipher_kt_name(cipher)), diff --git a/src/openvpn/crypto.h b/src/openvpn/crypto.h index 96ba8586..1edde2e3 100644 --- a/src/openvpn/crypto.h +++ b/src/openvpn/crypto.h @@ -318,7 +318,7 @@ void free_key_ctx(struct key_ctx *ctx); void init_key_ctx_bi(struct key_ctx_bi *ctx, const struct key2 *key2, int key_direction, const struct key_type *kt, - const char *name); + const char *name); void free_key_ctx_bi(struct key_ctx_bi *ctx); @@ -504,7 +504,8 @@ memcmp_constant_time(const void *a, const void *b, size_t size) int ret = 0; size_t i; - for (i = 0; i < size; i++) { + for (i = 0; i < size; i++) + { ret |= *a1++ ^ *b1++; } diff --git a/src/openvpn/crypto_backend.h b/src/openvpn/crypto_backend.h index 38b2c175..a04e01f4 100644 --- a/src/openvpn/crypto_backend.h +++ b/src/openvpn/crypto_backend.h @@ -51,7 +51,7 @@ typedef enum { MD_SHA1, MD_SHA256 -} hash_algo_type ; +} hash_algo_type; /** Struct used in cipher name translation table */ typedef struct { diff --git a/src/openvpn/crypto_mbedtls.c b/src/openvpn/crypto_mbedtls.c index 77e84c3c..2e931440 100644 --- a/src/openvpn/crypto_mbedtls.c +++ b/src/openvpn/crypto_mbedtls.c @@ -233,8 +233,8 @@ crypto_pem_encode(const char *name, struct buffer *dst, size_t out_len = 0; if (MBEDTLS_ERR_BASE64_BUFFER_TOO_SMALL != - mbedtls_pem_write_buffer(header, footer, BPTR(src), BLEN(src), - NULL, 0, &out_len)) + mbedtls_pem_write_buffer(header, footer, BPTR(src), BLEN(src), + NULL, 0, &out_len)) { return false; } @@ -858,7 +858,8 @@ md_ctx_new(void) return ctx; } -void md_ctx_free(mbedtls_md_context_t *ctx) +void +md_ctx_free(mbedtls_md_context_t *ctx) { free(ctx); } diff --git a/src/openvpn/crypto_mbedtls.h b/src/openvpn/crypto_mbedtls.h index 81b542bc..c4b13b7b 100644 --- a/src/openvpn/crypto_mbedtls.h +++ b/src/openvpn/crypto_mbedtls.h @@ -146,7 +146,8 @@ mbed_log_func_line_lite(unsigned int flags, int errval, #define mbed_ok(errval) \ mbed_log_func_line_lite(D_CRYPT_ERRORS, errval, __func__, __LINE__) -static inline bool cipher_kt_var_key_size(const cipher_kt_t *cipher) +static inline bool +cipher_kt_var_key_size(const cipher_kt_t *cipher) { return cipher->flags & MBEDTLS_CIPHER_VARIABLE_KEY_LEN; } diff --git a/src/openvpn/crypto_openssl.c b/src/openvpn/crypto_openssl.c index 0bff1a25..9691ce05 100644 --- a/src/openvpn/crypto_openssl.c +++ b/src/openvpn/crypto_openssl.c @@ -202,12 +202,12 @@ crypto_print_openssl_errors(const unsigned int flags) else if (ERR_GET_REASON(err) == SSL_R_UNSUPPORTED_PROTOCOL) { msg(D_CRYPT_ERRORS, "TLS error: Unsupported protocol. This typically " - "indicates that client and server have no common TLS version enabled. " - "This can be caused by mismatched tls-version-min and tls-version-max " - "options on client and server. " - "If your OpenVPN client is between v2.3.6 and v2.3.2 try adding " - "tls-version-min 1.0 to the client configuration to use TLS 1.0+ " - "instead of TLS 1.0 only"); + "indicates that client and server have no common TLS version enabled. " + "This can be caused by mismatched tls-version-min and tls-version-max " + "options on client and server. " + "If your OpenVPN client is between v2.3.6 and v2.3.2 try adding " + "tls-version-min 1.0 to the client configuration to use TLS 1.0+ " + "instead of TLS 1.0 only"); } msg(flags, "OpenSSL: %s", ERR_error_string(err, NULL)); } @@ -315,7 +315,8 @@ show_available_ciphers(void) qsort(cipher_list, num_ciphers, sizeof(*cipher_list), cipher_name_cmp); - for (i = 0; i < num_ciphers; i++) { + for (i = 0; i < num_ciphers; i++) + { if (!cipher_kt_insecure(cipher_list[i])) { print_cipher(cipher_list[i]); @@ -324,7 +325,8 @@ show_available_ciphers(void) printf("\nThe following ciphers have a block size of less than 128 bits, \n" "and are therefore deprecated. Do not use unless you have to.\n\n"); - for (i = 0; i < num_ciphers; i++) { + for (i = 0; i < num_ciphers; i++) + { if (cipher_kt_insecure(cipher_list[i])) { print_cipher(cipher_list[i]); @@ -405,7 +407,7 @@ crypto_pem_encode(const char *name, struct buffer *dst, cleanup: if (!BIO_free(bio)) { - ret = false;; + ret = false; } return ret; @@ -458,7 +460,7 @@ cleanup: OPENSSL_free(data_read); if (!BIO_free(bio)) { - ret = false;; + ret = false; } return ret; @@ -688,7 +690,7 @@ cipher_kt_insecure(const EVP_CIPHER *cipher) #ifdef NID_chacha20_poly1305 || EVP_CIPHER_nid(cipher) == NID_chacha20_poly1305 #endif - ); + ); } int @@ -729,13 +731,13 @@ cipher_kt_mode_aead(const cipher_kt_t *cipher) { switch (EVP_CIPHER_nid(cipher)) { - case NID_aes_128_gcm: - case NID_aes_192_gcm: - case NID_aes_256_gcm: + case NID_aes_128_gcm: + case NID_aes_192_gcm: + case NID_aes_256_gcm: #ifdef NID_chacha20_poly1305 - case NID_chacha20_poly1305: + case NID_chacha20_poly1305: #endif - return true; + return true; } } #endif @@ -962,7 +964,8 @@ md_ctx_new(void) return ctx; } -void md_ctx_free(EVP_MD_CTX *ctx) +void +md_ctx_free(EVP_MD_CTX *ctx) { EVP_MD_CTX_free(ctx); } diff --git a/src/openvpn/crypto_openssl.h b/src/openvpn/crypto_openssl.h index 1ea3e858..64754480 100644 --- a/src/openvpn/crypto_openssl.h +++ b/src/openvpn/crypto_openssl.h @@ -101,7 +101,8 @@ void crypto_print_openssl_errors(const unsigned int flags); msg((flags), __VA_ARGS__); \ } while (false) -static inline bool cipher_kt_var_key_size(const cipher_kt_t *cipher) +static inline bool +cipher_kt_var_key_size(const cipher_kt_t *cipher) { return EVP_CIPHER_flags(cipher) & EVP_CIPH_VARIABLE_LENGTH; } diff --git a/src/openvpn/cryptoapi.c b/src/openvpn/cryptoapi.c index fa057cb2..9687d8c2 100644 --- a/src/openvpn/cryptoapi.c +++ b/src/openvpn/cryptoapi.c @@ -159,7 +159,8 @@ ms_error_text(DWORD ms_err) /* trim to the left */ if (rv) { - for (p = rv + strlen(rv) - 1; p >= rv; p--) { + for (p = rv + strlen(rv) - 1; p >= rv; p--) + { if (isspace(*p)) { *p = '\0'; @@ -198,7 +199,8 @@ err_put_ms_error(DWORD ms_err, int func, const char *file, int line) } /* since MS error codes are 32 bit, and the ones in the ERR_... system is * only 12, we must have a mapping table between them. */ - for (i = 0; i < ERR_MAP_SZ; i++) { + for (i = 0; i < ERR_MAP_SZ; i++) + { if (err_map[i].ms_err == ms_err) { ERR_PUT_error(ERR_LIB_CRYPTOAPI, func, err_map[i].err, file, line); @@ -267,8 +269,8 @@ priv_enc_CNG(const CAPI_DATA *cd, const wchar_t *hash_algo, const unsigned char BCRYPT_PKCS1_PADDING_INFO padinfo = {hash_algo}; DWORD status; - status = NCryptSignHash(hkey, padding? &padinfo : NULL, (BYTE*) from, flen, - to, tlen, &len, padding? BCRYPT_PAD_PKCS1 : 0); + status = NCryptSignHash(hkey, padding ? &padinfo : NULL, (BYTE *) from, flen, + to, tlen, &len, padding ? BCRYPT_PAD_PKCS1 : 0); if (status != ERROR_SUCCESS) { SetLastError(status); @@ -375,7 +377,7 @@ rsa_priv_enc(int flen, const unsigned char *from, unsigned char *to, RSA *rsa, i */ static int rsa_sign_CNG(int type, const unsigned char *m, unsigned int m_len, - unsigned char *sig, unsigned int *siglen, const RSA *rsa) + unsigned char *sig, unsigned int *siglen, const RSA *rsa) { CAPI_DATA *cd = (CAPI_DATA *) RSA_meth_get0_app_data(RSA_get_method(rsa)); const wchar_t *alg = NULL; @@ -419,6 +421,7 @@ rsa_sign_CNG(int type, const unsigned char *m, unsigned int m_len, /* No DigestInfo header is required -- set alg-name to NULL */ alg = NULL; break; + default: msg(M_WARN, "cryptoapicert: Unknown hash type NID=0x%x", type); RSAerr(RSA_F_RSA_SIGN, RSA_R_UNKNOWN_ALGORITHM_TYPE); @@ -459,7 +462,7 @@ finish(RSA *rsa) return 0; } CAPI_DATA_free(cd); - RSA_meth_free((RSA_METHOD*) rsa_meth); + RSA_meth_free((RSA_METHOD *) rsa_meth); return 1; } @@ -586,7 +589,7 @@ ssl_ctx_set_eckey(SSL_CTX *ssl_ctx, CAPI_DATA *cd, EVP_PKEY *pkey) if (cd->key_spec != CERT_NCRYPT_KEY_SPEC) { msg(M_NONFATAL, "ERROR: cryptoapicert with only legacy private key handle available." - " EC certificate not supported."); + " EC certificate not supported."); goto err; } /* create a method struct with default callbacks filled in */ @@ -686,7 +689,8 @@ find_certificate_in_store(const char *cert_prop, HCERTSTORE cert_store) /* skip the tag */ cert_prop += 6; - for (p = (char *) cert_prop, i = 0; *p && i < sizeof(hash); i++) { + for (p = (char *) cert_prop, i = 0; *p && i < sizeof(hash); i++) + { if (*p >= '0' && *p <= '9') { x = (*p - '0') << 4; @@ -739,7 +743,7 @@ ssl_ctx_set_rsakey(SSL_CTX *ssl_ctx, CAPI_DATA *cd, EVP_PKEY *pkey) bool rsa_method_set = false; my_rsa_method = RSA_meth_new("Microsoft Cryptography API RSA Method", - RSA_METHOD_FLAG_NO_CHECK); + RSA_METHOD_FLAG_NO_CHECK); check_malloc_return(my_rsa_method); RSA_meth_set_pub_enc(my_rsa_method, rsa_pub_enc); RSA_meth_set_pub_dec(my_rsa_method, rsa_pub_dec); @@ -797,7 +801,7 @@ ssl_ctx_set_rsakey(SSL_CTX *ssl_ctx, CAPI_DATA *cd, EVP_PKEY *pkey) goto err; } /* SSL_CTX_use_RSAPrivateKey() increased the reference count in 'rsa', so - * we decrease it here with RSA_free(), or it will never be cleaned up. */ + * we decrease it here with RSA_free(), or it will never be cleaned up. */ RSA_free(rsa); return 1; @@ -867,7 +871,7 @@ SSL_CTX_use_CryptoAPI_certificate(SSL_CTX *ssl_ctx, const char *cert_prop) DWORD flags = CRYPT_ACQUIRE_COMPARE_KEY_FLAG | CRYPT_ACQUIRE_PREFER_NCRYPT_KEY_FLAG; if (!CryptAcquireCertificatePrivateKey(cd->cert_context, flags, NULL, - &cd->crypt_prov, &cd->key_spec, &cd->free_crypt_prov)) + &cd->crypt_prov, &cd->key_spec, &cd->free_crypt_prov)) { /* if we don't have a smart card reader here, and we try to access a * smart card certificate, we get: diff --git a/src/openvpn/env_set.c b/src/openvpn/env_set.c index e7fb2d83..0ab0262a 100644 --- a/src/openvpn/env_set.c +++ b/src/openvpn/env_set.c @@ -277,7 +277,7 @@ void setenv_long_long(struct env_set *es, const char *name, long long value) { char buf[64]; - openvpn_snprintf(buf, sizeof(buf), "%"PRIi64, (int64_t)value); + openvpn_snprintf(buf, sizeof(buf), "%" PRIi64, (int64_t)value); setenv_str(es, name, buf); } diff --git a/src/openvpn/env_set.h b/src/openvpn/env_set.h index 5dc33485..cf8415cc 100644 --- a/src/openvpn/env_set.h +++ b/src/openvpn/env_set.h @@ -94,13 +94,15 @@ void env_set_print(int msglevel, const struct env_set *es); void env_set_inherit(struct env_set *es, const struct env_set *src); /* returns true if environmental variable name starts with 'password' */ -static inline bool is_password_env_var(const char *str) +static inline bool +is_password_env_var(const char *str) { return (strncmp(str, "password", 8) == 0); } /* returns true if environmental variable safe to print to log */ -static inline bool env_safe_to_print(const char *str) +static inline bool +env_safe_to_print(const char *str) { #ifndef UNSAFE_DEBUG if (is_password_env_var(str)) diff --git a/src/openvpn/error.c b/src/openvpn/error.c index 51294687..b2492f2b 100644 --- a/src/openvpn/error.c +++ b/src/openvpn/error.c @@ -343,7 +343,7 @@ x_msg_va(const unsigned int flags, const char *format, va_list arglist) struct timeval tv; gettimeofday(&tv, NULL); - fprintf(fp, "%"PRIi64".%06ld %x %s%s%s%s", + fprintf(fp, "%" PRIi64 ".%06ld %x %s%s%s%s", (int64_t)tv.tv_sec, (long)tv.tv_usec, flags, diff --git a/src/openvpn/event.c b/src/openvpn/event.c index 37276905..49dfa861 100644 --- a/src/openvpn/event.c +++ b/src/openvpn/event.c @@ -1041,7 +1041,7 @@ se_wait_fast(struct event_set *es, const struct timeval *tv, struct event_set_re struct timeval tv_tmp = *tv; int stat; - dmsg(D_EVENT_WAIT, "SE_WAIT_FAST maxfd=%d tv=%"PRIi64"/%ld", + dmsg(D_EVENT_WAIT, "SE_WAIT_FAST maxfd=%d tv=%" PRIi64 "/%ld", ses->maxfd, (int64_t)tv_tmp.tv_sec, (long)tv_tmp.tv_usec); @@ -1065,7 +1065,7 @@ se_wait_scalable(struct event_set *es, const struct timeval *tv, struct event_se fd_set write = ses->writefds; int stat; - dmsg(D_EVENT_WAIT, "SE_WAIT_SCALEABLE maxfd=%d tv=%"PRIi64"/%ld", + dmsg(D_EVENT_WAIT, "SE_WAIT_SCALEABLE maxfd=%d tv=%" PRIi64 "/%ld", ses->maxfd, (int64_t)tv_tmp.tv_sec, (long)tv_tmp.tv_usec); stat = select(ses->maxfd + 1, &read, &write, NULL, &tv_tmp); diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c index f8faa810..37719dbe 100644 --- a/src/openvpn/forward.c +++ b/src/openvpn/forward.c @@ -757,7 +757,7 @@ static void process_coarse_timers(struct context *c) { /* flush current packet-id to file once per 60 - * seconds if --replay-persist was specified */ + * seconds if --replay-persist was specified */ check_packet_id_persist_flush(c); /* should we update status file? */ @@ -836,7 +836,7 @@ check_coarse_timers_dowork(struct context *c) process_coarse_timers(c); c->c2.coarse_timer_wakeup = now + c->c2.timeval.tv_sec; - dmsg(D_INTERVAL, "TIMER: coarse timer wakeup %"PRIi64" seconds", (int64_t)c->c2.timeval.tv_sec); + dmsg(D_INTERVAL, "TIMER: coarse timer wakeup %" PRIi64 " seconds", (int64_t)c->c2.timeval.tv_sec); /* Is the coarse timeout NOT the earliest one? */ if (c->c2.timeval.tv_sec > save.tv_sec) diff --git a/src/openvpn/forward.h b/src/openvpn/forward.h index 58b75d64..b534c723 100644 --- a/src/openvpn/forward.h +++ b/src/openvpn/forward.h @@ -80,10 +80,12 @@ void check_incoming_control_channel_dowork(struct context *c); void check_scheduled_exit_dowork(struct context *c); void check_push_request_dowork(struct context *c); + #endif /* P2MP */ #ifdef ENABLE_FRAGMENT void check_fragment_dowork(struct context *c); + #endif /* ENABLE_FRAGMENT */ void check_connection_established_dowork(struct context *c); diff --git a/src/openvpn/init.c b/src/openvpn/init.c index 2a1b38ea..a1841604 100644 --- a/src/openvpn/init.c +++ b/src/openvpn/init.c @@ -1077,13 +1077,13 @@ do_genkey(const struct options *options) } if (options->tls_crypt_v2_genkey_type) { - if(!strcmp(options->tls_crypt_v2_genkey_type, "server")) + if (!strcmp(options->tls_crypt_v2_genkey_type, "server")) { tls_crypt_v2_write_server_key_file(options->tls_crypt_v2_genkey_file); return true; } if (options->tls_crypt_v2_genkey_type - && !strcmp(options->tls_crypt_v2_genkey_type, "client")) + && !strcmp(options->tls_crypt_v2_genkey_type, "client")) { if (!options->tls_crypt_v2_file) { @@ -1091,8 +1091,8 @@ do_genkey(const struct options *options) } tls_crypt_v2_write_client_key_file(options->tls_crypt_v2_genkey_file, - options->tls_crypt_v2_metadata, options->tls_crypt_v2_file, - options->tls_crypt_v2_inline); + options->tls_crypt_v2_metadata, options->tls_crypt_v2_file, + options->tls_crypt_v2_inline); return true; } @@ -2570,8 +2570,8 @@ do_init_tls_wrap_key(struct context *c) if (!streq(options->authname, "none")) { c->c1.ks.tls_auth_key_type.digest = md_kt_get(options->authname); - c->c1.ks.tls_auth_key_type.hmac_length = - md_kt_size(c->c1.ks.tls_auth_key_type.digest); + c->c1.ks.tls_auth_key_type.hmac_length = + md_kt_size(c->c1.ks.tls_auth_key_type.digest); } else { @@ -2655,7 +2655,7 @@ do_init_crypto_tls_c1(struct context *c) return; #else /* if P2MP */ msg(M_FATAL, "Error: private key password verification failed"); -#endif +#endif /* if P2MP */ } /* Get cipher & hash algorithms */ @@ -2763,15 +2763,15 @@ do_init_crypto_tls(struct context *c, const unsigned int flags) { /* Add 10% jitter to reneg-sec by default (server side only) */ int auto_jitter = options->mode != MODE_SERVER ? 0 : - get_random() % max_int(options->renegotiate_seconds / 10, 1); + get_random() % max_int(options->renegotiate_seconds / 10, 1); to.renegotiate_seconds = options->renegotiate_seconds - auto_jitter; } else { /* Add user-specified jitter to reneg-sec */ - to.renegotiate_seconds = options->renegotiate_seconds - - (get_random() % max_int(options->renegotiate_seconds - - options->renegotiate_seconds_min, 1)); + to.renegotiate_seconds = options->renegotiate_seconds + -(get_random() % max_int(options->renegotiate_seconds + - options->renegotiate_seconds_min, 1)); } to.single_session = options->single_session; to.mode = options->mode; diff --git a/src/openvpn/integer.h b/src/openvpn/integer.h index b1ae0eda..3755f43f 100644 --- a/src/openvpn/integer.h +++ b/src/openvpn/integer.h @@ -28,12 +28,12 @@ #ifndef htonll #define htonll(x) ((1==htonl(1)) ? (x) : \ - ((uint64_t)htonl((x) & 0xFFFFFFFF) << 32) | htonl((x) >> 32)) + ((uint64_t)htonl((x) & 0xFFFFFFFF) << 32) | htonl((x) >> 32)) #endif #ifndef ntohll #define ntohll(x) ((1==ntohl(1)) ? (x) : \ - ((uint64_t)ntohl((x) & 0xFFFFFFFF) << 32) | ntohl((x) >> 32)) + ((uint64_t)ntohl((x) & 0xFFFFFFFF) << 32) | ntohl((x) >> 32)) #endif /* diff --git a/src/openvpn/manage.c b/src/openvpn/manage.c index 8b633f20..2d86dad4 100644 --- a/src/openvpn/manage.c +++ b/src/openvpn/manage.c @@ -3640,7 +3640,7 @@ management_query_multiline_flatten(struct management *man, char * /* returns allocated base64 signature */ management_query_pk_sig(struct management *man, - const char *b64_data) + const char *b64_data) { const char *prompt = "PK_SIGN"; const char *desc = "pk-sign"; @@ -3650,7 +3650,7 @@ management_query_pk_sig(struct management *man, desc = "rsa-sign"; } return management_query_multiline_flatten(man, b64_data, prompt, desc, - &man->connection.ext_key_state, &man->connection.ext_key_input); + &man->connection.ext_key_state, &man->connection.ext_key_input); } char * diff --git a/src/openvpn/mss.c b/src/openvpn/mss.c index facdf7b5..b0e2c42b 100644 --- a/src/openvpn/mss.c +++ b/src/openvpn/mss.c @@ -150,7 +150,7 @@ mss_fixup_dowork(struct buffer *buf, uint16_t maxmss) if (BLEN(buf) < (int) sizeof(struct openvpn_tcphdr)) { -	return; + return; } verify_align_4(buf); diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c index 8440f311..53d6f0cf 100644 --- a/src/openvpn/multi.c +++ b/src/openvpn/multi.c @@ -2394,7 +2394,7 @@ multi_process_post(struct multi_context *m, struct multi_instance *mi, const uns multi_set_pending(m, ANY_OUT(&mi->context) ? mi : NULL); #ifdef MULTI_DEBUG_EVENT_LOOP - printf("POST %s[%d] to=%d lo=%d/%d w=%"PRIi64"/%ld\n", + printf("POST %s[%d] to=%d lo=%d/%d w=%" PRIi64 "/%ld\n", id(mi), (int) (mi == m->pending), mi ? mi->context.c2.to_tun.len : -1, diff --git a/src/openvpn/multi.h b/src/openvpn/multi.h index 3d3d6875..7216865e 100644 --- a/src/openvpn/multi.h +++ b/src/openvpn/multi.h @@ -537,6 +537,7 @@ clear_prefix(void) #define MULTI_CACHE_ROUTE_TTL 60 void multi_reap_process_dowork(const struct multi_context *m); + void multi_process_per_second_timers_dowork(struct multi_context *m); static inline void diff --git a/src/openvpn/ntlm.c b/src/openvpn/ntlm.c index 077fa3e2..e3707484 100644 --- a/src/openvpn/ntlm.c +++ b/src/openvpn/ntlm.c @@ -314,8 +314,8 @@ ntlm_phase_3(const struct http_proxy_info *p, const char *phase_2, * byte order on the wire for the NTLM header is LE. */ const size_t hoff = 0x14; - unsigned long flags = buf2[hoff] | (buf2[hoff + 1] << 8) | - (buf2[hoff + 2] << 16) | (buf2[hoff + 3] << 24); + unsigned long flags = buf2[hoff] | (buf2[hoff + 1] << 8) + |(buf2[hoff + 2] << 16) | (buf2[hoff + 3] << 24); if ((flags & 0x00800000) == 0x00800000) { tib_len = buf2[0x28]; /* Get Target Information block size */ diff --git a/src/openvpn/openssl_compat.h b/src/openvpn/openssl_compat.h index 9560b29f..a4072b9a 100644 --- a/src/openvpn/openssl_compat.h +++ b/src/openvpn/openssl_compat.h @@ -193,8 +193,8 @@ X509_get0_pubkey(const X509 *x) * @param store X509 object store * @return the X509 object stack */ -static inline STACK_OF(X509_OBJECT) * -X509_STORE_get0_objects(X509_STORE *store) +static inline STACK_OF(X509_OBJECT) +*X509_STORE_get0_objects(X509_STORE *store) { return store ? store->objs : NULL; } @@ -369,7 +369,7 @@ RSA_set0_key(RSA *rsa, BIGNUM *n, BIGNUM *e, BIGNUM *d) return 1; } -#endif +#endif /* if !defined(HAVE_RSA_SET0_KEY) */ #if !defined(HAVE_RSA_BITS) /** @@ -483,9 +483,9 @@ RSA_meth_free(RSA_METHOD *meth) */ static inline int RSA_meth_set_pub_enc(RSA_METHOD *meth, - int (*pub_enc) (int flen, const unsigned char *from, - unsigned char *to, RSA *rsa, - int padding)) + int (*pub_enc)(int flen, const unsigned char *from, + unsigned char *to, RSA *rsa, + int padding)) { if (meth) { @@ -506,9 +506,9 @@ RSA_meth_set_pub_enc(RSA_METHOD *meth, */ static inline int RSA_meth_set_pub_dec(RSA_METHOD *meth, - int (*pub_dec) (int flen, const unsigned char *from, - unsigned char *to, RSA *rsa, - int padding)) + int (*pub_dec)(int flen, const unsigned char *from, + unsigned char *to, RSA *rsa, + int padding)) { if (meth) { @@ -529,9 +529,9 @@ RSA_meth_set_pub_dec(RSA_METHOD *meth, */ static inline int RSA_meth_set_priv_enc(RSA_METHOD *meth, - int (*priv_enc) (int flen, const unsigned char *from, - unsigned char *to, RSA *rsa, - int padding)) + int (*priv_enc)(int flen, const unsigned char *from, + unsigned char *to, RSA *rsa, + int padding)) { if (meth) { @@ -552,9 +552,9 @@ RSA_meth_set_priv_enc(RSA_METHOD *meth, */ static inline int RSA_meth_set_priv_dec(RSA_METHOD *meth, - int (*priv_dec) (int flen, const unsigned char *from, - unsigned char *to, RSA *rsa, - int padding)) + int (*priv_dec)(int flen, const unsigned char *from, + unsigned char *to, RSA *rsa, + int padding)) { if (meth) { @@ -574,7 +574,7 @@ RSA_meth_set_priv_dec(RSA_METHOD *meth, * @return 1 on success, 0 on error */ static inline int -RSA_meth_set_init(RSA_METHOD *meth, int (*init) (RSA *rsa)) +RSA_meth_set_init(RSA_METHOD *meth, int (*init)(RSA *rsa)) { if (meth) { @@ -594,11 +594,12 @@ RSA_meth_set_init(RSA_METHOD *meth, int (*init) (RSA *rsa)) * @return 1 on success, 0 on error */ static inline -int RSA_meth_set_sign(RSA_METHOD *meth, - int (*sign) (int type, const unsigned char *m, - unsigned int m_length, - unsigned char *sigret, unsigned int *siglen, - const RSA *rsa)) +int +RSA_meth_set_sign(RSA_METHOD *meth, + int (*sign)(int type, const unsigned char *m, + unsigned int m_length, + unsigned char *sigret, unsigned int *siglen, + const RSA *rsa)) { meth->rsa_sign = sign; return 1; @@ -614,7 +615,7 @@ int RSA_meth_set_sign(RSA_METHOD *meth, * @return 1 on success, 0 on error */ static inline int -RSA_meth_set_finish(RSA_METHOD *meth, int (*finish) (RSA *rsa)) +RSA_meth_set_finish(RSA_METHOD *meth, int (*finish)(RSA *rsa)) { if (meth) { @@ -669,7 +670,7 @@ RSA_meth_get0_app_data(const RSA_METHOD *meth) static inline int EC_GROUP_order_bits(const EC_GROUP *group) { - BIGNUM* order = BN_new(); + BIGNUM *order = BN_new(); EC_GROUP_get_order(group, order, NULL); int bits = BN_num_bits(order); BN_free(order); diff --git a/src/openvpn/options.c b/src/openvpn/options.c index 9ee1836b..6d53dea5 100644 --- a/src/openvpn/options.c +++ b/src/openvpn/options.c @@ -2134,7 +2134,7 @@ options_postprocess_verify_ce(const struct options *options, const struct connec "passwords is STRONGLY discouraged and considered insecure"); } -#endif +#endif /* ifdef ENABLE_MANAGEMENT */ /* * Windows-specific options. @@ -2859,8 +2859,10 @@ options_postprocess_mutate_ce(struct options *o, struct connection_entry *ce) { struct buffer in = buffer_read_from_file(o->tls_auth_file, &o->gc); if (!buf_valid(&in)) + { msg(M_FATAL, "Cannot pre-load tls-auth keyfile (%s)", o->tls_auth_file); + } ce->tls_auth_file = INLINE_FILE_TAG; ce->tls_auth_file_inline = (char *)in.data; @@ -2870,8 +2872,10 @@ options_postprocess_mutate_ce(struct options *o, struct connection_entry *ce) { struct buffer in = buffer_read_from_file(o->tls_crypt_file, &o->gc); if (!buf_valid(&in)) + { msg(M_FATAL, "Cannot pre-load tls-crypt keyfile (%s)", o->tls_auth_file); + } ce->tls_crypt_file = INLINE_FILE_TAG; ce->tls_crypt_inline = (char *)in.data; @@ -3040,7 +3044,7 @@ options_postprocess_mutate(struct options *o) { /* DH file is only meaningful in a tls-server context. */ msg(M_WARN, "WARNING: Ignoring option 'dh' in tls-client mode, please only " - "include this in your server configuration"); + "include this in your server configuration"); o->dh_file = NULL; } @@ -7146,7 +7150,7 @@ add_option(struct options *options, { if (strstr(p[2], ":")) { - ipv6dns=true; + ipv6dns = true; foreign_option(options, p, 3, es); dhcp_option_dns6_parse(p[2], o->dns6, &o->dns6_len, msglevel); } diff --git a/src/openvpn/otime.c b/src/openvpn/otime.c index e0b1b0ee..759a7fb5 100644 --- a/src/openvpn/otime.c +++ b/src/openvpn/otime.c @@ -88,7 +88,7 @@ const char * tv_string(const struct timeval *tv, struct gc_arena *gc) { struct buffer out = alloc_buf_gc(64, gc); - buf_printf(&out, "[%"PRIi64"/%ld]", + buf_printf(&out, "[%" PRIi64 "/%ld]", (int64_t)tv->tv_sec, (long)tv->tv_usec); return BSTR(&out); @@ -198,7 +198,7 @@ time_test(void) t = time(NULL); gettimeofday(&tv, NULL); #if 1 - msg(M_INFO, "t=%"PRIi64" s=%"PRIi64" us=%ld", + msg(M_INFO, "t=%" PRIi64 " s=%" PRIi64 " us=%ld", (int64_t)t, (int64_t)tv.tv_sec, (long)tv.tv_usec); diff --git a/src/openvpn/packet_id.c b/src/openvpn/packet_id.c index dc44f36b..0c744875 100644 --- a/src/openvpn/packet_id.c +++ b/src/openvpn/packet_id.c @@ -347,7 +347,7 @@ packet_id_send_update(struct packet_id_send *p, bool long_form) bool packet_id_write(struct packet_id_send *p, struct buffer *buf, bool long_form, - bool prepend) + bool prepend) { if (!packet_id_send_update(p, long_form)) { @@ -606,13 +606,13 @@ packet_id_debug_print(int msglevel, } buf_printf(&out, "%c", c); } - buf_printf(&out, "] %"PRIi64":" packet_id_format, (int64_t)p->time, (packet_id_print_type)p->id); + buf_printf(&out, "] %" PRIi64 ":" packet_id_format, (int64_t)p->time, (packet_id_print_type)p->id); if (pin) { - buf_printf(&out, " %"PRIi64":" packet_id_format, (int64_t)pin->time, (packet_id_print_type)pin->id); + buf_printf(&out, " %" PRIi64 ":" packet_id_format, (int64_t)pin->time, (packet_id_print_type)pin->id); } - buf_printf(&out, " t=%"PRIi64"[%d]", + buf_printf(&out, " t=%" PRIi64 "[%d]", (int64_t)prev_now, (int)(prev_now - tv.tv_sec)); @@ -666,7 +666,7 @@ packet_id_interactive_test(void) { packet_id_reap_test(&pid.rec); test = packet_id_test(&pid.rec, &pin); - printf("packet_id_test (%"PRIi64", " packet_id_format ") returned %d\n", + printf("packet_id_test (%" PRIi64 ", " packet_id_format ") returned %d\n", (int64_t)pin.time, (packet_id_print_type)pin.id, test); @@ -679,7 +679,7 @@ packet_id_interactive_test(void) { long_form = (count < 20); packet_id_alloc_outgoing(&pid.send, &pin, long_form); - printf("(%"PRIi64"(" packet_id_format "), %d)\n", + printf("(%" PRIi64 "(" packet_id_format "), %d)\n", (int64_t)pin.time, (packet_id_print_type)pin.id, long_form); diff --git a/src/openvpn/packet_id.h b/src/openvpn/packet_id.h index ec03e348..26b07975 100644 --- a/src/openvpn/packet_id.h +++ b/src/openvpn/packet_id.h @@ -263,7 +263,7 @@ bool packet_id_read(struct packet_id_net *pin, struct buffer *buf, bool long_for * @return true if successful, false otherwise. */ bool packet_id_write(struct packet_id_send *p, struct buffer *buf, - bool long_form, bool prepend); + bool long_form, bool prepend); /* * Inline functions. diff --git a/src/openvpn/ping.h b/src/openvpn/ping.h index a196b32b..b51f082a 100644 --- a/src/openvpn/ping.h +++ b/src/openvpn/ping.h @@ -84,4 +84,4 @@ check_ping_send(struct context *c) } } -#endif +#endif /* ifndef PING_H */ diff --git a/src/openvpn/plugin.c b/src/openvpn/plugin.c index 4d17c821..33687d44 100644 --- a/src/openvpn/plugin.c +++ b/src/openvpn/plugin.c @@ -552,8 +552,7 @@ plugin_call_item(const struct plugin *p, p->plugin_handle, per_client_context, (current_cert ? certdepth : -1), - current_cert - }; + current_cert}; struct openvpn_plugin_args_func_return retargs; diff --git a/src/openvpn/proxy.c b/src/openvpn/proxy.c index fdad3ed5..c8bd86dd 100644 --- a/src/openvpn/proxy.c +++ b/src/openvpn/proxy.c @@ -884,10 +884,10 @@ establish_http_proxy_passthru(struct http_proxy_info *p, const char *algor = get_pa_var("algorithm", pa, &gc); const char *opaque = get_pa_var("opaque", pa, &gc); - if ( !realm || !nonce ) + if (!realm || !nonce) { msg(D_LINK_ERRORS, "HTTP proxy: digest auth failed, malformed response " - "from server: realm= or nonce= missing" ); + "from server: realm= or nonce= missing" ); goto error; } diff --git a/src/openvpn/reliable.c b/src/openvpn/reliable.c index a7f4ed96..eae1e0cb 100644 --- a/src/openvpn/reliable.c +++ b/src/openvpn/reliable.c @@ -762,14 +762,14 @@ reliable_debug_print(const struct reliable *rel, char *desc) printf("********* struct reliable %s\n", desc); printf(" initial_timeout=%d\n", (int)rel->initial_timeout); printf(" packet_id=" packet_id_format "\n", rel->packet_id); - printf(" now=%"PRIi64"\n", (int64_t)now); + printf(" now=%" PRIi64 "\n", (int64_t)now); for (i = 0; i < rel->size; ++i) { const struct reliable_entry *e = &rel->array[i]; if (e->active) { printf(" %d: packet_id=" packet_id_format " len=%d", i, e->packet_id, e->buf.len); - printf(" next_try=%"PRIi64, (int64_t)e->next_try); + printf(" next_try=%" PRIi64, (int64_t)e->next_try); printf("\n"); } } diff --git a/src/openvpn/route.c b/src/openvpn/route.c index d97e8dba..346f08e2 100644 --- a/src/openvpn/route.c +++ b/src/openvpn/route.c @@ -3074,7 +3074,7 @@ do_route_ipv6_service(const bool add, const struct route_ipv6 *r, const struct t * (only do this for routes actually using the tun/tap device) */ if (tt->type == DEV_TYPE_TUN - && msg.iface.index == tt->adapter_index ) + && msg.iface.index == tt->adapter_index) { inet_pton(AF_INET6, "fe80::8", &msg.gateway.ipv6); } @@ -3298,7 +3298,7 @@ get_default_gateway(struct route_gateway_info *rgi) if (rgi->flags & RGI_ON_LINK) { /* check that interface name of current interface - * matches interface name of best default route */ + * matches interface name of best default route */ if (strcmp(ifreq.ifr_name, best_name)) { continue; diff --git a/src/openvpn/run_command.c b/src/openvpn/run_command.c index 4e198676..04ad2312 100644 --- a/src/openvpn/run_command.c +++ b/src/openvpn/run_command.c @@ -41,12 +41,14 @@ /* contains an SSEC_x value defined in platform.h */ static int script_security_level = SSEC_BUILT_IN; /* GLOBAL */ -int script_security(void) +int +script_security(void) { return script_security_level; } -void script_security_set(int level) +void +script_security_set(int level) { script_security_level = level; } diff --git a/src/openvpn/shaper.c b/src/openvpn/shaper.c index 92364f29..62579840 100644 --- a/src/openvpn/shaper.c +++ b/src/openvpn/shaper.c @@ -76,7 +76,7 @@ shaper_soonest_event(struct timeval *tv, int delay) } } #ifdef SHAPER_DEBUG - dmsg(D_SHAPER_DEBUG, "SHAPER shaper_soonest_event sec=%"PRIi64" usec=%ld ret=%d", + dmsg(D_SHAPER_DEBUG, "SHAPER shaper_soonest_event sec=%" PRIi64 " usec=%ld ret=%d", (int64_t)tv->tv_sec, (long)tv->tv_usec, (int)ret); #endif return ret; diff --git a/src/openvpn/shaper.h b/src/openvpn/shaper.h index 4efe398a..bcdb5e36 100644 --- a/src/openvpn/shaper.h +++ b/src/openvpn/shaper.h @@ -147,7 +147,7 @@ shaper_wrote_bytes(struct shaper *s, int nbytes) tv_add(&s->wakeup, &tv); #ifdef SHAPER_DEBUG - dmsg(D_SHAPER_DEBUG, "SHAPER shaper_wrote_bytes bytes=%d delay=%ld sec=%"PRIi64" usec=%ld", + dmsg(D_SHAPER_DEBUG, "SHAPER shaper_wrote_bytes bytes=%d delay=%ld sec=%" PRIi64 " usec=%ld", nbytes, (long)tv.tv_usec, (int64_t)s->wakeup.tv_sec, diff --git a/src/openvpn/socket.c b/src/openvpn/socket.c index 3f2b97e4..db944245 100644 --- a/src/openvpn/socket.c +++ b/src/openvpn/socket.c @@ -100,10 +100,12 @@ get_addr_generic(sa_family_t af, unsigned int flags, const char *hostname, bits = 0; max_bits = sizeof(in_addr_t) * 8; break; + case AF_INET6: bits = 64; max_bits = sizeof(struct in6_addr) * 8; break; + default: msg(M_WARN, "Unsupported AF family passed to getaddrinfo for %s (%d)", @@ -125,7 +127,7 @@ get_addr_generic(sa_family_t af, unsigned int flags, const char *hostname, } /* check if this hostname has a /bits suffix */ - sep = strchr(var_host , '/'); + sep = strchr(var_host, '/'); if (sep) { bits = strtoul(sep + 1, &endp, 10); @@ -156,10 +158,12 @@ get_addr_generic(sa_family_t af, unsigned int flags, const char *hostname, *ip4 = ntohl(*ip4); } break; + case AF_INET6: ip6 = network; *ip6 = ((struct sockaddr_in6 *)ai->ai_addr)->sin6_addr; break; + default: /* can't get here because 'af' was previously checked */ msg(M_WARN, diff --git a/src/openvpn/socket.h b/src/openvpn/socket.h index 0f22d479..17801418 100644 --- a/src/openvpn/socket.h +++ b/src/openvpn/socket.h @@ -138,7 +138,7 @@ struct stream_buf int len; /* -1 if not yet known */ bool error; /* if true, fatal TCP error has occurred, - * requiring that connection be restarted */ + * requiring that connection be restarted */ #if PORT_SHARE #define PS_DISABLED 0 #define PS_ENABLED 1 diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c index 74b88ce6..d783c598 100644 --- a/src/openvpn/ssl.c +++ b/src/openvpn/ssl.c @@ -680,7 +680,7 @@ init_ssl(const struct options *options, struct tls_root_ctx *new_ctx) { if (tls_ctx_use_management_external_key(new_ctx)) { - msg (M_WARN, "Cannot initialize mamagement-external-key"); + msg(M_WARN, "Cannot initialize mamagement-external-key"); goto err; } } @@ -1099,7 +1099,7 @@ tls_session_init(struct tls_multi *multi, struct tls_session *session) else { session->initial_opcode = session->opt->tls_crypt_v2 ? - P_CONTROL_HARD_RESET_CLIENT_V3 : P_CONTROL_HARD_RESET_CLIENT_V2; + P_CONTROL_HARD_RESET_CLIENT_V3 : P_CONTROL_HARD_RESET_CLIENT_V2; } } @@ -1527,9 +1527,9 @@ read_control_auth(struct buffer *buf, if (opcode == P_CONTROL_HARD_RESET_CLIENT_V3 && !tls_crypt_v2_extract_client_key(buf, ctx, opt)) { - msg (D_TLS_ERRORS, - "TLS Error: can not extract tls-crypt-v2 client key from %s", - print_link_socket_actual(from, &gc)); + msg(D_TLS_ERRORS, + "TLS Error: can not extract tls-crypt-v2 client key from %s", + print_link_socket_actual(from, &gc)); goto cleanup; } @@ -3781,7 +3781,7 @@ tls_pre_decrypt(struct tls_multi *multi, /* Save incoming ciphertext packet to reliable buffer */ struct buffer *in = reliable_get_buf(ks->rec_reliable); ASSERT(in); - if(!buf_copy(in, buf)) + if (!buf_copy(in, buf)) { msg(D_MULTI_DROPPED, "Incoming control channel packet too big, dropping."); @@ -4182,10 +4182,10 @@ show_available_tls_ciphers(const char *cipher_list, show_available_tls_ciphers_list(cipher_list, tls_cert_profile, false); printf("\n" - "Be aware that that whether a cipher suite in this list can actually work\n" - "depends on the specific setup of both peers. See the man page entries of\n" - "--tls-cipher and --show-tls for more details.\n\n" - ); + "Be aware that that whether a cipher suite in this list can actually work\n" + "depends on the specific setup of both peers. See the man page entries of\n" + "--tls-cipher and --show-tls for more details.\n\n" + ); } /* diff --git a/src/openvpn/ssl_common.h b/src/openvpn/ssl_common.h index 7bf82b3a..410b2163 100644 --- a/src/openvpn/ssl_common.h +++ b/src/openvpn/ssl_common.h @@ -215,10 +215,10 @@ struct tls_wrap_ctx struct buffer work; /**< Work buffer (only for --tls-crypt) */ struct key_ctx tls_crypt_v2_server_key; /**< Decrypts client keys */ const struct buffer *tls_crypt_v2_wkc; /**< Wrapped client key, - sent to server */ + * sent to server */ struct buffer tls_crypt_v2_metadata; /**< Received from client */ bool cleanup_key_ctx; /**< opt.key_ctx_bi is owned by - this context */ + * this context */ }; /* diff --git a/src/openvpn/ssl_mbedtls.c b/src/openvpn/ssl_mbedtls.c index f7e8c2d0..a4197cba 100644 --- a/src/openvpn/ssl_mbedtls.c +++ b/src/openvpn/ssl_mbedtls.c @@ -65,12 +65,12 @@ static const mbedtls_x509_crt_profile openvpn_x509_crt_profile_legacy = { /* Hashes from SHA-1 and above */ - MBEDTLS_X509_ID_FLAG( MBEDTLS_MD_SHA1 ) | - MBEDTLS_X509_ID_FLAG( MBEDTLS_MD_RIPEMD160 ) | - MBEDTLS_X509_ID_FLAG( MBEDTLS_MD_SHA224 ) | - MBEDTLS_X509_ID_FLAG( MBEDTLS_MD_SHA256 ) | - MBEDTLS_X509_ID_FLAG( MBEDTLS_MD_SHA384 ) | - MBEDTLS_X509_ID_FLAG( MBEDTLS_MD_SHA512 ), + MBEDTLS_X509_ID_FLAG( MBEDTLS_MD_SHA1 ) + |MBEDTLS_X509_ID_FLAG( MBEDTLS_MD_RIPEMD160 ) + |MBEDTLS_X509_ID_FLAG( MBEDTLS_MD_SHA224 ) + |MBEDTLS_X509_ID_FLAG( MBEDTLS_MD_SHA256 ) + |MBEDTLS_X509_ID_FLAG( MBEDTLS_MD_SHA384 ) + |MBEDTLS_X509_ID_FLAG( MBEDTLS_MD_SHA512 ), 0xFFFFFFF, /* Any PK alg */ 0xFFFFFFF, /* Any curve */ 1024, /* RSA-1024 and larger */ @@ -79,10 +79,10 @@ static const mbedtls_x509_crt_profile openvpn_x509_crt_profile_legacy = static const mbedtls_x509_crt_profile openvpn_x509_crt_profile_preferred = { /* SHA-2 and above */ - MBEDTLS_X509_ID_FLAG( MBEDTLS_MD_SHA224 ) | - MBEDTLS_X509_ID_FLAG( MBEDTLS_MD_SHA256 ) | - MBEDTLS_X509_ID_FLAG( MBEDTLS_MD_SHA384 ) | - MBEDTLS_X509_ID_FLAG( MBEDTLS_MD_SHA512 ), + MBEDTLS_X509_ID_FLAG( MBEDTLS_MD_SHA224 ) + |MBEDTLS_X509_ID_FLAG( MBEDTLS_MD_SHA256 ) + |MBEDTLS_X509_ID_FLAG( MBEDTLS_MD_SHA384 ) + |MBEDTLS_X509_ID_FLAG( MBEDTLS_MD_SHA512 ), 0xFFFFFFF, /* Any PK alg */ 0xFFFFFFF, /* Any curve */ 2048, /* RSA-2048 and larger */ @@ -232,7 +232,7 @@ tls_ctx_restrict_ciphers_tls13(struct tls_root_ctx *ctx, const char *ciphers) } msg(M_WARN, "mbed TLS does not support setting tls-ciphersuites. " - "Ignoring TLS 1.3 cipher list: %s", ciphers); + "Ignoring TLS 1.3 cipher list: %s", ciphers); } void @@ -299,7 +299,7 @@ tls_ctx_set_cert_profile(struct tls_root_ctx *ctx, const char *profile) } else { - msg (M_FATAL, "ERROR: Invalid cert profile: %s", profile); + msg(M_FATAL, "ERROR: Invalid cert profile: %s", profile); } } @@ -600,14 +600,14 @@ tls_ctx_use_external_signing_func(struct tls_root_ctx *ctx, if (ctx->crt_chain == NULL) { - msg (M_WARN, "ERROR: external key requires a certificate."); + msg(M_WARN, "ERROR: external key requires a certificate."); return 1; } if (mbedtls_pk_get_type(&ctx->crt_chain->pk) != MBEDTLS_PK_RSA) { msg(M_WARN, "ERROR: external key with mbed TLS requires a " - "certificate with an RSA key."); + "certificate with an RSA key."); return 1; } @@ -653,8 +653,8 @@ management_sign_func(void *sign_ctx, const void *src, size_t src_len, ret = true; cleanup: - free (src_b64); - free (dst_b64); + free(src_b64); + free(dst_b64); return ret; } diff --git a/src/openvpn/ssl_mbedtls.h b/src/openvpn/ssl_mbedtls.h index 998d6f2f..1328ceb7 100644 --- a/src/openvpn/ssl_mbedtls.h +++ b/src/openvpn/ssl_mbedtls.h @@ -71,8 +71,8 @@ typedef struct { * @return true if signing succeeded, false otherwise. */ typedef bool (*external_sign_func)( - void *sign_ctx, const void *src, size_t src_size, - void *dst, size_t dst_size); + void *sign_ctx, const void *src, size_t src_size, + void *dst, size_t dst_size); /** Context used by external_pkcs1_sign() */ struct external_context { diff --git a/src/openvpn/ssl_openssl.c b/src/openvpn/ssl_openssl.c index da573cfa..ddb78da7 100644 --- a/src/openvpn/ssl_openssl.c +++ b/src/openvpn/ssl_openssl.c @@ -217,7 +217,7 @@ tls_version_max(void) return TLS_VER_1_2; #elif defined(TLS1_1_VERSION) || defined(SSL_OP_NO_TLSv1_1) return TLS_VER_1_1; -#else +#else /* if defined(TLS1_3_VERSION) */ return TLS_VER_1_0; #endif } @@ -322,7 +322,7 @@ tls_ctx_set_options(struct tls_root_ctx *ctx, unsigned int ssl_flags) } void -convert_tls_list_to_openssl(char* openssl_ciphers, size_t len,const char *ciphers) +convert_tls_list_to_openssl(char *openssl_ciphers, size_t len,const char *ciphers) { /* Parse supplied cipher list and pass on to OpenSSL */ size_t begin_of_cipher, end_of_cipher; @@ -466,9 +466,9 @@ tls_ctx_restrict_ciphers_tls13(struct tls_root_ctx *ctx, const char *ciphers) } #if (OPENSSL_VERSION_NUMBER < 0x1010100fL) - crypto_msg(M_WARN, "Not compiled with OpenSSL 1.1.1 or higher. " - "Ignoring TLS 1.3 only tls-ciphersuites '%s' setting.", - ciphers); + crypto_msg(M_WARN, "Not compiled with OpenSSL 1.1.1 or higher. " + "Ignoring TLS 1.3 only tls-ciphersuites '%s' setting.", + ciphers); #else ASSERT(NULL != ctx); @@ -509,13 +509,13 @@ tls_ctx_set_cert_profile(struct tls_root_ctx *ctx, const char *profile) { msg(M_FATAL, "ERROR: Invalid cert profile: %s", profile); } -#else +#else /* ifdef HAVE_SSL_CTX_SET_SECURITY_LEVEL */ if (profile) { msg(M_WARN, "WARNING: OpenSSL 1.0.1 does not support --tls-cert-profile" ", ignoring user-set profile: '%s'", profile); } -#endif +#endif /* ifdef HAVE_SSL_CTX_SET_SECURITY_LEVEL */ } void @@ -658,7 +658,7 @@ tls_ctx_load_ecdh_params(struct tls_root_ctx *ctx, const char *curve_name { nid = EC_GROUP_get_curve_name(ecgrp); } -#endif +#endif /* if OPENSSL_VERSION_NUMBER >= 0x10002000L */ } /* Translate NID back to name , just for kicks */ @@ -1137,7 +1137,7 @@ rsa_priv_enc(int flen, const unsigned char *from, unsigned char *to, RSA *rsa, i ret = get_sig_from_man(from, flen, to, len); - return (ret == len)? ret : -1; + return (ret == len) ? ret : -1; } static int @@ -1325,7 +1325,7 @@ err: { EVP_PKEY_free(privkey); } - if(ec) + if (ec) { EC_KEY_free(ec); } @@ -1375,7 +1375,7 @@ tls_ctx_use_management_external_key(struct tls_root_ctx *ctx) crypto_msg(M_WARN, "management-external-key requires an RSA or EC certificate"); goto cleanup; } -#else +#else /* if OPENSSL_VERSION_NUMBER > 0x10100000L && !defined(OPENSSL_NO_EC) && !defined(LIBRESSL_VERSION_NUMBER) */ else { crypto_msg(M_WARN, "management-external-key requires an RSA certificate"); @@ -1644,7 +1644,7 @@ bio_debug_data(const char *mode, BIO *bio, const uint8_t *buf, int len, const ch if (len > 0) { open_biofp(); - fprintf(biofp, "BIO_%s %s time=%"PRIi64" bio=" ptr_format " len=%d data=%s\n", + fprintf(biofp, "BIO_%s %s time=%" PRIi64 " bio=" ptr_format " len=%d data=%s\n", mode, desc, (int64_t)time(NULL), (ptr_type)bio, len, format_hex(buf, len, 0, &gc)); fflush(biofp); } @@ -1655,7 +1655,7 @@ static void bio_debug_oc(const char *mode, BIO *bio) { open_biofp(); - fprintf(biofp, "BIO %s time=%"PRIi64" bio=" ptr_format "\n", + fprintf(biofp, "BIO %s time=%" PRIi64 " bio=" ptr_format "\n", mode, (int64_t)time(NULL), (ptr_type)bio); fflush(biofp); } @@ -1963,7 +1963,7 @@ print_details(struct key_state_ssl *ks_ssl, const char *prefix) { EC_KEY *ec = EVP_PKEY_get0_EC_KEY(pkey); const EC_GROUP *group = EC_KEY_get0_group(ec); - const char* curve; + const char *curve; int nid = EC_GROUP_get_curve_name(group); if (nid == 0 || (curve = OBJ_nid2sn(nid)) == NULL) @@ -2024,7 +2024,7 @@ show_available_tls_ciphers_list(const char *cipher_list, #else STACK_OF(SSL_CIPHER) *sk = SSL_get1_supported_ciphers(ssl); #endif - for (int i=0;i < sk_SSL_CIPHER_num(sk);i++) + for (int i = 0; i < sk_SSL_CIPHER_num(sk); i++) { const SSL_CIPHER *c = sk_SSL_CIPHER_value(sk, i); @@ -2035,7 +2035,7 @@ show_available_tls_ciphers_list(const char *cipher_list, if (tls13) { - printf("%s\n", cipher_name); + printf("%s\n", cipher_name); } else if (NULL == pair) { diff --git a/src/openvpn/ssl_verify.c b/src/openvpn/ssl_verify.c index 03c0b661..a7f51751 100644 --- a/src/openvpn/ssl_verify.c +++ b/src/openvpn/ssl_verify.c @@ -72,7 +72,7 @@ setenv_untrusted(struct tls_session *session) static void wipe_auth_token(struct tls_multi *multi) { - if(multi) + if (multi) { if (multi->auth_token) { @@ -712,24 +712,24 @@ verify_cert(struct tls_session *session, openvpn_x509_cert_t *cert, int cert_dep switch (opt->verify_hash_algo) { - case MD_SHA1: - ca_hash = x509_get_sha1_fingerprint(cert, &gc); - break; - - case MD_SHA256: - ca_hash = x509_get_sha256_fingerprint(cert, &gc); - break; - - default: - /* This should normally not happen at all; the algorithm used - * is parsed by add_option() [options.c] and set to a predefined - * value in an enumerated type. So if this unlikely scenario - * happens, consider this a failure - */ - msg(M_WARN, "Unexpected invalid algorithm used with " - "--verify-hash (%i)", opt->verify_hash_algo); - ret = FAILURE; - goto cleanup; + case MD_SHA1: + ca_hash = x509_get_sha1_fingerprint(cert, &gc); + break; + + case MD_SHA256: + ca_hash = x509_get_sha256_fingerprint(cert, &gc); + break; + + default: + /* This should normally not happen at all; the algorithm used + * is parsed by add_option() [options.c] and set to a predefined + * value in an enumerated type. So if this unlikely scenario + * happens, consider this a failure + */ + msg(M_WARN, "Unexpected invalid algorithm used with " + "--verify-hash (%i)", opt->verify_hash_algo); + ret = FAILURE; + goto cleanup; } if (memcmp(BPTR(&ca_hash), opt->verify_hash, BLEN(&ca_hash))) @@ -1178,8 +1178,8 @@ verify_user_pass_plugin(struct tls_session *session, const struct user_pass *up) /* generate filename for deferred auth control file */ if (!key_state_gen_auth_control_file(ks, session->opt)) { - msg (D_TLS_ERRORS, "TLS Auth Error (%s): " - "could not create deferred auth control file", __func__); + msg(D_TLS_ERRORS, "TLS Auth Error (%s): " + "could not create deferred auth control file", __func__); goto cleanup; } #endif diff --git a/src/openvpn/ssl_verify_backend.h b/src/openvpn/ssl_verify_backend.h index 2a9e8bb7..f4cc2c54 100644 --- a/src/openvpn/ssl_verify_backend.h +++ b/src/openvpn/ssl_verify_backend.h @@ -130,6 +130,7 @@ result_t backend_x509_get_username(char *common_name, int cn_len, * --x509-username-field option. */ bool x509_user... [truncated message content]

this is really just whitespace changes, but will make running uncrustify as pre-commit-check easier if the "base sources" won't see changes Signed-off-by: Gert Doering <ge...@gr...> --- src/openvpnserv/interactive.c | 4 ++-- src/openvpnserv/service.c | 4 ++-- src/openvpnserv/service.h | 2 ++ src/openvpnserv/validate.c | 3 ++- 4 files changed, 8 insertions(+), 5 deletions(-) diff --git a/src/openvpnserv/interactive.c b/src/openvpnserv/interactive.c index 2dc152c2..623c3ff7 100644 --- a/src/openvpnserv/interactive.c +++ b/src/openvpnserv/interactive.c @@ -514,7 +514,7 @@ GetStartupData(HANDLE pipe, STARTUP_DATA *sud) return TRUE; err: - sud->directory = NULL;	/* caller must not free() */ + sud->directory = NULL; /* caller must not free() */ free(data); return FALSE; } @@ -1318,7 +1318,7 @@ Undo(undo_lists_t *lists) break; case block_dns: - interface_data = (block_dns_data_t*)(item->data); + interface_data = (block_dns_data_t *)(item->data); delete_block_dns_filters(interface_data->engine); if (interface_data->metric_v4 >= 0) { diff --git a/src/openvpnserv/service.c b/src/openvpnserv/service.c index 7157bea2..8efe25f9 100644 --- a/src/openvpnserv/service.c +++ b/src/openvpnserv/service.c @@ -270,8 +270,8 @@ _tmain(int argc, TCHAR *argv[]) else if (argc > i + 2 && _tcsicmp(TEXT("instance"), argv[i] + 1) == 0) { dispatchTable = _tcsicmp(TEXT("interactive"), argv[i + 1]) != 0 ? - dispatchTable_automatic : - dispatchTable_interactive; + dispatchTable_automatic : + dispatchTable_interactive; service_instance = argv[i + 2]; i += 2; diff --git a/src/openvpnserv/service.h b/src/openvpnserv/service.h index cb7020fc..f5afe2f0 100644 --- a/src/openvpnserv/service.h +++ b/src/openvpnserv/service.h @@ -77,9 +77,11 @@ extern openvpn_service_t interactive_service; extern LPCTSTR service_instance; VOID WINAPI ServiceStartAutomaticOwn(DWORD argc, LPTSTR *argv); + VOID WINAPI ServiceStartAutomatic(DWORD argc, LPTSTR *argv); VOID WINAPI ServiceStartInteractiveOwn(DWORD argc, LPTSTR *argv); + VOID WINAPI ServiceStartInteractive(DWORD argc, LPTSTR *argv); BOOL openvpn_vsntprintf(LPTSTR str, size_t size, LPCTSTR format, va_list arglist); diff --git a/src/openvpnserv/validate.c b/src/openvpnserv/validate.c index 5506e902..9e1d7d2d 100644 --- a/src/openvpnserv/validate.c +++ b/src/openvpnserv/validate.c @@ -49,6 +49,7 @@ static const WCHAR *white_list[] = }; static BOOL IsUserInGroup(PSID sid, const PTOKEN_GROUPS groups, const WCHAR *group_name); + static PTOKEN_GROUPS GetTokenGroups(const HANDLE token); /* @@ -305,7 +306,7 @@ IsUserInGroup(PSID sid, const PTOKEN_GROUPS token_groups, const WCHAR *group_nam ret = EqualSid(members[i].lgrmi0_sid, sid); } NetApiBufferFree(members); - /* MSDN says the lookup should always iterate until err != ERROR_MORE_DATA */ + /* MSDN says the lookup should always iterate until err != ERROR_MORE_DATA */ } while (err == ERROR_MORE_DATA && nloop++ < 100); if (err != NERR_Success && err != NERR_GroupNotFound) -- 2.19.2 

Signed-off-by: Gert Doering <ge...@gr...> --- .../keying-material-exporter-demo/keyingmaterialexporter.c | 6 ++++-- sample/sample-plugins/log/log.c | 4 ++++ sample/sample-plugins/log/log_v3.c | 4 ++++ 3 files changed, 12 insertions(+), 2 deletions(-) diff --git a/sample/sample-plugins/keying-material-exporter-demo/keyingmaterialexporter.c b/sample/sample-plugins/keying-material-exporter-demo/keyingmaterialexporter.c index 9986461b..b53f13f6 100644 --- a/sample/sample-plugins/keying-material-exporter-demo/keyingmaterialexporter.c +++ b/sample/sample-plugins/keying-material-exporter-demo/keyingmaterialexporter.c @@ -230,7 +230,8 @@ tls_final(struct openvpn_plugin_args_func_in const *args, snprintf(sess->key, sizeof(sess->key) - 1, "%s", key); ovpn_note("app session key: %s", sess->key); - switch (plugin->type) { + switch (plugin->type) + { case SERVER: server_store(args); break; @@ -249,7 +250,8 @@ openvpn_plugin_func_v3(const int version, struct openvpn_plugin_args_func_in const *args, struct openvpn_plugin_args_func_return *rv) { - switch (args->type) { + switch (args->type) + { case OPENVPN_PLUGIN_TLS_VERIFY: return tls_verify(args); diff --git a/sample/sample-plugins/log/log.c b/sample/sample-plugins/log/log.c index ecf62c0e..b5c1c3be 100644 --- a/sample/sample-plugins/log/log.c +++ b/sample/sample-plugins/log/log.c @@ -156,11 +156,15 @@ show(const int type, const char *argv[], const char *envp[]) printf("ARGV\n"); for (i = 0; argv[i] != NULL; ++i) + { printf("%d '%s'\n", (int)i, argv[i]); + } printf("ENVP\n"); for (i = 0; envp[i] != NULL; ++i) + { printf("%d '%s'\n", (int)i, envp[i]); + } } OPENVPN_EXPORT int diff --git a/sample/sample-plugins/log/log_v3.c b/sample/sample-plugins/log/log_v3.c index 3ef4a1cb..17b83f3f 100644 --- a/sample/sample-plugins/log/log_v3.c +++ b/sample/sample-plugins/log/log_v3.c @@ -177,11 +177,15 @@ show(const int type, const char *argv[], const char *envp[]) printf("ARGV\n"); for (i = 0; argv[i] != NULL; ++i) + { printf("%d '%s'\n", (int)i, argv[i]); + } printf("ENVP\n"); for (i = 0; envp[i] != NULL; ++i) + { printf("%d '%s'\n", (int)i, envp[i]); + } } static void -- 2.19.2 

> Hi Jan Just, > > (forgot to add openvpn-devel in previous mail) > > Some background information. > > In openvpn3 we decided not to implement fragments, because: > > - this is quite a big feature which has to be supported through the whole > stack (client, server, kernel module) > - we assume that it is not used by most of users > > However, for those who needs --fragment, we'll implement: > > - mssfix support, this should solve problems with tcp-based protocols > - sending icmp "packet too big" for other protocols, we assume that > they'll respect icmp response > > --fragment is/was very useful on a system where you do not / cannot change >> the tun MTU size. Up to Windows XP, this was not >> possible without rebooting the OS. Since Vista, it *is* possible to >> change >> the MTU of an adapter on the fly (as explained in my >> trusty old cookbook, of course ;)) >> > > As James wrote a while ago (13 years ago :) > > https://openvpn.net/archive/openvpn-users/2005-10/msg00354.html > >> A lot of experience gained during the OpenVPN 1.x releases showed that > it's best to fix the tunnel MTU at >> 1500 and vary the other parameters (and use --mssfix to prevent > fragmentation rather than a lower tunnel MTU). > > Don't know if still holds. Assuming that we can change tun-mtu on any > supported platforms, do you think that it is better > to do _that_, rather than have mssfix/icmp ptb workaround? > > >> With that, it would be possible to fix the link-mtu to 1500 (or >> whatever is set on the outbound interface) and then subtract the header >> size to get a meaningful tun-mtu size. >> > > Do you think 1500 is a safe value? Arne just mentioned today that his > network interface MTU is 1500 and router's MTU is 1492 due to > PPPoE, and openvpn2 works because it assumes that mtu is 1450. I also think 1500 isn't safe because different types ob internet connections may reduce it, like the mentioned PPPoE. Regards, Simon 

Hi Jan Just, (forgot to add openvpn-devel in previous mail) Some background information. In openvpn3 we decided not to implement fragments, because: - this is quite a big feature which has to be supported through the whole stack (client, server, kernel module) - we assume that it is not used by most of users However, for those who needs --fragment, we'll implement: - mssfix support, this should solve problems with tcp-based protocols - sending icmp "packet too big" for other protocols, we assume that they'll respect icmp response --fragment is/was very useful on a system where you do not / cannot change > the tun MTU size. Up to Windows XP, this was not > possible without rebooting the OS. Since Vista, it *is* possible to change > the MTU of an adapter on the fly (as explained in my > trusty old cookbook, of course ;)) > As James wrote a while ago (13 years ago :) https://openvpn.net/archive/openvpn-users/2005-10/msg00354.html > A lot of experience gained during the OpenVPN 1.x releases showed that it's best to fix the tunnel MTU at > 1500 and vary the other parameters (and use --mssfix to prevent fragmentation rather than a lower tunnel MTU). Don't know if still holds. Assuming that we can change tun-mtu on any supported platforms, do you think that it is better to do _that_, rather than have mssfix/icmp ptb workaround? > With that, it would be possible to fix the link-mtu to 1500 (or > whatever is set on the outbound interface) and then subtract the header > size to get a meaningful tun-mtu size. > Do you think 1500 is a safe value? Arne just mentioned today that his network interface MTU is 1500 and router's MTU is 1492 due to PPPoE, and openvpn2 works because it assumes that mtu is 1450. > Having said all that, I do have one gripe with the way the link or tun mtu > is calculated in the current codebase: > In the 2.3 code base, the server tun mtu size was equal to the client > tun-mtu size. > In the 2.4+ code base, the server-side tun mtu size is set to some > ridiculous low level (with link-mtu 1500 set) in order to > accomocate all possible encryption ciphers - *even if I add ncp-disable* . > To me, that does not make a whole lot of sense. If I > add '--ncp-disable' I'd expect the client and the server to behave pretty > much like an OpenVPN 2.3 client (with a possible 3 > bytes difference to accomodate for the peer id). > With 2.4 codebase I noticed that what man page says is not what openvpn2 code does - it does include transport overhead, while man page says it doesn't (copying my mail here): Announce to TCP sessions running over the tunnel that they should limit their send packet sizes such that after OpenVPN has encapsulated them, the resulting UDP packet size that OpenVPN sends to its peer will not exceed max bytes. The default value is 1450. The max parameter is interpreted in the same way as the --link-mtu parameter, i.e. the UDP packet size after encapsulation overhead has been added in, but not including the UDP header itself. Resulting packet would be at most 28 bytes larger for IPv4 and 48 bytes for IPv6 (20/40 bytes for IP header and 8 bytes for UDP header). So it means that with "mssfix" 1300 resulting IPv4 packet size would be at most 1328. This is what I see in Wireshark (server - git master, client 2.4.6): Internet Protocol Version 4, Src: 128.199.xxx.yyy, Dst: 10.0.200.20 0100 .... = Version: 4 .... 0101 = Header Length: 20 bytes (5) Total Length: 1300 Protocol: UDP (17) User Datagram Protocol, Src Port: 1194, Dst Port: 1194 Source Port: 1194 Destination Port: 1194 Length: 1280 OpenVPN Protocol Type: 0x49 [opcode/key_id] Peer ID: 0 Data (1268 bytes) While man page statement is technically correct - UDP packet size is 1300, which is "at most 1328", I think it should say: > the resulting IP packet size that OpenVPN sends to its peer will not exceed max bytes and > The max parameter is interpreted in the same way as the --link-mtu parameter, i.e. the IP packet size after encapsulation overhead has been added in, including UDP and IP headers. -Lev 

Hi, On 29/11/18 09:03, Samuli Seppänen wrote: > [...] > Had a discussion about --fragment. Agreed that if we can fix internal > fragmentation without needing a change in frame format then we can > definitely deprecate --fragment in the long-term. Also noted that lack > of tun-mtu support on Windows might be for historic reasons and might > not apply to tap-windows6. Also questioned whether the default MTU of > 1500 is the best possible choice. > I wish I'd known about this discussion - I saw the topic list and figured I wouldn't be able to contribute much to the discussion. Regarding MTU/fragment however..... --fragment is/was very useful on a system where you do not / cannot change the tun MTU size. Up to Windows XP, this was not possible without rebooting the OS. Since Vista, it *is* possible to change the MTU of an adapter on the fly (as explained in my trusty old cookbook, of course ;)) --fragment is also quite useful in combination with compression, to ensure that the payload never exceeded a particular size *after* compression+encryption+encapsulation. Now that Windows XP and below are gone, and now that compression+encryption is considered evil (voracle) it might be possible to better calculate/predict the maximum encrypted payload size. With that, it would be possible to fix the link-mtu to 1500 (or whatever is set on the outbound interface) and then subtract the header size to get a meaningful tun-mtu size. As for mtu=1500 being the best possible choice: that depends if you are doing routing on your VPN or not. 95+ % of all Ethernet LANs have the MTU set to 1500 - increasing the tun MTU to 9000 does not increase performance, as forwarded packets are coming in from the LAN at 1500 bytes. If we can come up with an efficient method to gather a bunch of packets before processing them inside OpenVPN then I am all for increasing the MTU to something higher (why not go for 65000?). However, until we can do that increasing the tun mtu size will do little to improve (routing/forwarding) performance. Having said all that, I do have one gripe with the way the link or tun mtu is calculated in the current codebase: In the 2.3 code base, the server tun mtu size was equal to the client tun-mtu size. In the 2.4+ code base, the server-side tun mtu size is set to some ridiculous low level (with link-mtu 1500 set) in order to accomocate all possible encryption ciphers - *even if I add ncp-disable* . To me, that does not make a whole lot of sense. If I add '--ncp-disable' I'd expect the client and the server to behave pretty much like an OpenVPN 2.3 client (with a possible 3 bytes difference to accomodate for the peer id). JM2CW, JJK 

Hi, Here's the summary of the IRC meeting. --- COMMUNITY MEETING Place: #openvpn-meeting on irc.freenode.net Date: Wednesday 28th November 2018 Time: 11:30 CET (10:30 UTC) Planned meeting topics for this meeting were here: <https://community.openvpn.net/openvpn/wiki/Topics-2018-11-28> The next meeting has not been scheduled yet. Your local meeting time is easy to check from services such as <http://www.timeanddate.com/worldclock> SUMMARY cron2, dazo, ordex, plaisthos and rozmansi participated in this meeting. -- Talked about tap-windows6 HLK testing. Stephen Stair is working on it, and it looks promising, though progress is a bit slow due to other projects getting in the way. -- Discussed OSTIF's proposal to security audit the changes between OpenVPN 2.4.0 and 2.5.0. Agreed that auditing the new code paths would be beneficial. In particular these paths: - tls-crypt v2 - vlan patches - Refactored ifconfig handling Agreed that the audit should target OpenVPN 2.5.0, i.e. the already stabilized codebase. -- Had a discussion about --fragment. Agreed that if we can fix internal fragmentation without needing a change in frame format then we can definitely deprecate --fragment in the long-term. Also noted that lack of tun-mtu support on Windows might be for historic reasons and might not apply to tap-windows6. Also questioned whether the default MTU of 1500 is the best possible choice. -- Full chatlog attached. -- Samuli Seppänen Community Manager OpenVPN Technologies, Inc irc freenode net: mattock 

Hi, We're going to have an IRC meeting starting at 11:30 CET (10:30 UTC) on #openvpn-meeting <at> irc.freenode.net. You do not have to be logged in to Freenode to join the channel. Current topic list along with basic information is here: <https://community.openvpn.net/openvpn/wiki/Topics-2018-11-28> If you have any other things you'd like to bring up, respond to this mail, send me mail privately or add them to the list yourself. In case you can't attend the meeting, please feel free to make comments on the topics by responding to this email or to the summary email sent after the meeting. Whenever possible, we'll also respond to existing, related email threads. -- Samuli Seppänen Community Manager OpenVPN Technologies, Inc irc freenode net: mattock 

On 27/11/2018 02:04, Antonio Quartulli wrote: > [...snip...] >>> create mode 100644 src/openvpn/networking_ip.h >>> >>> diff --git a/src/openvpn/Makefile.am b/src/openvpn/Makefile.am >>> index 8afc4146..3caad17d 100644 >>> --- a/src/openvpn/Makefile.am >>> +++ b/src/openvpn/Makefile.am >>> @@ -80,7 +80,7 @@ openvpn_SOURCES = \ >>>	mtu.c mtu.h \ >>>	mudp.c mudp.h \ >>>	multi.c multi.h \ >>> -	networking.h \ >>> +	networking_ip.c networking_ip.h networking.h \ >> >> As ip is not really but a linux specific tool, I think naming this >> networking_linuxip.c is better as networking_ip sounds like something >> generic. > > I did not want to make any assumption - you never know if iproute will > later be ported to *BSD as well. But I am fine with either name. Just adding more paint to the shed :-P networking_ip.[ch] sounds a bit too generic, but I'd prefer networking_iproute2.[ch], which I consider more consistent to what these files implement. Otherwise, both Arne is right (iproute2 is currently only widely used on Linux) and Antonio is perhaps right to (iproute2 might be ported in the future). [...snip...] >>> +int >>> +net_addr_v4_add(openvpn_net_ctx_t *ctx, const char *iface, >>> + const in_addr_t *addr, int prefixlen, >>> + const in_addr_t *broadcast) >>> +{ >>> + struct argv argv = argv_new(); >>> + >>> + char *addr_str = (char *)print_in_addr_t(*addr, 0, NULL); >>> + char *brd_str = (char *)print_in_addr_t(*broadcast, 0, NULL); >>> + >>> + argv_printf(&argv, "%s addr add dev %s %s/%d broadcast %s", iproute_path, >>> + iface, addr_str, prefixlen, brd_str); >>> + argv_msg(M_INFO, &argv); >>> + openvpn_execve_check(&argv, ctx->es, S_FATAL, "Linux ip addr add failed"); >>> + >>> + free(addr_str); >>> + free(brd_str); >>> + >>> + argv_reset(&argv); >>> + >>> + return 0; >>> +} >>> + >> >> I initially written a comment that this method removes the ptp variant >> of the method but later saw that there is also a ptp variant of the >> method. I would suggest to either ranme this method to >> net_addr_bcast_v4_add or have a bool ptp paramter and then call out to >> two different methods. > > To me it feels like the PtP case is a special case, therefore I > preferred to keep the "standard & simple" name for the non-PtP case and > then add a special function just for PtP. > > Actually this function may even be called without a broadcast address. > > Mh..dunno to be honest. Maybe somebody else has an opinion? I agree with Antonio. It's not just the broadcast which is different, the ptp variant uses 'peer x.x.x.x' too. It is a different configuration type, so this makes it clearer to me when reading: net_addr_ptp_v4_addr(ctx, iface, local, remote); Instead of: net_addr_v4_add(ctx, iface, local, 0, remote , true); Of course, the 'true' could be a variable, but I don't see the real benefit of making net_addr_v4_add() implemented wit different modes. Especially when the "broadcast" and "peer" passing would most likely go via a shared variable. And it wouldn't make it cleared if peer and remote was separate arguments either; as only one of them can be present at the same time. So from code clarity, I like the distinction via the function names instead. It is more straight to the point for me. -- kind regards, David Sommerseth OpenVPN Inc 

Hi, On 26/11/2018 23:14, Arne Schwabe wrote: > Am 11.10.18 um 20:41 schrieb Antonio Quartulli: >> This patch introduces a tiny netlink interface, optimized >> for the openvpn use case. >> >> It basically exposes all those operations that are currently >> handled by directly calling the /sbin/ip command (or even >> ifconfig/route, if configured). >> >> By using netlink, openvpn won't need to spawn new processes >> when configuring the tun interface or routes. >> This new approach will also allow openvpn to be granted >> CAP_NET_ADMIN and be able to properly work even though it >> dropped the root privileges (currently handled via workarounds). >> >> By moving this logic into the sitnl module, tun.c and route.c >> also benefit from some code simplification > > Just FYI for the rest. The current code still has a bug handling ipv6 > and I am waiting for V3 that does not have this bug. > > So this is not a full review > >> +typedef union { >> + in_addr_t ipv4; >> + struct in6_addr ipv6; >> +} inet_address_t; >> + > > > >> +/** >> + * Link state request message >> + */ >> +struct sitnl_link_req { >> + struct nlmsghdr n; > > I think one two sentences that netlink requires messages in a certain > format but has no standard header that defines them so we need to define > the message ourselves would be good here. Ok, I can add some comments - doc is always good :) > >> + >> +/* if (((int)nladdr.nl_pid != peer) || (h->nlmsg_pid != nladdr.nl_pid) >> + || (h->nlmsg_seq != seq)) >> + { >> + rcv_len -= NLMSG_ALIGN(len); >> + h = (struct nlmsghdr *)((char *)h + NLMSG_ALIGN(len)); >> + msg(M_DEBUG, "%s: skipping unrelated message. nl_pid:%d (peer:%d) nl_msg_pid:%d nl_seq:%d seq:%d", >> + __func__, (int)nladdr.nl_pid, peer, h->nlmsg_pid, >> + h->nlmsg_seq, seq); >> + continue; >> + } >> +*/ > > The "normal" style for these debug commented out parts is using #ifdef > instead of comments. Right. I'll check if this is really useful - if not, I will just remove it. > > >> +typedef struct { >> + int addr_size; >> + inet_address_t gw; >> + char iface[IFNAMSIZ]; >> +} route_res_t; >> + > > This struct is in the middle of the file and I think it should either go > along with the other struct definition in the top of the file or all > structs should be defined before the function that they are used in first. > Right! Consistency is a good thing :) I'll move it to the top. > >> +/* used by iproute2 implementation too */ > > I would not add that comment. Where a function is used can be inferred > pretty easily and we don't have such comments elsewhere. And I fear this > comment might be overlooked when iproute2 is remove in 2.6/2.7 and then > more confusing than helpful. Agreed. Will remove the comment. Thanks! Regards, -- Antonio Quartulli 

 On 26/11/2018 22:44, Arne Schwabe wrote: > Am 11.10.18 um 20:41 schrieb Antonio Quartulli: >> iproute2 is the first user of the new networking API and >> its one of the two currently supported functionalities on >> Linux (the other being net-tools). >> >> This patch simply copies the current code from tun.c/route.c >> to networking_ip.c without introducing any funcional >> change to the code. >> > > I skipped the first patch and added notes about the proposed API in this > patch. > >> create mode 100644 src/openvpn/networking_ip.h >> >> diff --git a/src/openvpn/Makefile.am b/src/openvpn/Makefile.am >> index 8afc4146..3caad17d 100644 >> --- a/src/openvpn/Makefile.am >> +++ b/src/openvpn/Makefile.am >> @@ -80,7 +80,7 @@ openvpn_SOURCES = \ >>	mtu.c mtu.h \ >>	mudp.c mudp.h \ >>	multi.c multi.h \ >> -	networking.h \ >> +	networking_ip.c networking_ip.h networking.h \ > > As ip is not really but a linux specific tool, I think naming this > networking_linuxip.c is better as networking_ip sounds like something > generic. I did not want to make any assumption - you never know if iproute will later be ported to *BSD as well. But I am fine with either name. > > >>	ntlm.c ntlm.h \ >>	occ.c occ.h \ >>	openssl_compat.h \ >> diff --git a/src/openvpn/networking_ip.c b/src/openvpn/networking_ip.c >> new file mode 100644 >> index 00000000..ae667a9c >> --- /dev/null >> +++ b/src/openvpn/networking_ip.c >> @@ -0,0 +1,386 @@ >> +/* >> + * Networking API implementation for iproute2 >> + * >> + * Copyright (C) 2018 Antonio Quartulli <a...@un...> > > I think since you copied/pasted from other files this should also have > the normal copyrights, right? Yeah, it makes sense because in this file there is code coming from the current implementation. > >> + >> +#if defined(TARGET_LINUX) && defined(ENABLE_IPROUTE) >> + >> +#include "syshead.h" >> + >> +#include "networking.h" >> +#include "networking_ip.h" >> +#include "misc.h" >> +#include "openvpn.h" >> +#include "run_command.h" >> +#include "socket.h" >> + >> +#include <stdbool.h> >> +#include <netinet/in.h> >> + >> +int >> +net_ctx_init(struct context *c, openvpn_net_ctx_t *ctx) >> +{ >> + ctx->es = NULL; >> + if (c) >> + ctx->es = c->es; >> + >> + return 0; >> +} > > I know that calling our execve with es==NULL is valid but should not we > aim here for consistency and always require c to be non NULL aka ASSERT(c)? No, because this can be invoked when calling openvpn with "--show-gateway". In that case we need to talk to the network stack but there is no context. > > >> +int >> +net_iface_up(openvpn_net_ctx_t *ctx, const char *iface, bool up) >> +{ >> + struct argv argv = argv_new(); >> + >> + argv_printf(&argv, "%s link set dev %s %s", iproute_path, iface, >> + up ? "up" : "down"); >> + argv_msg(M_INFO, &argv); >> + openvpn_execve_check(&argv, ctx->es, S_FATAL, "Linux ip link set failed"); >> + >> + argv_reset(&argv); >> + >> + return 0; >> +} >> + >> +int >> +net_iface_mtu_set(openvpn_net_ctx_t *ctx, const char *iface, uint32_t mtu) >> +{ >> + struct argv argv = argv_new(); >> + >> + argv_printf(&argv, "%s link set dev %s up mtu %d", iproute_path, iface, >> + mtu); >> + argv_msg(M_INFO, &argv); >> + openvpn_execve_check(&argv, ctx->es, S_FATAL, "Linux ip link set failed"); >> + >> + return 0; >> +} >> + >> +int >> +net_addr_v4_add(openvpn_net_ctx_t *ctx, const char *iface, >> + const in_addr_t *addr, int prefixlen, >> + const in_addr_t *broadcast) >> +{ >> + struct argv argv = argv_new(); >> + >> + char *addr_str = (char *)print_in_addr_t(*addr, 0, NULL); >> + char *brd_str = (char *)print_in_addr_t(*broadcast, 0, NULL); >> + >> + argv_printf(&argv, "%s addr add dev %s %s/%d broadcast %s", iproute_path, >> + iface, addr_str, prefixlen, brd_str); >> + argv_msg(M_INFO, &argv); >> + openvpn_execve_check(&argv, ctx->es, S_FATAL, "Linux ip addr add failed"); >> + >> + free(addr_str); >> + free(brd_str); >> + >> + argv_reset(&argv); >> + >> + return 0; >> +} >> + > > I initially written a comment that this method removes the ptp variant > of the method but later saw that there is also a ptp variant of the > method. I would suggest to either ranme this method to > net_addr_bcast_v4_add or have a bool ptp paramter and then call out to > two different methods. To me it feels like the PtP case is a special case, therefore I preferred to keep the "standard & simple" name for the non-PtP case and then add a special function just for PtP. Actually this function may even be called without a broadcast address. Mh..dunno to be honest. Maybe somebody else has an opinion? > > > >> +net_addr_v6_add(openvpn_net_ctx_t *ctx, const char *iface, >> + const struct in6_addr *addr, int prefixlen) > > Same comment as for v4. >> +net_addr_v4_del(openvpn_net_ctx_t *ctx, const char *iface, >> + const in_addr_t *addr, int prefixlen) > >> +net_addr_v6_del(openvpn_net_ctx_t *ctx, const char *iface, >> + const struct in6_addr *addr, int prefixlen) >> > These methods assume that removing ip on ptp and broadcast is the same > for ptp and boradcast on all platforms, is that really a sane assumption? > Good point ! It is a sane assumption at least for netlink and iproute. I am not sure about nettols. How about Android? Regards, -- Antonio Quartulli 

Hi, On 26/11/2018 22:55, Arne Schwabe wrote: > Am 11.10.18 um 20:41 schrieb Antonio Quartulli: >> tun.c and route.c contain all the code used by openvpn >> to manage the tun interface and the routing table on all >> the supported platforms. >> >> Across the years, this resulted in a longer functions >> and series of ifdefs. >> >> This patch introduces a new "networking API" which aims at >> creating a simple abstraction between the tun/route logic >> and the platform dependent code. >> >> The is API expected to be implemented outside of tun.c/route.c >> by using platform specific functionalities. > > If I understand it right, the goal is to create a platform independent > API, Android needs either still hacks in the non platform specific code > or the API needs to communicate also > > - route/adding addr before interface up > - type of route to add (android wants to ignores the host route to VPN > server and wants to isntall a 0.0.0.0/0 instead of two /1 even with def1 > in place, the last one to workaround buggy Samsung code that special > cases 0.0.0.0/0 but sadly still needed. > > Should we try to design the API right from the start to allow also for > these specifics? > This sounds like a problem that has to be addressed at a layer above compared to what this networking API is doing. With this I mean that the specific "Android case" you explained cannot be addressed by changing the networking API (meant to be low level API to configure/deconfigure th enetworking stack), but should be taken care by the login in tun.c/route.c directly. What yo are suggesting is a change to the logic passing the information from the control plane to the platform dependent code. This change to me seems independent from the technology used to configure the interface (i.e. iproute/nettools/netlink/anything-else). What you are after is probably an abstraction similar to the one we have in OpenVPN 3, but that one is indeed implemented at an higher level compared to what we are introducing with this patchset. If you note, this is why my patches for OpenVPN 3 consists of two components. In my opinion, the logic change you are talking about is not required because: - this is already implemented in tun.c/route.c at the moment and won't be changed for now - this can be "improved" later once this networking API is in place, because the code will have to deal with will already be much slimmer. Attempting to work on both layers with one big change will just make things more complicated imho, hence my proposal to go step by step. Best Regards, -- Antonio Quartulli 

This custom actions are used by MSI setup to close OpenVPN GUI before performing an upgrade and relaunch it afterwards. --- src/openvpnmsica/Makefile.am | 2 +- src/openvpnmsica/openvpnmsica.c | 102 ++++++++++++++++++++++++++++++++ src/openvpnmsica/openvpnmsica.h | 25 ++++++++ 3 files changed, 128 insertions(+), 1 deletion(-) diff --git a/src/openvpnmsica/Makefile.am b/src/openvpnmsica/Makefile.am index f4725a53..f00a01d8 100644 --- a/src/openvpnmsica/Makefile.am +++ b/src/openvpnmsica/Makefile.am @@ -41,7 +41,7 @@ libopenvpnmsica_la_CFLAGS = \	-municode -D_UNICODE \	-UNTDDI_VERSION -U_WIN32_WINNT \	-D_WIN32_WINNT=_WIN32_WINNT_VISTA -libopenvpnmsica_la_LDFLAGS = -ladvapi32 -lole32 -lmsi -lsetupapi -liphlpapi -lshlwapi -lversion -no-undefined -avoid-version +libopenvpnmsica_la_LDFLAGS = -ladvapi32 -lole32 -lmsi -lsetupapi -liphlpapi -lshell32 -lshlwapi -lversion -no-undefined -avoid-version endif libopenvpnmsica_la_SOURCES = \ diff --git a/src/openvpnmsica/openvpnmsica.c b/src/openvpnmsica/openvpnmsica.c index e8988ac8..86434801 100644 --- a/src/openvpnmsica/openvpnmsica.c +++ b/src/openvpnmsica/openvpnmsica.c @@ -37,6 +37,7 @@ #include <malloc.h> #include <memory.h> #include <msiquery.h> +#include <shellapi.h> #include <shlwapi.h> #include <stdbool.h> #include <stdlib.h> @@ -44,6 +45,7 @@ #ifdef _MSC_VER #pragma comment(lib, "iphlpapi.lib") +#pragma comment(lib, "shell32.lib") #pragma comment(lib, "shlwapi.lib") #pragma comment(lib, "version.lib") #endif @@ -475,6 +477,106 @@ cleanup_CoInitialize: } +UINT __stdcall +CloseOpenVPNGUI(_In_ MSIHANDLE hInstall) +{ +#ifdef _MSC_VER +#pragma comment(linker, DLLEXP_EXPORT) +#endif + UNREFERENCED_PARAMETER(hInstall); /* This CA is does not interact with MSI session (report errors, access properties, tables, etc.). */ + + openvpnmsica_debug_popup(TEXT(__FUNCTION__)); + + /* Find OpenVPN GUI window. */ + HWND hWnd = FindWindow(TEXT("OpenVPN-GUI"), NULL); + if (hWnd) + { + /* Ask it to close and wait for 100ms. Unfortunately, this will succeed only for recent OpenVPN GUI that do not run elevated. */ + SendMessage(hWnd, WM_CLOSE, 0, 0); + Sleep(100); + } + + return ERROR_SUCCESS; +} + + +UINT __stdcall +StartOpenVPNGUI(_In_ MSIHANDLE hInstall) +{ +#ifdef _MSC_VER +#pragma comment(linker, DLLEXP_EXPORT) +#endif + + openvpnmsica_debug_popup(TEXT(__FUNCTION__)); + + UINT uiResult; + BOOL bIsCoInitialized = SUCCEEDED(CoInitialize(NULL)); + + /* Set MSI session handle in TLS. */ + struct openvpnmsica_tls_data *s = (struct openvpnmsica_tls_data *)TlsGetValue(openvpnmsica_tlsidx_session); + s->hInstall = hInstall; + + /* Create and populate a MSI record. */ + MSIHANDLE hRecord = MsiCreateRecord(1); + if (!hRecord) + { + uiResult = ERROR_INVALID_HANDLE; + msg(M_NONFATAL, "%s: MsiCreateRecord failed", __FUNCTION__); + goto cleanup_CoInitialize; + } + uiResult = MsiRecordSetString(hRecord, 0, TEXT("\"[#bin.openvpn_gui.exe]\"")); + if (uiResult != ERROR_SUCCESS) + { + SetLastError(uiResult); /* MSDN does not mention MsiRecordSetString() to set GetLastError(). But we do have an error code. Set last error manually. */ + msg(M_NONFATAL | M_ERRNO, "%s: MsiRecordSetString failed", __FUNCTION__); + goto cleanup_MsiCreateRecord; + } + + /* Format string. */ + TCHAR szStackBuf[MAX_PATH]; + DWORD dwPathSize = _countof(szStackBuf); + LPTSTR szPath = szStackBuf; + uiResult = MsiFormatRecord(hInstall, hRecord, szPath, &dwPathSize); + if (uiResult == ERROR_MORE_DATA) + { + /* Allocate buffer on heap (+1 for terminator), and retry. */ + szPath = (LPTSTR)malloc((++dwPathSize) * sizeof(TCHAR)); + uiResult = MsiFormatRecord(hInstall, hRecord, szPath, &dwPathSize); + } + if (uiResult != ERROR_SUCCESS) + { + SetLastError(uiResult); /* MSDN does not mention MsiFormatRecord() to set GetLastError(). But we do have an error code. Set last error manually. */ + msg(M_NONFATAL | M_ERRNO, "%s: MsiFormatRecord failed", __FUNCTION__); + goto cleanup_malloc_szPath; + } + + /* Launch the OpenVPN GUI. */ + SHELLEXECUTEINFO sei = { + .cbSize = sizeof(SHELLEXECUTEINFO), + .fMask = SEE_MASK_FLAG_NO_UI, /* Don't show error UI, we'll display it. */ + .lpFile = szPath, + .nShow = SW_SHOWNORMAL + }; + if (!ShellExecuteEx(&sei)) + { + uiResult = GetLastError(); + msg(M_NONFATAL | M_ERRNO, "%s: ShellExecuteEx(%s) failed", __FUNCTION__, szPath); + goto cleanup_malloc_szPath; + } + + uiResult = ERROR_SUCCESS; + +cleanup_malloc_szPath: + if (szPath != szStackBuf) + free(szPath); +cleanup_MsiCreateRecord: + MsiCloseHandle(hRecord); +cleanup_CoInitialize: + if (bIsCoInitialized) CoUninitialize(); + return uiResult; +} + + UINT __stdcall EvaluateTAPInterfaces(_In_ MSIHANDLE hInstall) { diff --git a/src/openvpnmsica/openvpnmsica.h b/src/openvpnmsica/openvpnmsica.h index da145062..4ee5c05f 100644 --- a/src/openvpnmsica/openvpnmsica.h +++ b/src/openvpnmsica/openvpnmsica.h @@ -91,6 +91,31 @@ DLLEXP_DECL UINT __stdcall FindTAPInterfaces(_In_ MSIHANDLE hInstall); +/** + * Find OpenVPN GUI window and send it a WM_CLOSE message. + * + * @param hInstall Handle to the installation provided to the DLL custom action + * + * @return ERROR_SUCCESS on success; An error code otherwise + * See: https://msdn.microsoft.com/en-us/library/windows/desktop/aa368072.aspx + */ +DLLEXP_DECL UINT __stdcall +CloseOpenVPNGUI(_In_ MSIHANDLE hInstall); + + +/** + * Launches OpenVPN GUI. It's path is obtained by expanding the `[#bin.openvpn_gui.exe]` + * therefore, its Id field in File table must be "bin.openvpn_gui.exe". + * + * @param hInstall Handle to the installation provided to the DLL custom action + * + * @return ERROR_SUCCESS on success; An error code otherwise + * See: https://msdn.microsoft.com/en-us/library/windows/desktop/aa368072.aspx + */ +DLLEXP_DECL UINT __stdcall +StartOpenVPNGUI(_In_ MSIHANDLE hInstall); + + /** * Evaluate the TAPInterface table of the MSI package database and prepare a list of TAP * interfaces to install/remove. -- 2.19.0.windows.1 

Your patch has been applied to the master branch. commit c3f565f0590b152c6ea18be230509be8f3c832f0 Author: Steffan Karger Date: Wed Oct 24 12:12:05 2018 +0200 Remove deprecated --compat-x509-names and --no-name-remapping Signed-off-by: Steffan Karger <ste...@fo...> Acked-by: Arne Schwabe <ar...@rf...> Message-Id: <154...@fo...> URL: https://www.mail-archive.com/ope...@li.../msg17804.html Signed-off-by: Gert Doering <ge...@gr...> -- kind regards, Gert Doering 

Am 19.10.18 um 17:56 schrieb David Sommerseth: > From: Heiko Hund <hei...@so...> > > The previous implementation had the problem that it was not fully > compatible with printf() and could only detect % format directives > following a space character (0x20). > > It modifies the format string and inserts marks to separate groups > before passing it to the regular printf in libc. The marks are > later used to separate the output string into individual command > line arguments. > > The choice of 0x1D as the argument delimiter is based on the > assumption that no "regular" string passed to argv_printf_*() will > ever have to contain that byte (and the fact that it actually is > the ASCII "group separator" control character, which fits its > purpose). > > This commit has been updated by David Sommerseth based on his feedback > on the mailing list discussions earlier on. > > Signed-off-by: Heiko Hund <hei...@so...> > Signed-off-by: David Sommerseth <da...@op...> > --- > src/openvpn/argv.c | 266 ++++++++++++--------------- > src/openvpn/argv.h | 4 +- > src/openvpn/route.c | 8 +- > src/openvpn/tun.c | 24 +-- > tests/unit_tests/openvpn/test_argv.c | 58 +++++- > 5 files changed, 192 insertions(+), 168 deletions(-) > > diff --git a/src/openvpn/argv.c b/src/openvpn/argv.c > index 9100a196..9606f382 100644 > --- a/src/openvpn/argv.c > +++ b/src/openvpn/argv.c > @@ -131,64 +131,6 @@ argv_insert_head(const struct argv *a, const char *head) > return r; > } > > -static char * > -argv_term(const char **f) > -{ > - const char *p = *f; > - const char *term = NULL; > - size_t termlen = 0; > - > - if (*p == '\0') > - { > - return NULL; > - } > - > - while (true) > - { > - const int c = *p; > - if (c == '\0') > - { > - break; > - } > - if (term) > - { > - if (!isspace(c)) > - { > - ++termlen; > - } > - else > - { > - break; > - } > - } > - else > - { > - if (!isspace(c)) > - { > - term = p; > - termlen = 1; > - } > - } > - ++p; > - } > - *f = p; > - > - if (term) > - { > - char *ret; > - ASSERT(termlen > 0); > - ret = malloc(termlen + 1); > - check_malloc_return(ret); > - memcpy(ret, term, termlen); > - ret[termlen] = '\0'; > - return ret; > - } > - else > - { > - return NULL; > - } > -} > - > const char * > argv_str(const struct argv *a, struct gc_arena *gc, const unsigned int flags) > { > @@ -218,119 +160,151 @@ argv_msg_prefix(const int msglev, const struct argv *a, const char *prefix) > gc_free(&gc); > } > > -static void > + > +/* > + * argv_prep_format - prepare argv format string for further processing > + * > + * Individual argument must be separated by space. Ignores leading and trailing spaces. > + * Consecutive spaces count as one. Returns prepared format string, with space replaced > + * by delim and adds the number of arguments to the count parameter. > + */ > +static char * > +argv_prep_format(const char *format, const char delim, size_t *count, struct gc_arena *gc) > +{ Lines longer than 80 chars (90). > + int i, j; > + char *f; All could be made inline with C99 > + bool in_token = false; > + > + if (format == NULL) > + { > + return NULL; > + } > + > + f = gc_malloc(strlen(format) + 1, true, gc); > + for (i = 0, j = 0; i < strlen(format); i++) > + { int i=0, j=0; > + if (format[i] == ' ') > + { > + in_token = false; > + continue; > + } > + > + if (!in_token) > + { > + ++*count; I am entirely sure about our style here but I would prefer brackets around (*count). And since you use i++ and j++, also should use (*count)++; > + > + /* > + * We don't add any delimiter to the resulting > + * format string before something has been added there. This sentence is hard to understand. "before something has been added there" => if it is empty? > + * The resulting format string will never start with a delimiter. Okay. But why? I think this is done to ignore, leading spaces? > + */ > + if (j > 0) > + { > + f[j++] = delim; > + } [...] > +/* > + * argv_printf_arglist - create a struct argv from a format string > + * > + * Instead of parsing the format string ourselves place delimiters via argv_prep_format() > + * before we let libc's printf() do the parsing. Then split the resulting string at the > + * injected delimiters. > + */ > +static bool > argv_printf_arglist(struct argv *a, const char *format, va_list arglist) > { > - char *term; > - const char *f = format; > + struct gc_arena gc = gc_new(); > + const char delim = 0x1D; /* ASCII Group Separator (GS) */ > + char *f, *buf, *end; > + size_t argc, size; > + bool res = false; > + va_list tmplist; > + int len; Making some of these inline or move their defintion closer where they are actually would make the code easier to understand. > argv_extend(a, 1); /* ensure trailing NULL */ This depends on the side effect of the allocating nulling things before. Not sure if that is okay or not. > - while ((term = argv_term(&f)) != NULL) > + argc = a->argc; > + f = argv_prep_format(format, delim, &argc, &gc); > + if (f == NULL) > { > - if (term[0] == '%') > - { > - if (!strcmp(term, "%s")) > - { > - char *s = va_arg(arglist, char *); > - if (!s) > - { > - s = ""; > - } > - argv_append(a, string_alloc(s, NULL)); > - } > - else if (!strcmp(term, "%d")) > - { > - char numstr[64]; > - openvpn_snprintf(numstr, sizeof(numstr), "%d", va_arg(arglist, int)); > - argv_append(a, string_alloc(numstr, NULL)); > - } > - else if (!strcmp(term, "%u")) > - { > - char numstr[64]; > - openvpn_snprintf(numstr, sizeof(numstr), "%u", va_arg(arglist, unsigned int)); > - argv_append(a, string_alloc(numstr, NULL)); > - } > - else if (!strcmp(term, "%lu")) > - { > - char numstr[64]; > - openvpn_snprintf(numstr, sizeof(numstr), "%lu", > - va_arg(arglist, unsigned long)); > - argv_append(a, string_alloc(numstr, NULL)); > - } > - else if (!strcmp(term, "%s/%d")) > - { > - char numstr[64]; > - char *s = va_arg(arglist, char *); > - > - if (!s) > - { > - s = ""; > - } > + goto out; > + } > > - openvpn_snprintf(numstr, sizeof(numstr), "%d", va_arg(arglist, int)); > + /* determine minimum buffer size */ > + va_copy(tmplist, arglist); > + len = vsnprintf(NULL, 0, f, tmplist); > + va_end(tmplist); > + if (len < 0) > + { > + goto out; > + } > > - { > - const size_t len = strlen(s) + strlen(numstr) + 2; > - char *combined = (char *) malloc(len); > - check_malloc_return(combined); > + size = adjust_power_of_2(len + 1); I am a bit sure why we need to increase the size to the next power of 2 and why len+1 shouldn't be enough > + buf = gc_malloc(size, false, &gc); > + len = vsnprintf(buf, size, f, arglist); > + if (len < 0 || len >= size) > + { > + goto out; > + } This vnsprintf should need the same space as the previous vsnprintf that was used to determine len. > > - strcpy(combined, s); > - strcat(combined, "/"); > - strcat(combined, numstr); > - argv_append(a, combined); > - } > - } > - else if (!strcmp(term, "%s%sc")) > - { > - char *s1 = va_arg(arglist, char *); > - char *s2 = va_arg(arglist, char *); > - char *combined; > + /* split the string at the delimiters */ > + end = strchr(buf, delim); > + while (end) > + { > + *end = '\0'; > + argv_append(a, string_alloc(buf, NULL)); > + buf = end + 1; > + end = strchr(buf, delim); > + } > + argv_append(a, string_alloc(buf, NULL)); > > - if (!s1) > - { > - s1 = ""; > - } > - if (!s2) > - { > - s2 = ""; > - } > - combined = (char *) malloc(strlen(s1) + strlen(s2) + 1); > - check_malloc_return(combined); > - strcpy(combined, s1); > - strcat(combined, s2); > - argv_append(a, combined); > - } > - else > - { > - ASSERT(0); > - } > - free(term); > - } > - else > - { > - argv_append(a, term); > - } > + if (a->argc != argc) > + { > + /* Someone snuck in a GS (0x1D), fail gracefully */ > + argv_reset(a); > + argv_extend(a, 1); /* ensure trailing NULL */ > + goto out; > } That is a bit weird way to check that but okay. > -void > + > + > +bool > argv_printf(struct argv *a, const char *format, ...) > { > + bool res; > va_list arglist; > - argv_reset(a); > + > va_start(arglist, format); > - argv_printf_arglist(a, format, arglist); > + argv_reset(a); > + res = argv_printf_arglist(a, format, arglist); > va_end(arglist); > + return res; > } The bool definition could be made inline (bool res=). > -void > +bool > argv_printf_cat(struct argv *a, const char *format, ...) > { > + bool res; > va_list arglist; > va_start(arglist, format); > - argv_printf_arglist(a, format, arglist); > + res = argv_printf_arglist(a, format, arglist); > va_end(arglist); > + return res; > } Same here. Rest looks good. Arne 

Am 24.10.18 um 12:12 schrieb Steffan Karger: > As promised, remove these options for OpenVPN 2.5. > > If a user still uses these, print an error that the user should update it's > configuration. Just printing a warning would cause much more confusing > errors, somewhere in middle of a failed connection attempt because the > (non-compat) names no longer match the expected names. ACK-By: Arne Schwabe <ar...@rf...> The patch does what it says. Arne 

Am 11.10.18 um 20:41 schrieb Antonio Quartulli: > This patch introduces a tiny netlink interface, optimized > for the openvpn use case. > > It basically exposes all those operations that are currently > handled by directly calling the /sbin/ip command (or even > ifconfig/route, if configured). > > By using netlink, openvpn won't need to spawn new processes > when configuring the tun interface or routes. > This new approach will also allow openvpn to be granted > CAP_NET_ADMIN and be able to properly work even though it > dropped the root privileges (currently handled via workarounds). > > By moving this logic into the sitnl module, tun.c and route.c > also benefit from some code simplification Just FYI for the rest. The current code still has a bug handling ipv6 and I am waiting for V3 that does not have this bug. So this is not a full review > +typedef union { > + in_addr_t ipv4; > + struct in6_addr ipv6; > +} inet_address_t; > + > +/** > + * Link state request message > + */ > +struct sitnl_link_req { > + struct nlmsghdr n; I think one two sentences that netlink requires messages in a certain format but has no standard header that defines them so we need to define the message ourselves would be good here. > + > +/* if (((int)nladdr.nl_pid != peer) || (h->nlmsg_pid != nladdr.nl_pid) > + || (h->nlmsg_seq != seq)) > + { > + rcv_len -= NLMSG_ALIGN(len); > + h = (struct nlmsghdr *)((char *)h + NLMSG_ALIGN(len)); > + msg(M_DEBUG, "%s: skipping unrelated message. nl_pid:%d (peer:%d) nl_msg_pid:%d nl_seq:%d seq:%d", > + __func__, (int)nladdr.nl_pid, peer, h->nlmsg_pid, > + h->nlmsg_seq, seq); > + continue; > + } > +*/ The "normal" style for these debug commented out parts is using #ifdef instead of comments. > +typedef struct { > + int addr_size; > + inet_address_t gw; > + char iface[IFNAMSIZ]; > +} route_res_t; > + This struct is in the middle of the file and I think it should either go along with the other struct definition in the top of the file or all structs should be defined before the function that they are used in first. > +/* used by iproute2 implementation too */ I would not add that comment. Where a function is used can be inferred pretty easily and we don't have such comments elsewhere. And I fear this comment might be overlooked when iproute2 is remove in 2.6/2.7 and then more confusing than helpful. Arne 

Am 11.10.18 um 20:41 schrieb Antonio Quartulli: > tun.c and route.c contain all the code used by openvpn > to manage the tun interface and the routing table on all > the supported platforms. > > Across the years, this resulted in a longer functions > and series of ifdefs. > > This patch introduces a new "networking API" which aims at > creating a simple abstraction between the tun/route logic > and the platform dependent code. > > The is API expected to be implemented outside of tun.c/route.c > by using platform specific functionalities. If I understand it right, the goal is to create a platform independent API, Android needs either still hacks in the non platform specific code or the API needs to communicate also - route/adding addr before interface up - type of route to add (android wants to ignores the host route to VPN server and wants to isntall a 0.0.0.0/0 instead of two /1 even with def1 in place, the last one to workaround buggy Samsung code that special cases 0.0.0.0/0 but sadly still needed. Should we try to design the API right from the start to allow also for these specifics? Arne 

Am 11.10.18 um 20:41 schrieb Antonio Quartulli: > iproute2 is the first user of the new networking API and > its one of the two currently supported functionalities on > Linux (the other being net-tools). > > This patch simply copies the current code from tun.c/route.c > to networking_ip.c without introducing any funcional > change to the code. > I skipped the first patch and added notes about the proposed API in this patch. > create mode 100644 src/openvpn/networking_ip.h > > diff --git a/src/openvpn/Makefile.am b/src/openvpn/Makefile.am > index 8afc4146..3caad17d 100644 > --- a/src/openvpn/Makefile.am > +++ b/src/openvpn/Makefile.am > @@ -80,7 +80,7 @@ openvpn_SOURCES = \ >	mtu.c mtu.h \ >	mudp.c mudp.h \ >	multi.c multi.h \ > -	networking.h \ > +	networking_ip.c networking_ip.h networking.h \ As ip is not really but a linux specific tool, I think naming this networking_linuxip.c is better as networking_ip sounds like something generic. >	ntlm.c ntlm.h \ >	occ.c occ.h \ >	openssl_compat.h \ > diff --git a/src/openvpn/networking_ip.c b/src/openvpn/networking_ip.c > new file mode 100644 > index 00000000..ae667a9c > --- /dev/null > +++ b/src/openvpn/networking_ip.c > @@ -0,0 +1,386 @@ > +/* > + * Networking API implementation for iproute2 > + * > + * Copyright (C) 2018 Antonio Quartulli <a...@un...> I think since you copied/pasted from other files this should also have the normal copyrights, right? > + > +#if defined(TARGET_LINUX) && defined(ENABLE_IPROUTE) > + > +#include "syshead.h" > + > +#include "networking.h" > +#include "networking_ip.h" > +#include "misc.h" > +#include "openvpn.h" > +#include "run_command.h" > +#include "socket.h" > + > +#include <stdbool.h> > +#include <netinet/in.h> > + > +int > +net_ctx_init(struct context *c, openvpn_net_ctx_t *ctx) > +{ > + ctx->es = NULL; > + if (c) > + ctx->es = c->es; > + > + return 0; > +} I know that calling our execve with es==NULL is valid but should not we aim here for consistency and always require c to be non NULL aka ASSERT(c)? > +int > +net_iface_up(openvpn_net_ctx_t *ctx, const char *iface, bool up) > +{ > + struct argv argv = argv_new(); > + > + argv_printf(&argv, "%s link set dev %s %s", iproute_path, iface, > + up ? "up" : "down"); > + argv_msg(M_INFO, &argv); > + openvpn_execve_check(&argv, ctx->es, S_FATAL, "Linux ip link set failed"); > + > + argv_reset(&argv); > + > + return 0; > +} > + > +int > +net_iface_mtu_set(openvpn_net_ctx_t *ctx, const char *iface, uint32_t mtu) > +{ > + struct argv argv = argv_new(); > + > + argv_printf(&argv, "%s link set dev %s up mtu %d", iproute_path, iface, > + mtu); > + argv_msg(M_INFO, &argv); > + openvpn_execve_check(&argv, ctx->es, S_FATAL, "Linux ip link set failed"); > + > + return 0; > +} > + > +int > +net_addr_v4_add(openvpn_net_ctx_t *ctx, const char *iface, > + const in_addr_t *addr, int prefixlen, > + const in_addr_t *broadcast) > +{ > + struct argv argv = argv_new(); > + > + char *addr_str = (char *)print_in_addr_t(*addr, 0, NULL); > + char *brd_str = (char *)print_in_addr_t(*broadcast, 0, NULL); > + > + argv_printf(&argv, "%s addr add dev %s %s/%d broadcast %s", iproute_path, > + iface, addr_str, prefixlen, brd_str); > + argv_msg(M_INFO, &argv); > + openvpn_execve_check(&argv, ctx->es, S_FATAL, "Linux ip addr add failed"); > + > + free(addr_str); > + free(brd_str); > + > + argv_reset(&argv); > + > + return 0; > +} > + I initially written a comment that this method removes the ptp variant of the method but later saw that there is also a ptp variant of the method. I would suggest to either ranme this method to net_addr_bcast_v4_add or have a bool ptp paramter and then call out to two different methods. > +net_addr_v6_add(openvpn_net_ctx_t *ctx, const char *iface, > + const struct in6_addr *addr, int prefixlen) Same comment as for v4. > +net_addr_v4_del(openvpn_net_ctx_t *ctx, const char *iface, > + const in_addr_t *addr, int prefixlen) > +net_addr_v6_del(openvpn_net_ctx_t *ctx, const char *iface, > + const struct in6_addr *addr, int prefixlen) > These methods assume that removing ip on ptp and broadcast is the same for ptp and boradcast on all platforms, is that really a sane assumption? Arne 

Patch V5: Fix typos, clarify man page section about deferred client-connect script. Add section to Changes.rst Signed-off-by: Arne Schwabe <ar...@rf...> --- Changes.rst | 4 +++ doc/openvpn.8 | 55 +++++++++++++++++++++++++++++++++++-- include/openvpn-plugin.h.in | 21 ++++++++++---- 3 files changed, 71 insertions(+), 9 deletions(-) diff --git a/Changes.rst b/Changes.rst index a7429b11..357b0901 100644 --- a/Changes.rst +++ b/Changes.rst @@ -3,6 +3,10 @@ Overview of changes in 2.5 New features ------------ +Deferred client-connect + client-connect and the connect plugin API allow now asynchronous/deferred + return of configuration file in the same way as the auth-plugin. + Client-specific tls-crypt keys (``--tls-crypt-v2``) ``tls-crypt-v2`` adds the ability to supply each client with a unique tls-crypt key. This allows large organisations and VPN providers to profit diff --git a/doc/openvpn.8 b/doc/openvpn.8 index 94b5cc4f..a5ec3b07 100644 --- a/doc/openvpn.8 +++ b/doc/openvpn.8 @@ -3346,6 +3346,13 @@ is significant. If .B script returns a non\-zero error status, it will cause the client to be disconnected. + +If a +.B \-\-client\-connect cmd +wants to defer the generating the configuration the script, should +use the client_connect_deferred_file and client_connect_config_file +environment variables and write status accordingly into these files +(See the environment section below for more details). .\"********************************************************* .TP .B \-\-client\-disconnect cmd @@ -3428,12 +3435,18 @@ This directory will be used by in the following cases: * .B \-\-client\-connect -scripts to dynamically generate client\-specific -configuration files. +scripts and +.B OPENVPN_PLUGIN_CLIENT_CONNECT +plugin hook +to dynamically generate client\-specific configuration files +and return success/failure via client_connect_deferred_file +when using deferred client connect method * .B OPENVPN_PLUGIN_AUTH_USER_PASS_VERIFY -plugin hook to return success/failure via auth_control_file +and + +plugin hook to return success/failure via auth_control_file/ when using deferred auth method * @@ -6431,6 +6444,42 @@ Set prior to execution of the script. .\"********************************************************* .TP +.B client_connect_config_file +The path to the configuration file that should be written by +the +.B \-\-client\-connect +The content of this environment variable is identical to the +file as a argument of the called +.B \-\-client\-connect +script. +.\"********************************************************* +.TP +.B client_connect_deferred_file +This file can be optionally written to communicate a status +code of the +.TP +.B \-\-client\-connect +script. If used for deferring, this file must be written +before the +.B \-\-client\-connect +script exits. The first character in the file is interpreted +and '1 is equal to normal script execution, '0' indicates an +error (in the same way that a non zero exit status does) and +'2 indicates that the script deferred returning the config +file. When the script defers returning the configuration, it must +must '2' to to the file to indicate the deferral. +A background process or similar must then take of writing the +configuration to the file indicated by the +.B +client_connect_config_file +environment variable and when finished, write the a '1' to this +file (or '0' in case of an error). + +The absence of any character in the file when the script finishes +executing is interpreted the same as '1'. This allows script that +are not written to support the defer mechanism to be used unmodified. +.\"********************************************************* +.TP .B common_name The X509 common name of an authenticated client. Set prior to execution of diff --git a/include/openvpn-plugin.h.in b/include/openvpn-plugin.h.in index 38fbe097..c776178f 100644 --- a/include/openvpn-plugin.h.in +++ b/include/openvpn-plugin.h.in @@ -557,12 +557,21 @@ OPENVPN_PLUGIN_DEF openvpn_plugin_handle_t OPENVPN_PLUGIN_FUNC(openvpn_plugin_op * OPENVPN_PLUGIN_FUNC_SUCCESS on success, OPENVPN_PLUGIN_FUNC_ERROR on failure * * In addition, OPENVPN_PLUGIN_FUNC_DEFERRED may be returned by - * OPENVPN_PLUGIN_AUTH_USER_PASS_VERIFY. This enables asynchronous - * authentication where the plugin (or one of its agents) may indicate - * authentication success/failure some number of seconds after the return - * of the OPENVPN_PLUGIN_AUTH_USER_PASS_VERIFY handler by writing a single - * char to the file named by auth_control_file in the environmental variable - * list (envp). + * OPENVPN_PLUGIN_AUTH_USER_PASS_VERIFY, OPENVPN_PLUGIN_CLIENT_CONNECT and + * OPENVPN_PLUGIN_CLIENT_CONNECT_V2. This enables asynchronous + * authentication or client connect where the plugin (or one of its agents) + * may indicate authentication success/failure or client configuration some + * number of seconds after the return of the function handler. + * For OPENVPN_PLUGIN_AUTH_USER_PASS_VERIFY and OPENVPN_PLUGIN_CLIENT_CONNECT + * this is done by writing a single char to the file named by + * auth_control_file/client_connect_deferred_file + * in the environmental variable list (envp). + * + * In addition the OPENVPN_PLUGIN_CLIENT_CONNECT_DEFER and + * OPENVPN_PLUGIN_CLIENT_CONNECT_DEFER_V2 are called when OpenVPN tries to + * get the deferred result. For a V2 call implementing this function is + * required as information is not passed by files. For the normal version + * the call is optional. * * first char of auth_control_file: * '0' -- indicates auth failure -- 2.19.1 

Hi, I think that man page statement about "mssfix" doesn't fully reflect actual functionality. Specifically, man page says: Announce to TCP sessions running over the tunnel that they should limit their send packet sizes such that after OpenVPN has encapsulated them, the resulting UDP packet size that OpenVPN sends to its peer will not exceed max bytes. The default value is 1450. The max parameter is interpreted in the same way as the --link-mtu parameter, i.e. the UDP packet size after encapsulation overhead has been added in, but not including the UDP header itself. Resulting packet would be at most 28 bytes larger for IPv4 and 48 bytes for IPv6 (20/40 bytes for IP header and 8 bytes for UDP header). So it means that with "mssfix" 1300 resulting IPv4 packet size would be at most 1328. This is what I see in Wireshark (server - git master, client 2.4.6): Internet Protocol Version 4, Src: 128.199.xxx.yyy, Dst: 10.0.200.20 0100 .... = Version: 4 .... 0101 = Header Length: 20 bytes (5) Total Length: 1300 Protocol: UDP (17) User Datagram Protocol, Src Port: 1194, Dst Port: 1194 Source Port: 1194 Destination Port: 1194 Length: 1280 OpenVPN Protocol Type: 0x49 [opcode/key_id] Peer ID: 0 Data (1268 bytes) While man page statement is technically correct - UDP packet size is 1300, which is "at most 1328", I think it should say: > the resulting IP packet size that OpenVPN sends to its peer will not exceed max bytes and > The max parameter is interpreted in the same way as the --link-mtu parameter, i.e. the IP packet size after encapsulation overhead has been added in, including UDP and IP headers. Same results without explicitly defining mssfix - IP packet size is 1450 (which is default value). Most likely initially mssfix has worked as stated in man but then implementation has changed. So either we may want to fix implementation or change man. Since mssfix behaves like link-mtu, maybe we could also change its default value to 1500 to get 50 more bytes for payload. What do you think? -- -Lev