Re: [Openvpn-devel] [PATCH] Change the default --tmp-dir path to a more suitable path

[Carsten Krüger]

Hello David,

> On Windows, it will look up %TEMP% and %TMP% first, and if that doesn't give 
> any clues, it
> will fallback to C:\WINDOWS\Temp in the end.

I think that's not the right location.

Use
http://msdn.microsoft.com/en-us/library/system.environment.getfolderpath.aspx

with this constant
CSIDL_LOCAL_APPDATA to locate system/language independant:
"C:\Documents and Settings\username\Local Settings\Application Data"
http://msdn.microsoft.com/en-us/library/bb762494.aspx
and than create OpenVPN\temp at this location.

Windows has no special temp location that is "allowed" from MS.

greetings
Carsten

Re: [Openvpn-devel] [PATCH] Change the default --tmp-dir path to a more suitable path

[Karl O. Pinc]

On 04/07/2011 07:51:55 AM, David Sommerseth wrote:
> [resend copy to openvpn-devel list as well]

> I checked for the $TMPDIR variable on CentOS 5.5, Fedora 14 and 
> Gentoo
> installations.  And $TMPDIR didn't show up at all, hence I thought
> this was
> not a really useful option.  However, I see from the wikipage that
> this is
> supposed to be part of SuS.  But it seems not to be respected in 
> Linux
> at
> least. 

FYI After poking about the man pages of various Un*xs I find:

login(1) does not have to set $TMPDIR, although the whole
login process can be configured to do so there's no 
requirement that $TMPDIR exist.



Karl 
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein

Re: [Openvpn-devel] [PATCH] Change the default --tmp-dir path to a more suitable path

[Gisle Vanem]

"Alon Bar-Lev"  wrote:


On Thu, Apr 7, 2011 at 3:48 PM, David Sommerseth  wrote:

Good idea! I wasn't aware of that one. I'll fix this. I will anyway
choose to fallback to C:\WINDOWS\Temp if %SystemRoot% is not found, even
though I believe this is most likely not something which should happen.


Can't happen, Windows will not boot.


But a user can still do "set SystemRoot=" after boot. But a lot of problems will 
arise though. A quick test; notepad.exe doesn't start for instance.


--gv

Re: [Openvpn-devel] [PATCH] Change the default --tmp-dir path to a more suitable path

[David Sommerseth]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

On 07/04/11 14:58, Jan Just Keijser wrote:
> David Sommerseth wrote:
>> -BEGIN PGP SIGNED MESSAGE-
>> Hash: SHA1
>>
>> [resend copy to openvpn-devel list as well]
>>
>> On 07/04/11 14:15, Alon Bar-Lev wrote:
>>  
>>> On Wed, Apr 6, 2011 at 7:10 PM, David Sommerseth  wrote:
>>>
 In commit 4e1cc5f6dda22e9 the create_temp_filename() function was
 reviewed and hardened, which in the end renamed this function to
 create_temp_file() in commit 495e3cec5d156.

 With these changes it became more evident that OpenVPN needs a directory
 where it can create temporary files.  The create_temp_file() will create
 such files f.ex. if --client-connect or --plugin which makes use of
 the OPENVPN_PLUGIN_AUTH_USER_PASS_VERIFY hook, such as
 openvpn-auth-pam.so.

 When this happens, OpenVPN will normally create these files in the
 directory
 OpenVPN was started.  In many cases, this will fail due to restricted
 access.
 By using --tmp-dir and pointing it to a directory writeable to the user
 running OpenVPN, it works again.

 This patch makes OpenVPN use a more suitable temproary directory by
 default,
 instead of the current working directory.  On non-Windows platforms this
 default value is set to '/tmp', but can be modified at compile-time by
 running ./configure --with-tmp-dir-path=.  On Windows, it
 will look up %TEMP% and %TMP% first, and if that doesn't give any
 clues, it
 will fallback to C:\WINDOWS\Temp in the end.
   
>>> I don't understand,
>>> if you use windows environment variables, then why not do the same on Unix?
>>> You have the standard TMPDIR [1] variable, and fallback to /tmp.
>>> 
>>
>> I checked for the $TMPDIR variable on CentOS 5.5, Fedora 14 and Gentoo
>> installations.  And $TMPDIR didn't show up at all, hence I thought this was
>> not a really useful option.  However, I see from the wikipage that this is
>> supposed to be part of SuS.  But it seems not to be respected in Linux at
>> least.  But fair point.  I can add a similar logic to non-Windows
>> installations as well, again with a hard-coded fallback.
>>
>>  
>>> Also, at Windows you should go into %SystemRoot%\Temp using
>>> ExpandEnvironmentVariable() and not hardcode C:\
>>> 
>>
>> Good idea!  I wasn't aware of that one.  I'll fix this.  I will anyway
>> choose to fallback to C:\WINDOWS\Temp if %SystemRoot% is not found, even
>> though I believe this is most likely not something which should happen.
>>
>> I'll implement the suggested change for autotools as well and propose an
>> additional patch to cover your comments.
>>
>>   
> 
> err , didn't we agree to use %TEMP% on windows? AFAIK this env var is
> always there...

%TEMP% and then %TMP% is checked.  Alon's suggestion is to expand the
default hardcoded C:\WINDOWS\Temp to use %SystemRoot%\Temp if %TEMP% and
%TMP% fails.  I like that approach, and will implement that, with
C:\WINDOWS\Temp as the final fallback if %SystemRoot% fails.

> And yes, on my Linux boxen there is no $TMPDIR, but I'd like to be able to
> overrule the temporary directory anyways
> So as far as I am concerned the Linux version of the patch is perfect.

Good!  I'll implement $TMPDIR anyway, just to have that covered, which is
more inline with SuS anyway [1].  Fallback will be as it is now anyway.


kind regards,

David Sommerseth


[1]

-BEGIN PGP SIGNATURE-
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iEYEARECAAYFAk2dty0ACgkQDC186MBRfroF6gCbB+Xoqu7sqYYLBDpsytH6umnD
GoEAn2hjJR5kqpTLDUsAbrS4dJl5yPs6
=yEiA
-END PGP SIGNATURE-

Re: [Openvpn-devel] [PATCH] Change the default --tmp-dir path to a more suitable path

[David Sommerseth]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

[resend copy to openvpn-devel list as well]

On 07/04/11 14:15, Alon Bar-Lev wrote:
> On Wed, Apr 6, 2011 at 7:10 PM, David Sommerseth  wrote:
>> In commit 4e1cc5f6dda22e9 the create_temp_filename() function was
>> reviewed and hardened, which in the end renamed this function to
>> create_temp_file() in commit 495e3cec5d156.
>>
>> With these changes it became more evident that OpenVPN needs a directory
>> where it can create temporary files.  The create_temp_file() will create
>> such files f.ex. if --client-connect or --plugin which makes use of
>> the OPENVPN_PLUGIN_AUTH_USER_PASS_VERIFY hook, such as openvpn-auth-pam.so.
>>
>> When this happens, OpenVPN will normally create these files in the directory
>> OpenVPN was started.  In many cases, this will fail due to restricted access.
>> By using --tmp-dir and pointing it to a directory writeable to the user
>> running OpenVPN, it works again.
>>
>> This patch makes OpenVPN use a more suitable temproary directory by default,
>> instead of the current working directory.  On non-Windows platforms this
>> default value is set to '/tmp', but can be modified at compile-time by
>> running ./configure --with-tmp-dir-path=.  On Windows, it
>> will look up %TEMP% and %TMP% first, and if that doesn't give any clues, it
>> will fallback to C:\WINDOWS\Temp in the end.
>
> I don't understand,
> if you use windows environment variables, then why not do the same on Unix?
> You have the standard TMPDIR [1] variable, and fallback to /tmp.

I checked for the $TMPDIR variable on CentOS 5.5, Fedora 14 and Gentoo
installations.  And $TMPDIR didn't show up at all, hence I thought this was
not a really useful option.  However, I see from the wikipage that this is
supposed to be part of SuS.  But it seems not to be respected in Linux at
least.  But fair point.  I can add a similar logic to non-Windows
installations as well, again with a hard-coded fallback.

> Also, at Windows you should go into %SystemRoot%\Temp using
> ExpandEnvironmentVariable() and not hardcode C:\

Good idea!  I wasn't aware of that one.  I'll fix this.  I will anyway
choose to fallback to C:\WINDOWS\Temp if %SystemRoot% is not found, even
though I believe this is most likely not something which should happen.

I'll implement the suggested change for autotools as well and propose an
additional patch to cover your comments.


Thanks a lot for your review!


kind regards,

David Sommerseth
-BEGIN PGP SIGNATURE-
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iEYEARECAAYFAk2ds2sACgkQDC186MBRfrqCGwCcDBv5jSlrgSbBG3CsPDFVuehO
ME8AnRFDvApIJEmO18inLiw3OoJfFGNW
=RKXA
-END PGP SIGNATURE-

Re: [Openvpn-devel] [PATCH] Change the default --tmp-dir path to a more suitable path

[Alon Bar-Lev]

On Wed, Apr 6, 2011 at 7:10 PM, David Sommerseth  wrote:
> In commit 4e1cc5f6dda22e9 the create_temp_filename() function was
> reviewed and hardened, which in the end renamed this function to
> create_temp_file() in commit 495e3cec5d156.
>
> With these changes it became more evident that OpenVPN needs a directory
> where it can create temporary files.  The create_temp_file() will create
> such files f.ex. if --client-connect or --plugin which makes use of
> the OPENVPN_PLUGIN_AUTH_USER_PASS_VERIFY hook, such as openvpn-auth-pam.so.
>
> When this happens, OpenVPN will normally create these files in the directory
> OpenVPN was started.  In many cases, this will fail due to restricted access.
> By using --tmp-dir and pointing it to a directory writeable to the user
> running OpenVPN, it works again.
>
> This patch makes OpenVPN use a more suitable temproary directory by default,
> instead of the current working directory.  On non-Windows platforms this
> default value is set to '/tmp', but can be modified at compile-time by
> running ./configure --with-tmp-dir-path=.  On Windows, it
> will look up %TEMP% and %TMP% first, and if that doesn't give any clues, it
> will fallback to C:\WINDOWS\Temp in the end.

I don't understand,
if you use windows environment variables, then why not do the same on Unix?
You have the standard TMPDIR [1] variable, and fallback to /tmp.

Also, at Windows you should go into %SystemRoot%\Temp using
ExpandEnvironmentVariable() and not hardcode C:\

And if you have a sane fallback ot system location, why you need to
add a compile time option to override?

[1] http://en.wikipedia.org/wiki/TMPDIR

> +AC_ARG_WITH(tmp-dir-path,
> +   [  --with-tmp-dir-path=PATH  Default tmp-dir to use when not configured 
> (unset defaults to /tmp)],
> +   [TMPDIRPATH="$withval"]
> +)
> +



> +dnl set the default temporary directory
> +if test -z "$TMPDIRPATH"; then
> +   TMPDIRPATH="/tmp"
> +fi
> +AC_DEFINE_UNQUOTED(DEFAULT_TMPDIR, "$TMPDIRPATH", [Default --tmp-dir])
> +

Both should be something like:
---
AC_ARG_WITH(
   [tmp-dir-path],
   [AS_HELP_STRING([--with-tmp-dir-path=PATH], [Default tmp-dir to use
when not configured (unset defaults to /tmp)])],
   [TMPDIRPATH="$withval"],
   [
  case "${host}" in
 *-mingw*) TMPDIRPATH="%SystemRoot%/Temp" ;;
 *) TMPDIRPATH="/tmp" ;;
  esac
)
AC_DEFINE_UNQUOTED([DEFAULT_TMPDIR], [${TMPDIRPATH}], [Default --tmp-dir])
---

Alon.

[Openvpn-devel] [PATCH] Change the default --tmp-dir path to a more suitable path

[David Sommerseth]

In commit 4e1cc5f6dda22e9 the create_temp_filename() function was
reviewed and hardened, which in the end renamed this function to
create_temp_file() in commit 495e3cec5d156.

With these changes it became more evident that OpenVPN needs a directory
where it can create temporary files.  The create_temp_file() will create
such files f.ex. if --client-connect or --plugin which makes use of
the OPENVPN_PLUGIN_AUTH_USER_PASS_VERIFY hook, such as openvpn-auth-pam.so.

When this happens, OpenVPN will normally create these files in the directory
OpenVPN was started.  In many cases, this will fail due to restricted access.
By using --tmp-dir and pointing it to a directory writeable to the user
running OpenVPN, it works again.

This patch makes OpenVPN use a more suitable temproary directory by default,
instead of the current working directory.  On non-Windows platforms this
default value is set to '/tmp', but can be modified at compile-time by
running ./configure --with-tmp-dir-path=.  On Windows, it
will look up %TEMP% and %TMP% first, and if that doesn't give any clues, it
will fallback to C:\WINDOWS\Temp in the end.

In any cases, this default value can be overridden in the configuration
file by using the --tmp-dir option, as before.

To check what the default is at runime, you can see this easily by doing
this:

  $ ./openvpn --verb 4 --dev tun | grep tmp_dir

Signed-off-by: David Sommerseth 
Tested-by: Jan Just Keijser 
---
 configure.ac|   11 +++
 options.c   |   15 +++
 win/config.h.in |3 +++
 win32.c |   19 +++
 win32.h |3 +++
 5 files changed, 47 insertions(+), 4 deletions(-)

diff --git a/configure.ac b/configure.ac
index e0847bc..a9f022e 100644
--- a/configure.ac
+++ b/configure.ac
@@ -279,6 +279,11 @@ AC_ARG_WITH(netstat-path,
 )
 AC_DEFINE_UNQUOTED(NETSTAT_PATH, "$NETSTAT", [Path to netstat tool])

+AC_ARG_WITH(tmp-dir-path,
+   [  --with-tmp-dir-path=PATH  Default tmp-dir to use when not configured 
(unset defaults to /tmp)],
+   [TMPDIRPATH="$withval"]
+)
+
 AC_ARG_WITH(mem-check,
[  --with-mem-check=TYPE  Build with debug memory checking, TYPE = dmalloc 
or valgrind],
[MEMCHECK="$withval"]
@@ -566,6 +571,12 @@ LDFLAGS="$LDFLAGS -Wl,--fatal-warnings"
 AC_CHECK_FUNC(epoll_create, AC_DEFINE(HAVE_EPOLL_CREATE, 1, [epoll_create 
function is defined]))
 LDFLAGS="$OLDLDFLAGS"

+dnl set the default temporary directory
+if test -z "$TMPDIRPATH"; then
+   TMPDIRPATH="/tmp"
+fi
+AC_DEFINE_UNQUOTED(DEFAULT_TMPDIR, "$TMPDIRPATH", [Default --tmp-dir])
+
 dnl
 dnl check for valgrind tool
 dnl
diff --git a/options.c b/options.c
index 36e8393..0b91657 100644
--- a/options.c
+++ b/options.c
@@ -766,11 +766,20 @@ init_options (struct options *o, const bool init_gc)
 #ifdef ENABLE_X509ALTUSERNAME
   o->x509_username_field = X509_USERNAME_FIELD_DEFAULT;
 #endif
-#endif
-#endif
+#endif /* USE_SSL */
+#endif /* USE_CRYPTO */
 #ifdef ENABLE_PKCS11
   o->pkcs11_pin_cache_period = -1;
 #endif /* ENABLE_PKCS11 */
+
+  /* Set default --tmp-dir */
+#ifdef WIN32
+  /* On Windows, find temp dir via enviroment variables */
+  o->tmp_dir = win_get_tempdir();
+#else
+  /* Non-windows platforms have this value defined via ./configure */
+  o->tmp_dir = DEFAULT_TMPDIR;
+#endif /* WIN32 */
 }

 void
@@ -1916,8 +1925,6 @@ options_postprocess_verify_ce (const struct options 
*options, const struct conne
msg (M_USAGE, "--client-connect requires --mode server");
   if (options->client_disconnect_script)
msg (M_USAGE, "--client-disconnect requires --mode server");
-  if (options->tmp_dir)
-   msg (M_USAGE, "--tmp-dir requires --mode server");
   if (options->client_config_dir || options->ccd_exclusive)
msg (M_USAGE, "--client-config-dir/--ccd-exclusive requires --mode 
server");
   if (options->enable_c2c)
diff --git a/win/config.h.in b/win/config.h.in
index fb349d0..abf71d2 100644
--- a/win/config.h.in
+++ b/win/config.h.in
@@ -286,6 +286,9 @@ typedef unsigned long in_addr_t;
 /* Route command */
 #define ROUTE_PATH "route"

+/* Default temporary directory, if not found via getenv() */
+#define DEFAULT_TMPDIR "C:\\WINDOWS\\Temp"   /* FIXME: Should be configurable 
*/
+
 #ifdef _MSC_VER
 /* MSVC++ hacks */
 #pragma warning(disable:4244) // conversion from 'foo' to 'bar', possible loss 
of data
diff --git a/win32.c b/win32.c
index 7c9901e..c9eb184 100644
--- a/win32.c
+++ b/win32.c
@@ -1093,4 +1093,23 @@ env_set_add_win32 (struct env_set *es)
   set_win_sys_path (DEFAULT_WIN_SYS_PATH, es);
 }

+
+const char *
+win_get_tempdir()
+{
+  static char *tmpdir = NULL;
+
+  /* Try to use %TEMP% or %TMP% */
+  tmpdir = getenv("TEMP");
+  if( !tmpdir ) {
+tmpdir = getenv("TMP");
+  }
+  if( !tmpdir ) {
+/* Warn if we're using a hard coded path */
+msg (M_WARN, "Could not find %TEMP% or %TMP% environment variables.  "
+ "Falling back to %s.  Consider to use