[Openvpn-devel,openvpn-devel] Feature request - Include daemon_pid in --tls-crypt-v2-verify env - V2

Message ID lWKeg7hMQJWx-9HJ5gxCC_45xPEstWOXF7bEyDgQnhS1E03ExZzYKnpLUGyVbc0k0knjJPCtF5A3OEKS9LIPs0BJogJBgAz--IJCPRd0kR4=@protonmail.com
State Superseded
Headers show
Series [Openvpn-devel,openvpn-devel] Feature request - Include daemon_pid in --tls-crypt-v2-verify env - V2 | expand

Commit Message

Kristof Provost via Openvpn-devel April 23, 2021, 11:16 a.m. UTC
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

Hi,

I am requesting that daemon_pid be added to --tls-crypt-v2-verify env.
Version 2

Justification:

With the notable exception of --tls-crypt-v2-verify ..
daemon_pid provides a verified process ID to All scripts. This ensures
that scripts which are intended to pass data along to the following scripts
have an index to which they can link that data.

Example:

An example is presented in Easy-TLS:
https://github.com/TinCanTech/easy-tls

This script passes hardware address from --tls-crypt-v2 key metadata along
to --client-connect, where the pushed client variable IV_HWADDR can be
matched against the fixed hardware address encrypted in the TLS Crypt V2
key metadata.

Security:

There are no known security concerns with regard to including the openvpn
process ID (daemon_pid) in the --tls-crypt-v2-verify environment.

Complexity:

Ongoing support of the required code would be minimal to zero.

Code:

This patch is included for review purposes only.

<git-formatted-patch>

</git-formatted-patch>

Conclusion:

Due to the OS in use and other environmental factors, the *nix built-in variable PPID
may not always be available. Without including $daemon_pid in the --tls-crypt-v2-verify
environment, openvpn is forcing the user to unnecessarily configure --writepid. 

The purpose of --writepid is to advertise the openvpn process ID to external processes
which do not have access to the internals of openvpn. By including daemon_pid
in the --tls-crypt-v2-verify environment all processes launched by openvpn have access
to this very useful identifier.

Provided there are no genuine reasons to NAK this request, I will send a correctly
formatted patch.

Addendum:

I know this is something which helps me in the short term and I already have a working
alternative but I would like you to reconsider your previous decision. In my opinion All
scripts launched by openvpn should have immediate access to daemon_pid.

Thank you for your time and consideration,
R
-----BEGIN PGP SIGNATURE-----
Version: ProtonMail

wsBzBAEBCAAGBQJggzkmACEJEE+XnPZrkLidFiEECbw9RGejjXJ5xVVVT5ec
9muQuJ0nVggAkf9tcCo7onTYoZ4WetX/6uePD2QzEYd8rHYbn1q6R8JvOqMi
JrDIRIYZw06v/r4pyzq8tYUvS+1VBY9cPIm+v3uudOhZ/WUlyGw180u2tA+w
eX+bx/AwA5FC4QGqgJlTEx9G5s0H5Ge2vSd1ChA52VjC5QZeorI/42nZpG2I
Gg7vC0JH9rr9LqAzVNH9YfWff7vNKvXAPdmL9/itf3Eq6uFytGsD77KjZaq7
RESDSO2cOnCyoVyktPhw64d77q6bCgFtl08CVQYJOTwg07cY+ZEWa3wRCEAb
bcDj6eDNDHy8e9iMzie3yrIgZsRDCbGiXCyaLk2abZtpFsqX7rP6jA==
=z4PC
-----END PGP SIGNATURE-----

Comments

Kristof Provost via Openvpn-devel April 27, 2021, 4:01 p.m. UTC | #1
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

Hi,

no complaints yet ?

Sent with ProtonMail Secure Email.
ProtonMail, as crap as googlemail.

‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Friday, 23 April 2021 22:16, tincantech via Openvpn-devel <openvpn-devel@lists.sourceforge.net> wrote:

> Hi,
>
> I am requesting that daemon_pid be added to --tls-crypt-v2-verify env.
> Version 2
>
> Justification:
>
> With the notable exception of --tls-crypt-v2-verify ..
> daemon_pid provides a verified process ID to All scripts. This ensures
> that scripts which are intended to pass data along to the following scripts
> have an index to which they can link that data.
>
> Example:
>
> An example is presented in Easy-TLS:
> https://github.com/TinCanTech/easy-tls
>
> This script passes hardware address from --tls-crypt-v2 key metadata along
> to --client-connect, where the pushed client variable IV_HWADDR can be
> matched against the fixed hardware address encrypted in the TLS Crypt V2
> key metadata.
>
> Security:
>
> There are no known security concerns with regard to including the openvpn
> process ID (daemon_pid) in the --tls-crypt-v2-verify environment.
>
> Complexity:
>
> Ongoing support of the required code would be minimal to zero.
>
> Code:
>
> This patch is included for review purposes only.
>
> <git-formatted-patch>
> diff --git a/src/openvpn/tls_crypt.c b/src/openvpn/tls_crypt.c
> index 7b5016d3..23d93a6c 100644
> --- a/src/openvpn/tls_crypt.c
> +++ b/src/openvpn/tls_crypt.c
> @@ -537,6 +537,7 @@ tls_crypt_v2_verify_metadata(const struct tls_wrap_ctx *ctx,
>      setenv_str(es, "script_type", "tls-crypt-v2-verify");
>      setenv_str(es, "metadata_type", metadata_type_str);
>      setenv_str(es, "metadata_file", tmp_file);
> +    setenv_int(es, "daemon_pid", platform_getpid());
>
>      struct argv argv = argv_new();
>      argv_parse_cmd(&argv, opt->tls_crypt_v2_verify_script);
>
> </git-formatted-patch>
>
> Conclusion:
>
> Due to the OS in use and other environmental factors, the *nix built-in variable PPID
> may not always be available. Without including $daemon_pid in the --tls-crypt-v2-verify
> environment, openvpn is forcing the user to unnecessarily configure --writepid. 
>
> The purpose of --writepid is to advertise the openvpn process ID to external processes
> which do not have access to the internals of openvpn. By including daemon_pid
> in the --tls-crypt-v2-verify environment all processes launched by openvpn have access
> to this very useful identifier.
>
> Provided there are no genuine reasons to NAK this request, I will send a correctly
> formatted patch.
>
> Addendum:
>
> I know this is something which helps me in the short term and I already have a working
> alternative but I would like you to reconsider your previous decision. In my opinion All
> scripts launched by openvpn should have immediate access to daemon_pid.
>
> Thank you for your time and consideration,
> R


-----BEGIN PGP SIGNATURE-----
Version: ProtonMail

wsBzBAEBCAAGBQJgiMIMACEJEE+XnPZrkLidFiEECbw9RGejjXJ5xVVVT5ec
9muQuJ2yHAf/VwSjdR6F5GQy7rfJLKkP+sbGgL1kgKPsB7bgiiSV47+GTg0J
lftyAS6lxyKhJ+7Xt+xm45janjMxnsxXrzIYjJdlfQSPMEfFOn9Uw17ohW0x
bO52oTqCqoR5Y/UhqlLQ+lpgUMJJalfWZtJ3uiQ1GfloJk9oKjJ1thmdnmQ+
048pGsBf2iRnvPJEDqJ/JxoKttvEAHQhVp3wI2aO70JzYujsuq5E6gnQsAT+
roDB8W2HRt5Ycbl+Y9lnzPM4HUk+W67j0+Af6Jf0mrfuK2IC2EFRBTkaVM5C
F9QICvlZ/wB9oaH4/OXfp1DXAHBHh2wf0Bw6Rxcsyg3ni8Ro0ARdsw==
=TmRk
-----END PGP SIGNATURE-----

Patch

diff --git a/src/openvpn/tls_crypt.c b/src/openvpn/tls_crypt.c
index 7b5016d3..23d93a6c 100644
--- a/src/openvpn/tls_crypt.c
+++ b/src/openvpn/tls_crypt.c
@@ -537,6 +537,7 @@  tls_crypt_v2_verify_metadata(const struct tls_wrap_ctx *ctx,
     setenv_str(es, "script_type", "tls-crypt-v2-verify");
     setenv_str(es, "metadata_type", metadata_type_str);
     setenv_str(es, "metadata_file", tmp_file);
+    setenv_int(es, "daemon_pid", platform_getpid());

     struct argv argv = argv_new();
     argv_parse_cmd(&argv, opt->tls_crypt_v2_verify_script);