adding support for systemd tty console wrapper

Hi,

as part of openSUSE switching to systemd by default, openVPN was made compatible with the systemd way of querying passphrase during boot (directly opening tty isn't supported).

Quoted 6 lines · Frederic Crozat

Hi,

as part of openSUSE switching to systemd by default, openVPN was made compatible with the systemd way of querying passphrase during boot (directly opening tty isn't supported).

Thanks a lot for looking into this! I feared that we would need to do some changes due to systemd.

Generally, this looks very fine. But I would like to see a few changes first before including it.

a) From what I've understood, systemd is by design a Linux only thing. So I'd like to see these code paths restricted to TARGET_LINUX (#ifdef statements). A lot of the dependencies is kernel related, so we probably won't see systemd for BSD, for example. And even less realistic is Solaris. Windows already goes a different route, so this won't be affected here.

b) As systemd is not targetted for embedded Linux devices (it's a massive resource hog, esp. in RAM and looks like to be most suitable in desktop/laptop environments), it could be useful to have this support compile time confiugrable via ./configure. I would even vote for disabling it by default. Right now, systemd only got traction from SuSE, Fedora. Debian is in the works and Ubuntu usually follows Debian. Arch Linux will move over as well.

However, I am systemd sceptic, and I would like to see if systemd really settles as a de-facto SySV replacement across the majority of Linux distributions, before enabling this support by default on Linux. I struggle to see that systemd will be beneficial on (enterprise) servers, as if the booting goes twice as fast or not on a server isn't that critical compared to a desktop/laptop system. And boot performance is one of the key points systemd tries to resolve.

c) It would probably be better to but the "check if systemd is available" into it's own function, for clarity - isolated in
#ifdef defined(TARGET_LINUX) && defined(USE_SYSTEMD) blocks. And likewise, do the /bin/systemd-ask-password in a separate function similar fashion as what is done with Windows' get_console_input_win32().

d) Instead of asprintf(), use openvpn_snprintf() functions. That can probably be just as fine work against a static sized buffer (say 256 bytes - it's for a password, right?) As this is called during startup, and it is serialised among all those places this might be asked for, there are no threading issues here, AFAICS.

e) Consider to make use of openvpn_execve_check() or openvpn_execve() instead of the popen() - or make a popen() variant which got similar error controls to popen() as openvpn_execve*() functions. I also think that this execution should honour the --script-security setting (which is implicitly checked for with openvpn_execve())

f) There are a few mixtures between tab and spaces in your patch. If you could please use space only, that would be great. The code is a mess in this regard, but we're trying to (very slowly) reduce the variety in new patches.

Quoted 1 line · David Sommerseth

On 31/10/11 16:30, Frederic Crozat wrote:

Quoted 6 lines

e) Consider to make use of openvpn_execve_check() or openvpn_execve() instead of the popen() - or make a popen() variant which got similar error controls to popen() as openvpn_execve*() functions. I also think that this execution should honour the --script-security setting (which is implicitly checked for with openvpn_execve())

Quoted 2 lines · Frederic Crozat

Quoted 1 line · David Sommerseth

On 31/10/11 16:30, Frederic Crozat wrote:

[...snip...]

Hey again, and thanks for this great rework! I've looked through it, and it looks a lot better now. However ...

Quoted 6 lines

Quoted 6 lines

e) Consider to make use of openvpn_execve_check() or openvpn_execve() instead of the popen() - or make a popen() variant which got similar error controls to popen() as openvpn_execve*() functions. I also think that this execution should honour the --script-security setting (which is implicitly checked for with openvpn_execve())

I like the approach of the openvpn_popen(). However, there are a few things.

(quoted diff) +3 -0

+              if (pipe(pipe_stdout) == 0) {
+		      pid = fork ();
+		      if (pid == (pid_t)0) /* child side */

(quoted diff) +6 -0

+		      else /* parent side */
+			{
+                            ret=pipe_stdout[0];
+			    close(pipe_stdout[1]);
+			}
+	      }

And:

(quoted diff) +4 -0

+  else
+    {
+      msg (M_WARN, "openvpn_popen: called with empty argv");
+    }

This should probably be M_FATAL and not M_WARN. If openvpn_popen() receives an empty argv array, it most likely is due to very bad programming. So halting execution is quite appropriate in my eyes.

In get_console_input_systemd() you do:

+ *input = 0;

I think it would be better to do: CLEAR(*input) instead. Just setting the first byte to 0, will not solve any leak possibilities. An alternative is to use memset() directly, which the CLEAR() macro uses (see basic.h:47), but we prefer to use CLEAR() over memset() directly.

Quoted 26 lines · David Sommerseth

Quoted 2 lines · Frederic Crozat

Quoted 1 line · David Sommerseth

On 31/10/11 16:30, Frederic Crozat wrote:

[...snip...]

Hey again, and thanks for this great rework! I've looked through it, and it looks a lot better now. However ...

Quoted 6 lines

Quoted 6 lines

e) Consider to make use of openvpn_execve_check() or openvpn_execve() instead of the popen() - or make a popen() variant which got similar error controls to popen() as openvpn_execve*() functions. I also think that this execution should honour the --script-security setting (which is implicitly checked for with openvpn_execve())

I like the approach of the openvpn_popen(). However, there are a few things.

misc.c +9 -0

+              if (pipe(pipe_stdout) == 0) {
+		      pid = fork ();
+		      if (pid == (pid_t)0) /* child side */
+			{
+			  close(pipe_stdout[0]);
+			  dup2(pipe_stdout[1],1);
+			  execve (cmd, argv, envp);
+			  exit (127);
+			}

misc.c openvpn_system (const char *command, const struct env_set *es, unsigned int flag

Quoted 4 lines

misc.c +2 -0

+		      else if (pid < (pid_t)0) /* fork failed */
+			;
                       ^^^^^  No error message if fork() fails?
                              And is the (pid_t) typecasting really needed?

Well, I stole the code from openvpn_execve function ;)) I'm ok to change it, but it would be better for both function to stay similar, I think.

Quoted 14 lines

misc.c +6 -0

+		      else /* parent side */
+			{
+                            ret=pipe_stdout[0];
+			    close(pipe_stdout[1]);
+			}
+	      }

And:

misc.c +4 -0

+  else
+    {
+      msg (M_WARN, "openvpn_popen: called with empty argv");
+    }

Quoted 3 lines

This should probably be M_FATAL and not M_WARN. If openvpn_popen() receives an empty argv array, it most likely is due to very bad programming. So halting execution is quite appropriate in my eyes.

Again, I stole this part from openvpn_execve, but I'll change it to M_FATAL.

Quoted 4 lines

In get_console_input_systemd() you do:

+ *input = 0;

Quoted 4 lines

I think it would be better to do: CLEAR(*input) instead. Just setting the first byte to 0, will not solve any leak possibilities. An alternative is to use memset() directly, which the CLEAR() macro uses (see basic.h:47), but we prefer to use CLEAR() over memset() directly.

done (and I've also fixed some missing spaces in various location).

See the new version attached.

Quoted 9 lines

Quoted 9 lines

In get_console_input_systemd() you do:

+ *input = 0;

I think it would be better to do: CLEAR(*input) instead. Just setting the first byte to 0, will not solve any leak possibilities. An alternative is to use memset() directly, which the CLEAR() macro uses (see basic.h:47), but we prefer to use CLEAR() over memset() directly.

Quoted 3 lines

done (and I've also fixed some missing spaces in various location).

See the new version attached.

Goodie! I presume you did some compile and sanity testing before submitting the patch, so that it won't bite our compile farm :) I think we have a F15 or F16 box (which uses systemd) in the farm, so we can enable systemd features on that box.

Quoted 27 lines · Frederic Crozat

Quoted 26 lines · David Sommerseth

Quoted 2 lines · Frederic Crozat

Quoted 1 line · David Sommerseth

On 31/10/11 16:30, Frederic Crozat wrote:

[...snip...]

Hey again, and thanks for this great rework! I've looked through it, and it looks a lot better now. However ...

Quoted 6 lines

Quoted 6 lines

e) Consider to make use of openvpn_execve_check() or openvpn_execve() instead of the popen() - or make a popen() variant which got similar error controls to popen() as openvpn_execve*() functions. I also think that this execution should honour the --script-security setting (which is implicitly checked for with openvpn_execve())

I like the approach of the openvpn_popen(). However, there are a few things.

(quoted diff) +9 -0

+              if (pipe(pipe_stdout) == 0) {
+		      pid = fork ();
+		      if (pid == (pid_t)0) /* child side */
+			{
+			  close(pipe_stdout[0]);
+			  dup2(pipe_stdout[1],1);
+			  execve (cmd, argv, envp);
+			  exit (127);
+			}

Quoted 8 lines

Review

Quoted 4 lines

(quoted diff) +2 -0

+		      else if (pid < (pid_t)0) /* fork failed */
+			;
                       ^^^^^  No error message if fork() fails?
                              And is the (pid_t) typecasting really needed?

Well, I stole the code from openvpn_execve function ;)) I'm ok to change it, but it would be better for both function to stay similar, I think.

Well, now you got me into looking deeper :) ... First of all, there might be^W^W are code paths in openvpn which isn't pretty. And we should fix up that as we find them.

I'm still in the opinion that not reporting that the execution isn't proper behaviour. Even though, it's not likely that fork() fails often, unless the system is low on memory or RLIMIT_NPROC is exceeded. But informing that the execution failed is still important for debugging.

I'd say, let's at least add an error message here, using M_ERR. That way, we'll get the errno message returned as well. I'll take care of posting a proper fix for openvpn_execve(). You may disregard my (pid_t) comment for now.

Quoted 14 lines

Quoted 14 lines

(quoted diff) +6 -0

+		      else /* parent side */
+			{
+                            ret=pipe_stdout[0];
+			    close(pipe_stdout[1]);
+			}
+	      }

And:

(quoted diff) +4 -0

+  else
+    {
+      msg (M_WARN, "openvpn_popen: called with empty argv");
+    }

Quoted 6 lines

Quoted 3 lines

This should probably be M_FATAL and not M_WARN. If openvpn_popen() receives an empty argv array, it most likely is due to very bad programming. So halting execution is quite appropriate in my eyes.

Again, I stole this part from openvpn_execve, but I'll change it to M_FATAL.

I'll send a fix for this as well, in regards to openvpn_execve()

Quoted 9 lines

Quoted 8 lines

In get_console_input_systemd() you do:

+ *input = 0;

I think it would be better to do: CLEAR(*input) instead. Just setting the first byte to 0, will not solve any leak possibilities. An alternative is to use memset() directly, which the CLEAR() macro uses (see basic.h:47), but we prefer to use CLEAR() over memset() directly.

Quoted 3 lines

done (and I've also fixed some missing spaces in various location).

See the new version attached.

Goodie! I presume you did some compile and sanity testing before submitting the patch, so that it won't bite our compile farm :) I think we have a F15 or F16 box (which uses systemd) in the farm, so we can enable systemd features on that box.

Quoted 44 lines · David Sommerseth

Quoted 29 lines · Frederic Crozat

Le vendredi 06 janvier 2012 à 17:22 +0100, David Sommerseth a écrit :

Quoted 19 lines · Frederic Crozat

Quoted 3 lines

Le lundi 31 octobre 2011 à 22:11 +0100, David Sommerseth a écrit :

Quoted 1 line · Frederic Crozat

[...snip...]

Hey again, and thanks for this great rework! I've looked through it, and it looks a lot better now. However ...

Quoted 6 lines

Quoted 6 lines

e) Consider to make use of openvpn_execve_check() or openvpn_execve() instead of the popen() - or make a popen() variant which got similar error controls to popen() as openvpn_execve*() functions. I also think that this execution should honour the --script-security setting (which is implicitly checked for with openvpn_execve())

I like the approach of the openvpn_popen(). However, there are a few things.

Quoted 7 lines

+ if (pipe(pipe_stdout) == 0) { + pid = fork
(); + if (pid == (pid_t)0) /* child side */ + { +
close(pipe_stdout[0]); + dup2(pipe_stdout[1],1); +
execve (cmd, argv, envp); + exit (127); + } + else
if (pid < (pid_t)0) /* fork failed */ + ; ^^^^^ No error
message if fork() fails? And is the (pid_t) typecasting really needed?

Quoted 3 lines

Well, I stole the code from openvpn_execve function ;)) I'm ok to change it, but it would be better for both function to stay similar, I think.

Well, now you got me into looking deeper :) ... First of all, there might be^W^W are code paths in openvpn which isn't pretty. And we should fix up that as we find them.

I'm still in the opinion that not reporting that the execution isn't proper behaviour. Even though, it's not likely that fork() fails often, unless the system is low on memory or RLIMIT_NPROC is exceeded. But informing that the execution failed is still important for debugging.

Quoted 4 lines

I'd say, let's at least add an error message here, using M_ERR. That way, we'll get the errno message returned as well. I'll take care of posting a proper fix for openvpn_execve(). You may disregard my (pid_t) comment for now.

Done.

Quoted 17 lines

Quoted 9 lines

Quoted 9 lines

(quoted diff) +1 -0

+		      else /* parent side */ +			{ +

ret=pipe_stdout[0]; + close(pipe_stdout[1]); + } +
}

And:

+ else + { + msg (M_WARN, "openvpn_popen: called with
empty argv"); + }

Quoted 7 lines

Quoted 4 lines

This should probably be M_FATAL and not M_WARN. If openvpn_popen() receives an empty argv array, it most likely is due to very bad programming. So halting execution is quite appropriate in my eyes.

Again, I stole this part from openvpn_execve, but I'll change it to M_FATAL.

Quoted 1 line

I'll send a fix for this as well, in regards to openvpn_execve()

Ok :)

Quoted 13 lines

Quoted 13 lines

Quoted 9 lines

In get_console_input_systemd() you do:

+ *input = 0;

I think it would be better to do: CLEAR(*input) instead. Just setting the first byte to 0, will not solve any leak possibilities. An alternative is to use memset() directly, which the CLEAR() macro uses (see basic.h:47), but we prefer to use CLEAR() over memset() directly.

done (and I've also fixed some missing spaces in various location).

Quoted 6 lines

Quoted 1 line

See the new version attached.

Goodie! I presume you did some compile and sanity testing before submitting the patch, so that it won't bite our compile farm :) I think we have a F15 or F16 box (which uses systemd) in the farm, so we can enable systemd features on that box.

Well, the initial patch has been shipped as part of openSUSE 12.1 and so far, I didn't got any bug report about it, so either it is working perfectly or nobody is able to connect home using their VPN and therefore can complain ;) I'd tested again attached patch and it is still working fine.