Re: [Openvpn-devel] [PATCH] Allow setting an empty auth-token in push replies

[Selva Nair]

On Wed, Oct 23, 2024 at 11:47 AM Razvan Cojocaru  wrote:

> On 10/23/24 18:25, Selva Nair wrote:
> > Wouldn't pushing "HALT" instead of "AUTH_FAILED" work in this case?
> > As in the management command "client-kill {cid} HALT" which calls
> > send_restart() with kill_msg = "HALT".
>
> Possibly, however the intent has always been to use this feature to
> reject (authorize) clients (so this is a corner case of that, just that
> we can retract authorization at a later time), and in addition
> considerable work has already been done that relies on the AUTH_FAILED
> code paths.
>
>
Looks like a misuse of AUTH_FAILED to me. To kill a client while not in the
authentication phase, use code paths meant for that purpose.

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

Re: [Openvpn-devel] [PATCH] Allow setting an empty auth-token in push replies

[Selva Nair]

On Wed, Oct 23, 2024 at 11:03 AM Razvan Cojocaru  wrote:

> On 10/23/24 17:50, Gert Doering wrote:
> > OK, so I see what is happening - you're sending an AUTH_FAILED "out of
> > the blue", not in response to a client handshake, right?
>
> Exactly. In response to a client handshake there's no problem.
>
> > OpenVPN 2 *should* invalidate the token upon the reconnect (and then
> > getting an AUTH_FAILED)... so what happens in this case if you let it
> > reconnect?
>
> If we let it reconnect, the first re-connection attempt will fail at the
> handshake stage as expected.
>

> But OpenVPN GUI tools will tend to show _two_ failed connections (the
> first "out of the blue" AUTH_FAILED one, and the second actual one),
> which is confusing for clients.
>

Wouldn't pushing "HALT" instead of "AUTH_FAILED" work in this case?
As in the management command "client-kill {cid} HALT" which calls
send_restart() with kill_msg = "HALT".

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

Re: [Openvpn-devel] [PATCH applied] Re: Interpret --key and --cert option argument as URI

[Selva Nair]

Hi,

On Tue, Sep 10, 2024 at 5:48 PM Arne Schwabe  wrote:

>
> >
> > Your patch has been applied to the master branch.
> >
> > commit 3512e8d3ada4fa7d04925a89fd9f3669655c7887 (master)
> > Author: Selva Nair
> > Date:   Fri Sep 6 12:37:34 2024 +0200
> >
> >   Interpret --key and --cert option argument as URI
> >
> There is something wonky going on. On my local macOS build I am getting
> errors with this on master now:
>
> openvpn --cert /Users/arne/ovpn/styx-secp.pem --key
> /Users/arne/ovpn/styx-secp.pem
>
> 2024-09-10 23:25:00 us=689075 OpenSSL: error:1608010C:STORE
> routines::unsupported:
> 2024-09-10 23:25:00 us=689082 Cannot load certificate from URI
> 


>
> styx-secp.pem is a file that contains both cert and key:
>

Such composite PEM files should work both as file name and
URI.

It seems I have overlooked the fact that OPENSSL_STORE_load()
can error out and return NULL and still have more readable objects
present.

I'm working on a patch.

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

[Openvpn-devel] [PATCH] Fix more of uninitialized struct user_pass local vars

[selva . nair]

From: Selva Nair 

Not all of these touch uninitialzed members of the struct, but that
could change in future.

Change-Id: I71bc847b48da54b70f60ccd7c4521ec699daf9cb
Signed-off-by: Selva Nair 
---
There is an unused & uninitialized struct user_pass in tun.c for ANDROID.
Could be removed?

 src/openvpn/auth_token.c | 1 +
 src/openvpn/pkcs11.c | 1 +
 src/openvpn/socks.c  | 2 +-
 3 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/openvpn/auth_token.c b/src/openvpn/auth_token.c
index 5de65cb6..c4b59b99 100644
--- a/src/openvpn/auth_token.c
+++ b/src/openvpn/auth_token.c
@@ -452,6 +452,7 @@ check_send_auth_token(struct context *c)
 }
 
 struct user_pass up;
+CLEAR(up);
 strncpynt(up.username, multi->locked_username, sizeof(up.username));
 
 generate_auth_token(&up, multi);
diff --git a/src/openvpn/pkcs11.c b/src/openvpn/pkcs11.c
index 418f6bb0..37cc1678 100644
--- a/src/openvpn/pkcs11.c
+++ b/src/openvpn/pkcs11.c
@@ -238,6 +238,7 @@ _pkcs11_openvpn_pin_prompt(
 {
 struct user_pass token_pass;
 char prompt[1024];
+CLEAR(token_pass);
 
 (void)global_data;
 (void)user_data;
diff --git a/src/openvpn/socks.c b/src/openvpn/socks.c
index d95f3013..e280453d 100644
--- a/src/openvpn/socks.c
+++ b/src/openvpn/socks.c
@@ -96,7 +96,7 @@ socks_username_password_auth(struct socks_proxy_info *p,
 ssize_t size;
 bool ret = false;
 
-creds.defined = 0;
+CLEAR(creds);
 if (!get_user_pass(&creds, p->authfile, UP_TYPE_SOCKS, 
GET_USER_PASS_MANAGEMENT))
 {
 msg(M_NONFATAL, "SOCKS failed to get username/password.");
-- 
2.40.1



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

Re: [Openvpn-devel] [PATCH applied] Re: Static-challenge concatenation option

[Selva Nair]

>
> Is the GUI support already committed?  I seem to remember seeing a PR
> for that "weeks ago"... and someone needs to bring Tunnelblick on board.
>

GUI patch is still in a private branch --  will submit a PR soon.

I've sent a note to Jon about the change.

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

Re: [Openvpn-devel] [PATCH applied] Re: Interpret --key and --cert option argument as URI

[Selva Nair]

>
>
>   But anyway, there might be an undefined variable lurking
> in
>
> /* If pkcs#11 Use custom prompt similar to pkcs11-helper */
> if (strstr(prompt, "PKCS#11"))
> {
> struct user_pass up;
> get_user_pass(&up, NULL, "PKCS#11 token", ...
>
> "up" is not initialized, and the first thing get_user_pass_cr() does
> is look at "if (!up->defined)".  So if I'm not misreading this, a
> followup patch to initialize "up" would be good.  At this point it
> might be nice to add a comment explaining how the wrapping of
> "pem_password_callback()" works ;-)
>

Thanks for catching this. A patch is on its way.

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

[Openvpn-devel] [PATCH] Initialize before use struct user_pass in ui_reader()

[selva . nair]

From: Selva Nair 

This was missed in commit 3512e8d3ad
Also add a comment to clarify how pem_password_callback is accessed
in ui_reader().

Change-Id: I82835ff8e1e31e067efd81bfb6e8cd19ee004d9c
Signed-off-by: Selva Nair 
---
 src/openvpn/ssl_openssl.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/src/openvpn/ssl_openssl.c b/src/openvpn/ssl_openssl.c
index 0a38..0d845f4a 100644
--- a/src/openvpn/ssl_openssl.c
+++ b/src/openvpn/ssl_openssl.c
@@ -776,7 +776,9 @@ tls_ctx_load_ecdh_params(struct tls_root_ctx *ctx, const 
char *curve_name)
 
 #if defined(HAVE_OPENSSL_STORE_API)
 /**
- * A wrapper for pem_password_callback for use with OpenSSL UI_METHOD.
+ * A wrapper for password callback for use with OpenSSL UI_METHOD.
+ * The callback is obtained using SSL_CTX_get_default_passwd_cb()
+ * which is set to pem_password_callback() in tls_ctx_set_options().
  */
 static int
 ui_reader(UI *ui, UI_STRING *uis)
@@ -791,6 +793,7 @@ ui_reader(UI *ui, UI_STRING *uis)
 if (strstr(prompt, "PKCS#11"))
 {
 struct user_pass up;
+CLEAR(up);
 get_user_pass(&up, NULL, "PKCS#11 token", 
GET_USER_PASS_MANAGEMENT|GET_USER_PASS_PASSWORD_ONLY);
 UI_set_result(ui, uis, up.password);
 purge_user_pass(&up, true);
-- 
2.40.1



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

[Openvpn-devel] [PATCH v1] Add test for static-challenge concatenation option

[selva . nair]

From: Selva Nair 

Change-Id: I41f6c1f7acb58f30fc4db0e32505018f64fcad28
Signed-off-by: Selva Nair 
Acked-by: Arne Schwabe 
Acked-by: Frank Lichtenheld 
---

This change was reviewed on Gerrit and approved by at least one
developer. I request to merge it to master.

Gerrit URL: 
https://gerrit.openvpn.net/c/openvpn/+/I41f6c1f7acb58f30fc4db0e32505018f64fcad28
This mail reflects revision 1 of this Change.

Signed-off-by line for the author was added as per our policy.

Acked-by according to Gerrit (reflected above):
Arne Schwabe 
Frank Lichtenheld 


diff --git a/tests/unit_tests/openvpn/test_user_pass.c 
b/tests/unit_tests/openvpn/test_user_pass.c
index b43e655..35fe60d 100644
--- a/tests/unit_tests/openvpn/test_user_pass.c
+++ b/tests/unit_tests/openvpn/test_user_pass.c
@@ -311,7 +311,25 @@
 
 reset_user_pass(&up);
 
-flags |= GET_USER_PASS_INLINE_CREDS;
+flags |= GET_USER_PASS_STATIC_CHALLENGE_CONCAT;
+
+expect_string(query_user_exec_builtin, query_user[i].prompt, "Enter UT 
Username:");
+will_return(query_user_exec_builtin, "c1user");
+expect_string(query_user_exec_builtin, query_user[i].prompt, "Enter UT 
Password:");
+will_return(query_user_exec_builtin, "c1password");
+will_return(query_user_exec_builtin, true);
+expect_string(query_user_exec_builtin, query_user[i].prompt, "CHALLENGE: 
Please enter token PIN");
+will_return(query_user_exec_builtin, "0123456");
+will_return(query_user_exec_builtin, true);
+assert_true(get_user_pass_cr(&up, NULL, "UT", flags, challenge));
+assert_true(up.defined);
+assert_string_equal(up.username, "c1user");
+/* password and response concatenated */
+assert_string_equal(up.password, "c1password0123456");
+
+reset_user_pass(&up);
+
+flags = GET_USER_PASS_STATIC_CHALLENGE|GET_USER_PASS_INLINE_CREDS;
 
 /*FIXME: query_user_exec() called even though nothing queued */
 will_return(query_user_exec_builtin, true);


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

[Openvpn-devel] [PATCH] test_pkcs11.c: set file offset to 0 after ftruncate

[selva . nair]

From: Selva Nair 

Currently key and cert file fd's are reused after ftruncate()
without setting the offset to zero. This causes subsequent
data to be written at some finite offset with the hole in
the file automatically filled by zeros. Fix it by calling
lseek() to set the offset to zero.

The test works nevertheless because p11tool seem to generously
ignore any junk before the "BEGIN" marker.

Change-Id: Ib0fe15a4ba18d89216b0288e6cd6be66ed377bd4
Signed-off-by: Selva Nair 
---
 tests/unit_tests/openvpn/test_pkcs11.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tests/unit_tests/openvpn/test_pkcs11.c 
b/tests/unit_tests/openvpn/test_pkcs11.c
index 5518fa15..3fe3e460 100644
--- a/tests/unit_tests/openvpn/test_pkcs11.c
+++ b/tests/unit_tests/openvpn/test_pkcs11.c
@@ -273,6 +273,8 @@ init(void **state)
 
 assert_int_equal(ftruncate(cert_fd, 0), 0);
 assert_int_equal(ftruncate(key_fd, 0), 0);
+assert_int_equal(lseek(cert_fd, 0, SEEK_SET), 0);
+assert_int_equal(lseek(key_fd, 0, SEEK_SET), 0);
 num_certs++;
 }
 
-- 
2.34.1



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

Re: [Openvpn-devel] [PATCH v3] interactive.c: Improve access control for gui<->service pipe

[Selva Nair]

PIPE_UNLIMITED_INSTANCES, 1024, 1024,
> 0, &sa);
> +
> +LocalFree(sd);
> +
>  if (pipe == INVALID_HANDLE_VALUE)
>  {
>  MsgToEventLog(M_SYSERR, TEXT("Could not create named pipe"));
>  return INVALID_HANDLE_VALUE;
>  }
>
> -if (GetSecurityInfo(pipe, SE_KERNEL_OBJECT, DACL_SECURITY_INFORMATION,
> -NULL, NULL, &old_dacl, NULL, &sd) !=
> ERROR_SUCCESS)
> -{
> -MsgToEventLog(M_SYSERR, TEXT("Could not get pipe security info"));
> -return CloseHandleEx(&pipe);
> -}
> -
> -if (SetEntriesInAcl(2, ea, old_dacl, &new_dacl) != ERROR_SUCCESS)
> -{
> -MsgToEventLog(M_SYSERR, TEXT("Could not set entries in new acl"));
> -return CloseHandleEx(&pipe);
> -}
> -
> -if (SetSecurityInfo(pipe, SE_KERNEL_OBJECT, DACL_SECURITY_INFORMATION,
> -NULL, NULL, new_dacl, NULL) != ERROR_SUCCESS)
> -{
> -MsgToEventLog(M_SYSERR, TEXT("Could not set pipe security info"));
> -return CloseHandleEx(&pipe);
> -}
> -
>  return pipe;
>  }
>
> --
> 2.42.0.windows.2
>

The only change from the 2.6 version is openvpn_swprintf -> swprintf.
I did not retest the build or executable, but looks good to me based on 2.6
tests.

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

Re: [Openvpn-devel] [PATCH] interactive.c: Improve access control for gui<->service pipe

[Selva Nair]

Forgot to add:

This applies only to 2.6 -- for master we'll need a rebased version.

On Wed, Jun 19, 2024 at 9:51 AM Selva Nair  wrote:

>
>
> On Wed, Jun 19, 2024 at 9:47 AM Lev Stipakov  wrote:
>
>> At the moment everyone but anonymous are permitted
>> to create a pipe with the same name as interactive service creates,
>> which makes it possible for malicious process with SeImpersonatePrivilege
>> impersonate as local user.
>>
>> This hardens the security of the pipe, making it possible only for
>> processes running as SYSTEM (such as interactive service) create the
>> pipe with the same name.
>>
>> While on it, replace EXPLICIT_ACCESS structures with SDDL string.
>>
>> CVE: 2024-4877
>>
>> Change-Id: I35e783b79a332d247606e05a39e41b4d35d39b5d
>> Reported by: Zeze with TeamT5 
>> Signed-off-by: Lev Stipakov 
>> ---
>>  v2:
>>   - ensure that sd is freed even if pipe creation failed
>>   - added Reported-By
>>
>
>  Acked-by: Selva Nair 
>
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Re: [Openvpn-devel] [PATCH] interactive.c: Improve access control for gui<->service pipe

[Selva Nair]

On Wed, Jun 19, 2024 at 9:47 AM Lev Stipakov  wrote:

> At the moment everyone but anonymous are permitted
> to create a pipe with the same name as interactive service creates,
> which makes it possible for malicious process with SeImpersonatePrivilege
> impersonate as local user.
>
> This hardens the security of the pipe, making it possible only for
> processes running as SYSTEM (such as interactive service) create the
> pipe with the same name.
>
> While on it, replace EXPLICIT_ACCESS structures with SDDL string.
>
> CVE: 2024-4877
>
> Change-Id: I35e783b79a332d247606e05a39e41b4d35d39b5d
> Reported by: Zeze with TeamT5 
> Signed-off-by: Lev Stipakov 
> ---
>  v2:
>   - ensure that sd is freed even if pipe creation failed
>   - added Reported-By
>

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

[Openvpn-devel] [PATCH v2] Document that auth-user-pass may be inlined

[selva . nair]

From: Selva Nair 

Commits 7d48d31b, 39619b7f added support for inlining username
and, optionally, password.
Add a description of its usage in the man page.

Github: resolves OpenVPN/openvpn#370

Change-Id: I7a1765661f7676eeba8016024080fd1026220ced
Signed-off-by: Selva Nair 
---
v2: Add '--' prefix when referring to auth-user-pass
and mention related github issue
 doc/man-sections/client-options.rst | 11 +++
 doc/man-sections/inline-files.rst   |  2 +-
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/doc/man-sections/client-options.rst 
b/doc/man-sections/client-options.rst
index b92b1a46..b75fe5bd 100644
--- a/doc/man-sections/client-options.rst
+++ b/doc/man-sections/client-options.rst
@@ -73,6 +73,17 @@ configuration.
   If ``up`` is omitted, username/password will be prompted from the
   console.
 
+  This option can also be inlined
+  ::
+
+
+username
+[password]
+
+
+  where password is optional, and will be prompted from the console if
+  missing.
+
   The server configuration must specify an ``--auth-user-pass-verify``
   script to verify the username/password provided by the client.
 
diff --git a/doc/man-sections/inline-files.rst 
b/doc/man-sections/inline-files.rst
index f46301e8..4dba73c9 100644
--- a/doc/man-sections/inline-files.rst
+++ b/doc/man-sections/inline-files.rst
@@ -5,7 +5,7 @@ OpenVPN allows including files in the main configuration for 
the ``--ca``,
 ``--cert``, ``--dh``, ``--extra-certs``, ``--key``, ``--pkcs12``,
 ``--crl-verify``, ``--http-proxy-user-pass``, ``--tls-auth``,
 ``--auth-gen-token-secret``, ``--peer-fingerprint``, ``--tls-crypt``,
-``--tls-crypt-v2`` and ``--verify-hash`` options.
+``--tls-crypt-v2``, ``--verify-hash`` and ``--auth-user-pass`` options.
 
 Each inline file started by the line  and ended by the line
 
-- 
2.34.1



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

[Openvpn-devel] [PATCH] Document that auth-user-pass may be inlined

[selva . nair]

From: Selva Nair 

Commits 7d48d31b, 39619b7f added support for inlining username
and, optionally, password.
Add a description of its usage in the man page.

Change-Id: I7a1765661f7676eeba8016024080fd1026220ced
Signed-off-by: Selva Nair 
---
Does this have to go through gerrit?

 doc/man-sections/client-options.rst | 11 +++
 doc/man-sections/inline-files.rst   |  2 +-
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/doc/man-sections/client-options.rst 
b/doc/man-sections/client-options.rst
index b92b1a46..b75fe5bd 100644
--- a/doc/man-sections/client-options.rst
+++ b/doc/man-sections/client-options.rst
@@ -73,6 +73,17 @@ configuration.
   If ``up`` is omitted, username/password will be prompted from the
   console.
 
+  This option can also be inlined
+  ::
+
+
+username
+[password]
+
+
+  where password is optional, and will be prompted from the console if
+  missing.
+
   The server configuration must specify an ``--auth-user-pass-verify``
   script to verify the username/password provided by the client.
 
diff --git a/doc/man-sections/inline-files.rst 
b/doc/man-sections/inline-files.rst
index f46301e8..ad02c855 100644
--- a/doc/man-sections/inline-files.rst
+++ b/doc/man-sections/inline-files.rst
@@ -5,7 +5,7 @@ OpenVPN allows including files in the main configuration for 
the ``--ca``,
 ``--cert``, ``--dh``, ``--extra-certs``, ``--key``, ``--pkcs12``,
 ``--crl-verify``, ``--http-proxy-user-pass``, ``--tls-auth``,
 ``--auth-gen-token-secret``, ``--peer-fingerprint``, ``--tls-crypt``,
-``--tls-crypt-v2`` and ``--verify-hash`` options.
+``--tls-crypt-v2``, ``--verify-hash`` and ``auth-user-pass`` options.
 
 Each inline file started by the line  and ended by the line
 
-- 
2.34.1



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

Re: [Openvpn-devel] [PATCH] man: extend description for "dhcp-option DNS" on Windows

[Selva Nair]

Hi,

On Tue, Sep 5, 2023 at 5:41 PM Antonio Quartulli  wrote:

> From: Antonio Quartulli 
>
> Add an important detail about the DNS configured via this option
> to be an "interface-specific" DNS. This detail is important when
> troubleshooting DNS issues since this logic will bypass the
> routing table.
>
> Signed-off-by: Antonio Quartulli 
> ---
>  doc/man-sections/vpn-network-options.rst | 6 ++
>  1 file changed, 6 insertions(+)
>
> diff --git a/doc/man-sections/vpn-network-options.rst
> b/doc/man-sections/vpn-network-options.rst
> index b7f33acd..17c7d4a2 100644
> --- a/doc/man-sections/vpn-network-options.rst
> +++ b/doc/man-sections/vpn-network-options.rst
> @@ -158,6 +158,12 @@ routing.
>  Set primary domain name server IPv4 or IPv6 address.
>  Repeat this option to set secondary DNS server addresses.
>
> +On Windows this option sets an "interface-specific" DNS, meaning
> +that the system will try to reach it via the VPN interface,


even if the routing table says otherwise.


> If the wanted DNS is not
>

If the desired DNS server is not

+expected to be reached via VPN, please don't use this option, but
>

please don't --> do not


> +rather set the DNS manually (or via DHCP).
>

delete "rather"


> +
>  Note: DNS IPv6 servers are currently set using netsh (the existing
>  DHCP code can only do IPv4 DHCP, and that protocol only permits
>  IPv4 addresses anywhere). The option will be put into the
>

That said, I'm not convinced this is the right way to document this. IMO,
using "--dhcp-option DNS n.n.n" to set a DNS server not reachable through
the tunnel is a misuse of that option on any platform.

Even on "non-windows" one shouldn't do that as it's very reasonable for a
script that handles it to set it on the interface, instead of globally, if
the platform permits it. For example, using systemd-resolved on Linux
(though I'm not totally sure how exactly interface-specific DNS works in
this case).

So, if at all, why not state that the DNS server specified here should be
reachable through the tunnel irrespective of the platform?

Regards,

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

[Openvpn-devel] [PATCH] Log OpenSSL errors on failure to set certificate

[selva . nair]

From: Selva Nair 

Currently we log a bogus error message saying private key password verification
failed when SSL_CTX_use_cert_and_key() fails in pkcs11_openssl.c. Instead print
OpenSSL error queue and exit promptly.

Also log OpenSSL errors when SSL_CTX_use_certiifcate() fails in cryptoapi.c
and elsewhere. Such logging could be useful especially when the ceritficate is
rejected by OpenSSL due to stricter security restrictions in recent versions
of the library.

Change-Id: Ic7ec25ac0503a91d5869b8da966d0065f264af22
Signed-off-by: Selva Nair 
---
 src/openvpn/cryptoapi.c   |  2 ++
 src/openvpn/pkcs11_openssl.c  |  6 --
 src/openvpn/ssl_openssl.c |  2 ++
 tests/unit_tests/openvpn/test_cryptoapi.c | 11 +++
 tests/unit_tests/openvpn/test_pkcs11.c| 11 +++
 5 files changed, 30 insertions(+), 2 deletions(-)

diff --git a/src/openvpn/cryptoapi.c b/src/openvpn/cryptoapi.c
index 3b92e481..f7e5b674 100644
--- a/src/openvpn/cryptoapi.c
+++ b/src/openvpn/cryptoapi.c
@@ -51,6 +51,7 @@
 #include "openssl_compat.h"
 #include "win32.h"
 #include "xkey_common.h"
+#include "crypto_openssl.h"
 
 #ifndef HAVE_XKEY_PROVIDER
 
@@ -505,6 +506,7 @@ SSL_CTX_use_CryptoAPI_certificate(SSL_CTX *ssl_ctx, const 
char *cert_prop)
 if (SSL_CTX_use_certificate(ssl_ctx, cert)
 && SSL_CTX_use_PrivateKey(ssl_ctx, privkey))
 {
+crypto_print_openssl_errors(M_WARN);
 ret = 1;
 }
 
diff --git a/src/openvpn/pkcs11_openssl.c b/src/openvpn/pkcs11_openssl.c
index 40080efa..aa0819f9 100644
--- a/src/openvpn/pkcs11_openssl.c
+++ b/src/openvpn/pkcs11_openssl.c
@@ -302,7 +302,8 @@ xkey_load_from_pkcs11h(pkcs11h_certificate_t certificate,
 
 if (!SSL_CTX_use_cert_and_key(ctx->ctx, x509, pkey, NULL, 0))
 {
-msg(M_WARN, "PKCS#11: Failed to set cert and private key for OpenSSL");
+crypto_print_openssl_errors(M_WARN);
+msg(M_FATAL, "PKCS#11: Failed to set cert and private key for 
OpenSSL");
 goto cleanup;
 }
 ret = 1;
@@ -369,7 +370,8 @@ pkcs11_init_tls_session(pkcs11h_certificate_t certificate,
 
 if (!SSL_CTX_use_certificate(ssl_ctx->ctx, x509))
 {
-msg(M_WARN, "PKCS#11: Cannot set certificate for openssl");
+crypto_print_openssl_errors(M_WARN);
+msg(M_FATAL, "PKCS#11: Cannot set certificate for openssl");
 goto cleanup;
 }
 ret = 0;
diff --git a/src/openvpn/ssl_openssl.c b/src/openvpn/ssl_openssl.c
index 0b310de3..b5cc9a7f 100644
--- a/src/openvpn/ssl_openssl.c
+++ b/src/openvpn/ssl_openssl.c
@@ -857,6 +857,7 @@ tls_ctx_load_pkcs12(struct tls_root_ctx *ctx, const char 
*pkcs12_file,
 /* Load Certificate */
 if (!SSL_CTX_use_certificate(ctx->ctx, cert))
 {
+crypto_print_openssl_errors(M_WARN);
 crypto_msg(M_FATAL, "Cannot use certificate");
 }
 
@@ -1007,6 +1008,7 @@ tls_ctx_load_cert_file(struct tls_root_ctx *ctx, const 
char *cert_file,
 end:
 if (!ret)
 {
+crypto_print_openssl_errors(M_WARN);
 if (cert_file_inline)
 {
 crypto_msg(M_FATAL, "Cannot load inline certificate file");
diff --git a/tests/unit_tests/openvpn/test_cryptoapi.c 
b/tests/unit_tests/openvpn/test_cryptoapi.c
index 008f41c0..d90bfc35 100644
--- a/tests/unit_tests/openvpn/test_cryptoapi.c
+++ b/tests/unit_tests/openvpn/test_cryptoapi.c
@@ -58,6 +58,17 @@ management_query_pk_sig(struct management *man, const char 
*b64_data,
 return NULL;
 }
 
+/* replacement for crypto_print_openssl_errors() */
+void
+crypto_print_openssl_errors(const unsigned int flags)
+{
+unsigned long e;
+while ((e = ERR_get_error()))
+{
+msg(flags, "OpenSSL error %lu: %s\n", e, ERR_error_string(e, NULL));
+}
+}
+
 /* tls_libctx is defined in ssl_openssl.c which we do not want to compile in */
 OSSL_LIB_CTX *tls_libctx;
 
diff --git a/tests/unit_tests/openvpn/test_pkcs11.c 
b/tests/unit_tests/openvpn/test_pkcs11.c
index 235cc43f..b6c130ec 100644
--- a/tests/unit_tests/openvpn/test_pkcs11.c
+++ b/tests/unit_tests/openvpn/test_pkcs11.c
@@ -44,6 +44,17 @@
 
 struct management *management; /* global */
 
+/* replacement for crypto_print_openssl_errors() */
+void
+crypto_print_openssl_errors(const unsigned int flags)
+{
+unsigned long e;
+while ((e = ERR_get_error()))
+{
+msg(flags, "OpenSSL error %lu: %s\n", e, ERR_error_string(e, NULL));
+}
+}
+
 /* stubs for some unused functions instead of pulling in too many dependencies 
*/
 int
 parse_line(const char *line, char **p, const int n, const char *file,
-- 
2.34.1



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

Re: [Openvpn-devel] pkcs11 config changes from 2.5.4 to 2.6.6 ?

[Selva Nair]

>
>
>
> > Good point. But, unless the config has "tls-cert-profile foo", we still
> > default to legacy and call SSL_CTX_set_security_level(ctx, 1), isn't it?
> > Wouldn't that allow SHA1 with 3.1.x ?
>
> For SHA1 you need security 0 aka tls-cert-profile insecure.
>
> But we might update OpenVPN to longer fiddle with the default security
> level in OpenSSL 3+ to avoid downgrading security from 2 to 1 on OpenSSL
> 3.1
>

Even OpenSSL 3.0 is built with -DOPENSSL_TLS_SECURITY_LEVEL=2
in Debian and Ubuntu packages (and likely other distributions). Looks like
a good idea to stop
overriding it by default.

We could also improve logging when SSL_CTX_set_certificate and similar fail
by printing OpenSSL error queue.

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

Re: [Openvpn-devel] pkcs11 config changes from 2.5.4 to 2.6.6 ?

[Selva Nair]

Hi Mike

I misunderstood Arne's comment. We default to security level 1 but that
forbids SHA1 signatures in OpenSSL 3.0+.

Could you test with "tls-cert-profile Insecure" in the config file? It's
not recommended but useful to check.

Thanks,

Selva

On Thu, Sep 28, 2023 at 7:08 PM mike tancsa  wrote:

> Hi Selva,
>
> Thank you for looking!
>
> My guess is that something in the certificate or private key is not to
> OpenSSL 3.1's liking and it rejects it. Is there any way for you to check
> the
> contents of the token independently using a tool linked against OpenSSL
> 3.1 ?
>
> What am I looking for in that case ?  Taking a look at the cert just with
> openssl 3.0 on FreeBSD releng14 it seems ok with it. Same with the Windows
> version 3.1.x that comes with OpenVPN. Is it possible it doesnt like the
> sha1RSA sig ?
>
> # openssl version
> OpenSSL 3.0.10 1 Aug 2023 (Library: OpenSSL 3.0.10 1 Aug 2023)
> #
>
> Certificate:
> Data:
> Version: 3 (0x2)
> Serial Number: 7109 (0x1bc5)
> Signature Algorithm: sha1WithRSAEncryption
> Issuer: C = CA, ST = ON, L = Cambridge, O = Sentex CA, CN = Sentex
> private1test CA CA, emailAddress = m...@sentex.ca
> Validity
> Not Before: Sep 27 19:43:01 2023 GMT
> Not After : Nov 13 19:43:01 2033 GMT
> Subject: C = CA, ST = ON, L = Cambridge, O = Sentex CA, OU =
> win10, CN = test123456mdt, emailAddress = m...@sentex.ca
> Subject Public Key Info:
> Public Key Algorithm: rsaEncryption
> Public-Key: (2048 bit)
> Modulus:
> 00:f5:e0:27:b5:28:0a:f8:a9:ce:13:33:a2:ca:27:
>
> ...
>
> ac:a8:b6:55:bb:a3:a4:43:e5:74:05:aa:c8:69:3d:
> ed:ef
> Exponent: 65537 (0x10001)
> X509v3 extensions:
> X509v3 Basic Constraints:
> CA:FALSE
> Netscape Comment:
> Easy-RSA Generated Certificate
> X509v3 Subject Key Identifier:
> 74:72:3A:87:0D:34:7B:1E:11:C6:18:D2:41:99:C6:5E:D1:8A:81:95
> X509v3 Authority Key Identifier:
>
> keyid:4F:A0:B0:94:92:6F:24:A7:D4:C6:93:A6:AA:25:63:6C:ED:1E:E3:8C
> DirName:/C=CA/ST=ON/L=Cambridge/O=Sentex Parklands
> CA/CN=Sentex Parklands CA CA/emailAddress=ppsupp...@sentex.ca
> serial:F5:3E:37:76:69:AC:EF:EC
> X509v3 Extended Key Usage:
> TLS Web Client Authentication
> X509v3 Key Usage:
> Digital Signature
> Signature Algorithm: sha1WithRSAEncryption
> Signature Value:
> 10:72:36:db:5c:f3:f5:fb:52:82:c7:4c:72:8f:31:ae:
>
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Re: [Openvpn-devel] pkcs11 config changes from 2.5.4 to 2.6.6 ?

[Selva Nair]

On Thu, Sep 28, 2023 at 8:55 PM Arne Schwabe  wrote:

>
> Am 29.09.2023 um 01:08 schrieb mike tancsa:
>
> Hi Selva,
>
> Thank you for looking!
>
> My guess is that something in the certificate or private key is not to
> OpenSSL 3.1's liking and it rejects it. Is there any way for you to check
> the
> contents of the token independently using a tool linked against OpenSSL
> 3.1 ?
>
> What am I looking for in that case ?  Taking a look at the cert just with
> openssl 3.0 on FreeBSD releng14 it seems ok with it. Same with the Windows
> version 3.1.x that comes with OpenVPN. Is it possible it doesnt like the
> sha1RSA sig ?
>
> OpenSSL 3.0 has security 1 by default (OpenSSL 3.1 has 2 by default)  and
> that does not allow SHA1 signatures anymore. See
> https://www.openssl.org/docs/man3.1/man3/SSL_CTX_set_security_level.html
>

Good point. But, unless the config has "tls-cert-profile foo", we still
default to legacy and call SSL_CTX_set_security_level(ctx, 1), isn't it?
Wouldn't that allow SHA1 with 3.1.x ?

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

Re: [Openvpn-devel] pkcs11 config changes from 2.5.4 to 2.6.6 ?

[Selva Nair]

Hi Mike,

On Thu, Sep 28, 2023 at 5:28 PM mike tancsa  wrote:

> I am starting to test out 2.6.6 with a config that worked in 2.5.4 but
> am getting a failure con connect.  I did have a look through the
> Changes.rst file but didnt see anything different ? The only pkcs11 bits
> I have in the config are
>
> pkcs11-providers eTpkcs11.dll
> pkcs11-id 'pkcs11:model=eToken;token='
>
> and the same config works with the older version. Are there new
> directives I need to add ?  This is an Gemalto/Thales etoken. Again, it
> works fine in this environment with the only change being the version of
> OpenVPN.
>

The main change is upgrade to OpenSSL 3.x which seems
not to like the certificate or key. Normally it should just work with no
changes
to the config.


>
> 2023-09-28 17:05:12 us=578000 PKCS#11: Failed to set cert and private
> key for OpenSSL
>

This implies the call to OpenSSL's "SSL_CTX_set_cert_and_key()" failed.
The certificate and private key handle from the token are acquired before
this
and set in  xkey-provider --- both of those tasks have completed without
errors.
Very unusual and rare to error out at this point.

Unfortunately we do not log the reason for this failure. Instead
we clear OpenSSL's error queue and print a generic error saying private
key password verification failed. A retry is triggered if "auth-retry" is
set to "interact",
else we exit as happened in your case.


> 2023-09-28 17:05:12 us=578000 Error: private key password verification
> failed
>

Not a very useful error message.

My guess is that something in the certificate or private key is not to
OpenSSL 3.1's liking and it rejects it. Is there any way for you to check
the
contents of the token independently using a tool linked against OpenSSL 3.1
?

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

Re: [Openvpn-devel] [PATCH v2] show extra info for OpenSSL errors

[Selva Nair]

On Fri, Aug 11, 2023 at 8:16 AM Arne Schwabe  wrote:

> This also shows the extra data from the OpenSSL error function that
> can contain extra information. For example, the command
>
> openvpn --providers vollbit
>
> will print out (on macOS):
>
>  OpenSSL: error:12800067:DSO support routines::could not load the
> shared 
> library:filename(/opt/homebrew/Cellar/openssl@3/3.1.1_1/lib/ossl-modules/vollbit.dylib):
> dlopen(/opt/homebrew/Cellar/openssl@3/3.1.1_1/lib/ossl-modules/vollbit.dylib,
> 0x0002): tried: 
> '/opt/homebrew/Cellar/openssl@3/3.1.1_1/lib/ossl-modules/vollbit.dylib'
> (no such file),
> '/System/Volumes/Preboot/Cryptexes/OS/opt/homebrew/Cellar/openssl@3/3.1.1_1/lib/ossl-modules/vollbit.dylib'
> (no such file), 
> '/opt/homebrew/Cellar/openssl@3/3.1.1_1/lib/ossl-modules/vollbit.dylib'
> (no such file)
>
> Patch v2: Format message more like current messages
>
> Change-Id: Ic2ee89937dcd85721bcacd1b700a20c640364f80
> Signed-off-by: Arne Schwabe 
> ---
>  src/openvpn/crypto_openssl.c | 21 +++--
>  src/openvpn/openssl_compat.h | 12 
>  2 files changed, 31 insertions(+), 2 deletions(-)
>
> diff --git a/src/openvpn/crypto_openssl.c b/src/openvpn/crypto_openssl.c
> index b043bb95e..22c6d6840 100644
> --- a/src/openvpn/crypto_openssl.c
> +++ b/src/openvpn/crypto_openssl.c
> @@ -238,9 +238,16 @@ void
>  crypto_print_openssl_errors(const unsigned int flags)
>  {
>  unsigned long err = 0;
> +int line, errflags;
> +const char *file, *data, *func;
>
> -while ((err = ERR_get_error()))
> +while ((err = ERR_get_error_all(&file, &line, &func, &data,
> &errflags)) != 0)
>  {
> +if (!(errflags & ERR_TXT_STRING))
> +{
> +data = "";
> +}
> +
>  /* Be more clear about frequently occurring "no shared cipher"
> error */
>  if (ERR_GET_REASON(err) == SSL_R_NO_SHARED_CIPHER)
>  {
> @@ -258,7 +265,17 @@ crypto_print_openssl_errors(const unsigned int flags)
>  "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));
> +
> +/* print file and line if verb >=8 */
> +if (!check_debug_level(D_TLS_DEBUG_MED))
> +{
> +msg(flags, "OpenSSL: %s:%s", ERR_error_string(err, NULL),
> data);
> +}
> +else
> +{
> +msg(flags, "OpenSSL: %s:%s:%s:%d:%s", ERR_error_string(err,
> NULL),
> +data, file, line, func);
> +}
>  }
>  }
>
> diff --git a/src/openvpn/openssl_compat.h b/src/openvpn/openssl_compat.h
> index ffb64adf6..736ce1bd5 100644
> --- a/src/openvpn/openssl_compat.h
> +++ b/src/openvpn/openssl_compat.h
> @@ -43,6 +43,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>
>  /* Functionality missing in 1.1.0 */
>  #if OPENSSL_VERSION_NUMBER < 0x10101000L &&
> !defined(ENABLE_CRYPTO_WOLFSSL)
> @@ -799,6 +800,17 @@ EVP_MD_free(const EVP_MD *md)
>  /* OpenSSL 1.1.1 and lower use only const EVP_MD, nothing to free */
>  }
>
> +static inline unsigned long
> +ERR_get_error_all(const char **file, int *line,
> +  const char **func,
> +  const char **data, int *flags)
> +{
> +static const char *empty = "";
> +*func = empty;
> +long err = ERR_get_error_line_data(file, line, data, flags);
>

I think you missed to change that to "unsigned long err = "

+return err;
> +}
> +
>  #endif /* OPENSSL_VERSION_NUMBER < 0x3000L */
>
>  #endif /* OPENSSL_COMPAT_H_ */
> --
> 2.39.2 (Apple Git-143)
>

The above could be handled at merge time, so:
Acked-by: Selva Nair 
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Re: [Openvpn-devel] [PATCH 2/2] signal_reset(): combine check and reset operations

[Selva Nair]

On Tue, Jul 25, 2023 at 6:18 AM Frank Lichtenheld 
wrote:

> On Sat, Jan 28, 2023 at 04:59:01PM -0500, selva.n...@gmail.com wrote:
> > From: Selva Nair 
> >
> > - "if (sig == X) signal_reset(sig)" now becomes
> >   "signal_reset(sig, X)" so that the check and assignment
> >   can be done in one place where signals are masked.
> >   This is required to avoid change of signal state between
> >   check and reset operations.
> >
> > - Avoid resetting the signal except when absolutely necessary
> >   (resetting has the potential of losing signals)
> >
> > - In 'pre_init_signal_catch()', when certain low priority signals
> >   are set to SIG_IGN, clear any pending signals of the same
> >   type. Also, reset signal at the end of the SIGUSR1 and
> >   SIGHUP loops where their values are checked instead of later. This
> >   avoids the need for 'signal_reset()' after SIGHUP or in
> 'init_instance()'
> >   which could cause a signal like SIGTERM to be lost.
> >
> [...]
> > diff --git a/src/openvpn/openvpn.c b/src/openvpn/openvpn.c
> > index cba58276..ad0aa8a2 100644
> > --- a/src/openvpn/openvpn.c
> > +++ b/src/openvpn/openvpn.c
> > @@ -194,7 +194,6 @@ openvpn_main(int argc, char *argv[])
> >  context_clear_all_except_first_time(&c);
> >
> >  /* static signal info object */
> > -CLEAR(siginfo_static);
> >  c.sig = &siginfo_static;
>
> Is that actually save? Doesn't that mean that siginfo_static might be
> used uninitialized?
>

siginfo_static is a global variable with no explicit initialization, so it
is guaranteed to
get initialized to {0} at start up.

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

Re: [Openvpn-devel] [PATCH applied] Re: Improve signal handling using POSIX sigaction

[Selva Nair]

On Sun, Jul 23, 2023 at 5:28 AM Gert Doering  wrote:

>
> The actual "change to POSIX ways" part of this is fairly trivial
> and easy to understand :-) - though I do wonder why you're using
> an extra variable for block_mask -> sa.sa_mask, and not using
> sigfillset(&sa.sa_mask) - at least on BSD on Linux, both are sigset_t,
> so it should do the same thing in a slightly more direct way.
>

I can't recall any particular reason for having that intermediate variable.
It does look redundant.

Now I wish someone could review 2/2 of this patch-set. That's where further
potential
loss of signal during signal-reset is avoided.

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

Re: [Openvpn-devel] [PATCH] show extra info for OpenSSL errors

[Selva Nair]

Hi,

This looks good except that the format of the log could be kept closer to
the current one:

On Fri, Jul 7, 2023 at 2:59 PM Arne Schwabe  wrote:

> This also shows the extra data from the OpenSSL error function that
> can contain extra information. For example, the command
>
> openvpn --providers vollbit
>
> will print out (on macOS):
>
>  OpenSSLerror:12800067:DSO support routines::could not load the shared
> library:filename(/opt/homebrew/Cellar/openssl@3/3.1.1_1/lib/ossl-modules/vollbit.dylib):
> dlopen(/opt/homebrew/Cellar/openssl@3/3.1.1_1/lib/ossl-modules/vollbit.dylib,
> 0x0002): tried: 
> '/opt/homebrew/Cellar/openssl@3/3.1.1_1/lib/ossl-modules/vollbit.dylib'
> (no such file),
> '/System/Volumes/Preboot/Cryptexes/OS/opt/homebrew/Cellar/openssl@3/3.1.1_1/lib/ossl-modules/vollbit.dylib'
> (no such file), 
> '/opt/homebrew/Cellar/openssl@3/3.1.1_1/lib/ossl-modules/vollbit.dylib'
> (no such file)
>
> Change-Id: Ic2ee89937dcd85721bcacd1b700a20c640364f80
> Signed-off-by: Arne Schwabe 
> ---
>  src/openvpn/crypto_openssl.c | 20 ++--
>  src/openvpn/openssl_compat.h | 12 
>  2 files changed, 30 insertions(+), 2 deletions(-)
>
> diff --git a/src/openvpn/crypto_openssl.c b/src/openvpn/crypto_openssl.c
> index b043bb95e..d6916fc9b 100644
> --- a/src/openvpn/crypto_openssl.c
> +++ b/src/openvpn/crypto_openssl.c
> @@ -238,9 +238,16 @@ void
>  crypto_print_openssl_errors(const unsigned int flags)
>  {
>  unsigned long err = 0;
> +int line, errflags;
> +const char *file, *data, *func;
>
> -while ((err = ERR_get_error()))
> +while ((err = ERR_get_error_all(&file, &line, &func, &data,
> &errflags)) != 0)
>  {
> +if (!(errflags & ERR_TXT_STRING))
> +{
> +data = "";
> +}
> +
>  /* Be more clear about frequently occurring "no shared cipher"
> error */
>  if (ERR_GET_REASON(err) == SSL_R_NO_SHARED_CIPHER)
>  {
> @@ -258,7 +265,16 @@ crypto_print_openssl_errors(const unsigned int flags)
>  "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));
> +
> +/* print file and line if verb >=8 */
> +if (!check_debug_level(D_TLS_DEBUG_MED))
> +{
> +msg(flags, "OpenSSL%s:%s", ERR_error_string(err, NULL), data);
>

Better to retain the ':" after "OpenSSL" in case anyone is parsing the logs

msg(flags, "OpenSSL: %s:%s, ERR_error_string(err, NULL), data);


> +}
> +else
> +{
> +msg(flags, "OpenSSL (%s:%d %s): %s:%s", file, line, func,
> ERR_error_string(err, NULL), data);
>

Again, I think we could keep the same order as above with additional info
(file:line:func) moved to the end with a consistent use of the delimiter ':'

msg(flags, "OpenSSL: %s:%s:%s:%s:%s, ERR_error_string(err, NULL), data,
file, line, func);


> +}
>  }
>  }
>
> diff --git a/src/openvpn/openssl_compat.h b/src/openvpn/openssl_compat.h
> index ffb64adf6..736ce1bd5 100644
> --- a/src/openvpn/openssl_compat.h
> +++ b/src/openvpn/openssl_compat.h
> @@ -43,6 +43,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>
>  /* Functionality missing in 1.1.0 */
>  #if OPENSSL_VERSION_NUMBER < 0x10101000L &&
> !defined(ENABLE_CRYPTO_WOLFSSL)
> @@ -799,6 +800,17 @@ EVP_MD_free(const EVP_MD *md)
>  /* OpenSSL 1.1.1 and lower use only const EVP_MD, nothing to free */
>  }
>
> +static inline unsigned long
> +ERR_get_error_all(const char **file, int *line,
> +  const char **func,
> +  const char **data, int *flags)
> +{
> +static const char *empty = "";
> +*func = empty;
> +long err = ERR_get_error_line_data(file, line, data, flags);
>

unsigned long err = 


> +return err;
> +}
> +
>  #endif /* OPENSSL_VERSION_NUMBER < 0x3000L */
>
>  #endif /* OPENSSL_COMPAT_H_ */
> --
>
> https://lists.sourceforge.net/lists/listinfo/openvpn-devel
>
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

[Openvpn-devel] Fwd: [PATCH] tun.c: enclose DNS domain in single quotes in WMIC call

[Selva Nair]

Hi,

On Mon, Jul 10, 2023 at 7:22 AM Lev Stipakov  wrote:

> From: Lev Stipakov 
>
> This is needed to support domains with hyphens.
>
> Not using double quotes here, since our code replaces
> them with underbars (see
> https://github.com/OpenVPN/openvpn/blob/master/src/openvpn/win32.c#L980).
>
> Fixes https://github.com/OpenVPN/openvpn/issues/363


>
> Change-Id: Iab536922d0731635cef529b5caf542f637b8d491
> Signed-off-by: Lev Stipakov 
> ---
>  src/openvpn/tun.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c
> index d1fd6def..60974208 100644
> --- a/src/openvpn/tun.c
> +++ b/src/openvpn/tun.c
> @@ -333,7 +333,7 @@ do_dns_domain_wmic(bool add, const struct tuntap *tt)
>  }
>
>  struct argv argv = argv_new();
> -argv_printf(&argv, "%s%s nicconfig where (InterfaceIndex=%ld) call
> SetDNSDomain %s",
> +argv_printf(&argv, "%s%s nicconfig where (InterfaceIndex=%ld) call
> SetDNSDomain '%s'",
>  get_win_sys_path(), WMIC_PATH_SUFFIX, tt->adapter_index,
> add ? tt->options.domain : "");
>  exec_command("WMIC", &argv, 1, M_WARN);
>

Quoting is required as wmic interprets characters such as hyphen and /.
Double quotes would have been better (as in interactive.c) as there are
some cases where characters within single quotes get interpreted special
(like 'foo>bar' vs "foo>bar").

That said, for valid domain names, the only expected characters are
alpha-numeric, hyphen and period, and single quotes should work. I have
only tested this using wmic command line, not the resulting openvpn.exe.

Acked-by: Selva Nair 

P.S.
We probably need to sanitize the user-supplied domain name before passing
it to wmic.
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Re: [Openvpn-devel] [PATCH v2 2/2] Fix CR_RESPONSE mangaement message using wrong key_id

[Selva Nair]

It seems I missed to send this ACK to the list.. Here it is.

On Mon, May 22, 2023 at 6:12 AM Arne Schwabe  wrote:

> the management interface expects the management key id instead
> of the openvpn key id. In the past they often were the same for low ids
> which hid the bug quite well.
>
> Also do not pick uninitialised keystates (management key_id is not valid
> in these).
>
> Patch v2: do not add logging
>
> Change-Id: If9fa1165a0e886b570b3738546ed810a32367cbe
> Signed-off-by: Arne Schwabe 
> ---
>  src/openvpn/push.c   | 4 ++--
>  src/openvpn/ssl_common.h | 2 +-
>  2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/src/openvpn/push.c b/src/openvpn/push.c
> index 8e9627199..8f0a534ac 100644
> --- a/src/openvpn/push.c
> +++ b/src/openvpn/push.c
> @@ -267,9 +267,9 @@ receive_cr_response(struct context *c, const struct
> buffer *buffer)
>  struct tls_session *session = &c->c2.tls_multi->session[TM_ACTIVE];
>  struct man_def_auth_context *mda = session->opt->mda_context;
>  struct env_set *es = session->opt->es;
> -int key_id = get_primary_key(c->c2.tls_multi)->key_id;
> +unsigned int mda_key_id =
> get_primary_key(c->c2.tls_multi)->mda_key_id;
>
> -management_notify_client_cr_response(key_id, mda, es, m);
> +management_notify_client_cr_response(mda_key_id, mda, es, m);
>  #endif
>  #if ENABLE_PLUGIN
>  verify_crresponse_plugin(c->c2.tls_multi, m);
> diff --git a/src/openvpn/ssl_common.h b/src/openvpn/ssl_common.h
> index ebfd25432..be0f18746 100644
> --- a/src/openvpn/ssl_common.h
> +++ b/src/openvpn/ssl_common.h
> @@ -733,7 +733,7 @@ get_key_by_management_key_id(struct tls_multi *multi,
> unsigned int mda_key_id)
>  for (int i = 0; i < KEY_SCAN_SIZE; ++i)
>  {
>  struct key_state *ks = get_key_scan(multi, i);
> -if (ks->mda_key_id == mda_key_id)
> +if (ks->mda_key_id == mda_key_id && ks->state > S_UNDEF)
>  {
>  return ks;
>  }
> --
> 2.39.2 (Apple Git-143)
>
>

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

Re: [Openvpn-devel] [PATCH 1/2] Improve signal handling using POSIX sigaction

[Selva Nair]

On Mon, May 29, 2023 at 3:07 PM Gert Doering  wrote:

> Hi,
>
> On Thu, May 25, 2023 at 02:41:10PM -0400, Selva Nair wrote:
> > Now that 2.6 appears to have reached a fairly stable state, may I request
> > you to look into this patch for 2.7 -- this one has an ACK (thanks to
> > Frank), 2/2 may need a closer look but that one is small.
> >
> > I dread the prospect of this developing serious merge conflicts and
> having
> > to drill down into the details to resolve them. Right now it looks like
> no
> > one has yet touched related chunks.
>
> Will not really have much available time next week, but I have heard
> the message - will look into this the week after.  If not, please feel
> free to send me another reminder.
>

Here it is :)

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

Re: [Openvpn-devel] [PATCH 1/2] Improve signal handling using POSIX sigaction

[Selva Nair]

Hi Gert,

Now that 2.6 appears to have reached a fairly stable state, may I request
you to look into this patch for 2.7 -- this one has an ACK (thanks to
Frank), 2/2 may need a closer look but that one is small.

I dread the prospect of this developing serious merge conflicts and having
to drill down into the details to resolve them. Right now it looks like no
one has yet touched related chunks.

Thanks,

Selva



On Tue, Jan 31, 2023 at 5:48 AM Frank Lichtenheld 
wrote:

> On Sat, Jan 28, 2023 at 04:59:00PM -0500, selva.n...@gmail.com wrote:
> > From: Selva Nair 
> >
> > Currently we use the old signal API which follows system-V or
> > BSD semantics depending on the platform and/or feature-set macros.
> > Further, signal has many weaknesses which makes proper masking
> > (deferring) of signals during update not possible.
> >
> > Improve this:
> >
> > - Use sigaction to properly mask signals when modifying.
> >
>
> Acked-By: Frank Lichtenheld 
>
> Stared at code intensively. AFAICT this should not change the
> general behavior except to be more generally safe.
>
> Only compile+t_client tested.
>
> Regards,
> --
>   Frank Lichtenheld
>
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Re: [Openvpn-devel] [PATCH v3] tapctl: generate driver-specific adapter names

[Selva Nair]

Acked-by: Selva Nair 

On Fri, May 19, 2023 at 4:27 AM Lev Stipakov  wrote:

> From: Lev Stipakov 
>
> At the moment if --name is not specified, adapter names
> are generated by Windows and they look a bit confusing
> like "Local Area Connection 2".
>
> This is also behavior of "Add a new  virtual network
> adapter" shortcuts.
>
> This makes tapctl generate driver-specific names for adapters
> if --name is missing, inclusing resolving duplicates. For instance
> following commands:
>
>   tapctl.exe create --hwid ovpn-dco
>
> will create an adapter named
>
>   OpenVPN Data Channel Offload
>
> If the name is taken, the next one will be
>
>   OpenVPN Data Channel Offload #1
>
> and so on up to 100.
>
> Fixes https://github.com/OpenVPN/openvpn/issues/337
>
> Change-Id: Ic5afb470d14ac7b231d91f0f5de0a0046043a7e0
> Signed-off-by: Lev Stipakov 
> ---
>  v3:
>   - use _stprintf_s instead of _tcscat_s
>   - make functions static
>   - ensure iResult is always assigned
>
>  v2: fix MinGW compilation
>
>  src/tapctl/main.c | 132 --
>  1 file changed, 105 insertions(+), 27 deletions(-)
>
> diff --git a/src/tapctl/main.c b/src/tapctl/main.c
> index 1194036f..d76d553c 100644
> --- a/src/tapctl/main.c
> +++ b/src/tapctl/main.c
> @@ -126,6 +126,85 @@ usage(void)
>title_string);
>  }
>
> +/**
> + * Checks if adapter with given name doesn't already exist
> + */
> +static BOOL
> +is_adapter_name_available(LPCTSTR name, struct tap_adapter_node
> *adapter_list, BOOL log)
> +{
> +for (struct tap_adapter_node *a = adapter_list; a; a = a->pNext)
> +{
> +if (_tcsicmp(name, a->szName) == 0)
> +{
> +if (log)
> +{
> +LPOLESTR adapter_id = NULL;
> +StringFromIID((REFIID)&a->guid, &adapter_id);
> +_ftprintf(stderr, TEXT("Adapter \"%") TEXT(PRIsLPTSTR)
> TEXT("\" already exists (GUID %")
> +  TEXT(PRIsLPOLESTR) TEXT(").\n"), a->szName,
> adapter_id);
> +CoTaskMemFree(adapter_id);
> +}
> +
> +return FALSE;
> +}
> +}
> +
> +return TRUE;
> +}
> +
> +/**
> + * Returns unique adapter name based on hwid or NULL if name cannot be
> generated.
> + * Caller is responsible for freeing it.
> + */
> +static LPTSTR
> +get_unique_adapter_name(LPCTSTR hwid, struct tap_adapter_node
> *adapter_list)
> +{
> +if (hwid == NULL)
> +{
> +return NULL;
> +}
> +
> +LPCTSTR base_name;
> +if (_tcsicmp(hwid, TEXT("ovpn-dco")) == 0)
> +{
> +base_name = TEXT("OpenVPN Data Channel Offload");
> +}
> +else if (_tcsicmp(hwid, TEXT("wintun")) == 0)
> +{
> +base_name = TEXT("OpenVPN Wintun");
> +}
> +else if (_tcsicmp(hwid, TEXT("root\\") TEXT(TAP_WIN_COMPONENT_ID)) ==
> 0)
> +{
> +base_name = TEXT("OpenVPN TAP-Windows6");
> +}
> +else
> +{
> +return NULL;
> +}
> +
> +if (is_adapter_name_available(base_name, adapter_list, FALSE))
> +{
> +return _tcsdup(base_name);
> +}
> +
> +size_t name_len = _tcslen(base_name) + 10;
> +LPTSTR name = malloc(name_len * sizeof(TCHAR));
> +if (name == NULL)
> +{
> +return NULL;
> +}
> +for (int i = 1; i < 100; ++i)
> +{
> +_stprintf_s(name, name_len, TEXT("%ls #%d"), base_name, i);
> +
> +if (is_adapter_name_available(name, adapter_list, FALSE))
> +{
> +return name;
> +}
> +}
> +
> +return NULL;
> +}
>
>  /**
>   * Program entry point
> @@ -210,50 +289,49 @@ _tmain(int argc, LPCTSTR argv[])
>  iResult = 1; goto quit;
>  }
>
> -if (szName)
> +/* Get existing network adapters. */
> +struct tap_adapter_node *pAdapterList = NULL;
> +dwResult = tap_list_adapters(NULL, NULL, &pAdapterList);
> +if (dwResult != ERROR_SUCCESS)
>  {
> -/* Get existing network adapters. */
> -struct tap_adapter_node *pAdapterList = NULL;
> -dwResult = tap_list_adapters(NULL, NULL, &pAdapterList);
> -if (dwResult != ERROR_SUCCESS)
> -{
> -_ftprintf(stderr, TEXT("Enumerating adapters failed
> (error 0x%x).\n"), dwResult);
> -

Re: [Openvpn-devel] [PATCH 2/2] Fix CR_RESPONSE mangaement message using wrong key_id

[Selva Nair]

Hi,

While this bugfix should be merged, I'm a conflicted about the way these
two patches are split up. It just makes reviewing harder than it should be.
They actually form two independent changes but with one half intersecting
with the other for no reason.

On Wed, May 17, 2023 at 7:03 AM Arne Schwabe  wrote:

> the management interface expects the management key id instead
> of the openvpn key id. In the past they often were the same for low ids
> which hid the bug quite well.
>
> Also do not pick uninitialised keystates (management key_id is not valid
> in these) and provide better logging when a key state is not found.
>
> Change-Id: If9fa1165a0e886b570b3738546ed810a32367cbe
> Signed-off-by: Arne Schwabe 
> ---
>  src/openvpn/push.c   | 4 ++--
>  src/openvpn/ssl_common.h | 2 +-
>  src/openvpn/ssl_verify.c | 5 +
>  3 files changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/src/openvpn/push.c b/src/openvpn/push.c
> index 8e9627199..8f0a534ac 100644
> --- a/src/openvpn/push.c
> +++ b/src/openvpn/push.c
> @@ -267,9 +267,9 @@ receive_cr_response(struct context *c, const struct
> buffer *buffer)
>  struct tls_session *session = &c->c2.tls_multi->session[TM_ACTIVE];
>  struct man_def_auth_context *mda = session->opt->mda_context;
>  struct env_set *es = session->opt->es;
> -int key_id = get_primary_key(c->c2.tls_multi)->key_id;
> +unsigned int mda_key_id =
> get_primary_key(c->c2.tls_multi)->mda_key_id;
>
> -management_notify_client_cr_response(key_id, mda, es, m);
> +management_notify_client_cr_response(mda_key_id, mda, es, m);
>  #endif
>  #if ENABLE_PLUGIN

 verify_crresponse_plugin(c->c2.tls_multi, m);
>

I think, this patch could end here the bugfix is complete without the need
for 1/2.

The rest of the patch depends on 1/2, but is not required for the above. It
could be combined with 1/2 and submitted as a follow up refactoring patch.


> diff --git a/src/openvpn/ssl_common.h b/src/openvpn/ssl_common.h
> index ebfd25432..be0f18746 100644
> --- a/src/openvpn/ssl_common.h
> +++ b/src/openvpn/ssl_common.h
> @@ -733,7 +733,7 @@ get_key_by_management_key_id(struct tls_multi *multi,
> unsigned int mda_key_id)
>  for (int i = 0; i < KEY_SCAN_SIZE; ++i)
>  {
>  struct key_state *ks = get_key_scan(multi, i);
> -if (ks->mda_key_id == mda_key_id)
> +if (ks->mda_key_id == mda_key_id && ks->state > S_UNDEF)
>  {
>  return ks;
>  }
> diff --git a/src/openvpn/ssl_verify.c b/src/openvpn/ssl_verify.c
> index 4c78c2b2c..567f55ea3 100644
> --- a/src/openvpn/ssl_verify.c
> +++ b/src/openvpn/ssl_verify.c
> @@ -1279,6 +1279,11 @@ tls_authenticate_key(struct tls_multi *multi, const
> unsigned int mda_key_id, con
>  {
>  ks->mda_status = auth ? ACF_SUCCEEDED : ACF_FAILED;
>  }
> +else
> +{
> +msg(D_TLS_DEBUG_LOW, "%s: no key state found for management
> key id "
> +"%d", __func__, mda_key_id);
> +}
>  }
>  return (bool) ks;
>  }


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

Re: [Openvpn-devel] [PATCH v2] tapctl: generate driver-specific adapter names

[Selva Nair]

Hi,

On Thu, May 18, 2023 at 12:25 PM Lev Stipakov  wrote:

> From: Lev Stipakov 
>
> At the moment if --name is not specified, adapter names
> are generated by Windows and they look a bit confusing
> like "Local Area Connection 2".
>
> This is also behavior of "Add a new  virtual network
> adapter" shortcuts.
>
> This makes tapctl generate driver-specific names for adapters
> if --name is missing, inclusing resolving duplicates. For instance
> following commands:
>
>   tapctl.exe create --hwid ovpn-dco
>
> will create an adapter named
>
>   OpenVPN Data Channel Offload
>
> If the name is taken, the next one will be
>
>   OpenVPN Data Channel Offload #1
>
> and so on up to 100.
>
> Fixes https://github.com/OpenVPN/openvpn/issues/337


Looks good but some comments below -- mostly nitpicks.


>
> Change-Id: Ic5afb470d14ac7b231d91f0f5de0a0046043a7e0
> Signed-off-by: Lev Stipakov 
> ---
>  src/tapctl/main.c | 133 +-
>  1 file changed, 107 insertions(+), 26 deletions(-)
>
> diff --git a/src/tapctl/main.c b/src/tapctl/main.c
> index 1194036f..e4d44794 100644
> --- a/src/tapctl/main.c
> +++ b/src/tapctl/main.c
> @@ -126,6 +126,90 @@ usage(void)
>title_string);
>  }
>
> +/**
> + * Checks if adapter with given name doesn't already exist
> + */
> +BOOL
>

I would make this static


> +is_adapter_name_available(LPCTSTR name, struct tap_adapter_node
> *adapter_list, BOOL log)
> +{
> +for (struct tap_adapter_node *a = adapter_list; a; a = a->pNext)
> +{
> +if (_tcsicmp(name, a->szName) == 0)
> +{
> +if (log)
> +{
> +LPOLESTR adapter_id = NULL;
> +StringFromIID((REFIID)&a->guid, &adapter_id);
> +_ftprintf(stderr, TEXT("Adapter \"%") TEXT(PRIsLPTSTR)
> TEXT("\" already exists (GUID %")
> +  TEXT(PRIsLPOLESTR) TEXT(").\n"), a->szName,
> adapter_id);
> +CoTaskMemFree(adapter_id);
> +}
> +
> +return FALSE;
> +}
> +}
> +
> +return TRUE;
> +}
> +
> +/**
> + * Returns unique adapter name based on hwid or NULL if name cannot be
> generated.
> + * Caller is responsible for freeing it.
> + */
> +LPTSTR
>

same static here too?


> +get_unique_adapter_name(LPCTSTR hwid, struct tap_adapter_node
> *adapter_list)
> +{
> +if (hwid == NULL)
> +{
> +return NULL;
> +}
> +
> +LPCTSTR base_name;
> +if (_tcsicmp(hwid, TEXT("ovpn-dco")) == 0)
> +{
> +base_name = TEXT("OpenVPN Data Channel Offload");
> +}
> +else if (_tcsicmp(hwid, TEXT("wintun")) == 0)
> +{
> +base_name = TEXT("OpenVPN Wintun");
> +}
> +else if (_tcsicmp(hwid, TEXT("root\\") TEXT(TAP_WIN_COMPONENT_ID)) ==
> 0)
> +{
> +base_name = TEXT("OpenVPN TAP-Windows6");
> +}
> +else
> +{
> +return NULL;
> +}
> +
> +if (is_adapter_name_available(base_name, adapter_list, FALSE))
> +{
> +return _tcsdup(base_name);
> +}
> +
> +TCHAR itot_buf[10];
> +size_t name_len = (_tcslen(base_name) + sizeof(itot_buf) /
> sizeof(TCHAR) + 2);
>

_countof(itot_buf)  is what we normally use when SIZE() is not available.
That said, do we need itot_buf[] at all? see below.


> +LPTSTR name = malloc(name_len * sizeof(TCHAR));
> +if (name == NULL)
> +{
> +return NULL;
> +}
> +for (int i = 1; i < 100; ++i)
> +{
> +*name = TEXT('\0');
> +_tcscat_s(name, name_len, base_name);
> +_tcscat_s(name, name_len, TEXT(" #"));
> +_itot_s(i, itot_buf, sizeof(itot_buf) / sizeof(TCHAR), 10);
> +_tcscat_s(name, name_len, itot_buf);
>

It looks much simpler to write the above 5 lines as

_stprintf_s(name, name_len, TEXT("%ls #%d"), base_name, i)

Well, the format string should be TEXT("%") TEXT(PRIsLPTSTR) TEXT(" #%d"),
but we are not going to build this ever without UNICODE.

+
> +if (is_adapter_name_available(name, adapter_list, FALSE))
> +{
> +return name;
> +}
> +}
> +
> +return NULL;
> +}
>
>  /**
>   * Program entry point
> @@ -210,50 +294,47 @@ _tmain(int argc, LPCTSTR argv[])
>  iResult = 1; goto quit;
>  }
>
> -if (szName)
> +/* Get existing network adapters. */
> +struct tap_adapter_node *pAdapterList = NULL;
> +dwResult = tap_list_adapters(NULL, NULL, &pAdapterList);
> +if (dwResult != ERROR_SUCCESS)
>  {
> -/* Get existing network adapters. */
> -struct tap_adapter_node *pAdapterList = NULL;
> -dwResult = tap_list_adapters(NULL, NULL, &pAdapterList);
> -if (dwResult != ERROR_SUCCESS)
> -{
> -_ftprintf(stderr, TEXT("Enumerating adapters failed
> (error 0x%x).\n"), dwResult);
> -iResult = 1; goto create_delete_adapter;
> -}
> +_ftprintf(stderr, TEXT("Enumerating

[Openvpn-devel] [PATCH] Interactive service: do not force a target desktop for openvpn.exe

[selva . nair]

From: Selva Nair 

Setting the desktop as "winsta0\default" does not always work when run
from a non-interactive session which may not have access to the
the window station "Winsta0". Leave this as NULL to let the system
automatically assign a window station and desktop.

Test runs on Win10 confirm that "Winsta0\Default" still gets selected
when run interactively (e.g., using the GUI or from task scheduler as
an interactive job). This is the same behaviour as now.

The change allows "interactive service" to be used for launching
OpenVPN from non-interactive sessions. For example, when service client
is a non-interactive task from the task scheduler, the default desktop
in a custom window station gets assigned to openvpn.exe.

Note that we already run openvpn.exe in a non-interactive window
station when directly launched by "automatic service".

Github: Fixes OpenVPN/openvpn-gui#626

Signed-off-by: Selva Nair 
---
 src/openvpnserv/interactive.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/src/openvpnserv/interactive.c b/src/openvpnserv/interactive.c
index ec196274..d73cef04 100644
--- a/src/openvpnserv/interactive.c
+++ b/src/openvpnserv/interactive.c
@@ -1868,7 +1868,6 @@ RunOpenvpn(LPVOID p)
 }
 
 startup_info.cb = sizeof(startup_info);
-startup_info.lpDesktop = L"winsta0\\default";
 startup_info.dwFlags = STARTF_USESTDHANDLES;
 startup_info.hStdInput = stdin_read;
 startup_info.hStdOutput = stdout_write;
-- 
2.34.1



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

Re: [Openvpn-devel] [PATCH] Fix CR_RESPONSE mangaement message using wrong key_id

[Selva Nair]

Hi,

Is this dependent on some patch not yet merged? See missing context below.

On Tue, May 16, 2023 at 12:36 PM Arne Schwabe  wrote:

> the management interface expects the management key id instead
> of the openvpn key id. In the past they often were the same for low ids
> which hid the bug quite well.
>
> Also do not pick uninitialised keystates (management key_id is not valid
> in these) and provide better logging when a key state is not found.
>
> Change-Id: If9fa1165a0e886b570b3738546ed810a32367cbe
> Signed-off-by: Arne Schwabe 
> ---
>  src/openvpn/push.c   | 4 ++--
>  src/openvpn/ssl_common.h | 2 +-
>  src/openvpn/ssl_verify.c | 5 +
>  3 files changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/src/openvpn/push.c b/src/openvpn/push.c
> index 54e53f6aa..99abe5440 100644
> --- a/src/openvpn/push.c
> +++ b/src/openvpn/push.c
> @@ -267,9 +267,9 @@ receive_cr_response(struct context *c, const struct
> buffer *buffer)
>  struct tls_session *session = &c->c2.tls_multi->session[TM_ACTIVE];
>  struct man_def_auth_context *mda = session->opt->mda_context;
>  struct env_set *es = session->opt->es;
> -int key_id = get_primary_key(c->c2.tls_multi)->key_id;
> +unsigned int mda_key_id =
> get_primary_key(c->c2.tls_multi)->mda_key_id;
>
> -management_notify_client_cr_response(key_id, mda, es, m);
> +management_notify_client_cr_response(mda_key_id, mda, es, m);
>  #endif
>  #if ENABLE_PLUGIN
>  verify_crresponse_plugin(c->c2.tls_multi, m);
> diff --git a/src/openvpn/ssl_common.h b/src/openvpn/ssl_common.h
> index ebfd25432..be0f18746 100644
> --- a/src/openvpn/ssl_common.h
> +++ b/src/openvpn/ssl_common.h
> @@ -733,7 +733,7 @@ get_key_by_management_key_id(struct tls_multi *multi,
> unsigned int mda_key_id)
>

ssl_common.h in master (and 2.6) has only 725 lines and no function by that
name. Am I missing something?

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

[Openvpn-devel] [PATCH] Correctly handle Unicode names for exit event

[selva . nair]

From: Selva Nair 

Currently we use the ANSI version of CreateEvent causing name of the
exit event to be interpreted differently depending on the code page
in effect. Internally all strings parsed from command line and config
file are stored as UTF8-encoded Uniode. When passed to Windows API calls,
these should be converted to UTF16 and wide character version of the API
should be used.

CreateEvent calls for unnamed events are left unchanged as there is no
text-encoding dependence in those cases.

Signed-off-by: Selva Nair 
---
 src/openvpn/win32.c | 11 ++-
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/src/openvpn/win32.c b/src/openvpn/win32.c
index 1ae3723f..25da54ab 100644
--- a/src/openvpn/win32.c
+++ b/src/openvpn/win32.c
@@ -509,19 +509,19 @@ win32_signal_open(struct win32_signal *ws,
 && !HANDLE_DEFINED(ws->in.read) && exit_event_name)
 {
 struct security_attributes sa;
+struct gc_arena gc = gc_new();
+const wchar_t *exit_event_nameW = wide_string(exit_event_name, &gc);
 
 if (!init_security_attributes_allow_all(&sa))
 {
 msg(M_ERR, "Error: win32_signal_open: init SA failed");
 }
 
-ws->in.read = CreateEvent(&sa.sa,
-  TRUE,
-  exit_event_initial_state ? TRUE : FALSE,
-  exit_event_name);
+ws->in.read = CreateEventW(&sa.sa, TRUE, exit_event_initial_state ? 
TRUE : FALSE,
+   exit_event_nameW);
 if (ws->in.read == NULL)
 {
-msg(M_WARN|M_ERRNO, "NOTE: CreateEvent '%s' failed", 
exit_event_name);
+msg(M_WARN|M_ERRNO, "NOTE: CreateEventW '%s' failed", 
exit_event_name);
 }
 else
 {
@@ -534,6 +534,7 @@ win32_signal_open(struct win32_signal *ws,
 ws->mode = WSO_MODE_SERVICE;
 }
 }
+gc_free(&gc);
 }
 /* set the ctrl handler in both console and service modes */
 if (!SetConsoleCtrlHandler((PHANDLER_ROUTINE) win_ctrl_handler, true))
-- 
2.34.1



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

[Openvpn-devel] [PATCH] Bugfix: dangling pointer passed to pkcs11-helper

[selva . nair]

From: Selva Nair 

Github: Fixes OpenVPN/openvpn#323

Signed-off-by: Selva Nair 
---
This will fix #323 is my best guess, untested as yet..
This is a bug that needs fixing, regardless.
 
 src/openvpn/pkcs11_openssl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/openvpn/pkcs11_openssl.c b/src/openvpn/pkcs11_openssl.c
index eee86e17..9b0ab39f 100644
--- a/src/openvpn/pkcs11_openssl.c
+++ b/src/openvpn/pkcs11_openssl.c
@@ -165,6 +165,7 @@ xkey_pkcs11h_sign(void *handle, unsigned char *sig,
 {
 pkcs11h_certificate_t cert = handle;
 CK_MECHANISM mech = {CKM_RSA_PKCS, NULL, 0}; /* default value */
+CK_RSA_PKCS_PSS_PARAMS pss_params = {0};
 
 unsigned char buf[EVP_MAX_MD_SIZE];
 size_t buflen;
@@ -203,7 +204,6 @@ xkey_pkcs11h_sign(void *handle, unsigned char *sig,
 }
 else if (!strcmp(sigalg.padmode, "pss"))
 {
-CK_RSA_PKCS_PSS_PARAMS pss_params = {0};
 mech.mechanism = CKM_RSA_PKCS_PSS;
 
 if (!set_pss_params(&pss_params, sigalg, cert))
-- 
2.34.1



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

Re: [Openvpn-devel] [PATCH v2] Add Apache2 linking with for new commits

[Selva Nair]

Hi,

On Tue, Apr 25, 2023 at 6:22 AM Arne Schwabe  wrote:

> After first round of mailing people with more than 10 commits we have
> almost all committers have agreed. This put this license in the realm
> of having a realistic change to work. Had any of these contributers
> disagreed, rewriting all their code might have been not feasible.
>
> The rationale of adding this exception now is to avoid having to
> have a second round of agreement for new contributers and ensure
> that all new code will include the exemption.
>
> patch v2: add explaination and use exception rather than excemption
>

looks like a wrong version of patch below ?


> Change-Id: Ide83f914f383b53ef37ddf628e4da5a78e241bf0
> Signed-off-by: Arne Schwabe 
> ---
>  COPYING | 23 +++
>  1 file changed, 23 insertions(+)
>
> diff --git a/COPYING b/COPYING
> index e12c51414..b4a148494 100644
> --- a/COPYING
> +++ b/COPYING
> @@ -31,6 +31,29 @@ OpenVPN license:
>file, but you are not obligated to do so.  If you do not wish to
>do so, delete this exception statement from your version.
>
> +Apache2 linking exemption:
>

--> exception


> +---
> +OpenVPN is currently undergoing a license change to add an exemption for
>

--> exception


> +Apache 2 linking. The following exemption is only valid for new
> contributions
>

--> exception


> +after COMMITDATE and past contribution where the authors have already
> agreed
> +to the exemption.
>

--> exception


> +
> +  In addition, as a special exception, OpenVPN Inc and the
> +  contributors give permission to link the code of this program to
> +  libraries (the "Libraries") licensed under the Apache License
> +  version 2.0 (this work and any linked library the "Combined Work")
> +  and copy and distribute the Combined Work without an obligation to
> +  license the Libraries under the GNU General Public License v2
> +  (GPL-2.0) as required by Section 2 of the GPL-2.0, and without an
> +  obligation to refrain from imposing any additional restrictions in
> +  the Apache License version 2 that are not in the GPL-2.0, as
> +  required by Section 6 of the GPL-2.0.  You must comply with the
> +  GPL-2.0 in all other respects for the Combined Work, including
> +  the obligation to provide source code.  If you modify this file, you
> +  may extend this exception to your version of the file, but you are
> +  not obligated to do so.  If you do not wish to do so, delete this
> +  exception statement from your version.
> +
>  LZO license:
>  
>
> --
> 2.39.2 (Apple Git-143)
>
>
>
> ___
> Openvpn-devel mailing list
> Openvpn-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/openvpn-devel
>
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Re: [Openvpn-devel] [PATCH] Add Apache2 linking with for new commits

[Selva Nair]

exemption -> exception in a number of places below

Though similar in meaning, better to use the standard wording here.

On Fri, Apr 21, 2023 at 11:02 AM Arne Schwabe  wrote:

> After first round of mailing people with more than 10 commits we have
> almost all committers have agreed. This put this license in the realm
> of having a realistic change to work. Had any of these contributers
> disagreed, rewriting all their code might have been not feasible.
>
> The rationale of adding this exception now is to avoid having to
> have a second round of agreement for new contributers and ensure
> that all new code will include the exemption.


> Change-Id: Ide83f914f383b53ef37ddf628e4da5a78e241bf0
> Signed-off-by: Arne Schwabe 
> ---
>  COPYING | 23 +++
>  1 file changed, 23 insertions(+)
>
> diff --git a/COPYING b/COPYING
> index e12c51414..b4a148494 100644
> --- a/COPYING
> +++ b/COPYING
> @@ -31,6 +31,29 @@ OpenVPN license:
>file, but you are not obligated to do so.  If you do not wish to
>do so, delete this exception statement from your version.
>
> +Apache2 linking exemption:
> +---
> +OpenVPN is currently undergoing a license change to add an exemption for
> +Apache 2 linking. The following exemption is only valid for new
> contributions
> +after COMMITDATE and past contribution where the authors have already
> agreed
> +to the exemption.
> +
> +  In addition, as a special exception, OpenVPN Inc and the
> +  contributors give permission to link the code of this program to
> +  libraries (the "Libraries") licensed under the Apache License
> +  version 2.0 (this work and any linked library the "Combined Work")
> +  and copy and distribute the Combined Work without an obligation to
> +  license the Libraries under the GNU General Public License v2
> +  (GPL-2.0) as required by Section 2 of the GPL-2.0, and without an
> +  obligation to refrain from imposing any additional restrictions in
> +  the Apache License version 2 that are not in the GPL-2.0, as
> +  required by Section 6 of the GPL-2.0.  You must comply with the
> +  GPL-2.0 in all other respects for the Combined Work, including
> +  the obligation to provide source code.  If you modify this file, you
> +  may extend this exception to your version of the file, but you are
> +  not obligated to do so.  If you do not wish to do so, delete this
> +  exception statement from your version.
> +
>  LZO license:
>  
>
> --
> 2.39.2 (Apple Git-143)
>
>
>
> ___
> Openvpn-devel mailing list
> Openvpn-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/openvpn-devel
>
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

[Openvpn-devel] [PATCH v2] Format Windows error message in Unicode

[selva . nair]

From: Selva Nair 

- We assume that all text passed to the management interface
  and written to log file are in Unicode (UTF-8). This is broken by
  the use of the ANSI version of FormatMessage() for Windows error
  messages. Fix by using FormatMessageW() and converting the UTF-16
  result to UTF-8.

v2: assign return value of FormatMessageW() to DWORD, not int

Github: fixes OpenVPN/openvpn#319

Signed-off-by: Selva Nair 
---
 src/openvpn/error.c | 15 ++-
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/src/openvpn/error.c b/src/openvpn/error.c
index a2c9aa4c..9a234e67 100644
--- a/src/openvpn/error.c
+++ b/src/openvpn/error.c
@@ -970,19 +970,24 @@ strerror_win32(DWORD errnum, struct gc_arena *gc)
 
 /* format a windows error message */
 {
-char message[256];
+wchar_t wmessage[256];
+char *message = NULL;
 struct buffer out = alloc_buf_gc(256, gc);
-const int status =  FormatMessage(
+const DWORD status =  FormatMessageW(
 FORMAT_MESSAGE_IGNORE_INSERTS
 | FORMAT_MESSAGE_FROM_SYSTEM
 | FORMAT_MESSAGE_ARGUMENT_ARRAY,
 NULL,
 errnum,
 0,
-message,
-sizeof(message),
+wmessage,
+SIZE(wmessage),
 NULL);
-if (!status)
+if (status)
+{
+message = utf16to8(wmessage, gc);
+}
+if (!status || !message)
 {
 buf_printf(&out, "[Unknown Win32 Error]");
 }
-- 
2.34.1



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

[Openvpn-devel] [PATCH] Format Windows error message in Unicode

[selva . nair]

From: Selva Nair 

- We assume that all text passed to the management interface
  and written to log file are in Unicode (UTF-8). This is broken by
  the use of the ANSI version of FormatMessage() for Windows error
  messages. Fix by using FormatMessageW() and converting the UTF-16
  result to UTF-8.

Github: fixes OpenVPN/openvpn#319

Signed-off-by: Selva Nair 
---
 src/openvpn/error.c | 15 ++-
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/src/openvpn/error.c b/src/openvpn/error.c
index a2c9aa4c..ef821fcd 100644
--- a/src/openvpn/error.c
+++ b/src/openvpn/error.c
@@ -970,19 +970,24 @@ strerror_win32(DWORD errnum, struct gc_arena *gc)
 
 /* format a windows error message */
 {
-char message[256];
+wchar_t wmessage[256];
+char *message = NULL;
 struct buffer out = alloc_buf_gc(256, gc);
-const int status =  FormatMessage(
+const int status =  FormatMessageW(
 FORMAT_MESSAGE_IGNORE_INSERTS
 | FORMAT_MESSAGE_FROM_SYSTEM
 | FORMAT_MESSAGE_ARGUMENT_ARRAY,
 NULL,
 errnum,
 0,
-message,
-sizeof(message),
+wmessage,
+SIZE(wmessage),
 NULL);
-if (!status)
+if (status)
+{
+message = utf16to8(wmessage, gc);
+}
+if (!status || !message)
 {
 buf_printf(&out, "[Unknown Win32 Error]");
 }
-- 
2.34.1



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

Re: [Openvpn-devel] [PATCH v2] Support of DNS domain for DHCP-less drivers

[Selva Nair]

t;);
> +argv_msg_prefix(M_INFO, a, prefix);
> +status = openvpn_execve_check(a, NULL, 0, "ERROR: command
> failed");
>  netcmd_semaphore_release();
>  if (status)
>  {
> @@ -5260,7 +5286,13 @@ netsh_command(const struct argv *a, int n, int
> msglevel)
>  }
>  management_sleep(4);
>  }
> -msg(msglevel, "NETSH: command failed");
> +msg(msglevel, "%s: command failed", prefix);
> +}
> +
> +static void
> +netsh_command(const struct argv *a, int n, int msglevel)
> +{
> +exec_command("NETSH", a, n, msglevel);
>  }
>
>  void
> @@ -6927,6 +6959,11 @@ close_tun(struct tuntap *tt, openvpn_net_ctx_t *ctx)
>  }
>  else
>  {
> +if (!tt->did_ifconfig_setup)
> +{
> +do_dns_domain_wmic(false, tt);
> +}
> +
>  netsh_delete_address_dns(tt, true, &gc);
>  }
>  }
> @@ -6947,9 +6984,14 @@ close_tun(struct tuntap *tt, openvpn_net_ctx_t *ctx)
>  do_dns_service(false, AF_INET, tt);
>  do_address_service(false, AF_INET, tt);
>  }
> -else if (tt->options.ip_win32_type == IPW32_SET_NETSH)
> +else
>  {
> -    netsh_delete_address_dns(tt, false, &gc);
> +do_dns_domain_wmic(false, tt);
> +
> +if (tt->options.ip_win32_type == IPW32_SET_NETSH)
> +{
> +netsh_delete_address_dns(tt, false, &gc);
> +}
>  }
>  }
>
> diff --git a/src/openvpn/win32.h b/src/openvpn/win32.h
> index 72ffb012..36059662 100644
> --- a/src/openvpn/win32.h
> +++ b/src/openvpn/win32.h
> @@ -38,6 +38,7 @@
>  #define WIN_ROUTE_PATH_SUFFIX "\\system32\\route.exe"
>  #define WIN_IPCONFIG_PATH_SUFFIX "\\system32\\ipconfig.exe"
>  #define WIN_NET_PATH_SUFFIX "\\system32\\net.exe"
> +#define WMIC_PATH_SUFFIX "\\system32\\wbem\\wmic.exe"
>
>  /*
>   * Win32-specific OpenVPN code, targeted at the mingw
>

Acked-by: Selva Nair 

Useful to have in 2.6 as well, as dco is the default now. The changes are
minimal and should not have any side effects.

Did some quick test runs on Win 10 using tap-windows6 and dco drivers with
and without "dhcp-option DOMAIN".

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

Re: [Openvpn-devel] [PATCH] Support of DNS domain for DHCP-less drivers

[Selva Nair]

Hi,

On Tue, Apr 4, 2023 at 4:01 AM Lev Stipakov  wrote:

> From: Lev Stipakov 
>
> We set DNS domain either via interactve service or DHCP.
> When interactive service is not used, for example,
> when profiles are started by OpenVPNService, this option
> is not working for DCO and wintun.
>
> This implements setting DNS domain via WMIC command,
> similar to implementation in interactive service.
> This is done when:
>
>  - interactive service is not used
>  - ip_win32_type is either METSH or IPAPI, which is
> the case for DCO and wintun.
>
> Fixes https://github.com/OpenVPN/openvpn/issues/306
>
> Change-Id: I9ab51bf1c0774564204c75ecce9ebfb818db2f5b
> ---
>  src/openvpn/options.c |  8 ++---
>  src/openvpn/tun.c | 72 ++-
>  src/openvpn/tun.h |  6 
>  src/openvpn/win32.h   |  1 +
>  4 files changed, 67 insertions(+), 20 deletions(-)
>
> diff --git a/src/openvpn/options.c b/src/openvpn/options.c
> index 2680f268..36d54ceb 100644
> --- a/src/openvpn/options.c
> +++ b/src/openvpn/options.c
> @@ -3396,11 +3396,11 @@ options_postprocess_mutate_invariant(struct
> options *options)
>  #ifdef _WIN32
>  const int dev = dev_type_enum(options->dev, options->dev_type);
>
> +const bool dhcp = tuntap_maybe_dhcp(&options->tuntap_options);
> +
>  /* when using wintun/ovpn-dco, kernel doesn't send DHCP requests, so
> don't use it */
>  if ((options->windows_driver == WINDOWS_DRIVER_WINTUN
> - || options->windows_driver == WINDOWS_DRIVER_DCO)
> -&& (options->tuntap_options.ip_win32_type == IPW32_SET_DHCP_MASQ
> -|| options->tuntap_options.ip_win32_type ==
> IPW32_SET_ADAPTIVE))
> + || options->windows_driver == WINDOWS_DRIVER_DCO) && dhcp)
>  {
>  options->tuntap_options.ip_win32_type = IPW32_SET_NETSH;
>  }
> @@ -3408,8 +3408,6 @@ options_postprocess_mutate_invariant(struct options
> *options)
>  if ((dev == DEV_TYPE_TUN || dev == DEV_TYPE_TAP) &&
> !options->route_delay_defined)
>  {
>  /* delay may only be necessary when we perform DHCP handshake */
> -const bool dhcp = (options->tuntap_options.ip_win32_type ==
> IPW32_SET_DHCP_MASQ)
> -  || (options->tuntap_options.ip_win32_type ==
> IPW32_SET_ADAPTIVE);
>  if ((options->mode == MODE_POINT_TO_POINT) && dhcp)
>  {
>  options->route_delay_defined = true;
> diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c
> index 2ebe4809..c2cc7e26 100644
> --- a/src/openvpn/tun.c
> +++ b/src/openvpn/tun.c
> @@ -84,6 +84,8 @@ static void netsh_set_dns6_servers(const struct in6_addr
> *addr_list,
>
>  static void netsh_command(const struct argv *a, int n, int msglevel);
>
> +static void exec_command(const char *prefix, const struct argv *a, int n,
> int msglevel);
> +
>  static const char *netsh_get_id(const char *dev_node, struct gc_arena
> *gc);
>
>  static bool
> @@ -324,6 +326,22 @@ out:
>  return ret;
>  }
>
> +static void
> +wmic_do_dns_domain(bool add, const struct tuntap *tt)
> +{
> +if (!tt->options.domain)
> +{
> +return;
> +}
> +
> +struct argv argv = argv_new();
> +argv_printf(&argv, "%s%s nicconfig where (InterfaceIndex=%ld) call
> SetDNSDomain %s",
> +get_win_sys_path(), WMIC_PATH_SUFFIX, tt->adapter_index,
> add ? tt->options.domain : "");
> +exec_command("WMIC", &argv, 1, M_WARN);
> +
> +argv_free(&argv);
> +}
> +
>  #endif /* ifdef _WIN32 */
>
>  #ifdef TARGET_SOLARIS
> @@ -1190,6 +1208,11 @@ do_ifconfig_ipv6(struct tuntap *tt, const char
> *ifname, int tun_mtu,
>  /* set ipv6 dns servers if any are specified */
>  netsh_set_dns6_servers(tt->options.dns6, tt->options.dns6_len,
> tt->adapter_index);
>  windows_set_mtu(tt->adapter_index, AF_INET6, tun_mtu);
> +
> +if (!tt->did_ifconfig_setup && !tuntap_maybe_dhcp(&tt->options))
>

Adding "!tuntap_maybe_dhcp()" here looks wrong. We must set the domain if
not set during IPv4 processing and that is indicated by did_ifconfig_setup.
Just as done in the case handled by interactive-service above.

For example: in case there is no IPv4 address, the domain will not be set
at this point but tuntap_may_dhcp() could still be true if the driver is
tap-windows6. And the above will wrongly skip the wmic call. Even
otherwise, adding a seemingly dhcp-related check in IPv6 case will only
confuse us in future.

Same in close_tun() to match.

Once that check is removed there is no need to introduce
tuntap_maybe_dhcp() in this patch at all. This can be kept simple by just
adding wmic_do_dns_domain() and calling it in two places under the same
conditions as done using service. Plus the netsh_command() to
exec_command() generalization. That makes it easier to review as well.

As for refactoring/cleaning up, we can address it separately as the whole
ip-win32 thing is a mess that could benefit from a thorough cleanup.

+{
> +wmic_do_dns_domain(true, tt)

Re: [Openvpn-devel] [PATCH applied] Re: Bug-fix: segfault in dco_get_peer_stats()

[Selva Nair]

On Tue, Mar 28, 2023 at 3:25 AM Gert Doering  wrote:

> Acked-by: Antonio Quartulli 
>
> Thanks for the good find.  Since I could reproduce the crash yesterday
> (and I do need management for it) I can verify that it does no longer
> crash with the patch.
>
> (For whatever reason, --tls-version-max 1.0 did not suffice to trigger
> the error this morning, but --tls-version-min 1.3 + management + bytecount
> still did)
>

Since then I've found this can be triggered quite simply by adding
"--status /dev/stdout" or some file to the client options.

Which means this should also fix #301

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

Re: [Openvpn-devel] [PATCH] Bug-fix: segfault in dco_get_peer_stats()

[Selva Nair]

Hi,

I should have added that as written the fault is with get_dco_peer stats().
It takes a pointer to "struct context" and dereferences a couple of layers
below it. The caller has no idea what it wants to see set in c. So, until
that is changed on all platforms (right now windows and linux), too much to
ask the caller to check that a valid dco handle is available.

Selva

On Mon, Mar 27, 2023 at 4:42 PM Selva Nair  wrote:

>
>
> On Mon, Mar 27, 2023 at 4:30 PM Antonio Quartulli  wrote:
>
>> Hi,
>>
>> On 27/03/2023 19:12, selva.n...@gmail.com wrote:
>> > From: Selva Nair 
>> >
>> >We persist peer-stats when restarting, but an early restart
>> >before open_tun results in a segfault in dco_get_peer_stats().
>> >To reproduce, trigger a TLS handshake error due to lack of common
>> >protocols, for example.
>> >
>> >Fix by checking  that tuntap is defined before dereferencing it.
>> >
>> > Signed-off-by: Selva Nair 
>>
>> Nice catch! Thanks a lot!
>>
>> > ---
>> > I'm not entirely sure this is the right place to fix this.
>> > Or is it the caller at fault exercising  dco_get_peer_stats()
>> > when tuntap is not set?
>>
>> Indeed that was my assumption.
>>
>> Does somebody have the stacktrace?
>> I wanted to check where this call is coming from exactly.
>>
>
> I didn't save the stacktrace.. The call comes through
>
>  persist_client_stats(c); (openvpn.c ~ line 100)
>
> which gets called on exiting the main eventloop for restart. Without it we
> used to lose
> the statistics on restart.
>
> Imho it'd be reasonable if the caller could check if we have a device
>> before invoking any DCO API.
>>
>
> Sure, I would think that the caller of dco_get_peer_stats() should just
> pass the dco handle to it
> and get the stats back. It's debatable who should check whether
> the dco handle is valid or not.
>
> But this may not be the time and place for a refactor. As dco is young, I
> think it can mature in 2.7..
>
> Selva
>
>
>
>>
>> Cheers,
>>
>> >
>> >   src/openvpn/dco_linux.c | 5 +
>> >   1 file changed, 5 insertions(+)
>> >
>> > diff --git a/src/openvpn/dco_linux.c b/src/openvpn/dco_linux.c
>> > index 317f9dc0..41540c0f 100644
>> > --- a/src/openvpn/dco_linux.c
>> > +++ b/src/openvpn/dco_linux.c
>> > @@ -975,6 +975,11 @@ dco_get_peer_stats(struct context *c)
>> >   uint32_t peer_id = c->c2.tls_multi->dco_peer_id;
>> >   msg(D_DCO_DEBUG, "%s: peer-id %d", __func__, peer_id);
>> >
>> > +if (!c->c1.tuntap)
>> > +{
>> > +return 0;
>> > +}
>> > +
>> >   dco_context_t *dco = &c->c1.tuntap->dco;
>> >   struct nl_msg *nl_msg = ovpn_dco_nlmsg_create(dco,
>> OVPN_CMD_GET_PEER);
>> >   struct nlattr *attr = nla_nest_start(nl_msg, OVPN_ATTR_GET_PEER);
>>
>> --
>> Antonio Quartulli
>>
>
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Re: [Openvpn-devel] [PATCH] Bug-fix: segfault in dco_get_peer_stats()

[Selva Nair]

On Mon, Mar 27, 2023 at 4:30 PM Antonio Quartulli  wrote:

> Hi,
>
> On 27/03/2023 19:12, selva.n...@gmail.com wrote:
> > From: Selva Nair 
> >
> >We persist peer-stats when restarting, but an early restart
> >before open_tun results in a segfault in dco_get_peer_stats().
> >To reproduce, trigger a TLS handshake error due to lack of common
> >protocols, for example.
> >
> >Fix by checking  that tuntap is defined before dereferencing it.
> >
> > Signed-off-by: Selva Nair 
>
> Nice catch! Thanks a lot!
>
> > ---
> > I'm not entirely sure this is the right place to fix this.
> > Or is it the caller at fault exercising  dco_get_peer_stats()
> > when tuntap is not set?
>
> Indeed that was my assumption.
>
> Does somebody have the stacktrace?
> I wanted to check where this call is coming from exactly.
>

I didn't save the stacktrace.. The call comes through

 persist_client_stats(c); (openvpn.c ~ line 100)

which gets called on exiting the main eventloop for restart. Without it we
used to lose
the statistics on restart.

Imho it'd be reasonable if the caller could check if we have a device
> before invoking any DCO API.
>

Sure, I would think that the caller of dco_get_peer_stats() should just
pass the dco handle to it
and get the stats back. It's debatable who should check whether
the dco handle is valid or not.

But this may not be the time and place for a refactor. As dco is young, I
think it can mature in 2.7..

Selva



>
> Cheers,
>
> >
> >   src/openvpn/dco_linux.c | 5 +
> >   1 file changed, 5 insertions(+)
> >
> > diff --git a/src/openvpn/dco_linux.c b/src/openvpn/dco_linux.c
> > index 317f9dc0..41540c0f 100644
> > --- a/src/openvpn/dco_linux.c
> > +++ b/src/openvpn/dco_linux.c
> > @@ -975,6 +975,11 @@ dco_get_peer_stats(struct context *c)
> >   uint32_t peer_id = c->c2.tls_multi->dco_peer_id;
> >   msg(D_DCO_DEBUG, "%s: peer-id %d", __func__, peer_id);
> >
> > +if (!c->c1.tuntap)
> > +{
> > +return 0;
> > +}
> > +
> >   dco_context_t *dco = &c->c1.tuntap->dco;
> >   struct nl_msg *nl_msg = ovpn_dco_nlmsg_create(dco,
> OVPN_CMD_GET_PEER);
> >   struct nlattr *attr = nla_nest_start(nl_msg, OVPN_ATTR_GET_PEER);
>
> --
> Antonio Quartulli
>
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

[Openvpn-devel] [PATCH] Bug-fix: segfault in dco_get_peer_stats()

[selva . nair]

From: Selva Nair 

  We persist peer-stats when restarting, but an early restart
  before open_tun results in a segfault in dco_get_peer_stats().
  To reproduce, trigger a TLS handshake error due to lack of common
  protocols, for example.

  Fix by checking  that tuntap is defined before dereferencing it.

Signed-off-by: Selva Nair 
---
I'm not entirely sure this is the right place to fix this.
Or is it the caller at fault exercising  dco_get_peer_stats()
when tuntap is not set?

 src/openvpn/dco_linux.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/src/openvpn/dco_linux.c b/src/openvpn/dco_linux.c
index 317f9dc0..41540c0f 100644
--- a/src/openvpn/dco_linux.c
+++ b/src/openvpn/dco_linux.c
@@ -975,6 +975,11 @@ dco_get_peer_stats(struct context *c)
 uint32_t peer_id = c->c2.tls_multi->dco_peer_id;
 msg(D_DCO_DEBUG, "%s: peer-id %d", __func__, peer_id);
 
+if (!c->c1.tuntap)
+{
+return 0;
+}
+
 dco_context_t *dco = &c->c1.tuntap->dco;
 struct nl_msg *nl_msg = ovpn_dco_nlmsg_create(dco, OVPN_CMD_GET_PEER);
 struct nlattr *attr = nla_nest_start(nl_msg, OVPN_ATTR_GET_PEER);
-- 
2.34.1



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

Re: [Openvpn-devel] [PATCH] Make cert_data.h and test_cryptoapi/pkcs11.c MSVC compliant

[Selva Nair]

Hi,

On Mon, Mar 27, 2023 at 9:59 AM Matthias Andree 
wrote:

> Am 27.03.23 um 13:49 schrieb selva.n...@gmail.com:
> > From: Selva Nair 
> >
> > - Do not use non-literal initializers for static objects
> > - Replace empty initializer {} by {0}
>
> Should we go to a revision, I would suggest to not make something
> compliant to a compiler because that is assigning it way too much power
> and control. A terms such as "ready to be compiled with MSVC" or so
> would be in order, or making this code compliant with some wisely chosen
> version of the ISO 9899 standard, aka. standard C.
>

In this case "MSVC compliant" was arguably a poor choice of words as what
the change does is to avoid non-C99 constructs. However it made sense to
refer to MSVC in the context of what prompted this patch.

That said, in a cross-platform code base one often has to make changes to
please compilers just to get things to build.

> Signed-off-by: Selva Nair 
> > ---
> > To be applied after the test-pkcs11 patch set
> >
> >   tests/unit_tests/openvpn/cert_data.h  |  6 ++---
> >   tests/unit_tests/openvpn/test_cryptoapi.c | 24 ++--
> >   tests/unit_tests/openvpn/test_pkcs11.c| 27 ---
> >   3 files changed, 39 insertions(+), 18 deletions(-)
> >
> > diff --git a/tests/unit_tests/openvpn/cert_data.h
> b/tests/unit_tests/openvpn/cert_data.h
> > index 33de35ec..0886b071 100644
> > --- a/tests/unit_tests/openvpn/cert_data.h
> > +++ b/tests/unit_tests/openvpn/cert_data.h
> > @@ -79,7 +79,7 @@ static const char *const cert2 =
> >
>  "HeTsAlHjfFEReVDiNCI9vMQLKFKKWnAorT2+iyRueA3bt2gchf863BBhZvJddL7Q\n"
> >   "KBa0osXw+eGBRAwsm7m1qCho3b3fN2nFAa+k07ptRkOeablmFdXE81nVlA==\n"
> >   "-END CERTIFICATE-\n";
> > -static const char *const key2 = key1;
> > +#define key2 key1
>
> So we are not getting rid of #defines yet? Can't we make this data
> somehow local? Personally, I would probably - at least outside unit

tests - write (key1) instead of bare key, so that it's clear this is one

name and can't be somehow extended.


I was not getting rid of any defines -- just trying to avoid defines if it
can be done without jumping through too many hoops. Right, (key1) would be
better.

Instead of making key2 local, we can write it out fully without referring
to the fact that key1 = key2. The latter was anyway a lazy man's choice,
not relevant for the working of the tests.

If anyone feels strongly I can submit a v2 with key2, key4, and cname4
fully written out like:

static const char *const key2 = "-BEGIN PRIVATE KEY-\n" ...
etc..
static const char *cname4 = "ovpn-test-rsa1";

That may also make maintaining this data easier -- ie., any cert/key pair
could be replaced without affecting others.

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

Re: [Openvpn-devel] [PATCH] Make cert_data.h and test_cryptoapi/pkcs11.c MSVC compliant

[Selva Nair]

On Mon, Mar 27, 2023 at 8:09 AM Frank Lichtenheld 
wrote:

> On Mon, Mar 27, 2023 at 07:49:37AM -0400, selva.n...@gmail.com wrote:
> > From: Selva Nair 
> >
> > - Do not use non-literal initializers for static objects
> > - Replace empty initializer {} by {0}
> >
> > Signed-off-by: Selva Nair 
>

I should have referenced Frank's original patch with:

Co-authored-by: Frank Lichtenheld 

> ---
> > To be applied after the test-pkcs11 patch set
>
> Tested MSVC build (in my cmake branch) and verified that this is a
> suitable replacement for my earlier attempt to fix same issue.
>
> Acked-By: Frank Lichtenheld 
>
> Regards,
> --
>   Frank Lichtenheld
>
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

[Openvpn-devel] [PATCH] Make cert_data.h and test_cryptoapi/pkcs11.c MSVC compliant

[selva . nair]

From: Selva Nair 

- Do not use non-literal initializers for static objects
- Replace empty initializer {} by {0}

Signed-off-by: Selva Nair 
---
To be applied after the test-pkcs11 patch set

 tests/unit_tests/openvpn/cert_data.h  |  6 ++---
 tests/unit_tests/openvpn/test_cryptoapi.c | 24 ++--
 tests/unit_tests/openvpn/test_pkcs11.c| 27 ---
 3 files changed, 39 insertions(+), 18 deletions(-)

diff --git a/tests/unit_tests/openvpn/cert_data.h 
b/tests/unit_tests/openvpn/cert_data.h
index 33de35ec..0886b071 100644
--- a/tests/unit_tests/openvpn/cert_data.h
+++ b/tests/unit_tests/openvpn/cert_data.h
@@ -79,7 +79,7 @@ static const char *const cert2 =
 "HeTsAlHjfFEReVDiNCI9vMQLKFKKWnAorT2+iyRueA3bt2gchf863BBhZvJddL7Q\n"
 "KBa0osXw+eGBRAwsm7m1qCho3b3fN2nFAa+k07ptRkOeablmFdXE81nVlA==\n"
 "-END CERTIFICATE-\n";
-static const char *const key2 = key1;
+#define key2 key1
 static const char *const hash2 = "FA18FD34BAABE47D6E2910E080F421C109CA97F5";
 static const char *const cname2 = "ovpn-test-ec2";
 
@@ -159,8 +159,8 @@ static const char *const cert4 =
 "353PpJJ9s2b/Fqoc4d7udqhQogA7jqbayTKhJxbT134l2NzqDROzuS0kXbX8bXCi\n"
 "mXSa4c8=\n"
 "-END CERTIFICATE-\n";
-static const char *const key4 = key3;
+#define key4 key3
 static const char *const hash4 = "E1401D4497C944783E3D62CDBD2A1F69F5E5071E";
-static const char *const cname4 = cname3; /* same CN as that of cert3 */
+#define cname4 cname3 /* same CN as that of cert3 */
 
 #endif /* CERT_DATA_H */
diff --git a/tests/unit_tests/openvpn/test_cryptoapi.c 
b/tests/unit_tests/openvpn/test_cryptoapi.c
index c8468103..2150b77c 100644
--- a/tests/unit_tests/openvpn/test_cryptoapi.c
+++ b/tests/unit_tests/openvpn/test_cryptoapi.c
@@ -99,17 +99,26 @@ static struct test_cert
 const char *const friendly_name;/* identifies certs loaded to the 
store -- keep unique */
 const char *hash;   /* SHA1 fingerprint */
 int valid;  /* nonzero if certificate has not 
expired */
-} certs[] = {
-{cert1,  key1,  cname1,  "OVPN TEST CA1",  "OVPN Test Cert 1",  hash1,  1},
-{cert2,  key2,  cname2,  "OVPN TEST CA2",  "OVPN Test Cert 2",  hash2,  1},
-{cert3,  key3,  cname3,  "OVPN TEST CA1",  "OVPN Test Cert 3",  hash3,  1},
-{cert4,  key4,  cname4,  "OVPN TEST CA2",  "OVPN Test Cert 4",  hash4,  0},
-{}
-};
+} certs[5];
 
 static bool certs_loaded;
 static HCERTSTORE user_store;
 
+/* Fill-in certs[] array */
+void
+init_cert_data()
+{
+struct test_cert certs_local[] = {
+{cert1,  key1,  cname1,  "OVPN TEST CA1",  "OVPN Test Cert 1",  hash1, 
 1},
+{cert2,  key2,  cname2,  "OVPN TEST CA2",  "OVPN Test Cert 2",  hash2, 
 1},
+{cert3,  key3,  cname3,  "OVPN TEST CA1",  "OVPN Test Cert 3",  hash3, 
 1},
+{cert4,  key4,  cname4,  "OVPN TEST CA2",  "OVPN Test Cert 4",  hash4, 
 0},
+{0}
+};
+assert(sizeof(certs_local) == sizeof(certs));
+memcpy(certs, certs_local, sizeof(certs_local));
+}
+
 /* Lookup a certificate in our certificate/key db */
 static struct test_cert *
 lookup_cert(const char *friendly_name)
@@ -131,6 +140,7 @@ import_certs(void **state)
 {
 return;
 }
+init_cert_data();
 user_store = CertOpenStore(CERT_STORE_PROV_SYSTEM, 0, 0, 
CERT_SYSTEM_STORE_CURRENT_USER
|CERT_STORE_OPEN_EXISTING_FLAG, L"MY");
 assert_non_null(user_store);
diff --git a/tests/unit_tests/openvpn/test_pkcs11.c 
b/tests/unit_tests/openvpn/test_pkcs11.c
index ea394bea..df5f8c7e 100644
--- a/tests/unit_tests/openvpn/test_pkcs11.c
+++ b/tests/unit_tests/openvpn/test_pkcs11.c
@@ -112,13 +112,7 @@ static struct test_cert
 const char *const friendly_name;/* identifies certs loaded to the 
store -- keep unique */
 uint8_t hash[HASHSIZE]; /* SHA1 fingerprint: computed and 
filled in later */
 char *p11_id;   /* PKCS#11 id -- filled in later */
-} certs[] = {
-{cert1,  key1,  cname1,  "OVPN TEST CA1",  "OVPN Test Cert 1",  {},  NULL},
-{cert2,  key2,  cname2,  "OVPN TEST CA2",  "OVPN Test Cert 2",  {},  NULL},
-{cert3,  key3,  cname3,  "OVPN TEST CA1",  "OVPN Test Cert 3",  {},  NULL},
-{cert4,  key4,  cname4,  "OVPN TEST CA2",  "OVPN Test Cert 4",  {},  NULL},
-{}
-};
+} certs[5];
 
 static bool pkcs11_id_management;
 static char softhsm2_tokens_path[] = "softhsm2_tokens_XX";
@@ -127,6 +121,21 @@ int num_certs;
 static const char *pkcs11_id_current;
 struct env_set *es;
 
+/* Fill-in certs[] array */
+void
+init_cert_data()
+{
+str

Re: [Openvpn-devel] [PATCH] unit_tests: make cert_data.h compile with MSVC

[Selva Nair]

On Mon, Mar 27, 2023 at 4:49 AM Frank Lichtenheld 
wrote:

> On Fri, Mar 24, 2023 at 01:13:22PM -0400, Selva Nair wrote:
> > Would the attached small patch be acceptable instead? It covers only
> > test_cryptoapi --- if this will do, I can incorporate similar changes for
> > test_pkcs11 into a v2 of the relevant patch as those have not been
> > ACK-ed yet.
>
> Actually, I acked that whole series. However there was a problem with
> my mail configuration on that day and so Gmail rejected them. They
> are on the list however.


> But feel free to integrate this and I can re-ack the patch.
>

In that case I'll send this as a single patch covering both cryptoapi and
pkcs11 tests.

Thanks,

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

Re: [Openvpn-devel] [PATCH] unit_tests: make cert_data.h compile with MSVC

[Selva Nair]

Hi,

On Fri, Mar 24, 2023 at 4:18 PM Matthias Andree 
wrote:

> Am 23.03.23 um 15:31 schrieb Frank Lichtenheld:
> > Currently this is not obvious since we never build the
> > UTs with MSVC, but it doesn't like the initializers with
> > "const" variables. They cause
> > error C2099: initializer is not a constant
>
> What MSVC version are you using? What options? I've tried with a minimal
> C program with static const char *const foo = "something"; and it
> compiled with the latest 2017 or 2022 MSVC.
>

That will work but the following wont:

const char *const foo = "something";
const char *const bar = foo;

which is essentially the issue at hand. And more of the same with struct
initialization. As bar has static storage duration, it has to be intialized
by a "constant expression" which in C does not include "const variables".
Though it works with gcc.  In C99, automatic variables can be intialized
so, and the alternative I suggested uses that approach.

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

Re: [Openvpn-devel] [PATCH] unit_tests: make cert_data.h compile with MSVC

[Selva Nair]

Hi,

On Thu, Mar 23, 2023 at 10:31 AM Frank Lichtenheld 
wrote:

> Currently this is not obvious since we never build the
> UTs with MSVC, but it doesn't like the initializers with
> "const" variables. They cause
> error C2099: initializer is not a constant
> when used in an initializer.
> So change all of them to preprocessor defines instead.
>
> It also doesn't like the empty initializer.
> error C2059: syntax error: '}'
>
> CC: Selva Nair 
> Signed-off-by: Frank Lichtenheld 
> ---
>  tests/unit_tests/openvpn/cert_data.h  | 240 +++---
>  tests/unit_tests/openvpn/test_cryptoapi.c |  10 +-
>  tests/unit_tests/openvpn/test_pkcs11.c|  10 +-
>  3 files changed, 127 insertions(+), 133 deletions(-)
>
> Notes:
> * this is an prereq for my CMake patch but is otherwise independent,
>   so submitting separately
> * this is on top of Selva's "Unit tests: Test for PKCS#11 using a
>   softhsm2 token"
>
> diff --git a/tests/unit_tests/openvpn/cert_data.h
> b/tests/unit_tests/openvpn/cert_data.h
> index 33de35ec..359718bb 100644
> --- a/tests/unit_tests/openvpn/cert_data.h
> +++ b/tests/unit_tests/openvpn/cert_data.h
> @@ -36,131 +36,125 @@
>   */
>
...


>  /* sample-ec.crt */
> +#define cd_cert1 "-BEGIN CERTIFICATE-\n" \
> +"MIIClzCCAX+gAwIBAgIRAIJr3cy95V63CPEtaAA8JN4wDQYJKoZIhvcNAQELBQAw\n" \
> +"GDEWMBQGA1UEAwwNT1ZQTiBURVNUIENBMTAgFw0yMzAzMTMxNjExMjhaGA8yMTIz\n" \
> +"MDIxNzE2MTEyOFowGDEWMBQGA1UEAwwNb3Zwbi10ZXN0LWVjMTBZMBMGByqGSM49\n" \
> +"AgEGCCqGSM49AwEHA0IABHhJG+dK4Z0mY+K0pupwVtyDLOwwGWHjBY6u3LgjRmUh\n" \
> +"fFjaoSfJvdgrPg50wbOkrsUt9Bl6EeDosZuVwuzgRbujgaQwgaEwCQYDVR0TBAIw\n" \
> +"ADAdBgNVHQ4EFgQUPWeU5BEmD8VEOSKeNf9kAvhcVuowUwYDVR0jBEwwSoAU3MLD\n" \
> +"NDOK13DqflQ8ra7FeGBXK06hHKQaMBgxFjAUBgNVBAMMDU9WUE4gVEVTVCBDQTGC\n" \
> +"FD55ErHXpK2JXS3WkfBm0NB1r3vKMBMGA1UdJQQMMAoGCCsGAQUFBwMCMAsGA1Ud\n" \
> +"DwQEAwIHgDANBgkqhkiG9w0BAQsFAAOCAQEAhH/wOFqP4R+FK5QvU+oW/XacFMku\n" \
> +"+qT8lL9J7BG28WhZ0ZcAy/AmtnyynkDyuZSwnlzGgJ5m4L/RfwTzJKhEHiSU3BvB\n" \
> +"5C1Z1Q8k67MHSfb565iCn8GzPUQLK4zsILCoTkJPvimv2bJ/RZmNaD+D4LWiySD4\n" \
> +"tuOEdHKrxIrbJ5eAaN0WxRrvDdwGlyPvbMFvfhXzd/tbkP4R2xvlm7S2DPeSTJ8s\n" \
> +"srXMaPe0lAea4etMSZsjIRPwGRMXBrwbRmb6iN2Cq40867HdaJoAryYig7IiDwSX\n" \
> +"htCbOA6sX+60+FEOYDEx5cmkogl633Pw7LJ3ICkyzIrUSEt6BOT1Gsc1eQ==\n" \
> +"-END CERTIFICATE-\n"
>

I would like to avoid such long preprocessor #defines with continuation
lines.

Would the attached small patch be acceptable instead? It covers only
test_cryptoapi --- if this will do, I can incorporate similar changes for
test_pkcs11 into a v2 of the relevant patch as those have not been
ACK-ed yet.


> --- a/tests/unit_tests/openvpn/test_pkcs11.c
> +++ b/tests/unit_tests/openvpn/test_pkcs11.c
> @@ -113,11 +113,11 @@ static struct test_cert
>  uint8_t hash[HASHSIZE]; /* SHA1 fingerprint: computed and
> filled in later */
>  char *p11_id;   /* PKCS#11 id -- filled in later
> */
>  } certs[] = {
> -{cert1,  key1,  cname1,  "OVPN TEST CA1",  "OVPN Test Cert 1",  {},
> NULL},
> -{cert2,  key2,  cname2,  "OVPN TEST CA2",  "OVPN Test Cert 2",  {},
> NULL},
> -{cert3,  key3,  cname3,  "OVPN TEST CA1",  "OVPN Test Cert 3",  {},
> NULL},
> -{cert4,  key4,  cname4,  "OVPN TEST CA2",  "OVPN Test Cert 4",  {},
> NULL},
> -{}
> +{cd_cert1,  cd_key1,  cd_cname1,  "OVPN TEST CA1",  "OVPN Test Cert
> 1",  {},  NULL},
>

Isn't it necessary to change these {} to {0} as well? The build may not
report it now as we haven't yet enabled test_pkcs11 on Windows.


> +{cd_cert2,  cd_key2,  cd_cname2,  "OVPN TEST CA2",  "OVPN Test Cert
> 2",  {},  NULL},
> +{cd_cert3,  cd_key3,  cd_cname3,  "OVPN TEST CA1",  "OVPN Test Cert
> 3",  {},  NULL},
> +{cd_cert4,  cd_key4,  cd_cname4,  "OVPN TEST CA2",  "OVPN Test Cert
> 4",  {},  NULL},
> +{NULL,  NULL, NULL,   NULL,  NULL,
>{},  NULL}
>

A single {0} should be enough here as well.

Selva
From 08d4813fdc3c90969c49dc07be529111a7e84ac4 Mon Sep 17 00:00:00 2001
From: Selva Nair 
Date: Thu, 23 Mar 2023 23:24:38 -0400
Subject: [PATCH 1/2] Make cert_data.h and test_cryptoapi.c MSVC compliant

- Do not use non-literal initializers for static objects
- Replace empty initializer {} by {0}

Signed-off-by: Selva Nair 
---
 tests/unit_tests/openvp

[Openvpn-devel] Fwd: [PATCH] Print DCO client stats on SIGUSR2

[Selva Nair]

I didn't realize it until Lev pointed out that this reply yesterday
didn't go to the list.  FTR, copying to the list.

-- Forwarded message -----
From: Selva Nair 
Date: Wed, Mar 22, 2023 at 9:42 AM
Subject: Re: [Openvpn-devel] [PATCH] Print DCO client stats on SIGUSR2
To: Lev Stipakov 



>
>
> >> +status_printf(so, "TCP/UDP read bytes," counter_format,
> c->c2.link_read_bytes + c->c2.dco_read_bytes);
> >> +status_printf(so, "TCP/UDP write bytes," counter_format,
> c->c2.link_write_bytes + c->c2.dco_write_bytes);
> >
> > Same here --  have a place to read the total count from independent of
> dco.
>
> But what is wrong in fetching dco stats when we need them? What is the
> advantage of timer?
>

What happens when we have three other drivers like DCO? Not saying that
will ever happen, but using that as an example to clarify why I think this
is "wrong" style.

That said, I agree why a timer may not be a good idea either. If an
approach that can keep internal details "private" is going to be too
complex, we can live with this.

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

[Openvpn-devel] [PATCH 3/3] Enable pkcs11 an dtest_pkcs11 in github actions

[selva . nair]

From: Selva Nair 

- Enabled for the Ubuntu 22.04 build (OpenSSL 3) and one of the
  Ubuntu 20.04 builds (OpenSSL 1.1.1).

Signed-off-by: Selva Nair 
---
 .github/workflows/build.yaml | 8 ++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/.github/workflows/build.yaml b/.github/workflows/build.yaml
index a3ca7a2e..2538c818 100644
--- a/.github/workflows/build.yaml
+++ b/.github/workflows/build.yaml
@@ -297,11 +297,14 @@ jobs:
 sslpkg: "libssl-dev"
 libname: OpenSSL 3.0.2
 ssllib: openssl
+pkcs11pkg: "libpkcs11-helper1-dev softhsm2 gnutls-bin"
+extraconf: --enable-pkcs11
   - os: ubuntu-20.04
 sslpkg: "libssl-dev"
 libname: OpenSSL 1.1.1
 ssllib: openssl
-extraconf: "--enable-iproute2"
+pkcs11pkg: "libpkcs11-helper1-dev softhsm2 gnutls-bin"
+extraconf: "--enable-iproute2 --enable-pkcs11"
   - os: ubuntu-20.04
 sslpkg: "libssl-dev"
 libname: OpenSSL 1.1.1
@@ -326,11 +329,12 @@ jobs:
 name: "gcc - ${{matrix.os}} - ${{matrix.libname}} ${{matrix.extraconf}}"
 env:
   SSLPKG: "${{matrix.sslpkg}}"
+  PKCS11PKG: "${{matrix.pkcs11pkg}}"
 
 runs-on: ${{matrix.os}}
 steps:
   - name: Install dependencies
-run: sudo apt update && sudo apt install -y liblzo2-dev libpam0g-dev 
liblz4-dev libcap-ng-dev libnl-genl-3-dev linux-libc-dev man2html libcmocka-dev 
python3-docutils libtool automake autoconf ${SSLPKG}
+run: sudo apt update && sudo apt install -y liblzo2-dev libpam0g-dev 
liblz4-dev libcap-ng-dev libnl-genl-3-dev linux-libc-dev man2html libcmocka-dev 
python3-docutils libtool automake autoconf ${SSLPKG} ${PKCS11PKG}
   - name: Checkout OpenVPN
 uses: actions/checkout@v3
   - name: autoconf
-- 
2.34.1



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

[Openvpn-devel] [PATCH 1/3] Move digest_sign_verify out of test_cryptoapi.c

[selva . nair]

From: Selva Nair 

- This function will be reused for testing pkcs11

Signed-off-by: Selva Nair 
---
 tests/unit_tests/openvpn/Makefile.am   |   1 +
 tests/unit_tests/openvpn/pkey_test_utils.c | 141 +
 tests/unit_tests/openvpn/test_cryptoapi.c  |  98 +-
 3 files changed, 143 insertions(+), 97 deletions(-)
 create mode 100644 tests/unit_tests/openvpn/pkey_test_utils.c

diff --git a/tests/unit_tests/openvpn/Makefile.am 
b/tests/unit_tests/openvpn/Makefile.am
index 4391a54e..30659561 100644
--- a/tests/unit_tests/openvpn/Makefile.am
+++ b/tests/unit_tests/openvpn/Makefile.am
@@ -156,6 +156,7 @@ cryptoapi_testdriver_CFLAGS  = @TEST_CFLAGS@ \
 cryptoapi_testdriver_LDFLAGS = @TEST_LDFLAGS@ \
$(OPTIONAL_CRYPTO_LIBS) -lcrypt32 -lncrypt
 cryptoapi_testdriver_SOURCES = test_cryptoapi.c mock_msg.c \
+   pkey_test_utils.c \
$(top_srcdir)/src/openvpn/xkey_helper.c \
$(top_srcdir)/src/openvpn/xkey_provider.c \
$(top_srcdir)/src/openvpn/buffer.c \
diff --git a/tests/unit_tests/openvpn/pkey_test_utils.c 
b/tests/unit_tests/openvpn/pkey_test_utils.c
new file mode 100644
index ..7adaf33f
--- /dev/null
+++ b/tests/unit_tests/openvpn/pkey_test_utils.c
@@ -0,0 +1,141 @@
+/*
+ *  OpenVPN -- An application to securely tunnel IP networks
+ * over a single UDP port, with support for SSL/TLS-based
+ * session authentication and key exchange,
+ * packet encryption, packet authentication, and
+ * packet compression.
+ *
+ *  Copyright (C) 2023 Selva Nair 
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by the
+ *  Free Software Foundation, either version 2 of the License,
+ *  or (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  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.
+ */
+
+#ifdef HAVE_CONFIG_H
+#include "config.h"
+#elif defined(_MSC_VER)
+#include "config-msvc.h"
+#endif
+
+
+#include "syshead.h"
+#include "xkey_common.h"
+#include 
+#include 
+
+#ifdef HAVE_XKEY_PROVIDER
+
+#include 
+#include 
+
+extern OSSL_LIB_CTX *tls_libctx;
+
+/* A message for signing */
+static const char *test_msg = "Lorem ipsum dolor sit amet, consectetur "
+  "adipisici elit, sed eiusmod tempor incidunt "
+  "ut labore et dolore magna aliqua.";
+
+/**
+ * Sign "test_msg" using a private key. The key may be a "provided" key
+ * in which case its signed by the provider's backend -- cryptoapi in our
+ * case. Then verify the signature using OpenSSL.
+ * Returns 1 on success, 0 on error.
+ */
+int
+digest_sign_verify(EVP_PKEY *privkey, EVP_PKEY *pubkey)
+{
+uint8_t *sig = NULL;
+size_t siglen = 0;
+int ret = 0;
+
+OSSL_PARAM params[2] = {OSSL_PARAM_END};
+const char *mdname = "SHA256";
+
+if (EVP_PKEY_get_id(privkey) == EVP_PKEY_RSA)
+{
+const char *padmode = "pss"; /* RSA_PSS: for all other params, use 
defaults */
+params[0] = 
OSSL_PARAM_construct_utf8_string(OSSL_SIGNATURE_PARAM_PAD_MODE,
+ (char *)padmode, 0);
+params[1] = OSSL_PARAM_construct_end();
+}
+else if (EVP_PKEY_get_id(privkey) == EVP_PKEY_EC)
+{
+params[0] = OSSL_PARAM_construct_end();
+}
+else
+{
+print_error("Unknown key type in digest_sign_verify()");
+return ret;
+}
+
+EVP_PKEY_CTX *pctx = NULL;
+EVP_MD_CTX *mctx = EVP_MD_CTX_new();
+
+if (!mctx
+|| EVP_DigestSignInit_ex(mctx, &pctx, mdname, tls_libctx, NULL, 
privkey,  params) <= 0)
+{
+/* cmocka assert output for these kinds of failures is hardly 
explanatory,
+ * print a message and assert in caller. */
+print_error("Failed to initialize EVP_DigestSignInit_ex()\n");
+goto done;
+}
+
+/* sign with sig = NULL to get required siglen */
+if (EVP_DigestSign(mctx, sig, &siglen, (uint8_t *)test_msg, 
strlen(test_msg)) != 1)
+{
+print_error("EVP_DigestSign: failed to get required signature size");
+goto done;
+}
+assert_true(siglen > 0);
+
+if ((sig = test_calloc(1, siglen)) == NULL)
+{
+print_error("Out of memory");
+goto done;
+}
+if (EVP_DigestSign(mctx, sig, &siglen, (uint8_t *)test_msg, 

[Openvpn-devel] [PATCH 2/3] Unit tests: Test for PKCS#11 using a softhsm2 token

[selva . nair]

From: Selva Nair 

- Load some test certificate/key pairs into a temporary softhsm2 token
  and enumerate available objects through pkcs11-helper interface

- For each object, load it into SSL_CTX and test sign (if using OpenSSL 3)
  or check the certificate and public-key match (if using OpenSSl 1.1.1.).
  The pkcs11-id for each object is specified directly or
  through a mocked management callback to test pkcs11-id-management

Limitations:
  Depends on libsofthsm2.so and p11tool (install softhsm2 and gnutls-bin
  packages). Mbed-TLS/pkcs11-helper combination is not tested.

  If locations of these binaries are not auto-detected or need to be
  overridden, use -DSOFTHSM2_UTIL= -DP11TOOL= to configure.
  Location of SOFTHSM2_MODULE is not auto-detected and defaults to
  /usr/lib/softhsm/libsofthsm2.so. It may be changed by passing
  -DSOFTHSM2_MODULE=/some-path/libsofthsm2.so to configure.
  Also see "configure --help".

  The test is enabled only if --enable-pkcs11 is in use, and SOFTHSM2_UTIL
  & P11TOOL are found in path or manually defined during configuring.

Changes relative to github PR
  - Explicitly disable building the test on Windows: need to port mkstemp,
mkdtemp, setenv etc., before enabling this on Windows.

Signed-off-by: Selva Nair 
---
 configure.ac   |  16 +
 tests/unit_tests/openvpn/Makefile.am   |  29 ++
 tests/unit_tests/openvpn/mock_msg.c|   6 +
 tests/unit_tests/openvpn/test_pkcs11.c | 453 +
 4 files changed, 504 insertions(+)
 create mode 100644 tests/unit_tests/openvpn/test_pkcs11.c

diff --git a/configure.ac b/configure.ac
index 67f680b2..ca85e5ed 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1345,6 +1345,7 @@ if test "${enable_comp_stub}" = "yes"; then
AC_DEFINE([ENABLE_COMP_STUB], [1], [Enable compression stub capability])
 fi
 
+AM_CONDITIONAL([HAVE_SOFTHSM2], [false])
 if test "${enable_pkcs11}" = "yes"; then
test "${have_pkcs11_helper}" != "yes" && AC_MSG_ERROR([PKCS11 enabled 
but libpkcs11-helper is missing])
OPTIONAL_PKCS11_HELPER_CFLAGS="${PKCS11_HELPER_CFLAGS}"
@@ -1357,6 +1358,21 @@ if test "${enable_pkcs11}" = "yes"; then
 AC_DEFINE_UNQUOTED([DEFAULT_PKCS11_MODULE], "${proxy_module}", 
[p11-kit proxy])],
[]
)
+   #
+   # softhsm2 for pkcs11 tests
+   #
+   AC_ARG_VAR([P11TOOL], [full path to p11tool])
+   AC_PATH_PROGS([P11TOOL], [p11tool],, 
[$PATH:/usr/local/bin:/usr/bin:/bin])
+   AC_DEFINE_UNQUOTED([P11TOOL_PATH], ["$P11TOOL"], [Path to p11tool])
+   AC_ARG_VAR([SOFTHSM2_UTIL], [full path to softhsm2-util])
+   AC_ARG_VAR([SOFTHSM2_MODULE], [full path to softhsm2 module 
@<:@default=/usr/lib/softhsm/libsofthsm2.so@:>@])
+   AC_PATH_PROGS([SOFTHSM2_UTIL], [softhsm2-util],, 
[$PATH:/usr/local/bin:/usr/bin:/bin])
+   test -z "$SOFTHSM2_MODULE" && 
SOFTHSM2_MODULE=/usr/lib/softhsm/libsofthsm2.so
+   AC_DEFINE_UNQUOTED([SOFTHSM2_UTIL_PATH], ["$SOFTHSM2_UTIL"], [Path to 
softhsm2-util])
+   AC_DEFINE_UNQUOTED([SOFTHSM2_MODULE_PATH], ["$SOFTHSM2_MODULE"], [Path 
to softhsm2 module])
+   if test "${with_crypto_library}" = "openssl"; then
+   AM_CONDITIONAL([HAVE_SOFTHSM2], [test "${P11TOOL}" -a 
"${SOFTHSM2_UTIL}" -a "${SOFTHSM2_MODULE}"])
+   fi
 fi
 
 # When testing a compiler option, we add -Werror to force
diff --git a/tests/unit_tests/openvpn/Makefile.am 
b/tests/unit_tests/openvpn/Makefile.am
index 30659561..ccd0e37b 100644
--- a/tests/unit_tests/openvpn/Makefile.am
+++ b/tests/unit_tests/openvpn/Makefile.am
@@ -21,6 +21,12 @@ test_binaries += cryptoapi_testdriver
 LDADD = -lws2_32
 endif
 
+if HAVE_SOFTHSM2
+if !WIN32
+test_binaries += pkcs11_testdriver
+endif
+endif
+
 if !CROSS_COMPILING
 TESTS = $(test_binaries)
 endif
@@ -166,6 +172,29 @@ cryptoapi_testdriver_SOURCES = test_cryptoapi.c mock_msg.c 
\
$(top_srcdir)/src/openvpn/win32-util.c
 endif
 
+if HAVE_SOFTHSM2
+if !WIN32
+pkcs11_testdriver_CFLAGS  = @TEST_CFLAGS@ \
+   -I$(top_srcdir)/include -I$(top_srcdir)/src/compat 
-I$(top_srcdir)/src/openvpn \
+   $(OPTIONAL_CRYPTO_CFLAGS)
+pkcs11_testdriver_LDFLAGS = @TEST_LDFLAGS@ \
+   $(OPTIONAL_CRYPTO_LIBS)
+pkcs11_testdriver_SOURCES = test_pkcs11.c mock_msg.c \
+   pkey_test_utils.c mock_get_random.c \
+   $(top_srcdir)/src/openvpn/xkey_helper.c \
+   $(top_srcdir)/src/openvpn/xkey_provider.c \
+   $(top_srcdir)/src/openvpn/argv.c \
+   $(top_srcdir)/src/openvpn/env_set.c \
+   $(top_srcdir)/src/openvpn/buffer.c \
+   $(top_srcdir)/src/openvpn/otime.c \
+   $(top_srcdir)/src/openvpn/run_command.c \
+   $(top_srcdir)/src/openvpn/base64.c \
+   $(top_srcdir)/src/openvpn/platform.c \
+   $

[Openvpn-devel] [PATCH] Make error in setting metric for IPv6 interface non-fatal

[selva . nair]

From: Selva Nair 

- Unfortunately there are still users out there who disable IPv6
  on tun/tap/dco interfaces or even system-wide.

Fixes: Github issue #294
Signed-off-by: Selva Nair 
---
 src/openvpnserv/interactive.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/openvpnserv/interactive.c b/src/openvpnserv/interactive.c
index a3d43752..98a42b1f 100644
--- a/src/openvpnserv/interactive.c
+++ b/src/openvpnserv/interactive.c
@@ -847,8 +847,8 @@ AddBlockDNS(const block_dns_message_t *msg, undo_lists_t 
*lists)
BLOCK_DNS_IFACE_METRIC);
 if (!err)
 {
-err = set_interface_metric(msg->iface.index, AF_INET6,
-   BLOCK_DNS_IFACE_METRIC);
+set_interface_metric(msg->iface.index, AF_INET6,
+ BLOCK_DNS_IFACE_METRIC);
 }
 if (err)
 {
-- 
2.34.1



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

Re: [Openvpn-devel] [PATCH] Print DCO client stats on SIGUSR2

[Selva Nair]

Hi,

On Wed, Mar 22, 2023 at 7:34 AM Lev Stipakov  wrote:

> From: Lev Stipakov 
>
> Change-Id: I465febdf7ee5fe573e88255844f718efb60f8e8a
> Signed-off-by: Lev Stipakov 
> ---
>  src/openvpn/sig.c | 13 +
>  src/openvpn/sig.h |  2 +-
>  2 files changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/src/openvpn/sig.c b/src/openvpn/sig.c
> index 5b89bb42..05c0054b 100644
> --- a/src/openvpn/sig.c
> +++ b/src/openvpn/sig.c
> @@ -300,18 +300,23 @@ restore_signal_state(void)
>   * Triggered by SIGUSR2 or F2 on Windows.
>   */
>  void
> -print_status(const struct context *c, struct status_output *so)
> +print_status(struct context *c, struct status_output *so)

 {
>  struct gc_arena gc = gc_new();
>
>  status_reset(so);
>
> +if (dco_enabled(&c->options))
> +{
> +dco_get_peer_stats(c);
> +}
> +
>

While this is expedient, adding dco dependence to parts of code which
should be independent of such inner details look wrong style to me. Finding
a way to keep struct context a const here will lead to a better logic, I
think.

Can't we have a coarse timer that keeps the stats updated? It doesn't have
to be frequent as this is just for information.


>  status_printf(so, "OpenVPN STATISTICS");
>  status_printf(so, "Updated,%s", time_string(0, 0, false, &gc));
>  status_printf(so, "TUN/TAP read bytes," counter_format,
> c->c2.tun_read_bytes);
>  status_printf(so, "TUN/TAP write bytes," counter_format,
> c->c2.tun_write_bytes);
> -status_printf(so, "TCP/UDP read bytes," counter_format,
> c->c2.link_read_bytes);
> -status_printf(so, "TCP/UDP write bytes," counter_format,
> c->c2.link_write_bytes);
> +status_printf(so, "TCP/UDP read bytes," counter_format,
> c->c2.link_read_bytes + c->c2.dco_read_bytes);
> +status_printf(so, "TCP/UDP write bytes," counter_format,
> c->c2.link_write_bytes + c->c2.dco_write_bytes);
>

Same here --  have a place to read the total count from independent of dco.

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

Re: [Openvpn-devel] [PATCH applied] Re: Improve error message on short read from socks proxy

[Selva Nair]

>
>
>
> Thanks, this change makes sense.  I have not actively tried to provoke
> it (like, by connecting to a "fake SOCKS server" that will trigger it),
> but the change is obviously an improvement to "if it's not ==1, it
> must be a TCP error!").
>

An easy way to "provoke" this is to use openssh proxy (say, -D 1080) and
use it to proxy to a udp server. SSH will close the connection as it does
not
support udp association. Probably it should return one of the socks5 error
code instead, but doesn't. Even if it did, our recv_socks_reply() is not
capable of
handling such errors.

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

[Openvpn-devel] [PATCH] Unit tests: add test for SSL_CTX_use_Cryptoapi_certificate()

[selva . nair]

From: Selva Nair 

- This is the only remaining function in cryptoapi.c that has no
  direct or indirect test.

  This test confirms that an SSL_CTX context gets a certificate and
  private key loaded into it and the public key in the certificate
  matches the private key. As signing with certificate/key pairs
  fetched from the store is independently tested by the 'cryptoapi_sign'
  test, signing is not re-tested here.

  The functions "setup_/teardown_cryptoapi_sign()" are renamed to
  "setup_/teardown_xkey_provider()" to better reflect their purpose.
  These are also reused for the new test.

  While touching this context, also fix a memory leak in
  test_cryptoapi_sign: X509_get_pubkey() -> X509_get0_pubkey()

Signed-off-by: Selva Nair 
---
 tests/unit_tests/openvpn/test_cryptoapi.c | 51 ---
 1 file changed, 46 insertions(+), 5 deletions(-)

diff --git a/tests/unit_tests/openvpn/test_cryptoapi.c 
b/tests/unit_tests/openvpn/test_cryptoapi.c
index b07e8935..e64a1de3 100644
--- a/tests/unit_tests/openvpn/test_cryptoapi.c
+++ b/tests/unit_tests/openvpn/test_cryptoapi.c
@@ -343,7 +343,7 @@ test_find_cert_byissuer(void **state)
 }
 
 static int
-setup_cryptoapi_sign(void **state)
+setup_xkey_provider(void **state)
 {
 (void) state;
 /* Initialize providers in a way matching what OpenVPN core does */
@@ -358,7 +358,7 @@ setup_cryptoapi_sign(void **state)
 }
 
 static int
-teardown_cryptoapi_sign(void **state)
+teardown_xkey_provider(void **state)
 {
 (void) state;
 for (size_t i = 0; i < _countof(prov); i++)
@@ -493,13 +493,52 @@ test_cryptoapi_sign(void **state)
 fail_msg("Load_CryptoAPI_certificate failed: <%s>", 
c->friendly_name);
 return;
 }
-EVP_PKEY *pubkey = X509_get_pubkey(x509);
+EVP_PKEY *pubkey = X509_get0_pubkey(x509);
+assert_non_null(pubkey);
 assert_int_equal(digest_sign_verify(privkey, pubkey), 1);
 X509_free(x509);
 EVP_PKEY_free(privkey);
 }
 }
 
+/* Test that SSL_CTX_use_Cryptoapi_certificate() sets a matching certificate
+ * and key in ssl_ctx.
+ */
+void
+test_ssl_ctx_use_cryptoapicert(void **state)
+{
+(void) state;
+char select_string[64];
+
+import_certs(state); /* a no-op if already imported */
+assert_true(certs_loaded);
+
+for (struct test_cert *c = certs; c->cert; c++)
+{
+if (c->valid == 0)
+{
+continue;
+}
+SSL_CTX *ssl_ctx = SSL_CTX_new_ex(tls_libctx, NULL, 
SSLv23_client_method());
+assert_non_null(ssl_ctx);
+
+openvpn_snprintf(select_string, sizeof(select_string), "THUMB:%s", 
c->hash);
+if (!SSL_CTX_use_CryptoAPI_certificate(ssl_ctx, select_string))
+{
+fail_msg("SSL_CTX_use_CryptoAPI_certificate failed: <%s>", 
c->friendly_name);
+return;
+}
+/* Use OpenSSL to check that the cert and private key in ssl_ctx 
"match" */
+if (!SSL_CTX_check_private_key(ssl_ctx))
+{
+fail_msg("Certificate and private key in ssl_ctx do not match for 
<%s>", c->friendly_name);
+return;
+}
+
+SSL_CTX_free(ssl_ctx);
+}
+}
+
 static void
 test_parse_hexstring(void **state)
 {
@@ -530,8 +569,10 @@ main(void)
 cmocka_unit_test(test_find_cert_bythumb),
 cmocka_unit_test(test_find_cert_byname),
 cmocka_unit_test(test_find_cert_byissuer),
-cmocka_unit_test_setup_teardown(test_cryptoapi_sign, 
setup_cryptoapi_sign,
-teardown_cryptoapi_sign),
+cmocka_unit_test_setup_teardown(test_cryptoapi_sign, 
setup_xkey_provider,
+teardown_xkey_provider),
+cmocka_unit_test_setup_teardown(test_ssl_ctx_use_cryptoapicert, 
setup_xkey_provider,
+teardown_xkey_provider),
 };
 
 int ret = cmocka_run_group_tests_name("cryptoapi tests", tests, NULL, 
cleanup);
-- 
2.34.1



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

[Openvpn-devel] [PATCH] Improve error message on short read from socks proxy

[selva . nair]

From: Selva Nair 

Change-Id: Id6bf8ea705d02eff2cbfba7d841e1cdb6ae1
Signed-off-by: Selva Nair 
---
 src/openvpn/socks.c | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/src/openvpn/socks.c b/src/openvpn/socks.c
index 6a672c25..2cf0cc9f 100644
--- a/src/openvpn/socks.c
+++ b/src/openvpn/socks.c
@@ -357,11 +357,16 @@ recv_socks_reply(socket_descriptor_t sd,
 size = recv(sd, &c, 1, MSG_NOSIGNAL);
 
 /* error? */
-if (size != 1)
+if (size < 0)
 {
 msg(D_LINK_ERRORS | M_ERRNO, "recv_socks_reply: TCP port read 
failed on recv()");
 return false;
 }
+else if (size == 0)
+{
+msg(D_LINK_ERRORS, "ERROR: recv_socks_reply: empty response from 
socks server");
+return false;
+}
 
 if (len == 3)
 {
-- 
2.34.1



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

Re: [Openvpn-devel] [PATCH applied] Re: Import some sample certificates into Windows store for testing

[Selva Nair]

>
>
> I have tried testing this on a "real" Win10 VM, but I am missing some
> intricacies with the cert store - I wanted to see if certs are properly
> cleaned up, but I can't even see my own .p12 I have imported, so I am
> doing something wrong...


This works for me:
Run "certmgr" from a user command prompt. The UI that opens up will show
the "Certificates - Current User" snapin. Under it select
"Personal->Certificates", and a list of all imported certificates will open
up. This will not include any Root certificates that may have been
imported. Those go to "Trusted Root Certificates" listed below "Personal".

I'm not entirely sure whether it's possible to select a wrong destination
during import, causing client certificates to go into root certificates.
During import I only select the store (user or machine) and let it decide
the appropriate location within the store.

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

Re: [Openvpn-devel] [PATCH 0/4] Add some tests for cryptoapi.c functions

[Selva Nair]

On Wed, Mar 15, 2023 at 4:30 AM Gert Doering  wrote:

> Hi,
>
> On Tue, Mar 14, 2023 at 09:35:12PM -0400, selva.n...@gmail.com wrote:
> > From: Selva Nair 
> >
> >   Import some sample certificates into Windows store for testing
> > - 4 test certificates&keys imported to user store
> >   and removed at the end.
> [..]
>
> This is extremely valuable.  Thanks.
>
> If I apply these 4 patches on top of "master", will the test then succeed,
> or will it show failure due to the not-yet-merged padding patch?
>

As cryptoapi works correctly without the padding patch (only PKCS11 was
affected), and this tests only exercises those bits should work without the
padding patch. Though I tested it only with both included.

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

[Openvpn-devel] [PATCH 1/4] Import some sample certificates into Windows store for testing

[selva . nair]

From: Selva Nair 

- A few sample certificates are defined and imported into
  Windows certificate store (user store).
  This only tests the import process. Use of these certs to test the
  core functionality of 'cryptoapicert' are in following commits.

Change-Id: Ida5fc12c5bad5fde202da0bf0e8cdc71efe548c2
Signed-off-by: Selva Nair 
---
 tests/unit_tests/openvpn/cert_data.h  | 166 ++
 tests/unit_tests/openvpn/test_cryptoapi.c | 160 -
 2 files changed, 324 insertions(+), 2 deletions(-)
 create mode 100644 tests/unit_tests/openvpn/cert_data.h

diff --git a/tests/unit_tests/openvpn/cert_data.h 
b/tests/unit_tests/openvpn/cert_data.h
new file mode 100644
index ..33de35ec
--- /dev/null
+++ b/tests/unit_tests/openvpn/cert_data.h
@@ -0,0 +1,166 @@
+/*
+ *  OpenVPN -- An application to securely tunnel IP networks
+ * over a single UDP port, with support for SSL/TLS-based
+ * session authentication and key exchange,
+ * packet encryption, packet authentication, and
+ * packet compression.
+ *
+ *  Copyright (C) 2023 Selva Nair 
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by the
+ *  Free Software Foundation, either version 2 of the License,
+ *  or (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  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 CERT_DATA_H
+#define CERT_DATA_H
+
+/* Some certificates and their private keys for testing cryptoapi.c.
+ * Two certificates, cert1 (EC) and cert3 (RSA) are signed by one CA
+ * and the other two, cert2 (EC) and cert4 (RSA), by another to have a
+ * different issuer name. The common name of cert4 is the same as
+ * that of cert3 but the former has expired. It is used to test
+ * retrieval of valid certificate by name when an expired one with same
+ * common name exists.
+ * To reduce data volume, certs of same keytype use the same private key.
+ */
+
+/* sample-ec.crt */
+static const char *const cert1 =
+"-BEGIN CERTIFICATE-\n"
+"MIIClzCCAX+gAwIBAgIRAIJr3cy95V63CPEtaAA8JN4wDQYJKoZIhvcNAQELBQAw\n"
+"GDEWMBQGA1UEAwwNT1ZQTiBURVNUIENBMTAgFw0yMzAzMTMxNjExMjhaGA8yMTIz\n"
+"MDIxNzE2MTEyOFowGDEWMBQGA1UEAwwNb3Zwbi10ZXN0LWVjMTBZMBMGByqGSM49\n"
+"AgEGCCqGSM49AwEHA0IABHhJG+dK4Z0mY+K0pupwVtyDLOwwGWHjBY6u3LgjRmUh\n"
+"fFjaoSfJvdgrPg50wbOkrsUt9Bl6EeDosZuVwuzgRbujgaQwgaEwCQYDVR0TBAIw\n"
+"ADAdBgNVHQ4EFgQUPWeU5BEmD8VEOSKeNf9kAvhcVuowUwYDVR0jBEwwSoAU3MLD\n"
+"NDOK13DqflQ8ra7FeGBXK06hHKQaMBgxFjAUBgNVBAMMDU9WUE4gVEVTVCBDQTGC\n"
+"FD55ErHXpK2JXS3WkfBm0NB1r3vKMBMGA1UdJQQMMAoGCCsGAQUFBwMCMAsGA1Ud\n"
+"DwQEAwIHgDANBgkqhkiG9w0BAQsFAAOCAQEAhH/wOFqP4R+FK5QvU+oW/XacFMku\n"
+"+qT8lL9J7BG28WhZ0ZcAy/AmtnyynkDyuZSwnlzGgJ5m4L/RfwTzJKhEHiSU3BvB\n"
+"5C1Z1Q8k67MHSfb565iCn8GzPUQLK4zsILCoTkJPvimv2bJ/RZmNaD+D4LWiySD4\n"
+"tuOEdHKrxIrbJ5eAaN0WxRrvDdwGlyPvbMFvfhXzd/tbkP4R2xvlm7S2DPeSTJ8s\n"
+"srXMaPe0lAea4etMSZsjIRPwGRMXBrwbRmb6iN2Cq40867HdaJoAryYig7IiDwSX\n"
+"htCbOA6sX+60+FEOYDEx5cmkogl633Pw7LJ3ICkyzIrUSEt6BOT1Gsc1eQ==\n"
+"-END CERTIFICATE-\n";
+static const char *const key1 =
+"-BEGIN PRIVATE KEY-\n"
+"MIGHAgEAMBMGByqGSM49AgEGCCqGSM49AwEHBG0wawIBAQQg5Xpw/lLvBrWjAWDq\n"
+"L6dm/4a1or6AQ6O3yXYgw78B23ihRANCAAR4SRvnSuGdJmPitKbqcFbcgyzsMBlh\n"
+"4wWOrty4I0ZlIXxY2qEnyb3YKz4OdMGzpK7FLfQZehHg6LGblcLs4EW7\n"
+"-END PRIVATE KEY-\n";
+static const char *const hash1 = "A4B74F1D68AF50691F62CBD675E24C8655369567";
+static const char *const cname1 = "ovpn-test-ec1";
+
+static const char *const cert2 =
+"-BEGIN CERTIFICATE-\n"
+"MIIClzCCAX+gAwIBAgIRAN9fIkTDOjX0Bd9adHVcLx8wDQYJKoZIhvcNAQELBQAw\n"
+"GDEWMBQGA1UEAwwNT1ZQTiBURVNUIENBMjAgFw0yMzAzMTMxODAzMzFaGA8yMTIz\n"
+"MDIxNzE4MDMzMVowGDEWMBQGA1UEAwwNb3Zwbi10ZXN0LWVjMjBZMBMGByqGSM49\n"
+"AgEGCCqGSM49AwEHA0IABHhJG+dK4Z0mY+K0pupwVtyDLOwwGWHjBY6u3LgjRmUh\n"
+"fFjaoSfJvdgrPg50wbOkrsUt9Bl6EeDosZuVwuzgRbujgaQwgaEwCQYDVR0TBAIw\n"
+"ADAdBgNVHQ4EFgQUPWeU5BEmD8VEOSKeNf9kAvhcVuowUwYDVR0jBEwwSoAUyX3c\n"
+"tpRP5cKlESsG80rOGhEphsGhHKQaMBgxFjAUBgNVBAMMDU9WUE4gVEVTVCBDQTKC\n&q

[Openvpn-devel] [PATCH 4/4] Add a test for signing with certificates in Windows store

[selva . nair]

From: Selva Nair 

- For each sample certificate/key pair imported into the store,
  load the key into xkey-provider and sign a test message.
  As the key is "provided", signing will use appropriate
  backend (Windows CNG in this case).

  The signature is then verified using OpenSSL.

Change-Id: I520b34ba51e8c6d0247a82edc52bde181ab5a717
Signed-off-by: Selva Nair 
---
 tests/unit_tests/openvpn/Makefile.am  |   1 +
 tests/unit_tests/openvpn/test_cryptoapi.c | 166 ++
 2 files changed, 167 insertions(+)

diff --git a/tests/unit_tests/openvpn/Makefile.am 
b/tests/unit_tests/openvpn/Makefile.am
index 339c7ef3..4391a54e 100644
--- a/tests/unit_tests/openvpn/Makefile.am
+++ b/tests/unit_tests/openvpn/Makefile.am
@@ -157,6 +157,7 @@ cryptoapi_testdriver_LDFLAGS = @TEST_LDFLAGS@ \
$(OPTIONAL_CRYPTO_LIBS) -lcrypt32 -lncrypt
 cryptoapi_testdriver_SOURCES = test_cryptoapi.c mock_msg.c \
$(top_srcdir)/src/openvpn/xkey_helper.c \
+   $(top_srcdir)/src/openvpn/xkey_provider.c \
$(top_srcdir)/src/openvpn/buffer.c \
$(top_srcdir)/src/openvpn/base64.c \
$(top_srcdir)/src/openvpn/platform.c \
diff --git a/tests/unit_tests/openvpn/test_cryptoapi.c 
b/tests/unit_tests/openvpn/test_cryptoapi.c
index ccb3207c..b07e8935 100644
--- a/tests/unit_tests/openvpn/test_cryptoapi.c
+++ b/tests/unit_tests/openvpn/test_cryptoapi.c
@@ -47,6 +47,7 @@
 #include  /* pull-in the whole file to test static functions */
 
 struct management *management; /* global */
+static OSSL_PROVIDER *prov[2];
 
 /* mock a management function that xkey_provider needs */
 char *
@@ -66,6 +67,11 @@ OSSL_LIB_CTX *tls_libctx;
 #define _countof(x) sizeof((x))/sizeof(*(x))
 #endif
 
+/* A message for signing */
+static const char *test_msg = "Lorem ipsum dolor sit amet, consectetur "
+  "adipisici elit, sed eiusmod tempor incidunt "
+  "ut labore et dolore magna aliqua.";
+
 /* test data */
 static const uint8_t test_hash[] = {
 0x77, 0x38, 0x65, 0x00, 0x1e, 0x96, 0x48, 0xc6, 0x57, 0x0b, 0xae,
@@ -336,6 +342,164 @@ test_find_cert_byissuer(void **state)
 gc_free(&gc);
 }
 
+static int
+setup_cryptoapi_sign(void **state)
+{
+(void) state;
+/* Initialize providers in a way matching what OpenVPN core does */
+tls_libctx = OSSL_LIB_CTX_new();
+prov[0] = OSSL_PROVIDER_load(tls_libctx, "default");
+OSSL_PROVIDER_add_builtin(tls_libctx, "ovpn.xkey", xkey_provider_init);
+prov[1] = OSSL_PROVIDER_load(tls_libctx, "ovpn.xkey");
+
+/* set default propq as we do in ssl_openssl.c */
+EVP_set_default_properties(tls_libctx, "?provider!=ovpn.xkey");
+return 0;
+}
+
+static int
+teardown_cryptoapi_sign(void **state)
+{
+(void) state;
+for (size_t i = 0; i < _countof(prov); i++)
+{
+if (prov[i])
+{
+OSSL_PROVIDER_unload(prov[i]);
+prov[i] = NULL;
+}
+}
+OSSL_LIB_CTX_free(tls_libctx);
+tls_libctx = NULL;
+return 0;
+}
+
+/**
+ * Sign "test_msg" using a private key. The key may be a "provided" key
+ * in which case its signed by the provider's backend -- cryptoapi in our
+ * case. Then verify the signature using OpenSSL.
+ * Returns 1 on success, 0 on error.
+ */
+static int
+digest_sign_verify(EVP_PKEY *privkey, EVP_PKEY *pubkey)
+{
+uint8_t *sig = NULL;
+size_t siglen = 0;
+int ret = 0;
+
+OSSL_PARAM params[2] = {OSSL_PARAM_END};
+const char *mdname = "SHA256";
+
+if (EVP_PKEY_get_id(privkey) == EVP_PKEY_RSA)
+{
+const char *padmode = "pss"; /* RSA_PSS: for all other params, use 
defaults */
+params[0] = 
OSSL_PARAM_construct_utf8_string(OSSL_SIGNATURE_PARAM_PAD_MODE,
+ (char *)padmode, 0);
+params[1] = OSSL_PARAM_construct_end();
+}
+else if (EVP_PKEY_get_id(privkey) == EVP_PKEY_EC)
+{
+params[0] = OSSL_PARAM_construct_end();
+}
+else
+{
+print_error("Unknown key type in digest_sign_verify()");
+return ret;
+}
+
+EVP_PKEY_CTX *pctx = NULL;
+EVP_MD_CTX *mctx = EVP_MD_CTX_new();
+
+if (!mctx
+|| EVP_DigestSignInit_ex(mctx, &pctx, mdname, tls_libctx, NULL, 
privkey,  params) <= 0)
+{
+/* cmocka assert output for these kinds of failures is hardly 
explanatory,
+ * print a message and assert in caller. */
+print_error("Failed to initialize EVP_DigestSignInit_ex()\n");
+goto done;
+}
+
+/* sign with sig = NULL to get required siglen */
+if (EVP_DigestSign(mctx, sig, &siglen, (uint8_t *)test_msg, 
strlen(test_msg)) != 1)
+{
+print_error("EVP_DigestSign: failed to get required signature size");
+goto done;
+}
+assert_true(siglen

[Openvpn-devel] [PATCH 2/4] Add tests for finding certificates in Windows cert store

[selva . nair]

From: Selva Nair 

- find_certificate_in_store tested using 'SUBJ:', 'THUMB:'
  and 'ISSUER:' select strings. Uses test certificates
  imported into the store during the import test.

Change-Id: Ib5138465e6228538af592ca98b3d877277355f59
Signed-off-by: Selva Nair 
---
 tests/unit_tests/openvpn/test_cryptoapi.c | 102 ++
 1 file changed, 102 insertions(+)

diff --git a/tests/unit_tests/openvpn/test_cryptoapi.c 
b/tests/unit_tests/openvpn/test_cryptoapi.c
index 54dbd094..ccb3207c 100644
--- a/tests/unit_tests/openvpn/test_cryptoapi.c
+++ b/tests/unit_tests/openvpn/test_cryptoapi.c
@@ -237,6 +237,105 @@ cleanup(void **state)
 return 0;
 }
 
+static void
+test_find_cert_bythumb(void **state)
+{
+(void) state;
+char select_string[64];
+struct gc_arena gc = gc_new();
+const CERT_CONTEXT *ctx;
+
+import_certs(state); /* a no-op if already imported */
+assert_non_null(user_store);
+
+for (struct test_cert *c = certs; c->cert; c++)
+{
+openvpn_snprintf(select_string, sizeof(select_string), "THUMB:%s", 
c->hash);
+ctx = find_certificate_in_store(select_string, user_store);
+if (ctx)
+{
+/* check we got the right certificate and is valid */
+assert_int_equal(c->valid, 1);
+char *friendly_name = get_cert_name(ctx, &gc);
+assert_string_equal(c->friendly_name, friendly_name);
+CertFreeCertificateContext(ctx);
+}
+else
+{
+/* find should fail only if the certificate has expired */
+assert_int_equal(c->valid, 0);
+}
+}
+
+gc_free(&gc);
+}
+
+static void
+test_find_cert_byname(void **state)
+{
+(void) state;
+char select_string[64];
+struct gc_arena gc = gc_new();
+const CERT_CONTEXT *ctx;
+
+import_certs(state); /* a no-op if already imported */
+assert_non_null(user_store);
+
+for (struct test_cert *c = certs; c->cert; c++)
+{
+openvpn_snprintf(select_string, sizeof(select_string), "SUBJ:%s", 
c->cname);
+ctx = find_certificate_in_store(select_string, user_store);
+/* In this case we expect a successful return as there is at least one 
valid
+ * cert that matches the common name. But the returned cert may not 
exactly match
+ * c->cert as multiple certs with same common names exist in the db. 
We check that
+ * the return cert is one from our db, has a matching common name and 
is valid.
+ */
+assert_non_null(ctx);
+
+char *friendly_name = get_cert_name(ctx, &gc);
+struct test_cert *found = lookup_cert(friendly_name);
+assert_non_null(found);
+assert_string_equal(found->cname, c->cname);
+assert_int_equal(found->valid, 1);
+CertFreeCertificateContext(ctx);
+}
+
+gc_free(&gc);
+}
+
+static void
+test_find_cert_byissuer(void **state)
+{
+(void) state;
+char select_string[64];
+struct gc_arena gc = gc_new();
+const CERT_CONTEXT *ctx;
+
+import_certs(state); /* a no-op if already imported */
+assert_non_null(user_store);
+
+for (struct test_cert *c = certs; c->cert; c++)
+{
+openvpn_snprintf(select_string, sizeof(select_string), "ISSUER:%s", 
c->issuer);
+ctx = find_certificate_in_store(select_string, user_store);
+/* In this case we expect a successful return as there is at least one 
valid
+ * cert that matches the issuer. But the returned cert may not exactly 
match
+ * c->cert as multiple certs with same issuer exist in the db. We 
check that
+ * the returned cert is one from our db, has a matching issuer name 
and is valid.
+ */
+assert_non_null(ctx);
+
+char *friendly_name = get_cert_name(ctx, &gc);
+struct test_cert *found = lookup_cert(friendly_name);
+assert_non_null(found);
+assert_string_equal(found->issuer, c->issuer);
+assert_int_equal(found->valid, 1);
+CertFreeCertificateContext(ctx);
+}
+
+gc_free(&gc);
+}
+
 static void
 test_parse_hexstring(void **state)
 {
@@ -264,6 +363,9 @@ main(void)
 const struct CMUnitTest tests[] = {
 cmocka_unit_test(test_parse_hexstring),
 cmocka_unit_test(import_certs),
+cmocka_unit_test(test_find_cert_bythumb),
+cmocka_unit_test(test_find_cert_byname),
+cmocka_unit_test(test_find_cert_byissuer),
 };
 
 int ret = cmocka_run_group_tests_name("cryptoapi tests", tests, NULL, 
cleanup);
-- 
2.34.1



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

[Openvpn-devel] [PATCH 3/4] Refactor SSL_CTX_use_CryptoAPI_certificate()

[selva . nair]

From: Selva Nair 

- Loading the certificate and key into the provider is split out of
  setting up the SSL context. This allows testing of signing by
  cryptoapi-provider interface without dependence on SSL context
  or link-time wrapping.

Change-Id: I269b94589636425e1ba9bf953047d238fa830376
Signed-off-by: Selva Nair 
---
 src/openvpn/cryptoapi.c | 63 +++--
 1 file changed, 41 insertions(+), 22 deletions(-)

diff --git a/src/openvpn/cryptoapi.c b/src/openvpn/cryptoapi.c
index 022f53d4..20b7d985 100644
--- a/src/openvpn/cryptoapi.c
+++ b/src/openvpn/cryptoapi.c
@@ -401,11 +401,17 @@ get_cert_name(const CERT_CONTEXT *cc, struct gc_arena *gc)
 return name;
 }
 
-int
-SSL_CTX_use_CryptoAPI_certificate(SSL_CTX *ssl_ctx, const char *cert_prop)
+/**
+ * Load certificate matching 'cert_prop' from Windows cert store
+ * into xkey provider and return pointers to X509 cert and private key.
+ * Returns 1 on success, 0 on error.
+ * Caller must free 'cert' and 'privkey' after use.
+ */
+static int
+Load_CryptoAPI_certificate(const char *cert_prop, X509 **cert, EVP_PKEY 
**privkey)
 {
+
 HCERTSTORE cs;
-X509 *cert = NULL;
 CAPI_DATA *cd = calloc(1, sizeof(*cd));
 struct gc_arena gc = gc_new();
 
@@ -450,9 +456,9 @@ SSL_CTX_use_CryptoAPI_certificate(SSL_CTX *ssl_ctx, const 
char *cert_prop)
 }
 
 /* cert_context->pbCertEncoded is the cert X509 DER encoded. */
-cert = d2i_X509(NULL, (const unsigned char **) 
&cd->cert_context->pbCertEncoded,
-cd->cert_context->cbCertEncoded);
-if (cert == NULL)
+*cert = d2i_X509(NULL, (const unsigned char **) 
&cd->cert_context->pbCertEncoded,
+ cd->cert_context->cbCertEncoded);
+if (*cert == NULL)
 {
 msg(M_NONFATAL, "Error in cryptoapicert: X509 certificate decode 
failed");
 goto err;
@@ -468,28 +474,16 @@ SSL_CTX_use_CryptoAPI_certificate(SSL_CTX *ssl_ctx, const 
char *cert_prop)
 /* private key may be in a token not available, or incompatible with 
CNG */
 msg(M_NONFATAL|M_ERRNO, "Error in cryptoapicert: failed to acquire 
key. Key not present or "
 "is in a legacy token not supported by Windows CNG API");
-goto err;
-}
-
-/* Public key in cert is NULL until we call SSL_CTX_use_certificate(),
- * so we do it here then...  */
-if (!SSL_CTX_use_certificate(ssl_ctx, cert))
-{
+X509_free(*cert);
 goto err;
 }
 
 /* the public key */
-EVP_PKEY *pkey = X509_get_pubkey(cert);
+EVP_PKEY *pkey = X509_get_pubkey(*cert);
 cd->pubkey = pkey; /* will be freed with cd */
 
-/* SSL_CTX_use_certificate() increased the reference count in 'cert', so
- * we decrease it here with X509_free(), or it will never be cleaned up. */
-X509_free(cert);
-cert = NULL;
-
-EVP_PKEY *privkey = xkey_load_generic_key(tls_libctx, cd, pkey,
-  xkey_cng_sign, 
(XKEY_PRIVKEY_FREE_fn *) CAPI_DATA_free);
-SSL_CTX_use_PrivateKey(ssl_ctx, privkey);
+*privkey = xkey_load_generic_key(tls_libctx, cd, pkey,
+ xkey_cng_sign, (XKEY_PRIVKEY_FREE_fn *) 
CAPI_DATA_free);
 gc_free(&gc);
 return 1; /* do not free cd -- its kept by xkey provider */
 
@@ -498,5 +492,30 @@ err:
 gc_free(&gc);
 return 0;
 }
+
+int
+SSL_CTX_use_CryptoAPI_certificate(SSL_CTX *ssl_ctx, const char *cert_prop)
+{
+X509 *cert = NULL;
+EVP_PKEY *privkey = NULL;
+int ret = 0;
+
+if (!Load_CryptoAPI_certificate(cert_prop, &cert, &privkey))
+{
+return ret;
+}
+if (SSL_CTX_use_certificate(ssl_ctx, cert)
+&& SSL_CTX_use_PrivateKey(ssl_ctx, privkey))
+{
+ret = 1;
+}
+
+/* Always free cert and privkey even if retained by ssl_ctx as
+ * they are reference counted */
+X509_free(cert);
+EVP_PKEY_free(privkey);
+return ret;
+}
+
 #endif  /* HAVE_XKEY_PROVIDER */
 #endif  /* _WIN32 */
-- 
2.34.1



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

[Openvpn-devel] [PATCH 0/4] Add some tests for cryptoapi.c functions

[selva . nair]

From: Selva Nair 

  Import some sample certificates into Windows store for testing
- 4 test certificates&keys imported to user store
  and removed at the end.

  Add tests for finding certificates in Windows certficate store
- test using SUBJ:, THUMB: and ISSUER: select-strings

  Refactor SSL_CTX_use_CryptoAPI_certificate()
- A minor reorganization avoids wrapping for the next test

  Add a test for signing with certificates in Windows store
- Test loading keys into xkey-provider and sign a test message.
  The signature is then verified using OpenSSL.

Sample output the test runs:
https://github.com/selvanair/openvpn/actions/runs/4418774866/jobs/7746404938#step:8:1

 src/openvpn/cryptoapi.c   |  63 ++--
 tests/unit_tests/openvpn/Makefile.am  |   1 +
 tests/unit_tests/openvpn/cert_data.h  | 166 +
 tests/unit_tests/openvpn/test_cryptoapi.c | 429 +-
 4 files changed, 635 insertions(+), 24 deletions(-)
 create mode 100644 tests/unit_tests/openvpn/cert_data.h

-- 
2.34.1



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

Re: [Openvpn-devel] [PATCH] Bugfix: Convert ECDSA signature form pkcs11-helper to DER encoded form

[Selva Nair]

Hi,

On Tue, Mar 14, 2023 at 5:54 AM David Sommerseth 
wrote:

>
>
> Just got feedback from the reporter in the Fedora bugzilla; this patch
> works well on Fedora 38.
>
> I suggest adding this tag to the commit log.  Feel free to add the URL
> tag to the bugzilla ticket too.
>
> Tested-by: flor...@apolloner.eu


Thanks. Updated patch is on the list.

I "heard" whispers of "2.6.2 coming soon" in another thread. Would be great
if this fix can make it.

As this also touches cryptoapicert (via a refactor), an independent test on
Windows
would be useful.

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

[Openvpn-devel] [PATCH v2] Bugfix: Convert ECDSA signature form pkcs11-helper to DER encoded form

[selva . nair]

From: Selva Nair 

- With OpenSSL 3.0 and xkey-provider, we use pkcs11h_certificate_signAny_ex()
  which returns EC signature as raw r|s concatenated. But OpenSSL expects
  a DER encoded ASN.1 structure.

  Do this conversion as done in cryptoapi.c. For code re-use, ecdsa_bin2sig()
  is consolidated with sig to DER conversion as ecdsa_bin2der() and
  moved to xkey_helper.c

  In the past when we used OpenSSL hooks installed by pkcs11-helper,
  such a conversion was not required as it was internally handled by
  the library.

Reported by: Tom 
Also see: https://bugzilla.redhat.com/show_bug.cgi?id=2177834
Tested-by: Florian Apolloner 

Change-Id: Ie20cf81edd643ab8ef3c41321353d11fd66c188c
Signed-off-by: Selva Nair 
---
Same as v1 with Tested by: and link to redhat bugzilla ticket added

 src/openvpn/cryptoapi.c  | 61 +---
 src/openvpn/pkcs11_openssl.c | 23 --
 src/openvpn/xkey_common.h| 15 +
 src/openvpn/xkey_helper.c| 45 ++
 4 files changed, 88 insertions(+), 56 deletions(-)

diff --git a/src/openvpn/cryptoapi.c b/src/openvpn/cryptoapi.c
index 136c6ffc..022f53d4 100644
--- a/src/openvpn/cryptoapi.c
+++ b/src/openvpn/cryptoapi.c
@@ -146,40 +146,6 @@ CAPI_DATA_free(CAPI_DATA *cd)
 free(cd);
 }
 
-/**
- * Helper to convert ECDSA signature returned by NCryptSignHash
- * to an ECDSA_SIG structure.
- * On entry 'buf[]' of length len contains r and s concatenated.
- * Returns a newly allocated ECDSA_SIG or NULL (on error).
- */
-static ECDSA_SIG *
-ecdsa_bin2sig(unsigned char *buf, int len)
-{
-ECDSA_SIG *ecsig = NULL;
-DWORD rlen = len/2;
-BIGNUM *r = BN_bin2bn(buf, rlen, NULL);
-BIGNUM *s = BN_bin2bn(buf+rlen, rlen, NULL);
-if (!r || !s)
-{
-goto err;
-}
-ecsig = ECDSA_SIG_new(); /* in openssl 1.1 this does not allocate r, s */
-if (!ecsig)
-{
-goto err;
-}
-if (!ECDSA_SIG_set0(ecsig, r, s)) /* ecsig takes ownership of r and s */
-{
-ECDSA_SIG_free(ecsig);
-goto err;
-}
-return ecsig;
-err:
-BN_free(r); /* it is ok to free NULL BN */
-BN_free(s);
-return NULL;
-}
-
 /**
  * Parse a hex string with optional embedded spaces into
  * a byte array.
@@ -287,12 +253,11 @@ static int
 xkey_cng_ec_sign(CAPI_DATA *cd, unsigned char *sig, size_t *siglen, const 
unsigned char *tbs,
  size_t tbslen)
 {
-BYTE buf[1024]; /* large enough for EC keys upto 1024 bits */
-DWORD len = _countof(buf);
+DWORD len = *siglen;
 
 msg(D_LOW, "Signing using NCryptSignHash with EC key");
 
-DWORD status = NCryptSignHash(cd->crypt_prov, NULL, (BYTE *)tbs, tbslen, 
buf, len, &len, 0);
+DWORD status = NCryptSignHash(cd->crypt_prov, NULL, (BYTE *)tbs, tbslen, 
sig, len, &len, 0);
 
 if (status != ERROR_SUCCESS)
 {
@@ -301,26 +266,14 @@ xkey_cng_ec_sign(CAPI_DATA *cd, unsigned char *sig, 
size_t *siglen, const unsign
 return 0;
 }
 
-/* NCryptSignHash returns r|s -- convert to OpenSSL's ECDSA_SIG */
-ECDSA_SIG *ecsig = ecdsa_bin2sig(buf, len);
-if (!ecsig)
+/* NCryptSignHash returns r|s -- convert to DER encoded buffer expected by 
OpenSSL */
+int derlen = ecdsa_bin2der(sig, (int) len, *siglen);
+if (derlen <= 0)
 {
-msg(M_NONFATAL, "Error in cryptopicert: Failed to convert ECDSA 
signature");
 return 0;
 }
-
-/* convert internal signature structure 's' to DER encoded byte array in 
sig */
-if (i2d_ECDSA_SIG(ecsig, NULL) > EVP_PKEY_size(cd->pubkey))
-{
-ECDSA_SIG_free(ecsig);
-msg(M_NONFATAL, "Error in cryptoapicert: DER encoded ECDSA signature 
is too long");
-return 0;
-}
-
-*siglen = i2d_ECDSA_SIG(ecsig, &sig);
-ECDSA_SIG_free(ecsig);
-
-return (*siglen > 0);
+*siglen = derlen;
+return 1;
 }
 
 /** Sign hash in tbs using RSA key in cd and NCryptSignHash */
diff --git a/src/openvpn/pkcs11_openssl.c b/src/openvpn/pkcs11_openssl.c
index 5c1de44e..eee86e17 100644
--- a/src/openvpn/pkcs11_openssl.c
+++ b/src/openvpn/pkcs11_openssl.c
@@ -168,6 +168,7 @@ xkey_pkcs11h_sign(void *handle, unsigned char *sig,
 
 unsigned char buf[EVP_MAX_MD_SIZE];
 size_t buflen;
+size_t siglen_max = *siglen;
 
 unsigned char enc[EVP_MAX_MD_SIZE + 32]; /* 32 bytes enough for DigestInfo 
header */
 size_t enc_len = sizeof(enc);
@@ -234,8 +235,26 @@ xkey_pkcs11h_sign(void *handle, unsigned char *sig,
 ASSERT(0);  /* coding error -- we couldnt have created any such key */
 }
 
-return CKR_OK == pkcs11h_certificate_signAny_ex(cert, &mech,
-tbs, tbslen, sig, siglen);
+if (CKR_OK != pkcs11h_certificate_signAny_ex(cert, &mech,
+ tbs, tbslen, sig, siglen))
+{
+return 0;
+}

[Openvpn-devel] [PATCH] Bugfix: Convert ECDSA signature form pkcs11-helper to DER encoded form

[selva . nair]

From: Selva Nair 

- With OpenSSL 3.0 and xkey-provider, we use pkcs11h_certificate_signAny_ex()
  which returns EC signature as raw r|s concatenated. But OpenSSL expects
  a DER encoded ASN.1 structure.

  Do this conversion as done in cryptoapi.c. For code re-use, ecdsa_bin2sig()
  is consolidated with sig to DER conversion as ecdsa_bin2der() and
  moved to xkey_helper.c

  In the past when we used OpenSSL hooks installed by pkcs11-helper,
  such a conversion was not required as it was internally handled by
  the library.

Reported by: Tom 

Signed-off-by: Selva Nair 
---
 src/openvpn/cryptoapi.c  | 61 +---
 src/openvpn/pkcs11_openssl.c | 23 --
 src/openvpn/xkey_common.h| 15 +
 src/openvpn/xkey_helper.c| 45 ++
 4 files changed, 88 insertions(+), 56 deletions(-)

diff --git a/src/openvpn/cryptoapi.c b/src/openvpn/cryptoapi.c
index 136c6ffc..022f53d4 100644
--- a/src/openvpn/cryptoapi.c
+++ b/src/openvpn/cryptoapi.c
@@ -146,40 +146,6 @@ CAPI_DATA_free(CAPI_DATA *cd)
 free(cd);
 }
 
-/**
- * Helper to convert ECDSA signature returned by NCryptSignHash
- * to an ECDSA_SIG structure.
- * On entry 'buf[]' of length len contains r and s concatenated.
- * Returns a newly allocated ECDSA_SIG or NULL (on error).
- */
-static ECDSA_SIG *
-ecdsa_bin2sig(unsigned char *buf, int len)
-{
-ECDSA_SIG *ecsig = NULL;
-DWORD rlen = len/2;
-BIGNUM *r = BN_bin2bn(buf, rlen, NULL);
-BIGNUM *s = BN_bin2bn(buf+rlen, rlen, NULL);
-if (!r || !s)
-{
-goto err;
-}
-ecsig = ECDSA_SIG_new(); /* in openssl 1.1 this does not allocate r, s */
-if (!ecsig)
-{
-goto err;
-}
-if (!ECDSA_SIG_set0(ecsig, r, s)) /* ecsig takes ownership of r and s */
-{
-ECDSA_SIG_free(ecsig);
-goto err;
-}
-return ecsig;
-err:
-BN_free(r); /* it is ok to free NULL BN */
-BN_free(s);
-return NULL;
-}
-
 /**
  * Parse a hex string with optional embedded spaces into
  * a byte array.
@@ -287,12 +253,11 @@ static int
 xkey_cng_ec_sign(CAPI_DATA *cd, unsigned char *sig, size_t *siglen, const 
unsigned char *tbs,
  size_t tbslen)
 {
-BYTE buf[1024]; /* large enough for EC keys upto 1024 bits */
-DWORD len = _countof(buf);
+DWORD len = *siglen;
 
 msg(D_LOW, "Signing using NCryptSignHash with EC key");
 
-DWORD status = NCryptSignHash(cd->crypt_prov, NULL, (BYTE *)tbs, tbslen, 
buf, len, &len, 0);
+DWORD status = NCryptSignHash(cd->crypt_prov, NULL, (BYTE *)tbs, tbslen, 
sig, len, &len, 0);
 
 if (status != ERROR_SUCCESS)
 {
@@ -301,26 +266,14 @@ xkey_cng_ec_sign(CAPI_DATA *cd, unsigned char *sig, 
size_t *siglen, const unsign
 return 0;
 }
 
-/* NCryptSignHash returns r|s -- convert to OpenSSL's ECDSA_SIG */
-ECDSA_SIG *ecsig = ecdsa_bin2sig(buf, len);
-if (!ecsig)
+/* NCryptSignHash returns r|s -- convert to DER encoded buffer expected by 
OpenSSL */
+int derlen = ecdsa_bin2der(sig, (int) len, *siglen);
+if (derlen <= 0)
 {
-msg(M_NONFATAL, "Error in cryptopicert: Failed to convert ECDSA 
signature");
 return 0;
 }
-
-/* convert internal signature structure 's' to DER encoded byte array in 
sig */
-if (i2d_ECDSA_SIG(ecsig, NULL) > EVP_PKEY_size(cd->pubkey))
-{
-ECDSA_SIG_free(ecsig);
-msg(M_NONFATAL, "Error in cryptoapicert: DER encoded ECDSA signature 
is too long");
-return 0;
-}
-
-*siglen = i2d_ECDSA_SIG(ecsig, &sig);
-ECDSA_SIG_free(ecsig);
-
-return (*siglen > 0);
+*siglen = derlen;
+return 1;
 }
 
 /** Sign hash in tbs using RSA key in cd and NCryptSignHash */
diff --git a/src/openvpn/pkcs11_openssl.c b/src/openvpn/pkcs11_openssl.c
index 5c1de44e..eee86e17 100644
--- a/src/openvpn/pkcs11_openssl.c
+++ b/src/openvpn/pkcs11_openssl.c
@@ -168,6 +168,7 @@ xkey_pkcs11h_sign(void *handle, unsigned char *sig,
 
 unsigned char buf[EVP_MAX_MD_SIZE];
 size_t buflen;
+size_t siglen_max = *siglen;
 
 unsigned char enc[EVP_MAX_MD_SIZE + 32]; /* 32 bytes enough for DigestInfo 
header */
 size_t enc_len = sizeof(enc);
@@ -234,8 +235,26 @@ xkey_pkcs11h_sign(void *handle, unsigned char *sig,
 ASSERT(0);  /* coding error -- we couldnt have created any such key */
 }
 
-return CKR_OK == pkcs11h_certificate_signAny_ex(cert, &mech,
-tbs, tbslen, sig, siglen);
+if (CKR_OK != pkcs11h_certificate_signAny_ex(cert, &mech,
+ tbs, tbslen, sig, siglen))
+{
+return 0;
+}
+if (strcmp(sigalg.keytype, "EC"))
+{
+return 1;
+}
+
+/* For EC keys, pkcs11 returns signature as r|s: convert to der encoded */
+int derlen = ecdsa_bin2der(sig, (in

Re: [Openvpn-devel] [PATCH] tests/unit_tests: Fix 'make distcheck' with subdir-objects enabled

[Selva Nair]

Hi,

On Wed, Mar 8, 2023 at 10:08 AM Frank Lichtenheld 
wrote:

> Commit 7f72abcf8a56bb35a510a3409e03a4e2aaba50da enabled subdir-objects
> when using automake 1.16+.
>
> There is an issue with the handling of .deps directories with this option.
> While automake 1.16 fixed subdir-objects to work at all when _SOURCES
> contains "unexpanded references" and it did fix subdir-objects to work
> with out-of-tree build for "source files specified with an explicit
> '$(srcdir)'" those fixes are not transitive. "unexpanded references"
> still break out-of-tree builds when enforcing a read-only source dir
> like 'make distcheck' does. When using *explicit* references to
> srcdir and top_srcdir it works correctly.
>

Thanks for quickly finding a fix for this. Even after re-reading the related
changelog in automake a dozen times, I cannot figure arbitrary variable
expansion won't work as expected for out-of-tree builds with read-only
sources. But testing does show only $(srcdir) and $(top_srcdir) get
correctly handled, and the fix below appears to be a reasonable way out.

Tested "make distcheck" and also compared with a locally generated
search and replace version. LGTM.

Acked-by: Selva Nair 

We waited long enough to enable subdir-objects though automake
has been warning about this for almost a decade. Now they have softened
their stand on it from "this behaviour will change ..." and
"unconditionally cause ..." etc.. to "may change" in future. And, the
implementation is still lacking. In retrospect, we (I) could have waited
longer.


Selva


> Signed-off-by: Frank Lichtenheld 
> ---
>  tests/unit_tests/openvpn/Makefile.am  | 220 +-
>  tests/unit_tests/plugins/auth-pam/Makefile.am |   6 +-
>  2 files changed, 110 insertions(+), 116 deletions(-)
>
> diff --git a/tests/unit_tests/openvpn/Makefile.am
> b/tests/unit_tests/openvpn/Makefile.am
> index ee0a3d8a..339c7ef3 100644
> --- a/tests/unit_tests/openvpn/Makefile.am
> +++ b/tests/unit_tests/openvpn/Makefile.am
> @@ -30,189 +30,185 @@ if HAVE_SITNL
>  check_PROGRAMS += networking_testdriver
>  endif
>
> -openvpn_includedir = $(top_srcdir)/include
> -openvpn_srcdir = $(top_srcdir)/src/openvpn
> -compat_srcdir = $(top_srcdir)/src/compat
> -
> -argv_testdriver_CFLAGS  = @TEST_CFLAGS@ -I$(openvpn_srcdir)
> -I$(compat_srcdir)
> -argv_testdriver_LDFLAGS = @TEST_LDFLAGS@ -L$(openvpn_srcdir)
> -Wl,--wrap=parse_line
> +argv_testdriver_CFLAGS  = @TEST_CFLAGS@ -I$(top_srcdir)/src/openvpn
> -I$(top_srcdir)/src/compat
> +argv_testdriver_LDFLAGS = @TEST_LDFLAGS@ -L$(top_srcdir)/src/openvpn
> -Wl,--wrap=parse_line
>  argv_testdriver_SOURCES = test_argv.c mock_msg.c mock_msg.h \
> mock_get_random.c \
> -   $(openvpn_srcdir)/platform.c \
> -   $(openvpn_srcdir)/buffer.c \
> -   $(openvpn_srcdir)/win32-util.c \
> -   $(openvpn_srcdir)/argv.c
> +   $(top_srcdir)/src/openvpn/platform.c \
> +   $(top_srcdir)/src/openvpn/buffer.c \
> +   $(top_srcdir)/src/openvpn/win32-util.c \
> +   $(top_srcdir)/src/openvpn/argv.c
>
> -buffer_testdriver_CFLAGS  = @TEST_CFLAGS@ -I$(openvpn_srcdir)
> -I$(compat_srcdir)
> -buffer_testdriver_LDFLAGS = @TEST_LDFLAGS@ -L$(openvpn_srcdir)
> -Wl,--wrap=parse_line
> +buffer_testdriver_CFLAGS  = @TEST_CFLAGS@ -I$(top_srcdir)/src/openvpn
> -I$(top_srcdir)/src/compat
> +buffer_testdriver_LDFLAGS = @TEST_LDFLAGS@ -L$(top_srcdir)/src/openvpn
> -Wl,--wrap=parse_line
>  buffer_testdriver_SOURCES = test_buffer.c mock_msg.c mock_msg.h \
> mock_get_random.c \
> -   $(openvpn_srcdir)/win32-util.c \
> -   $(openvpn_srcdir)/platform.c
> +   $(top_srcdir)/src/openvpn/win32-util.c \
> +   $(top_srcdir)/src/openvpn/platform.c
>
>  crypto_testdriver_CFLAGS  = @TEST_CFLAGS@ \
> -   -I$(openvpn_includedir) -I$(compat_srcdir) -I$(openvpn_srcdir)
> +   -I$(top_srcdir)/include -I$(top_srcdir)/src/compat
> -I$(top_srcdir)/src/openvpn
>  crypto_testdriver_LDFLAGS = @TEST_LDFLAGS@
>  crypto_testdriver_SOURCES = test_crypto.c mock_msg.c mock_msg.h \
> -   $(openvpn_srcdir)/buffer.c \
> -   $(openvpn_srcdir)/crypto.c \
> -   $(openvpn_srcdir)/crypto_mbedtls.c \
> -   $(openvpn_srcdir)/crypto_openssl.c \
> -   $(openvpn_srcdir)/otime.c \
> -   $(openvpn_srcdir)/packet_id.c \
> -   $(openvpn_srcdir)/platform.c \
> -   $(openvpn_srcdir)/mtu.c \
> -   $(openvpn_srcdir)/win32-util.c \
> -   $(openvpn_srcdir)/mss.c
> +   $(top_srcdir)/src/openvpn/buffer.c \
> +   $(top_srcdir)/src/openvpn/crypto.c \
> +   $(top_srcdir)/src/openvpn/crypto_mbedtls.c \
> +   $(top_srcdir)/src/openvpn/crypto_openssl.

Re: [Openvpn-devel] [PATCH 2/2] Include supplementary groups when checking management-client-group

[Selva Nair]

Hi

FTR, I just noticed that the patch is missing an endgrent() call:

On Mon, Mar 6, 2023 at 12:33 AM  wrote:

>
> +struct group *gr = getgrent();
> +char **members = NULL;
> +while (gr)
> +{
> +if (gr->gr_gid == gid)
> +{
> +/* found the group -- check user is a member */
> +members = gr->gr_mem;
> +for (char *s = *members; s && !ret; s++)
> +{
> +ret = !strcmp(s, pw->pw_name);
> +}
> +break; /* out of the while loop */
> +}
> +gr = getgrent();
> +}
>

  endgrent();


> +#endif /* if defined(HAVE_GETPWUID) && defined(HAVE_GETGRENT) */
> +return ret;
> +}
> +
>

Will delay v2 depending on the fate of this patch.

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

Re: [Openvpn-devel] [PATCH 2/2] Include supplementary groups when checking management-client-group

[Selva Nair]

Hi,

On Mon, Mar 6, 2023 at 3:24 AM Gert Doering  wrote:

> Hi,
>
> On Mon, Mar 06, 2023 at 12:33:46AM -0500, selva.n...@gmail.com wrote:
> > From: Selva Nair 
> >
> > - When management-client-group is in use, allow access if any of
> >   the supplementary groups of the user matches the specified group.
> >
> >   Currently only the effective gid of the peer socket is checked
> >   which is normally the primary group of user. As unprivileged users
> >   have no easy way of changing the effective gid of a process,
> >   group based access control is of very limited use without this change.
>
> I'm not really convinced this is a good way forward - it's yet another
> extra function for a very niche use case, as in "the code has been like
> this for 100 years, and nobody has ever used it".
>
> What you do is also not exactly "the user connecting has this gid at the
> time of connection" but "the uid reported by getpeerid() has this group
> listed in /etc/groups", which will be the same in many cases, but is
> still checking something different.
>

I was of the same opinion when I first saw issue #264, but I think the
original
intent of "--management-client-group has to be what I've implemented or
something similar.

Docs refer to it as group (not effective gid of the socket), and
restricting based on
gid reported by getpeerid is kind of useless. Why use "group" if it can't
be used
to restrict access to a set of users using a supplementary group...

I would propose to extend the documentation with "this only checks the
> primary gid at time of connection, if this does not work for your use
> case, put the socket into a directory with the right gid and mode 750
> and do not use that openvpn option" - as you wrote in the github ticket.
>

It's impossible to save that option by documenting. In that case, let's
just
make it a no-op and document that this option has no effect. And suggest
the above for how to achieve group-based access control..


> > - Also accept if uid = 0 irrespective of the group.
>
> This part is fine for me, as it follows the traditional unix way of
> "root may pass".
>

I'm not keen on touching some code and ignore that a closely related
code stays crippled (IMO). Unless we can make --management-client-group
a no-op as mentioned above.

As you say this is an obscure option -- we can leave it "as is".

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

[Openvpn-devel] [PATCH 2/2] Include supplementary groups when checking management-client-group

[selva . nair]

From: Selva Nair 

- When management-client-group is in use, allow access if any of
  the supplementary groups of the user matches the specified group.

  Currently only the effective gid of the peer socket is checked
  which is normally the primary group of user. As unprivileged users
  have no easy way of changing the effective gid of a process,
  group based access control is of very limited use without this change.

- Also accept if uid = 0 irrespective of the group.

Github: OpenVPN/openvpn#264

Signed-off-by: Selva Nair 
---
 configure.ac |  2 +-
 src/openvpn/manage.c | 43 ++-
 2 files changed, 43 insertions(+), 2 deletions(-)

diff --git a/configure.ac b/configure.ac
index 67f680b2..979903a8 100644
--- a/configure.ac
+++ b/configure.ac
@@ -659,7 +659,7 @@ AC_CHECK_FUNCS([ \
daemon chroot getpwnam setuid nice system dup dup2 \
syslog openlog mlockall getrlimit getgrnam setgid \
setgroups flock readv writev time gettimeofday \
-   setsid chdir \
+   setsid chdir getpwuid getgrent \
chsize ftruncate execve getpeereid basename dirname access \
epoll_create strsep \
 ])
diff --git a/src/openvpn/manage.c b/src/openvpn/manage.c
index db88e347..73dc7d57 100644
--- a/src/openvpn/manage.c
+++ b/src/openvpn/manage.c
@@ -1771,6 +1771,42 @@ man_new_connection_post(struct management *man, const 
char *description)
 }
 
 #if UNIX_SOCK_SUPPORT
+/**
+ * Return true if user uid is a member of group gid.
+ * On error or lack of platform support, we return false.
+ */
+
+static bool
+is_user_in_group(uid_t uid, gid_t gid)
+{
+bool ret = false;
+#if defined(HAVE_GETPWUID) && defined(HAVE_GETGRENT)
+struct passwd *pw = getpwuid(uid);
+if (!pw)
+{
+return ret;
+}
+
+struct group *gr = getgrent();
+char **members = NULL;
+while (gr)
+{
+if (gr->gr_gid == gid)
+{
+/* found the group -- check user is a member */
+members = gr->gr_mem;
+for (char *s = *members; s && !ret; s++)
+{
+ret = !strcmp(s, pw->pw_name);
+}
+break; /* out of the while loop */
+}
+gr = getgrent();
+}
+#endif /* if defined(HAVE_GETPWUID) && defined(HAVE_GETGRENT) */
+return ret;
+}
+
 static bool
 man_verify_unix_peer_uid_gid(struct management *man, const socket_descriptor_t 
sd)
 {
@@ -1780,13 +1816,18 @@ man_verify_unix_peer_uid_gid(struct management *man, 
const socket_descriptor_t s
 int uid, gid;
 if (unix_socket_get_peer_uid_gid(man->connection.sd_cli, &uid, &gid))
 {
+if (uid == 0) /* accept if root */
+{
+return true;
+}
 if (man->settings.client_uid != -1 && man->settings.client_uid != 
uid)
 {
 msg(D_MANAGEMENT, "%s UID of socket peer (%d) doesn't match 
required value (%d) as given by --management-client-user",
 err_prefix, uid, man->settings.client_uid);
 return false;
 }
-if (man->settings.client_gid != -1 && man->settings.client_gid != 
gid)
+if (man->settings.client_gid != -1 && man->settings.client_gid != 
gid
+&& !is_user_in_group(uid, man->settings.client_gid))
 {
 msg(D_MANAGEMENT, "%s GID of socket peer (%d) doesn't match 
required value (%d) as given by --management-client-group",
 err_prefix, gid, man->settings.client_gid);
-- 
2.34.1



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

[Openvpn-devel] [PATCH 1/2] Do not save pointer to 'struct passwd' returned by getpwnam etc.

[selva . nair]

From: Selva Nair 

- This pointer is to a static area which can change on further
  calls to getpwnam, getpwuid etc.
  Same with struct group returned by getgrnam.

  As the only field later referred to is uid or gid, fix
  by saving them instead.

Signed-off-by: Selva Nair 
---
Though we call getpwnam()/getgrnam() more than once with potentially
different user/group names, this has not yet led to a bug because
only in one place the state is persisted, and that call happens later.
But looks like a bug waiting to happen.

 src/openvpn/platform.c | 36 +++-
 src/openvpn/platform.h | 14 --
 src/openvpn/tun.c  |  4 ++--
 3 files changed, 29 insertions(+), 25 deletions(-)

diff --git a/src/openvpn/platform.c b/src/openvpn/platform.c
index 580c4cb8..f6b856f3 100644
--- a/src/openvpn/platform.c
+++ b/src/openvpn/platform.c
@@ -85,11 +85,16 @@ platform_user_get(const char *username, struct 
platform_state_user *state)
 if (username)
 {
 #if defined(HAVE_GETPWNAM) && defined(HAVE_SETUID)
-state->pw = getpwnam(username);
-if (!state->pw)
+state->uid = -1;
+const struct passwd *pw = getpwnam(username);
+if (!pw)
 {
 msg(M_ERR, "failed to find UID for user %s", username);
 }
+else
+{
+state->uid = pw->pw_uid;
+}
 state->username = username;
 ret = true;
 #else  /* if defined(HAVE_GETPWNAM) && defined(HAVE_SETUID) */
@@ -103,9 +108,9 @@ static void
 platform_user_set(const struct platform_state_user *state)
 {
 #if defined(HAVE_GETPWNAM) && defined(HAVE_SETUID)
-if (state->username && state->pw)
+if (state->username && state->uid >= 0)
 {
-if (setuid(state->pw->pw_uid))
+if (setuid(state->uid))
 {
 msg(M_ERR, "setuid('%s') failed", state->username);
 }
@@ -124,11 +129,16 @@ platform_group_get(const char *groupname, struct 
platform_state_group *state)
 if (groupname)
 {
 #if defined(HAVE_GETGRNAM) && defined(HAVE_SETGID)
-state->gr = getgrnam(groupname);
-if (!state->gr)
+state->gid = -1;
+const struct group *gr = getgrnam(groupname);
+if (!gr)
 {
 msg(M_ERR, "failed to find GID for group %s", groupname);
 }
+else
+{
+state->gid = gr->gr_gid;
+}
 state->groupname = groupname;
 ret = true;
 #else  /* if defined(HAVE_GETGRNAM) && defined(HAVE_SETGID) */
@@ -142,9 +152,9 @@ static void
 platform_group_set(const struct platform_state_group *state)
 {
 #if defined(HAVE_GETGRNAM) && defined(HAVE_SETGID)
-if (state->groupname && state->gr)
+if (state->groupname && state->gid >= 0)
 {
-if (setgid(state->gr->gr_gid))
+if (setgid(state->gid))
 {
 msg(M_ERR, "setgid('%s') failed", state->groupname);
 }
@@ -152,7 +162,7 @@ platform_group_set(const struct platform_state_group *state)
 #ifdef HAVE_SETGROUPS
 {
 gid_t gr_list[1];
-gr_list[0] = state->gr->gr_gid;
+gr_list[0] = state->gid;
 if (setgroups(1, gr_list))
 {
 msg(M_ERR, "setgroups('%s') failed", state->groupname);
@@ -225,13 +235,13 @@ platform_user_group_set(const struct platform_state_user 
*user_state,
  * new_uid/new_gid defaults to -1, which will not make
  * libcap-ng change the UID/GID unless configured
  */
-if (group_state->groupname && group_state->gr)
+if (group_state->groupname && group_state->gid >= 0)
 {
-new_gid = group_state->gr->gr_gid;
+new_gid = group_state->gid;
 }
-if (user_state->username && user_state->pw)
+if (user_state->username && user_state->uid >= 0)
 {
-new_uid = user_state->pw->pw_uid;
+new_uid = user_state->uid;
 }
 
 /* Prepare capabilities before dropping UID/GID */
diff --git a/src/openvpn/platform.h b/src/openvpn/platform.h
index a35a571c..d8dad74b 100644
--- a/src/openvpn/platform.h
+++ b/src/openvpn/platform.h
@@ -63,7 +63,7 @@ struct context;
 struct platform_state_user {
 #if defined(HAVE_GETPWNAM) && defined(HAVE_SETUID)
 const char *username;
-struct passwd *pw;
+uid_t uid;
 #else
 int dummy;
 #endif
@@ -74,7 +74,7 @@ struct platform_state_user {
 struct platform_state_group {
 #if defined(HAVE_GETGRNAM) && defined(HAVE_SETGID)
 const char *groupname;
-struct group *gr;
+gid_t gid;
 #else
 int dummy;
 #endif
@@ -97,10 +97,7 @@ static inline int
 platform_state_user_uid(const stru

Re: [Openvpn-devel] [PATCH applied] Re: Add a unit test for functions in cryptoapi.c

[Selva Nair]

Hi

On Sat, Feb 25, 2023 at 11:29 AM Gert Doering  wrote:

> Acked-by: Gert Doering 
>
> Thanks for the v4.  This enabled me to just push to GH to have
> to build and run the tests, without having to bother myself with
> copying binaries around :-)
>
> OTOH, there might be a bit of polishing needed - the other tests
> print out what they are doing ("Running 7 test(s)"), while the
> cryptoapi unit test "just succeeds".  Is there something missing
> wrt cmocka initalization?


>   https://github.com/cron2/openvpn/actions/runs/4270790193/jobs/7434873211


That one is for OpenSSL 1.1.1 while this test is built only for OpenSSL3,
so nothing to run. Do we need to continue testing Windows builds of 2.6 for
OpenSSL 1.1?

Logs for OpenSSL 3 build look okay:

>
https://github.com/OpenVPN/openvpn/actions/runs/4270860888/jobs/7434989645#logs

>
Run ./unittests/cryptoapi_testdriver.exe

> 4[==] Running 1 test(s).

> 5[ RUN ] test_parse_hexstring

> 6[ OK ] test_parse_hexstring

> 7[ PASSED ] 1 test(s).

> 8[==] 1 test(s) run.

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

[Openvpn-devel] [PATCH v4 3/3] Add a unit test for functions in cryptoapi.c

[selva . nair]

From: Selva Nair 

- Though named cryptoapi_testdriver, right now this only tests
  parsing of thumbprint specified as a selector for --cryptioapicert
  option. More tests coming..

v2: a line that belongs here was mistakenly included in the previous
commit. Corrected.
v3: add to list of tests run in github actions
v4: - correct comment above invalid strings (copy paste error)
- make invalid strings differ from correct value only in the
  explicitly introduced invalid characters/separators (one had
  two distinct errors which is not a robust test).

Signed-off-by: Selva Nair 
---
Sorry for not noticing these earlier...

 .github/workflows/build.yaml  |   3 +
 tests/unit_tests/openvpn/Makefile.am  |  16 +++
 tests/unit_tests/openvpn/test_cryptoapi.c | 126 ++
 3 files changed, 145 insertions(+)
 create mode 100644 tests/unit_tests/openvpn/test_cryptoapi.c

diff --git a/.github/workflows/build.yaml b/.github/workflows/build.yaml
index 699964fd..5bd6da89 100644
--- a/.github/workflows/build.yaml
+++ b/.github/workflows/build.yaml
@@ -243,6 +243,9 @@ jobs:
   - name: Run bufferunit test
 run: ./unittests/buffer_testdriver.exe
 
+  - name: Run cryptoapi unit test
+run: ./unittests/cryptoapi_testdriver.exe
+
   - name: Run cryptounit test
 run: ./unittests/crypto_testdriver.exe
 
diff --git a/tests/unit_tests/openvpn/Makefile.am 
b/tests/unit_tests/openvpn/Makefile.am
index 8d2386e0..ee0a3d8a 100644
--- a/tests/unit_tests/openvpn/Makefile.am
+++ b/tests/unit_tests/openvpn/Makefile.am
@@ -17,6 +17,7 @@ endif
 test_binaries += provider_testdriver
 
 if WIN32
+test_binaries += cryptoapi_testdriver
 LDADD = -lws2_32
 endif
 
@@ -152,6 +153,21 @@ provider_testdriver_SOURCES = test_provider.c mock_msg.c \
$(openvpn_srcdir)/win32-util.c \
$(openvpn_srcdir)/platform.c
 
+if WIN32
+cryptoapi_testdriver_CFLAGS  = @TEST_CFLAGS@ \
+   -I$(openvpn_includedir) -I$(compat_srcdir) -I$(openvpn_srcdir) \
+   $(OPTIONAL_CRYPTO_CFLAGS)
+cryptoapi_testdriver_LDFLAGS = @TEST_LDFLAGS@ \
+   $(OPTIONAL_CRYPTO_LIBS) -lcrypt32 -lncrypt
+cryptoapi_testdriver_SOURCES = test_cryptoapi.c mock_msg.c \
+   $(openvpn_srcdir)/xkey_helper.c \
+   $(openvpn_srcdir)/buffer.c \
+   $(openvpn_srcdir)/base64.c \
+   $(openvpn_srcdir)/platform.c \
+   mock_get_random.c \
+   $(openvpn_srcdir)/win32-util.c
+endif
+
 auth_token_testdriver_CFLAGS  = @TEST_CFLAGS@ \
-I$(openvpn_includedir) -I$(compat_srcdir) -I$(openvpn_srcdir) \
$(OPTIONAL_CRYPTO_CFLAGS)
diff --git a/tests/unit_tests/openvpn/test_cryptoapi.c 
b/tests/unit_tests/openvpn/test_cryptoapi.c
new file mode 100644
index ..73ef34e9
--- /dev/null
+++ b/tests/unit_tests/openvpn/test_cryptoapi.c
@@ -0,0 +1,126 @@
+/*
+ *  OpenVPN -- An application to securely tunnel IP networks
+ * over a single UDP port, with support for SSL/TLS-based
+ * session authentication and key exchange,
+ * packet encryption, packet authentication, and
+ * packet compression.
+ *
+ *  Copyright (C) 2023 Selva Nair 
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by the
+ *  Free Software Foundation, either version 2 of the License,
+ *  or (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  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.
+ */
+
+#ifdef HAVE_CONFIG_H
+#include "config.h"
+#elif defined(_MSC_VER)
+#include "config-msvc.h"
+#endif
+
+#include "syshead.h"
+#include "manage.h"
+#include "integer.h"
+#include "xkey_common.h"
+
+#if defined(HAVE_XKEY_PROVIDER) && defined (ENABLE_CRYPTOAPI)
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+#include  /* pull-in the whole file to test static functions */
+
+struct management *management; /* global */
+
+/* mock a management function that xkey_provider needs */
+char *
+management_query_pk_sig(struct management *man, const char *b64_data,
+const char *algorithm)
+{
+(void) man;
+(void) b64_data;
+(void) algorithm;
+return NULL;
+}
+
+/* tls_libctx is defined in ssl_openssl.c which we do not want to compile in */
+OSSL_LIB_CTX *tls_libctx;
+
+#ifndef _countof
+#define _countof(x) sizeof((x))/sizeof(*(x))
+#endif
+
+/* test data */
+static const uint8_t test_hash[] = {
+0x77, 0x38, 0x65, 0x00, 0x1e, 0x96, 0x48, 0xc6, 0

[Openvpn-devel] [PATCH v3 3/3] Add a unit test for functions in cryptoapi.c

[selva . nair]

From: Selva Nair 

- Though named cryptoapi_testdriver, right now this only tests
  parsing of thumbprint specified as a selector for --cryptioapicert
  option. More tests coming..

v2: a line that belongs here was mistakenly included in the previous
commit. Corrected.
v3: add to list of tests run in github actions

Signed-off-by: Selva Nair 

fixup
---
 .github/workflows/build.yaml  |   3 +
 tests/unit_tests/openvpn/Makefile.am  |  16 +++
 tests/unit_tests/openvpn/test_cryptoapi.c | 126 ++
 3 files changed, 145 insertions(+)
 create mode 100644 tests/unit_tests/openvpn/test_cryptoapi.c

diff --git a/.github/workflows/build.yaml b/.github/workflows/build.yaml
index 699964fd..5bd6da89 100644
--- a/.github/workflows/build.yaml
+++ b/.github/workflows/build.yaml
@@ -243,6 +243,9 @@ jobs:
   - name: Run bufferunit test
 run: ./unittests/buffer_testdriver.exe
 
+  - name: Run cryptoapi unit test
+run: ./unittests/cryptoapi_testdriver.exe
+
   - name: Run cryptounit test
 run: ./unittests/crypto_testdriver.exe
 
diff --git a/tests/unit_tests/openvpn/Makefile.am 
b/tests/unit_tests/openvpn/Makefile.am
index 8d2386e0..ee0a3d8a 100644
--- a/tests/unit_tests/openvpn/Makefile.am
+++ b/tests/unit_tests/openvpn/Makefile.am
@@ -17,6 +17,7 @@ endif
 test_binaries += provider_testdriver
 
 if WIN32
+test_binaries += cryptoapi_testdriver
 LDADD = -lws2_32
 endif
 
@@ -152,6 +153,21 @@ provider_testdriver_SOURCES = test_provider.c mock_msg.c \
$(openvpn_srcdir)/win32-util.c \
$(openvpn_srcdir)/platform.c
 
+if WIN32
+cryptoapi_testdriver_CFLAGS  = @TEST_CFLAGS@ \
+   -I$(openvpn_includedir) -I$(compat_srcdir) -I$(openvpn_srcdir) \
+   $(OPTIONAL_CRYPTO_CFLAGS)
+cryptoapi_testdriver_LDFLAGS = @TEST_LDFLAGS@ \
+   $(OPTIONAL_CRYPTO_LIBS) -lcrypt32 -lncrypt
+cryptoapi_testdriver_SOURCES = test_cryptoapi.c mock_msg.c \
+   $(openvpn_srcdir)/xkey_helper.c \
+   $(openvpn_srcdir)/buffer.c \
+   $(openvpn_srcdir)/base64.c \
+   $(openvpn_srcdir)/platform.c \
+   mock_get_random.c \
+   $(openvpn_srcdir)/win32-util.c
+endif
+
 auth_token_testdriver_CFLAGS  = @TEST_CFLAGS@ \
-I$(openvpn_includedir) -I$(compat_srcdir) -I$(openvpn_srcdir) \
$(OPTIONAL_CRYPTO_CFLAGS)
diff --git a/tests/unit_tests/openvpn/test_cryptoapi.c 
b/tests/unit_tests/openvpn/test_cryptoapi.c
new file mode 100644
index ..2bea3f42
--- /dev/null
+++ b/tests/unit_tests/openvpn/test_cryptoapi.c
@@ -0,0 +1,126 @@
+/*
+ *  OpenVPN -- An application to securely tunnel IP networks
+ * over a single UDP port, with support for SSL/TLS-based
+ * session authentication and key exchange,
+ * packet encryption, packet authentication, and
+ * packet compression.
+ *
+ *  Copyright (C) 2023 Selva Nair 
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by the
+ *  Free Software Foundation, either version 2 of the License,
+ *  or (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  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.
+ */
+
+#ifdef HAVE_CONFIG_H
+#include "config.h"
+#elif defined(_MSC_VER)
+#include "config-msvc.h"
+#endif
+
+#include "syshead.h"
+#include "manage.h"
+#include "integer.h"
+#include "xkey_common.h"
+
+#if defined(HAVE_XKEY_PROVIDER) && defined (ENABLE_CRYPTOAPI)
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+#include  /* pull-in the whole file to test static functions */
+
+struct management *management; /* global */
+
+/* mock a management function that xkey_provider needs */
+char *
+management_query_pk_sig(struct management *man, const char *b64_data,
+const char *algorithm)
+{
+(void) man;
+(void) b64_data;
+(void) algorithm;
+return NULL;
+}
+
+/* tls_libctx is defined in ssl_openssl.c which we do not want to compile in */
+OSSL_LIB_CTX *tls_libctx;
+
+#ifndef _countof
+#define _countof(x) sizeof((x))/sizeof(*(x))
+#endif
+
+/* test data */
+static const uint8_t test_hash[] = {
+0x77, 0x38, 0x65, 0x00, 0x1e, 0x96, 0x48, 0xc6, 0x57, 0x0b, 0xae,
+0xc0, 0xb7, 0x96, 0xf9, 0x66, 0x4d, 0x5f, 0xd0, 0xb7
+};
+
+/* valid test strings to test with and without embedded and trailing spaces */
+static const char *valid_str[] = {
+"773865001e9648c6570baec0b796f

Re: [Openvpn-devel] [PATCH v3 2/3] Build unit tests in mingw Windows build

[Selva Nair]

Hi,

On Sat, Feb 11, 2023 at 1:32 PM Gert Doering  wrote:

>
>
> /bin/bash ../../../libtool  --tag=CC   --mode=link i686-w64-mingw32-gcc


This is a 32 bit build which I never tried. Maybe cmocka is not built for
32 bit?


> -I/home/gert/mingw/opt/include  -I/home/gert/mingw/opt/include
> -I../../../include -I/home/gert/mingw/opt/include -Wall
> -Wno-stringop-truncation -I/home/gert/mingw/opt/include -std=c99
> -L/home/gert/mingw/lib -L/home/gert/mingw/opt/lib64 -lssl -lcrypto
> -L/home/gert/mingw/opt/lib -llzo2 -L/home/gert/mingw/opt/lib -lcmocka
> -L/home/gert/mingw/opt/lib -o example_testdriver.exe
> example_testdriver-test.o
> libtool: link: i686-w64-mingw32-gcc -I/home/gert/mingw/opt/include
> -I/home/gert/mingw/opt/include -I../../../include
> -I/home/gert/mingw/opt/include -Wall -Wno-stringop-truncation
> -I/home/gert/mingw/opt/include -std=c99 -o .libs/example_testdriver.exe
> example_testdriver-test.o  -L/home/gert/mingw/lib
> -L/home/gert/mingw/opt/lib64 -lssl -lcrypto -L/home/gert/mingw/opt/lib
> /home/gert/mingw/opt/lib/liblzo2.a -lwinmm -lcmocka
> /usr/bin/i686-w64-mingw32-ld:
> example_testdriver-test.o:test.c:(.text+0x89): undefined reference to
> `_assert_int_equal'
> /usr/bin/i686-w64-mingw32-ld:
> example_testdriver-test.o:test.c:(.text+0xc6): undefined reference to
> `_assert_int_equal'
> /usr/bin/i686-w64-mingw32-ld:
> example_testdriver-test.o:test.c:(.text+0x153): undefined reference to
> `_cmocka_run_group_tests'
>

I had seen something similar with  the master branch -- the def had some
functions missing. But 1.1.5 worked out of the box.

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

Re: [Openvpn-devel] [PATCH v3 2/3] Build unit tests in mingw Windows build

[Selva Nair]

Hi

On Fri, Feb 10, 2023 at 4:13 PM Gert Doering  wrote:

> Hi,
>
> On Tue, Feb 07, 2023 at 07:59:25PM -0500, selva.n...@gmail.com wrote:
> > From: Selva Nair 
> >
> > - Minor changes to the build system to include some
> >   dependencies for Windows build
> >
> > - test_tls_crypt not built as it will pull in win32.c and
> >   its dependencies
> >
> > - If cross-compiling, "make check" will only build the tests but not
> >   run any. Copy to Windows and run manually. Executables are in
> >   /tests/unit_tests/openvpn/.libs/ and these depend on
> >   cmocka.dll in addition to openssl libs that some tests link to.
>
> I am something wrong, or I'm missing some bits.
>
> I took this patch, on top of commit e80720ef939 ("top of master as of
> now").  MinGW on Ubuntu.  "autoreconf -iv", make clean, then
> make && make check.
>
> make check claims
>
> make[4]: Entering directory '/home/gert/mingw/openvpn.git/tests'
> ==
> All 0 tests passed
> ==
>
> ... but it seems to have done... nothing?
>
> gert@ubuntu2204:~/mingw/openvpn.git$ ls -l tests/unit_test/openvpn/.libs
> ls: cannot access 'tests/unit_test/openvpn/.libs': No such file or
> directory
>
> ... given argv_testdriver exists, I should find "something" with argv*.o*
> here...
>
> gert@ubuntu2204:~/mingw/openvpn.git$ find . -name "*argv*o"
> ./tests/unit_tests/openvpn/.deps/argv_testdriver-mock_get_random.Po
> ./tests/unit_tests/openvpn/.deps/tls_crypt_testdriver-argv.Po
> ./tests/unit_tests/openvpn/.deps/argv_testdriver-test_argv.Po
> ./tests/unit_tests/openvpn/.deps/argv_testdriver-mock_msg.Po
> ./tests/unit_tests/openvpn/.deps/argv_testdriver-platform.Po
> ./tests/unit_tests/openvpn/.deps/argv_testdriver-buffer.Po
> ./tests/unit_tests/openvpn/.deps/pkt_testdriver-argv.Po
> ./tests/unit_tests/openvpn/.deps/argv_testdriver-argv.Po
> ./src/openvpn/.deps/tls_crypt_testdriver-argv.Po
> ./src/openvpn/.deps/argv.Po
> ./src/openvpn/.deps/argv_testdriver-win32-util.Po
> ./src/openvpn/.deps/argv_testdriver-platform.Po
> ./src/openvpn/.deps/argv_testdriver-buffer.Po
> ./src/openvpn/.deps/pkt_testdriver-argv.Po
> ./src/openvpn/.deps/argv_testdriver-argv.Po
> ./src/openvpn/argv.o
>
> ... so it seems as if it did the "make depend" dance for the unit_tests,
> but then didn't compile anything for real.
>
>
> Did I apply stuff in the wrong order?  Do I need to more forcefully clean
> my build tree?
>

Ignore the previous email  -- (I behaved as if I can't read more than a
pageful).

Looks like unit-tests are not enabled during configure. For that, cmocka
should be available in the search path or set using CMOCKA_LIBS and
CMOCKA_INCLUDES.

It took a while for me to figure out how to cross-compile cmocka using
cmake and mingw on Linux. Finally what worked was pretty simple:

git clone https://github.com/clibs/cmocka.git
cd cmocka
checkout cmocka-1.1.5
mkdir build
cd build
cmake -DCMAKE_SYSTEM_NAME=Windows -DCMAKE_C_COMPILER=x86_64-w64-mingw32-gcc
\
-DCMAKE_SHARED_LINKER_FLAGS=-static-libgcc \
-DCMAKE_INSTALL_PREFIX=$HOME/mingw-cmocka/ -S .. -B .
make install

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

Re: [Openvpn-devel] [PATCH v3 2/3] Build unit tests in mingw Windows build

[Selva Nair]

On Fri, Feb 10, 2023 at 4:13 PM Gert Doering  wrote:

> Hi,
>
> On Tue, Feb 07, 2023 at 07:59:25PM -0500, selva.n...@gmail.com wrote:
> > From: Selva Nair 
> >
> > - Minor changes to the build system to include some
> >   dependencies for Windows build
> >
> > - test_tls_crypt not built as it will pull in win32.c and
> >   its dependencies
> >
> > - If cross-compiling, "make check" will only build the tests but not
> >   run any. Copy to Windows and run manually. Executables are in
> >   /tests/unit_tests/openvpn/.libs/ and these depend on
> >   cmocka.dll in addition to openssl libs that some tests link to.
>
> I am something wrong, or I'm missing some bits.
>
> I took this patch, on top of commit e80720ef939 ("top of master as of
> now").  MinGW on Ubuntu.  "autoreconf -iv", make clean, then
> make && make check.
>
> make check claims
>
> make[4]: Entering directory '/home/gert/mingw/openvpn.git/tests'
> ==
> All 0 tests passed
> ==
>
> ... but it seems to have done... nothing?
>

My patch only enables to build the tests while cross-compiling but disables
running them. As the executables  are not native, you have to manually copy
them to Windows and run there.

With the next patch from Arne, GHA actions will copy it automagically to
Windows and run there. But that works only for github actions.

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

Re: [Openvpn-devel] [PATCH] Improve format specifier for socket handle in Windows

[Selva Nair]

Hi

On Fri, Feb 10, 2023 at 2:21 PM Antonio Quartulli  wrote:

> Hi,
>
> On 10/02/2023 14:31, Lev Stipakov wrote:
> > From: Lev Stipakov 
> >
> > Socket is a handle on Windows, which is usually logged in hex.
> > Also an interesting value is INVALID_SOCKET, which is ~0.
> >
> > PRIuPTR prints decimals, and for INVALID_SOCKET it prints something like
> >
> >2023-02-10 14:45:21 us=906000 write to TUN/TAP : Jrjestelmkutsulle
> > annettu data-alue on liian pieni.   (fd=18446744073709551615,code=122)
> >
> > PRIxPTR prints hex, and INVALID_SOCKET looks a bit nicer:
> >
> >2023-02-10 15:17:11 us=828000 write to TUN/TAP : Jrjestelmkutsulle
> > annettu data-alue on liian pieni.   (fd=,code=122)
> >
> > Reported-by: Selva Nair 
> > Signed-off-by: Lev Stipakov 
>
> I also discussed this with Lev and, despite this being different from
> what we do in the *nix world (where decimal representations make sense
> for file descriptors), it seems to be the right hting to do on Windows
> when using HANDLEs (shrug).
>

Windows kernel handles are very much "like" fds though the handle table
also covers processes, threads,  semaphores etc. They are not pointers, are
generally smallish numbers or ~0 (== -1). But not as small as fds in the
Unix/Linux world --- mainly because several objects share the name space
and they have to be multiples of 4 for some arcane reason.

The problem here was not printing as "decimal" but as "unsigned decimal".
Use of the latter would have caused the same issue on Linux too.

Using hex is a compromise here as handles are defined as "unsigned ints" of
the same size as pointers. So PRIdPTR somehow looks improper though I would
have chosen that.

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

Re: [Openvpn-devel] [PATCH 3/5] Do not run check_engine test when crosscompiling

[Selva Nair]

Hi,

On Tue, Feb 7, 2023 at 7:19 PM Arne Schwabe  wrote:

> Signed-off-by: Arne Schwabe 
> ---
>  tests/unit_tests/engine-key/Makefile.am | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/tests/unit_tests/engine-key/Makefile.am
> b/tests/unit_tests/engine-key/Makefile.am
> index 246222514..0c2888576 100644
> --- a/tests/unit_tests/engine-key/Makefile.am
> +++ b/tests/unit_tests/engine-key/Makefile.am
> @@ -12,7 +12,9 @@ TESTS_ENVIRONMENT = srcdir="$(abs_srcdir)"; \
> top_srcdir="$(top_srcdir)"; \
> export srcdir builddir top_builddir top_srcdir;
>
> +if !CROSS_COMPILING
>  TESTS = check_engine_keys.sh
> +endif
>  check_engine_keys.sh: $(conffiles)
>
>  CLEANFILES = \
>

FTR, this patch is no longer needed (included in #2 below). So, the order
of interdependent patches are: (All acked).

1. [Openvpn-devel,1/3] Conditionally add subdir-objects option to automake

2. [Openvpn-devel,v3,2/3] Build unit tests in mingw Windows build

3. [Openvpn-devel,4/5] Add missing stdint.h includes in unit tests files

4. [Openvpn-devel,v2,5/5] Add building unit tests with mingw to github
actions


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

[Openvpn-devel] Fwd: [PATCH v2 3/5] Windows: fix wrong printf format in x_check_status

[Selva Nair]

CC: list

-- Forwarded message -
From: Selva Nair 
Date: Tue, Feb 7, 2023 at 11:57 AM
Subject: Re: [Openvpn-devel] [PATCH v2 3/5] Windows: fix wrong printf
format in x_check_status
To: Frank Lichtenheld 


Nitpicking:


>  - use PRIuPTR as discussed on IRC (added relevant defines to
>commit message)
>

The most interesting value to log for SOCKET on Windows is  INVALID_SOCKET
= ~0 (i.e, -1). It will print with PRIuPTR as 18446744073709551615. PRIdPTR
which will give -1 or PRIxPTR () may be better in this case?
Though socket is a handle on Windows, I think it never gets too large to be
-ve except for this special value.

I'm fine with this too as unsigned decimals for -1 in both 32 bit and 64
bit are not hard to spot.

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

[Openvpn-devel] Fwd: [PATCH 4/5] Add missing stdint.h includes in unit tests files

[Selva Nair]

CC: list

-- Forwarded message -
From: Selva Nair 
Date: Wed, Feb 8, 2023 at 11:34 PM
Subject: Re: [Openvpn-devel] [PATCH 4/5] Add missing stdint.h includes in
unit tests files
To: Arne Schwabe 


Hi,

On Tue, Feb 7, 2023 at 7:19 PM Arne Schwabe  wrote:

> My mingw compiler/headers (mingw-w64 10.0.0 on macOS) seem to be more
> pendantic than the one that comes with Ubuntu 22.04 (github actions) or
> any of the other platforms including msvc/normal windows header.
>
> Signed-off-by: Arne Schwabe 
> ---
>  tests/unit_tests/example_test/test.c  | 1 +
>  tests/unit_tests/example_test/test2.c | 1 +
>  tests/unit_tests/openvpn/mock_msg.c   | 1 +
>  3 files changed, 3 insertions(+)
>
> diff --git a/tests/unit_tests/example_test/test.c
> b/tests/unit_tests/example_test/test.c
> index ea31b884d..c174025cc 100644
> --- a/tests/unit_tests/example_test/test.c
> +++ b/tests/unit_tests/example_test/test.c
> @@ -4,6 +4,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>
>  static int
> diff --git a/tests/unit_tests/example_test/test2.c
> b/tests/unit_tests/example_test/test2.c
> index 5a186d5d7..bb54633c8 100644
> --- a/tests/unit_tests/example_test/test2.c
> +++ b/tests/unit_tests/example_test/test2.c
> @@ -4,6 +4,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>
>
> diff --git a/tests/unit_tests/openvpn/mock_msg.c
> b/tests/unit_tests/openvpn/mock_msg.c
> index 3ede98c00..3fa9a166f 100644
> --- a/tests/unit_tests/openvpn/mock_msg.c
> +++ b/tests/unit_tests/openvpn/mock_msg.c
> @@ -32,6 +32,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 


Actually it may not be your setup: mingw on Linux also requires this if
using cmocka-master branch. cmocka-1.1.5 works without this include for me.
That would explain why we haven't seen this error yet in native builds.

Anyway, cmocka docs also tell us to always include .

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

[Openvpn-devel] Fwd: [PATCH v2 5/5] Add building unit tests with mingw to github actions

[Selva Nair]

CC: list was missed..

-- Forwarded message -
From: Selva Nair 
Date: Thu, Feb 9, 2023 at 2:54 PM
Subject: Re: [Openvpn-devel] [PATCH v2 5/5] Add building unit tests with
mingw to github actions
To: Arne Schwabe 


Hi,

Thanks, this is much better with tests grouped together.

Some nitpicks below that could be fixed while merging.

On Thu, Feb 9, 2023 at 11:38 AM Arne Schwabe  wrote:

> This runs each test in its own action since order of stderr and stdout
> is seemingly random in github action Windows output and this way at least
> tests outputs are groups gy test
>

"test outputs are grouped by"


>
> Patch v2: use -static-libgcc to avoid comping gcc runtime libraries.
>
> Signed-off-by: Arne Schwabe 
> ---
>  .github/workflows/build.yaml | 111 ++-
>  1 file changed, 109 insertions(+), 2 deletions(-)
>
> diff --git a/.github/workflows/build.yaml b/.github/workflows/build.yaml
> index 5888e91e5..a1026fddb 100644
> --- a/.github/workflows/build.yaml
> +++ b/.github/workflows/build.yaml
> @@ -55,9 +55,10 @@ jobs:
>PKCS11_HELPER_VERSION: "1.29.0"
>OPENSSL_VERSION: "${{ matrix.osslver }}"
>TAP_WINDOWS_VERSION: "9.23.3"
> +  CMOCKA_VERSION: "1.1.5"
>  steps:
>- name: Install dependencies
> -run: sudo apt update && sudo apt install -y mingw-w64 libtool
> automake autoconf man2html unzip
> +run: sudo apt update && sudo apt install -y mingw-w64 libtool
> automake autoconf man2html unzip cmake ninja-build build-essential wget
>- name: Checkout OpenVPN
>  uses: actions/checkout@v3
>  with:
> @@ -72,7 +73,7 @@ jobs:
>  uses: actions/cache@v3
>  with:
>path: '~/mingw/'
> -  key: ${{ matrix.target }}-mingw-${{ matrix.osslver }}-${{
> env.LZO_VERSION }}-${{ env.PKCS11_HELPER_VERSION }}-${{
> env.TAP_WINDOWS_VERSION }}
> +  key: ${{ matrix.target }}-mingw-${{ matrix.osslver }}-${{
> env.LZO_VERSION }}-${{ env.PKCS11_HELPER_VERSION }}-${{
> env.TAP_WINDOWS_VERSION }}--${{ env.CMOCKA_VERSION }}
>
># Repeating  if: steps.cache.outputs.cache-hit != 'true'
># on every step for building dependencies is ugly but
> @@ -84,12 +85,33 @@ jobs:
>wget -c -P download-cache/ "
> https://build.openvpn.net/downloads/releases/tap-windows-${TAP_WINDOWS_VERSION}.zip
> "
>wget -c -P download-cache/ "
> https://www.oberhumer.com/opensource/lzo/download/lzo-${LZO_VERSION}.tar.gz
> "
>wget -c -P download-cache/ "
> https://github.com/OpenSC/pkcs11-helper/releases/download/pkcs11-helper-${PKCS11_HELPER_VERSION}/pkcs11-helper-${PKCS11_HELPER_VERSION}.tar.bz2
> "
> +  wget -c -P download-cache/ "
> https://github.com/coreboot/cmocka/archive/refs/tags/cmocka-${CMOCKA_VERSION}.tar.gz
> "
>tar jxf
> "download-cache/pkcs11-helper-${PKCS11_HELPER_VERSION}.tar.bz2"
>wget -c -P download-cache/ "
> https://www.openssl.org/source/old/1.1.1/openssl-${OPENSSL_VERSION}.tar.gz";
> || wget -c -P download-cache/ "
> https://www.openssl.org/source/openssl-${OPENSSL_VERSION}.tar.gz";
>tar zxf "download-cache/openssl-${OPENSSL_VERSION}.tar.gz"
>tar zxf "download-cache/lzo-${LZO_VERSION}.tar.gz"
> +  tar zxf "download-cache/cmocka-${CMOCKA_VERSION}.tar.gz"
>unzip download-cache/tap-windows-${TAP_WINDOWS_VERSION}.zip
>
> +  - name: create cmocka build directory
> +if: steps.cache.outputs.cache-hit != 'true'
> +run: mkdir cmocka-build
> +
> +  - name: configure cmocka
> +if: steps.cache.outputs.cache-hit != 'true'
> +working-directory: "./cmocka-build"
> +run: cmake -GNinja -DCMAKE_C_COMPILER=${{ matrix.chost }}-gcc
> -DCMAKE_CXX_COMPILER=${{ matrix.chost }}-g++ -DCMAKE_SYSTEM_NAME=Windows
> -DCMAKE_SHARED_LINKER_FLAGS=-static-libgcc
> -DCMAKE_PREFIX_PATH=${HOME}/mingw/opt/lib/pkgconfig/
> -DCMAKE_INCLUDE_PATH=${HOME}/mingw/opt/lib/include
> -DCMAKE_LIBRARY_PATH=${HOME}/mingw/opt/lib
> -DCMAKE_INSTALL_PREFIX=${HOME}/mingw/opt/ ../cmocka-cmocka-${{
> env.CMOCKA_VERSION }}
> +
> +  - name: build cmocka
> +if: steps.cache.outputs.cache-hit != 'true'
> +working-directory: "./cmocka-build"
> +run: ninja
> +
> +  - name: install cmocka
> +if: steps.cache.outputs.cache-hit != 'true'
> +working-directory: "./cmocka-build"
> +run: ninja install
> +
&

Re: [Openvpn-devel] [PATCH 5/5] Add building and running mingw unittests to github actions

[Selva Nair]

Hi,

It's "absolutely splendid" to see cross-compiled tests running in GHA!

Some issues below:

On Tue, Feb 7, 2023 at 7:19 PM Arne Schwabe  wrote:

> Signed-off-by: Arne Schwabe 
> ---
>  .github/workflows/build.yaml | 110 ++-
>  1 file changed, 108 insertions(+), 2 deletions(-)
>
> diff --git a/.github/workflows/build.yaml b/.github/workflows/build.yaml
> index 132624547..c71bd2c4d 100644
> --- a/.github/workflows/build.yaml
> +++ b/.github/workflows/build.yaml
> @@ -55,9 +55,10 @@ jobs:
>PKCS11_HELPER_VERSION: "1.29.0"
>OPENSSL_VERSION: "${{ matrix.osslver }}"
>TAP_WINDOWS_VERSION: "9.23.3"
> +  CMOCKA_VERSION: "1.1.5"
>  steps:
>- name: Install dependencies
> -run: sudo apt update && sudo apt install -y mingw-w64 libtool
> automake autoconf man2html unzip
> +run: sudo apt update && sudo apt install -y mingw-w64 libtool
> automake autoconf man2html unzip cmake ninja-build build-essential wget
>- name: Checkout OpenVPN
>  uses: actions/checkout@v3
>  with:
> @@ -72,7 +73,7 @@ jobs:
>  uses: actions/cache@v3
>  with:
>path: '~/mingw/'
> -  key: ${{ matrix.target }}-mingw-${{ matrix.osslver }}-${{
> env.LZO_VERSION }}-${{ env.PKCS11_HELPER_VERSION }}-${{
> env.TAP_WINDOWS_VERSION }}
> +  key: ${{ matrix.target }}-mingw-${{ matrix.osslver }}-${{
> env.LZO_VERSION }}-${{ env.PKCS11_HELPER_VERSION }}-${{
> env.TAP_WINDOWS_VERSION }}--${{ env.CMOCKA_VERSION }}
>
># Repeating  if: steps.cache.outputs.cache-hit != 'true'
># on every step for building dependencies is ugly but
> @@ -84,12 +85,33 @@ jobs:
>wget -c -P download-cache/ "
> https://build.openvpn.net/downloads/releases/tap-windows-${TAP_WINDOWS_VERSION}.zip
> "
>wget -c -P download-cache/ "
> https://www.oberhumer.com/opensource/lzo/download/lzo-${LZO_VERSION}.tar.gz
> "
>wget -c -P download-cache/ "
> https://github.com/OpenSC/pkcs11-helper/releases/download/pkcs11-helper-${PKCS11_HELPER_VERSION}/pkcs11-helper-${PKCS11_HELPER_VERSION}.tar.bz2
> "
> +  wget -c -P download-cache/ "
> https://github.com/coreboot/cmocka/archive/refs/tags/cmocka-${CMOCKA_VERSION}.tar.gz
> "
>tar jxf
> "download-cache/pkcs11-helper-${PKCS11_HELPER_VERSION}.tar.bz2"
>wget -c -P download-cache/ "
> https://www.openssl.org/source/old/1.1.1/openssl-${OPENSSL_VERSION}.tar.gz";
> || wget -c -P download-cache/ "
> https://www.openssl.org/source/openssl-${OPENSSL_VERSION}.tar.gz";
>tar zxf "download-cache/openssl-${OPENSSL_VERSION}.tar.gz"
>tar zxf "download-cache/lzo-${LZO_VERSION}.tar.gz"
> +  tar zxf "download-cache/cmocka-${CMOCKA_VERSION}.tar.gz"
>unzip download-cache/tap-windows-${TAP_WINDOWS_VERSION}.zip
>
> +  - name: create cmocka build directory
> +if: steps.cache.outputs.cache-hit != 'true'
> +run: mkdir cmocka-build
> +
> +  - name: configure cmocka
> +if: steps.cache.outputs.cache-hit != 'true'
> +working-directory: "./cmocka-build"
> +run: cmake -GNinja -DCMAKE_C_COMPILER=${{ matrix.chost }}-gcc
> -DCMAKE_CXX_COMPILER=${{ matrix.chost }}-g++ -DCMAKE_SYSTEM_NAME=Windows
> -DCMAKE_PREFIX_PATH=${HOME}/mingw/opt/lib/pkgconfig/
> -DCMAKE_INCLUDE_PATH=${HOME}/mingw/opt/lib/include
> -DCMAKE_LIBRARY_PATH=${HOME}/mingw/opt/lib
> -DCMAKE_INSTALL_PREFIX=${HOME}/mingw/opt/ ../cmocka-cmocka-${{
> env.CMOCKA_VERSION }}
>

Please add "-DCMAKE_SHARED_LINKER_FLAGS=-static-libgcc" so that cmocka.dll
will only depend on native Windows dlls. We already do this while building
openssl dlls.


> +
> +  - name: build cmocka
> +if: steps.cache.outputs.cache-hit != 'true'
> +working-directory: "./cmocka-build"
> +run: ninja
> +
> +  - name: install cmocka
> +if: steps.cache.outputs.cache-hit != 'true'
> +working-directory: "./cmocka-build"
> +run: ninja install
> +
>- name: Configure OpenSSL
>  if: steps.cache.outputs.cache-hit != 'true'
>  run: ./Configure --cross-compile-prefix=${{ matrix.chost }}-
> shared ${{ matrix.target }} no-capieng --prefix="${HOME}/mingw/opt"
> --openssldir="${HOME}/mingw/opt" -static-libgcc
> @@ -154,6 +176,89 @@ jobs:
>- name: build OpenVPN
>  run: make -j3
>  working-directory: openvpn
> +  - name: build OpenVPN unittests
> +run: make -j3 check
> +working-directory: openvpn
> +
> +  # We use multiple upload-artifact here, so it becomes a flat folder
> +  # structure since we need the ddls on the same level as the binaries
>

ddls -> dlls

+  - name: Archive cmocka/openssl/lzo ddls
> +uses: actions/upload-artifact@v3
> +with:
> +  retention-days: 1
> +  name: mingw-unittest-${{matrix.target}}-ossl${{ matrix.osslver
> }}-dlls
> +  path: '~/mingw/opt/bin/*.dll'
> +
> 

Re: [Openvpn-devel] [PATCH v2 3/3] Add a unit test for functions in cryptoapi.c

[Selva Nair]

On Wed, Feb 8, 2023 at 6:16 AM Arne Schwabe  wrote:

> Am 08.02.23 um 02:05 schrieb Selva Nair:
> > Hi,
> >
> > On Tue, Feb 7, 2023 at 7:18 PM Arne Schwabe  > <mailto:a...@rfc2549.org>> wrote:
> >
> > Am 04.02.23 um 07:40 schrieb selva.n...@gmail.com
> >     <mailto:selva.n...@gmail.com>:
> >  > From: Selva Nair  selva.n...@gmail.com>>
> >  >
> >  > - Though named cryptoapi_testdriver, right now this only tests
> >  >parsing of thumbprint specified as a selector for
> --cryptioapicert
> >  >option. More tests coming..
> >  >
> >  > v2: a line that belongs here was mistakenly included in the
> previous
> >  > commit. Corrected.
> >  >
> >  > Signed-off-by: Selva Nair  > <mailto:selva.n...@gmail.com>>
> >
> > While a good step, this still has problems. With OpenSSL 1.1.1 it
> still
> > tries to run the check-engine.sh script and unsurprisingly fails:
> >
> >  begin log.txt 
> > ./check_engine_keys.sh: 25: ../../../src/openvpn/openvpn: not found
> > --- end log.txt ---
> > Key mismatch not detected
> > FAIL: check_engine_keys.sh
> >
> > This can be fixed by:
> >
> > --- a/tests/unit_tests/engine-key/Makefile.am
> > +++ b/tests/unit_tests/engine-key/Makefile.am
> > @@ -12,7 +12,9 @@ TESTS_ENVIRONMENT = srcdir="$(abs_srcdir)"; \
> >   top_srcdir="$(top_srcdir)"; \
> >   export srcdir builddir top_builddir top_srcdir;
> >
> > +if !CROSS_COMPILING
> >TESTS = check_engine_keys.sh
> > +endif
> >check_engine_keys.sh: $(conffiles)
> >
> >CLEANFILES = \
> >
> >
> > Thanks for this. I added this to 2/3 where it belongs and submitted it
> > as v3 2/3. With that, this patch should be okay as is.
> >
> > For those not following closely, this patch  should be applied after the
> > cryptoapi patch set where the function being tested here is introduced.
> >
> > Selva
>
> No, I think this that is independent. This is about the engine keys
> support added by James Bottomley.  I needed and tested this patch
> without the cryptoapi patch.
>

Got that. I think gmail is messing up my thread view -- this email appear
for me as linked to my original 3/3 which is on the test for cryptoapi.
That's why I wrote "this patch should be okay as is."  Anyway...

Will look into your patches later today.

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

Re: [Openvpn-devel] [PATCH v2 3/3] Add a unit test for functions in cryptoapi.c

[Selva Nair]

Hi,

On Tue, Feb 7, 2023 at 7:18 PM Arne Schwabe  wrote:

> Am 04.02.23 um 07:40 schrieb selva.n...@gmail.com:
> > From: Selva Nair 
> >
> > - Though named cryptoapi_testdriver, right now this only tests
> >parsing of thumbprint specified as a selector for --cryptioapicert
> >option. More tests coming..
> >
> > v2: a line that belongs here was mistakenly included in the previous
> > commit. Corrected.
> >
> > Signed-off-by: Selva Nair 
>
> While a good step, this still has problems. With OpenSSL 1.1.1 it still
> tries to run the check-engine.sh script and unsurprisingly fails:
>
>  begin log.txt 
> ./check_engine_keys.sh: 25: ../../../src/openvpn/openvpn: not found
> --- end log.txt ---
> Key mismatch not detected
> FAIL: check_engine_keys.sh
>
> This can be fixed by:
>
> --- a/tests/unit_tests/engine-key/Makefile.am
> +++ b/tests/unit_tests/engine-key/Makefile.am
> @@ -12,7 +12,9 @@ TESTS_ENVIRONMENT = srcdir="$(abs_srcdir)"; \
>  top_srcdir="$(top_srcdir)"; \
>  export srcdir builddir top_builddir top_srcdir;
>
> +if !CROSS_COMPILING
>   TESTS = check_engine_keys.sh
> +endif
>   check_engine_keys.sh: $(conffiles)
>
>   CLEANFILES = \
>

Thanks for this. I added this to 2/3 where it belongs and submitted it as
v3 2/3. With that, this patch should be okay as is.

For those not following closely, this patch  should be applied after the
cryptoapi patch set where the function being tested here is introduced.

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

[Openvpn-devel] [PATCH v3 2/3] Build unit tests in mingw Windows build

[selva . nair]

From: Selva Nair 

- Minor changes to the build system to include some
  dependencies for Windows build

- test_tls_crypt not built as it will pull in win32.c and
  its dependencies

- If cross-compiling, "make check" will only build the tests but not
  run any. Copy to Windows and run manually. Executables are in
  /tests/unit_tests/openvpn/.libs/ and these depend on
  cmocka.dll in addition to openssl libs that some tests link to.

  Building with mingw on Windows should run the tests (untested).

v2: networking_testdriver was mistakenly enabled to run, while
originally it was only set to build. Corrected.

v3: exclude check_engine_keys.sh when cross-compiling
As suggested by Arne Schwabe 

Signed-off-by: Selva Nair 
---
 configure.ac  |  2 ++
 tests/Makefile.am |  2 ++
 tests/unit_tests/engine-key/Makefile.am   |  2 ++
 tests/unit_tests/example_test/Makefile.am |  2 ++
 tests/unit_tests/openvpn/Makefile.am  | 28 +++
 5 files changed, 31 insertions(+), 5 deletions(-)

diff --git a/configure.ac b/configure.ac
index 95d795c3..66ba6f38 100644
--- a/configure.ac
+++ b/configure.ac
@@ -364,6 +364,8 @@ case "$host" in
;;
 esac
 
+AM_CONDITIONAL([CROSS_COMPILING], test "${cross_compiling}" = "yes")
+
 PKG_PROG_PKG_CONFIG
 AC_PROG_CPP
 AC_PROG_INSTALL
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 87dd7e17..a46f2573 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -14,10 +14,12 @@ MAINTAINERCLEANFILES = \
 
 SUBDIRS = unit_tests
 
+if !WIN32
 test_scripts = t_client.sh t_lpback.sh t_cltsrv.sh
 if HAVE_SITNL
 test_scripts += t_net.sh
 endif
+endif
 
 TESTS_ENVIRONMENT = top_srcdir="$(top_srcdir)"
 TESTS = $(test_scripts)
diff --git a/tests/unit_tests/engine-key/Makefile.am 
b/tests/unit_tests/engine-key/Makefile.am
index 24622251..0c288857 100644
--- a/tests/unit_tests/engine-key/Makefile.am
+++ b/tests/unit_tests/engine-key/Makefile.am
@@ -12,7 +12,9 @@ TESTS_ENVIRONMENT = srcdir="$(abs_srcdir)"; \
top_srcdir="$(top_srcdir)"; \
export srcdir builddir top_builddir top_srcdir;
 
+if !CROSS_COMPILING
 TESTS = check_engine_keys.sh
+endif
 check_engine_keys.sh: $(conffiles)
 
 CLEANFILES = \
diff --git a/tests/unit_tests/example_test/Makefile.am 
b/tests/unit_tests/example_test/Makefile.am
index 04a5ad35..24eb0ba1 100644
--- a/tests/unit_tests/example_test/Makefile.am
+++ b/tests/unit_tests/example_test/Makefile.am
@@ -2,7 +2,9 @@ AUTOMAKE_OPTIONS = foreign
 
 check_PROGRAMS = example_testdriver example2_testdriver
 
+if !CROSS_COMPILING
 TESTS = $(check_PROGRAMS)
+endif
 
 example_testdriver_CFLAGS  = @TEST_CFLAGS@
 example_testdriver_LDFLAGS = @TEST_LDFLAGS@
diff --git a/tests/unit_tests/openvpn/Makefile.am 
b/tests/unit_tests/openvpn/Makefile.am
index 7720a85d..8d2386e0 100644
--- a/tests/unit_tests/openvpn/Makefile.am
+++ b/tests/unit_tests/openvpn/Makefile.am
@@ -7,14 +7,22 @@ test_binaries += argv_testdriver buffer_testdriver
 endif
 
 test_binaries += crypto_testdriver packet_id_testdriver auth_token_testdriver 
ncp_testdriver misc_testdriver \
-pkt_testdriver
+   pkt_testdriver
 if HAVE_LD_WRAP_SUPPORT
+if !WIN32
 test_binaries += tls_crypt_testdriver
 endif
+endif
 
 test_binaries += provider_testdriver
 
+if WIN32
+LDADD = -lws2_32
+endif
+
+if !CROSS_COMPILING
 TESTS = $(test_binaries)
+endif
 check_PROGRAMS = $(test_binaries)
 
 if HAVE_SITNL
@@ -31,12 +39,14 @@ argv_testdriver_SOURCES = test_argv.c mock_msg.c mock_msg.h 
\
mock_get_random.c \
$(openvpn_srcdir)/platform.c \
$(openvpn_srcdir)/buffer.c \
+   $(openvpn_srcdir)/win32-util.c \
$(openvpn_srcdir)/argv.c
 
 buffer_testdriver_CFLAGS  = @TEST_CFLAGS@ -I$(openvpn_srcdir) 
-I$(compat_srcdir)
 buffer_testdriver_LDFLAGS = @TEST_LDFLAGS@ -L$(openvpn_srcdir) 
-Wl,--wrap=parse_line
 buffer_testdriver_SOURCES = test_buffer.c mock_msg.c mock_msg.h \
mock_get_random.c \
+   $(openvpn_srcdir)/win32-util.c \
$(openvpn_srcdir)/platform.c
 
 crypto_testdriver_CFLAGS  = @TEST_CFLAGS@ \
@@ -51,6 +61,7 @@ crypto_testdriver_SOURCES = test_crypto.c mock_msg.c 
mock_msg.h \
$(openvpn_srcdir)/packet_id.c \
$(openvpn_srcdir)/platform.c \
$(openvpn_srcdir)/mtu.c \
+   $(openvpn_srcdir)/win32-util.c \
$(openvpn_srcdir)/mss.c
 
 packet_id_testdriver_CFLAGS  = @TEST_CFLAGS@ \
@@ -63,14 +74,14 @@ packet_id_testdriver_SOURCES = test_packet_id.c mock_msg.c 
mock_msg.h \
$(openvpn_srcdir)/packet_id.c \
$(openvpn_srcdir)/platform.c \
$(openvpn_srcdir)/reliable.c \
+   $(openvpn_srcdir)/win32-util.c \
$(openvpn_srcdir)/session_id.c
 
-
 pkt_testdriver_CFLAGS  = @TEST_CFLAGS@ \
-I$(openvpn_includedir) -I$(compat_srcdir) -I$(openvpn_srcdir)
 pkt_testdriver_LDFLAGS = @TEST_LDFLAGS@
 pkt_testdriver_SOURCES = test_pkt.c mock_msg.c moc

Re: [Openvpn-devel] [PATCH 1/5] Conditionally add subdir-objects option to automake

[Selva Nair]

On Tue, Feb 7, 2023 at 7:18 PM Arne Schwabe  wrote:

> From: Selva Nair 
>
> - Eliminates repeated warnings such as
>   warning: source file '$(openvpn_srcdir)/env_set.c' is in a subdirectory,
>   but option 'subdir-objects' is disabled
> - Enabled only for automake >= 1.16 as older versions have a buggy
> implementation
>   of this option
>
> Main side effect of this option is that object files like
> openvpnserv-blockdns.o
> are now created in src/openvpn where block-dns.c resides instead of in
> src/openvpnserv. Same for object files for sources from $(openvpn_srcdir)
> compiled
> into test executables.
>
> See also past discussion on this topic:
>
> https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg00013.html
>
> Signed-off-by: Selva Nair 
> Signed-off-by: Arne Schwabe 
>

I'm a bit confused here -- it seems you have made some edits to my patch
(signed-off), but not clear what has changed.

As for my original 3/3, I will add your fix for engine-test and submit as
v3 -- thanks for working on this.

I've no idea why libtool adds that wrong line break in its wrapper script
on OSX.

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

Re: [Openvpn-devel] [PATCH v2 2/3] Build unit tests in mingw Windows build

[Selva Nair]

Hi,

On Tue, Feb 7, 2023 at 6:59 AM Arne Schwabe  wrote:

> Am 04.02.23 um 07:40 schrieb selva.n...@gmail.com:
> > From: Selva Nair 
> >
> > - Minor changes to the build system to include some
> >dependencies for Windows build
> >
> > - test_tls_crypt not built as it will pull in win32.c and
> >its dependencies
> >
> > - If cross-compiling, "make check" will only build the tests but not
> >run any. Copy to Windows and run manually. Executables are in
> >/tests/unit_tests/openvpn/.libs/ and these depend on
> >cmocka.dll in addition to openssl libs that some tests link to.
> >
> >Building with mingw on Windows should run the tests (untested).
> >
> > v2: networking_testdriver was mistakenly enabled to run, while
> > originally it was only set to build. Corrected.
> >
>
> I am currently testing this and also trying to add this to the Github
> actions. This patch seems fine but I get other weird errors and cmocka
> builds fine crosscompiling on my own machine but breaks for some reason
> in Github Actions. I am looking into these problems and will give a full
> review after I solved those issues.
>

That would be nice. I was not sure how to copy the artifacts into a Windows
instance and add a job for testing them in GHA.

Regarding "weird errors" an obvious thing easy to overlook is that the
exe's are in .libs/ -- the wrapper that libtool generates is only useful
for running native builds in-place with uninstalled shared libraries.

Also, I just noticed that I had locally disabled the example test because
of some missing header warning -- it needs an include  to build.

Regards,

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

Re: [Openvpn-devel] [PATCH v3] configure: enable DCO by default on FreeBSD/Linux

[Selva Nair]

 be version 3.4.0 or newer for DCO])
> + ]
> +   )
> +   CFLAGS="${CFLAGS} ${LIBNL_GENL_CFLAGS}"
> +   LIBS="${LIBS} ${LIBNL_GENL_LIBS}"
>
> -   AC_DEFINE(ENABLE_DCO, 1, [Enable shared data
> channel offload])
> -   AC_MSG_NOTICE([Enabled ovpn-dco support for Linux])
> +   AC_DEFINE(ENABLE_DCO, 1, [Enable shared
> data channel offload])
> +   AC_MSG_NOTICE([Enabled ovpn-dco support
> for Linux])
> +   fi
> ;;
> *-*-freebsd*)
> -   LIBS="${LIBS} -lnv"
> -   AC_DEFINE(ENABLE_DCO, 1, [Enable data channel
> offload for FreeBSD])
> -   AC_MSG_NOTICE([Enabled ovpn-dco support for
> FreeBSD])
> +   AC_CHECK_LIB(
> +   [nv],
> +   [nvlist_create],
> +   [
> +LIBS="${LIBS} -lnv"
> +AC_DEFINE(ENABLE_DCO, 1, [Enable data
> channel offload for FreeBSD])
> +AC_MSG_NOTICE([Enabled ovpn-dco support
> for FreeBSD])
> +   ],
> +   [
> +enable_dco="no"
> +    AC_MSG_WARN([Name/Value pair library not
> found.])
> +   ]
> +   )
> +   if test "$enable_dco" = "no"; then
> +   if test "$enable_dco_arg" = "auto"; then
> +   AC_MSG_WARN([DCO support disabled])
> +   else
> +   AC_MSG_ERROR([DCO support can't be
> enabled])
> +   fi
> +   fi
> ;;
> *-mingw*)
> AC_MSG_NOTICE([NOTE: --enable-dco ignored on
> Windows because it's always enabled])
>

Looks good to me.

Acked-by: Selva Nair 

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

Re: [Openvpn-devel] [PATCH v2] configure: enable DCO by default on FreeBSD/Linux

[Selva Nair]

Hi,

On Mon, Feb 6, 2023 at 6:24 AM Frank Lichtenheld 
wrote:

> Automatically disabled when
> - iproute2 is enabled
>   (Don't want to force people specifying --disable-dco explicitely)
> - libnv is missing on FreeBSD
>   (FreeBSD version too old anyway)
>
> Will still error out if libnl-genl is missing on Linux to
> make people aware of new dependency.
>
> Signed-off-by: Frank Lichtenheld 
> ---
>  configure.ac | 78 
>  1 file changed, 61 insertions(+), 17 deletions(-)
>
> v2: error out when libnl-genl is missing as discussed with ordex on
> IRC.
>
>
> diff --git a/configure.ac b/configure.ac
> index 91500087..acfa4bc1 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -144,14 +144,27 @@ AC_ARG_ENABLE(
>
>  AC_ARG_ENABLE(
> [dco],
> -   [AS_HELP_STRING([--enable-dco], [enable data channel offload
> support using the ovpn-dco kernel module (always enabled on Windows)
> @<:@default=no@:>@])],
> +   [AS_HELP_STRING([--disable-dco], [enable data channel offload
> support using the ovpn-dco kernel module (always enabled on Windows)
> @<:@default=yes@:>@])],
>

The help string should be
"disable data channel offload support ..." instead of "enable data channel
offload ..." as it's now prefixed with "--disable-dco" .

The "default=yes" in this case may be better written as "default=auto" (or
"platform specific") but it's not critical. Here, default always refers to
the "--enable-FEATURE" form of the option. Confusing but consistent with
how other options are described.

We could be more helpful by describing both enable and disable forms of all
options, as well as custom args that the enable form takes (if any), but
that's beyond the scope of this patch.

-if test "$enable_dco" = "yes"; then
> +if test "$enable_dco" != "no"; then
> +   enable_dco_arg="$enable_dco"
> +   if test "${enable_iproute2}" = "yes"; then
> +   AC_MSG_WARN([iproute2 support cannot be enabled when using
> DCO])
>

This is mildly misleading as here iproute2 is going to win over DCO but the
message appears to indicate otherwise. It would result in outputs like:

WARNING: iproute2 support cannot be enabled when using DCO
WARNING: DCO support disabled
(or in an error)

Phrasing the first as "DCO cannot be enabled when using iproute2" would be
better.


> +   enable_dco="no"
> +   fi
> +   case "$host" in
> +   *-*-linux*)


I'll defer to those who understand DCO better to ACK the technical part.

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

[Openvpn-devel] [PATCH v2 2/3] Build unit tests in mingw Windows build

[selva . nair]

From: Selva Nair 

- Minor changes to the build system to include some
  dependencies for Windows build

- test_tls_crypt not built as it will pull in win32.c and
  its dependencies

- If cross-compiling, "make check" will only build the tests but not
  run any. Copy to Windows and run manually. Executables are in
  /tests/unit_tests/openvpn/.libs/ and these depend on
  cmocka.dll in addition to openssl libs that some tests link to.

  Building with mingw on Windows should run the tests (untested).

v2: networking_testdriver was mistakenly enabled to run, while
originally it was only set to build. Corrected.

Signed-off-by: Selva Nair 
---
 configure.ac  |  2 ++
 tests/Makefile.am |  2 ++
 tests/unit_tests/example_test/Makefile.am |  2 ++
 tests/unit_tests/openvpn/Makefile.am  | 28 +++
 4 files changed, 29 insertions(+), 5 deletions(-)

diff --git a/configure.ac b/configure.ac
index 95d795c3..66ba6f38 100644
--- a/configure.ac
+++ b/configure.ac
@@ -364,6 +364,8 @@ case "$host" in
;;
 esac
 
+AM_CONDITIONAL([CROSS_COMPILING], test "${cross_compiling}" = "yes")
+
 PKG_PROG_PKG_CONFIG
 AC_PROG_CPP
 AC_PROG_INSTALL
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 87dd7e17..a46f2573 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -14,10 +14,12 @@ MAINTAINERCLEANFILES = \
 
 SUBDIRS = unit_tests
 
+if !WIN32
 test_scripts = t_client.sh t_lpback.sh t_cltsrv.sh
 if HAVE_SITNL
 test_scripts += t_net.sh
 endif
+endif
 
 TESTS_ENVIRONMENT = top_srcdir="$(top_srcdir)"
 TESTS = $(test_scripts)
diff --git a/tests/unit_tests/example_test/Makefile.am 
b/tests/unit_tests/example_test/Makefile.am
index 04a5ad35..24eb0ba1 100644
--- a/tests/unit_tests/example_test/Makefile.am
+++ b/tests/unit_tests/example_test/Makefile.am
@@ -2,7 +2,9 @@ AUTOMAKE_OPTIONS = foreign
 
 check_PROGRAMS = example_testdriver example2_testdriver
 
+if !CROSS_COMPILING
 TESTS = $(check_PROGRAMS)
+endif
 
 example_testdriver_CFLAGS  = @TEST_CFLAGS@
 example_testdriver_LDFLAGS = @TEST_LDFLAGS@
diff --git a/tests/unit_tests/openvpn/Makefile.am 
b/tests/unit_tests/openvpn/Makefile.am
index 7720a85d..8d2386e0 100644
--- a/tests/unit_tests/openvpn/Makefile.am
+++ b/tests/unit_tests/openvpn/Makefile.am
@@ -7,14 +7,22 @@ test_binaries += argv_testdriver buffer_testdriver
 endif
 
 test_binaries += crypto_testdriver packet_id_testdriver auth_token_testdriver 
ncp_testdriver misc_testdriver \
-pkt_testdriver
+   pkt_testdriver
 if HAVE_LD_WRAP_SUPPORT
+if !WIN32
 test_binaries += tls_crypt_testdriver
 endif
+endif
 
 test_binaries += provider_testdriver
 
+if WIN32
+LDADD = -lws2_32
+endif
+
+if !CROSS_COMPILING
 TESTS = $(test_binaries)
+endif
 check_PROGRAMS = $(test_binaries)
 
 if HAVE_SITNL
@@ -31,12 +39,14 @@ argv_testdriver_SOURCES = test_argv.c mock_msg.c mock_msg.h 
\
mock_get_random.c \
$(openvpn_srcdir)/platform.c \
$(openvpn_srcdir)/buffer.c \
+   $(openvpn_srcdir)/win32-util.c \
$(openvpn_srcdir)/argv.c
 
 buffer_testdriver_CFLAGS  = @TEST_CFLAGS@ -I$(openvpn_srcdir) 
-I$(compat_srcdir)
 buffer_testdriver_LDFLAGS = @TEST_LDFLAGS@ -L$(openvpn_srcdir) 
-Wl,--wrap=parse_line
 buffer_testdriver_SOURCES = test_buffer.c mock_msg.c mock_msg.h \
mock_get_random.c \
+   $(openvpn_srcdir)/win32-util.c \
$(openvpn_srcdir)/platform.c
 
 crypto_testdriver_CFLAGS  = @TEST_CFLAGS@ \
@@ -51,6 +61,7 @@ crypto_testdriver_SOURCES = test_crypto.c mock_msg.c 
mock_msg.h \
$(openvpn_srcdir)/packet_id.c \
$(openvpn_srcdir)/platform.c \
$(openvpn_srcdir)/mtu.c \
+   $(openvpn_srcdir)/win32-util.c \
$(openvpn_srcdir)/mss.c
 
 packet_id_testdriver_CFLAGS  = @TEST_CFLAGS@ \
@@ -63,14 +74,14 @@ packet_id_testdriver_SOURCES = test_packet_id.c mock_msg.c 
mock_msg.h \
$(openvpn_srcdir)/packet_id.c \
$(openvpn_srcdir)/platform.c \
$(openvpn_srcdir)/reliable.c \
+   $(openvpn_srcdir)/win32-util.c \
$(openvpn_srcdir)/session_id.c
 
-
 pkt_testdriver_CFLAGS  = @TEST_CFLAGS@ \
-I$(openvpn_includedir) -I$(compat_srcdir) -I$(openvpn_srcdir)
 pkt_testdriver_LDFLAGS = @TEST_LDFLAGS@
 pkt_testdriver_SOURCES = test_pkt.c mock_msg.c mock_msg.h \
-$(openvpn_srcdir)/argv.c \
+   $(openvpn_srcdir)/argv.c \
$(openvpn_srcdir)/base64.c \
$(openvpn_srcdir)/buffer.c \
$(openvpn_srcdir)/crypto.c \
@@ -84,9 +95,10 @@ $(openvpn_srcdir)/argv.c \
$(openvpn_srcdir)/run_command.c \
$(openvpn_srcdir)/session_id.c \
$(openvpn_srcdir)/ssl_pkt.c \
+   $(openvpn_srcdir)/win32-util.c \
$(openvpn_srcdir)/tls_crypt.c
 
-
+if !WIN32
 tls_crypt_testdriver_CFLAGS  = @TEST_CFLAGS@ \
-I$(openvpn_includedir) -I$(compat_srcdir) -I$(openvpn_srcdir)
 tls_crypt_testdriver_LDFLAGS = @TEST_LDFLAGS@ \
@@ -106,6 +118,7 @@ tls_crypt_testdriver

[Openvpn-devel] [PATCH v2 3/3] Add a unit test for functions in cryptoapi.c

[selva . nair]

From: Selva Nair 

- Though named cryptoapi_testdriver, right now this only tests
  parsing of thumbprint specified as a selector for --cryptioapicert
  option. More tests coming..

v2: a line that belongs here was mistakenly included in the previous
commit. Corrected.

Signed-off-by: Selva Nair 
---
 tests/unit_tests/openvpn/Makefile.am  |  16 +++
 tests/unit_tests/openvpn/test_cryptoapi.c | 126 ++
 2 files changed, 142 insertions(+)
 create mode 100644 tests/unit_tests/openvpn/test_cryptoapi.c

diff --git a/tests/unit_tests/openvpn/Makefile.am 
b/tests/unit_tests/openvpn/Makefile.am
index 8d2386e0..ee0a3d8a 100644
--- a/tests/unit_tests/openvpn/Makefile.am
+++ b/tests/unit_tests/openvpn/Makefile.am
@@ -17,6 +17,7 @@ endif
 test_binaries += provider_testdriver
 
 if WIN32
+test_binaries += cryptoapi_testdriver
 LDADD = -lws2_32
 endif
 
@@ -152,6 +153,21 @@ provider_testdriver_SOURCES = test_provider.c mock_msg.c \
$(openvpn_srcdir)/win32-util.c \
$(openvpn_srcdir)/platform.c
 
+if WIN32
+cryptoapi_testdriver_CFLAGS  = @TEST_CFLAGS@ \
+   -I$(openvpn_includedir) -I$(compat_srcdir) -I$(openvpn_srcdir) \
+   $(OPTIONAL_CRYPTO_CFLAGS)
+cryptoapi_testdriver_LDFLAGS = @TEST_LDFLAGS@ \
+   $(OPTIONAL_CRYPTO_LIBS) -lcrypt32 -lncrypt
+cryptoapi_testdriver_SOURCES = test_cryptoapi.c mock_msg.c \
+   $(openvpn_srcdir)/xkey_helper.c \
+   $(openvpn_srcdir)/buffer.c \
+   $(openvpn_srcdir)/base64.c \
+   $(openvpn_srcdir)/platform.c \
+   mock_get_random.c \
+   $(openvpn_srcdir)/win32-util.c
+endif
+
 auth_token_testdriver_CFLAGS  = @TEST_CFLAGS@ \
-I$(openvpn_includedir) -I$(compat_srcdir) -I$(openvpn_srcdir) \
$(OPTIONAL_CRYPTO_CFLAGS)
diff --git a/tests/unit_tests/openvpn/test_cryptoapi.c 
b/tests/unit_tests/openvpn/test_cryptoapi.c
new file mode 100644
index ..2bea3f42
--- /dev/null
+++ b/tests/unit_tests/openvpn/test_cryptoapi.c
@@ -0,0 +1,126 @@
+/*
+ *  OpenVPN -- An application to securely tunnel IP networks
+ * over a single UDP port, with support for SSL/TLS-based
+ * session authentication and key exchange,
+ * packet encryption, packet authentication, and
+ * packet compression.
+ *
+ *  Copyright (C) 2023 Selva Nair 
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by the
+ *  Free Software Foundation, either version 2 of the License,
+ *  or (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  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.
+ */
+
+#ifdef HAVE_CONFIG_H
+#include "config.h"
+#elif defined(_MSC_VER)
+#include "config-msvc.h"
+#endif
+
+#include "syshead.h"
+#include "manage.h"
+#include "integer.h"
+#include "xkey_common.h"
+
+#if defined(HAVE_XKEY_PROVIDER) && defined (ENABLE_CRYPTOAPI)
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+#include  /* pull-in the whole file to test static functions */
+
+struct management *management; /* global */
+
+/* mock a management function that xkey_provider needs */
+char *
+management_query_pk_sig(struct management *man, const char *b64_data,
+const char *algorithm)
+{
+(void) man;
+(void) b64_data;
+(void) algorithm;
+return NULL;
+}
+
+/* tls_libctx is defined in ssl_openssl.c which we do not want to compile in */
+OSSL_LIB_CTX *tls_libctx;
+
+#ifndef _countof
+#define _countof(x) sizeof((x))/sizeof(*(x))
+#endif
+
+/* test data */
+static const uint8_t test_hash[] = {
+0x77, 0x38, 0x65, 0x00, 0x1e, 0x96, 0x48, 0xc6, 0x57, 0x0b, 0xae,
+0xc0, 0xb7, 0x96, 0xf9, 0x66, 0x4d, 0x5f, 0xd0, 0xb7
+};
+
+/* valid test strings to test with and without embedded and trailing spaces */
+static const char *valid_str[] = {
+"773865001e9648c6570baec0b796f9664d5fd0b7",
+" 77 386500 1e 96 48 c6570b aec0b7   96f9664d5f  d0 b7",
+"   773865001e9648c6570baec0b796f9664d5fd0b7  ",
+};
+
+/* some invalid strings to test with and without embedded and trailing spaces 
*/
+static const char *invalid_str[] = {
+"773 865001e9648c6570baec0b796f9664d5fd012",  /* space within byte */
+"77:38:65001e9648c6570baec0b796f9664d5fd0b7", /* invalid separator */
+"7738x5001e9648c6570baec0b796f9664d5fd0b7",   /* non hex character */
+};
+
+static void
+test_parse_hexstring(void **state)
+{
+unsigned c

[Openvpn-devel] [PATCH 3/3] Add a unit test for functions in cryptoapi.c

[selva . nair]

From: Selva Nair 

- Though named cryptoapi_testdriver, right now this only tests
  parsing of thumbprint specified as a selector for --cryptioapicert
  option. More cryptoapi tests coming..

Signed-off-by: Selva Nair 
---

As requested during review of the 4/4 patch of cryptoapi series.
Note that this doesn't currently build with MSVC -- either cross
compile on Linux and run the binary on Windows or use mingw to
compile on Windows.

 tests/unit_tests/openvpn/Makefile.am  |  15 +++
 tests/unit_tests/openvpn/test_cryptoapi.c | 126 ++
 2 files changed, 141 insertions(+)
 create mode 100644 tests/unit_tests/openvpn/test_cryptoapi.c

diff --git a/tests/unit_tests/openvpn/Makefile.am 
b/tests/unit_tests/openvpn/Makefile.am
index 909ac4e2..0a1ad439 100644
--- a/tests/unit_tests/openvpn/Makefile.am
+++ b/tests/unit_tests/openvpn/Makefile.am
@@ -153,6 +153,21 @@ provider_testdriver_SOURCES = test_provider.c mock_msg.c \
$(openvpn_srcdir)/win32-util.c \
$(openvpn_srcdir)/platform.c
 
+if WIN32
+cryptoapi_testdriver_CFLAGS  = @TEST_CFLAGS@ \
+   -I$(openvpn_includedir) -I$(compat_srcdir) -I$(openvpn_srcdir) \
+   $(OPTIONAL_CRYPTO_CFLAGS)
+cryptoapi_testdriver_LDFLAGS = @TEST_LDFLAGS@ \
+   $(OPTIONAL_CRYPTO_LIBS) -lcrypt32 -lncrypt
+cryptoapi_testdriver_SOURCES = test_cryptoapi.c mock_msg.c \
+   $(openvpn_srcdir)/xkey_helper.c \
+   $(openvpn_srcdir)/buffer.c \
+   $(openvpn_srcdir)/base64.c \
+   $(openvpn_srcdir)/platform.c \
+   mock_get_random.c \
+   $(openvpn_srcdir)/win32-util.c
+endif
+
 auth_token_testdriver_CFLAGS  = @TEST_CFLAGS@ \
-I$(openvpn_includedir) -I$(compat_srcdir) -I$(openvpn_srcdir) \
$(OPTIONAL_CRYPTO_CFLAGS)
diff --git a/tests/unit_tests/openvpn/test_cryptoapi.c 
b/tests/unit_tests/openvpn/test_cryptoapi.c
new file mode 100644
index ..2bea3f42
--- /dev/null
+++ b/tests/unit_tests/openvpn/test_cryptoapi.c
@@ -0,0 +1,126 @@
+/*
+ *  OpenVPN -- An application to securely tunnel IP networks
+ * over a single UDP port, with support for SSL/TLS-based
+ * session authentication and key exchange,
+ * packet encryption, packet authentication, and
+ * packet compression.
+ *
+ *  Copyright (C) 2023 Selva Nair 
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by the
+ *  Free Software Foundation, either version 2 of the License,
+ *  or (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  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.
+ */
+
+#ifdef HAVE_CONFIG_H
+#include "config.h"
+#elif defined(_MSC_VER)
+#include "config-msvc.h"
+#endif
+
+#include "syshead.h"
+#include "manage.h"
+#include "integer.h"
+#include "xkey_common.h"
+
+#if defined(HAVE_XKEY_PROVIDER) && defined (ENABLE_CRYPTOAPI)
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+#include  /* pull-in the whole file to test static functions */
+
+struct management *management; /* global */
+
+/* mock a management function that xkey_provider needs */
+char *
+management_query_pk_sig(struct management *man, const char *b64_data,
+const char *algorithm)
+{
+(void) man;
+(void) b64_data;
+(void) algorithm;
+return NULL;
+}
+
+/* tls_libctx is defined in ssl_openssl.c which we do not want to compile in */
+OSSL_LIB_CTX *tls_libctx;
+
+#ifndef _countof
+#define _countof(x) sizeof((x))/sizeof(*(x))
+#endif
+
+/* test data */
+static const uint8_t test_hash[] = {
+0x77, 0x38, 0x65, 0x00, 0x1e, 0x96, 0x48, 0xc6, 0x57, 0x0b, 0xae,
+0xc0, 0xb7, 0x96, 0xf9, 0x66, 0x4d, 0x5f, 0xd0, 0xb7
+};
+
+/* valid test strings to test with and without embedded and trailing spaces */
+static const char *valid_str[] = {
+"773865001e9648c6570baec0b796f9664d5fd0b7",
+" 77 386500 1e 96 48 c6570b aec0b7   96f9664d5f  d0 b7",
+"   773865001e9648c6570baec0b796f9664d5fd0b7  ",
+};
+
+/* some invalid strings to test with and without embedded and trailing spaces 
*/
+static const char *invalid_str[] = {
+"773 865001e9648c6570baec0b796f9664d5fd012",  /* space within byte */
+"77:38:65001e9648c6570baec0b796f9664d5fd0b7", /* invalid separator */
+"7738x5001e9648c6570baec0b796f9664d5fd0b7",   /* non hex character */
+};
+
+static void
+test_parse_hexstring(void **state)
+{
+unsig

[Openvpn-devel] [PATCH 2/3] Build unit tests in mingw Windows build

[selva . nair]

From: Selva Nair 

- Minor changes to the build system to include some
  dependencies for Windows build

- test_tls_crypt not built as it will pull in win32.c and
  its dependencies

- If cross-compiling, "make check" will only build the tests but not
  run any. Copy to Windows and run manually. Executables are in
  /tests/unit_tests/openvpn/.libs/ and these depend on
  cmocka.dll in addition to openssl libs that some tests link to.

  Building with mingw on Windows should run the tests (untested).

Signed-off-by: Selva Nair 
---
 configure.ac  |  2 ++
 tests/Makefile.am |  2 ++
 tests/unit_tests/example_test/Makefile.am |  2 ++
 tests/unit_tests/openvpn/Makefile.am  | 35 +--
 4 files changed, 33 insertions(+), 8 deletions(-)

diff --git a/configure.ac b/configure.ac
index 95d795c3..66ba6f38 100644
--- a/configure.ac
+++ b/configure.ac
@@ -364,6 +364,8 @@ case "$host" in
;;
 esac
 
+AM_CONDITIONAL([CROSS_COMPILING], test "${cross_compiling}" = "yes")
+
 PKG_PROG_PKG_CONFIG
 AC_PROG_CPP
 AC_PROG_INSTALL
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 87dd7e17..a46f2573 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -14,10 +14,12 @@ MAINTAINERCLEANFILES = \
 
 SUBDIRS = unit_tests
 
+if !WIN32
 test_scripts = t_client.sh t_lpback.sh t_cltsrv.sh
 if HAVE_SITNL
 test_scripts += t_net.sh
 endif
+endif
 
 TESTS_ENVIRONMENT = top_srcdir="$(top_srcdir)"
 TESTS = $(test_scripts)
diff --git a/tests/unit_tests/example_test/Makefile.am 
b/tests/unit_tests/example_test/Makefile.am
index 04a5ad35..24eb0ba1 100644
--- a/tests/unit_tests/example_test/Makefile.am
+++ b/tests/unit_tests/example_test/Makefile.am
@@ -2,7 +2,9 @@ AUTOMAKE_OPTIONS = foreign
 
 check_PROGRAMS = example_testdriver example2_testdriver
 
+if !CROSS_COMPILING
 TESTS = $(check_PROGRAMS)
+endif
 
 example_testdriver_CFLAGS  = @TEST_CFLAGS@
 example_testdriver_LDFLAGS = @TEST_LDFLAGS@
diff --git a/tests/unit_tests/openvpn/Makefile.am 
b/tests/unit_tests/openvpn/Makefile.am
index 7720a85d..909ac4e2 100644
--- a/tests/unit_tests/openvpn/Makefile.am
+++ b/tests/unit_tests/openvpn/Makefile.am
@@ -7,20 +7,29 @@ test_binaries += argv_testdriver buffer_testdriver
 endif
 
 test_binaries += crypto_testdriver packet_id_testdriver auth_token_testdriver 
ncp_testdriver misc_testdriver \
-pkt_testdriver
+pkt_testdriver
 if HAVE_LD_WRAP_SUPPORT
+if !WIN32
 test_binaries += tls_crypt_testdriver
 endif
+endif
 
 test_binaries += provider_testdriver
 
-TESTS = $(test_binaries)
-check_PROGRAMS = $(test_binaries)
+if WIN32
+test_binaries += cryptoapi_testdriver
+LDADD = -lws2_32
+endif
 
 if HAVE_SITNL
-check_PROGRAMS += networking_testdriver
+test_binaries += networking_testdriver
 endif
 
+if !CROSS_COMPILING
+TESTS = $(test_binaries)
+endif
+check_PROGRAMS = $(test_binaries)
+
 openvpn_includedir = $(top_srcdir)/include
 openvpn_srcdir = $(top_srcdir)/src/openvpn
 compat_srcdir = $(top_srcdir)/src/compat
@@ -31,12 +40,14 @@ argv_testdriver_SOURCES = test_argv.c mock_msg.c mock_msg.h 
\
mock_get_random.c \
$(openvpn_srcdir)/platform.c \
$(openvpn_srcdir)/buffer.c \
+   $(openvpn_srcdir)/win32-util.c \
$(openvpn_srcdir)/argv.c
 
 buffer_testdriver_CFLAGS  = @TEST_CFLAGS@ -I$(openvpn_srcdir) 
-I$(compat_srcdir)
 buffer_testdriver_LDFLAGS = @TEST_LDFLAGS@ -L$(openvpn_srcdir) 
-Wl,--wrap=parse_line
 buffer_testdriver_SOURCES = test_buffer.c mock_msg.c mock_msg.h \
mock_get_random.c \
+   $(openvpn_srcdir)/win32-util.c \
$(openvpn_srcdir)/platform.c
 
 crypto_testdriver_CFLAGS  = @TEST_CFLAGS@ \
@@ -51,6 +62,7 @@ crypto_testdriver_SOURCES = test_crypto.c mock_msg.c 
mock_msg.h \
$(openvpn_srcdir)/packet_id.c \
$(openvpn_srcdir)/platform.c \
$(openvpn_srcdir)/mtu.c \
+   $(openvpn_srcdir)/win32-util.c \
$(openvpn_srcdir)/mss.c
 
 packet_id_testdriver_CFLAGS  = @TEST_CFLAGS@ \
@@ -63,14 +75,14 @@ packet_id_testdriver_SOURCES = test_packet_id.c mock_msg.c 
mock_msg.h \
$(openvpn_srcdir)/packet_id.c \
$(openvpn_srcdir)/platform.c \
$(openvpn_srcdir)/reliable.c \
+   $(openvpn_srcdir)/win32-util.c \
$(openvpn_srcdir)/session_id.c
 
-
 pkt_testdriver_CFLAGS  = @TEST_CFLAGS@ \
-I$(openvpn_includedir) -I$(compat_srcdir) -I$(openvpn_srcdir)
 pkt_testdriver_LDFLAGS = @TEST_LDFLAGS@
 pkt_testdriver_SOURCES = test_pkt.c mock_msg.c mock_msg.h \
-$(openvpn_srcdir)/argv.c \
+   $(openvpn_srcdir)/argv.c \
$(openvpn_srcdir)/base64.c \
$(openvpn_srcdir)/buffer.c \
$(openvpn_srcdir)/crypto.c \
@@ -84,9 +96,10 @@ $(openvpn_srcdir)/argv.c \
$(openvpn_srcdir)/run_command.c \
$(openvpn_srcdir)/session_id.c \
$(openvpn_srcdir)/ssl_pkt.c \
+   $(openvpn_srcdir)/win32-util.c \
$(openvpn_srcdir)/tls_crypt.c
 
-
+if !WIN32
 tls_cry

[Openvpn-devel] [PATCH 1/3] Conditionally add subdir-objects option to automake

[selva . nair]

From: Selva Nair 

- Eliminates repeated warnings such as
  warning: source file '$(openvpn_srcdir)/env_set.c' is in a subdirectory,
  but option 'subdir-objects' is disabled
- Enabled only for automake >= 1.16 as older versions have a buggy 
implementation
  of this option

Main side effect of this option is that object files like openvpnserv-blockdns.o
are now created in src/openvpn where block-dns.c resides instead of in
src/openvpnserv. Same for object files for sources from $(openvpn_srcdir) 
compiled
into test executables.

See also past discussion on this topic:
  https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg00013.html

Signed-off-by: Selva Nair 
---
 configure.ac | 15 ++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/configure.ac b/configure.ac
index 91500087..95d795c3 100644
--- a/configure.ac
+++ b/configure.ac
@@ -54,9 +54,22 @@ m4_define([serial_tests], [
 awk '{split ($NF,a,"."); if (a[1] == 1 && a[2] >= 12) { print 
"serial-tests" }}'
 ])
 ])
+
+dnl Automake 1.14+ warns if sources are in sub-directories but subdir-objects
+dnl options is not enabled. However, automake before 1.15a has a bug that 
causes
+dnl variable expansion to fail in foo_SOURCES when this option is used.
+dnl As most of our build systems are now likely to use automake 1.16+ add a
+dnl work around to conditionally add subdir-objects option.
+m4_define([subdir_objects], [
+m4_esyscmd([automake --version |
+head -1 |
+awk '{split ($NF,a,"."); if (a[1] == 1 && a[2] >= 16) { print 
"subdir-objects" }}'
+])
+])
+
 # This foreign option prevents autoreconf from overriding our COPYING and
 # INSTALL targets:
-AM_INIT_AUTOMAKE(foreign serial_tests 1.9) dnl NB: Do not [quote] this 
parameter.
+AM_INIT_AUTOMAKE(foreign serial_tests subdir_objects 1.9) dnl NB: Do not 
[quote] this parameter.
 AC_CANONICAL_HOST
 AC_USE_SYSTEM_EXTENSIONS
 
-- 
2.34.1



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

[Openvpn-devel] [PATCH v2 4/4] cryptoapi.c: simplify parsing of thumbprint hex string

[selva . nair]

From: Selva Nair 

v2: Moved the "parse_hexstring" chunk to a function for clarity
and to permit unit-testing.

A test is submitted as a follow up patch.

Signed-off-by: Selva Nair 
---
 src/openvpn/cryptoapi.c | 77 -
 1 file changed, 37 insertions(+), 40 deletions(-)

diff --git a/src/openvpn/cryptoapi.c b/src/openvpn/cryptoapi.c
index eafef1b1..136c6ffc 100644
--- a/src/openvpn/cryptoapi.c
+++ b/src/openvpn/cryptoapi.c
@@ -180,6 +180,39 @@ err:
 return NULL;
 }
 
+/**
+ * Parse a hex string with optional embedded spaces into
+ * a byte array.
+ * @param p pointer to the input string
+ * @param arr   on output contains the parsed bytes
+ * @param capacity  capacity of the byte array arr
+ * @returns the number of bytes parsed or 0 on error
+ */
+int
+parse_hexstring(const char *p, unsigned char *arr, size_t capacity)
+{
+int i = 0;
+for ( ; *p && i < capacity; p += 2)
+{
+/* skip spaces */
+while (*p == ' ')
+{
+p++;
+}
+if (!*p) /* ending with spaces is not an error */
+{
+break;
+}
+
+if (!isxdigit(p[0]) || !isxdigit(p[1])
+|| sscanf(p, "%2hhx", &arr[i++]) != 1)
+{
+return 0;
+}
+}
+return i;
+}
+
 static const CERT_CONTEXT *
 find_certificate_in_store(const char *cert_prop, HCERTSTORE cert_store)
 {
@@ -210,51 +243,15 @@ find_certificate_in_store(const char *cert_prop, 
HCERTSTORE cert_store)
 }
 else if (!strncmp(cert_prop, "THUMB:", 6))
 {
-const char *p;
-int i, x = 0;
 find_type = CERT_FIND_HASH;
 find_param = &blob;
 
-/* skip the tag */
-cert_prop += 6;
-for (p = cert_prop, i = 0; *p && i < sizeof(hash); i++)
+blob.cbData = parse_hexstring(cert_prop + 6, hash, sizeof(hash));
+if (blob.cbData == 0)
 {
-if (*p >= '0' && *p <= '9')
-{
-x = (*p - '0') << 4;
-}
-else if (*p >= 'A' && *p <= 'F')
-{
-x = (*p - 'A' + 10) << 4;
-}
-else if (*p >= 'a' && *p <= 'f')
-{
-x = (*p - 'a' + 10) << 4;
-}
-if (!*++p)  /* unexpected end of string */
-{
-msg(M_WARN|M_INFO, "WARNING: cryptoapicert: error parsing 
.", cert_prop);
-goto out;
-}
-if (*p >= '0' && *p <= '9')
-{
-x += *p - '0';
-}
-else if (*p >= 'A' && *p <= 'F')
-{
-x += *p - 'A' + 10;
-}
-else if (*p >= 'a' && *p <= 'f')
-{
-x += *p - 'a' + 10;
-}
-hash[i] = x;
-/* skip any space(s) between hex numbers */
-for (p++; *p && *p == ' '; p++)
-{
-}
+msg(M_WARN|M_INFO, "WARNING: cryptoapicert: error parsing <%s>.", 
cert_prop);
+goto out;
 }
-blob.cbData = i;
 }
 else
 {
-- 
2.34.1



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