Re: [Openvpn-devel] [PATCH 35/52] build: proper selinux detection and usage

[Mr Dash Four]

However, from my experience it is not needed, people are most capable
of manage this without creating noise.
  
On the contrary, displaying meaningful error messages from configure is 
*never* a "noise". As Fabian pointed out - quite rightly - if make fails 
due to wrong/incorrect/missing dependencies then it is a bug in 
configure, no ifs, no buts, therefore displaying an error messages when 
some of the headers/libs are missing or misplaced makes perfect sense.

Re: [Openvpn-devel] [PATCH 35/52] build: proper selinux detection and usage

[Alon Bar-Lev]

On Fri, Mar 16, 2012 at 6:29 PM, Fabian Knittel
 wrote:
> So it sounds as if we're actually somewhat in agreement! You don't
> want complex, automatic detection of non-standard situations and I
> agree wholeheartedly.
> But I _do_ want basic checking to achieve clear error messages at
> configuration time, not obscure build errors far later in the build
> process. Rule of thumb: If "make" fails due to missing or wrong
> dependencies, it's a bug in configure, because it didn't detect the
> problem beforehand.

I understand your argument.
However, from my experience it is not needed, people are most capable
of manage this without creating noise.
I suggest to leave this as-is and if for some reason the audience of
openvpn is different than other project I handle, we modify it.

Alon.

Re: [Openvpn-devel] [PATCH 35/52] build: proper selinux detection and usage

[Fabian Knittel]

Hi Alon,

>> AC_CHECK_LIB([selinux], [setcon], [SELINUX_LIBS="-lselinux"])
>>
>> versus
>>
>> AC_CHECK_HEADER([selinux/selinux.h], [
>>    AC_CHECK_LIB([selinux], [setcon], [SELINUX_LIBS="-lselinux"],
>>        [AC_MSG_RESULT([SELinux library not found.])]
>>    )],  [AC_MSG_ERROR([SELinux headers not found.])]
>> )
>>
>> doesn't really qualify as "very complex" to me.
>
> This is untrue.
> As most features needs custom CFLAGs as well, it looks like:
>
> old_CFLAGS="${CFLAGS}"
> CFLAGS="${CFLAGS} ${SOME_CFLAGS}"
> AC_CHECK_HEADER([...])
> CFLAGS="${old_CFLAGS}"

I can see where this would be necessary if you wanted to "search
additional non-standard paths for the libraries and headers" (see my
last email), but it wouldn't be necessary for the simple case, where
you want to make sure that library and header are available and
compilable with the flags the user provided.

So it sounds as if we're actually somewhat in agreement! You don't
want complex, automatic detection of non-standard situations and I
agree wholeheartedly.
But I _do_ want basic checking to achieve clear error messages at
configuration time, not obscure build errors far later in the build
process. Rule of thumb: If "make" fails due to missing or wrong
dependencies, it's a bug in configure, because it didn't detect the
problem beforehand.

Cheers
Fabian

Re: [Openvpn-devel] [PATCH 35/52] build: proper selinux detection and usage

[Samuli Seppänen]

Thanks for the clarification. ACK then.

-- 
Samuli Seppänen
Community Manager
OpenVPN Technologies, Inc

irc freenode net: mattock


> I wrote this in the introduction of the patch set.
>
> There are two approaches to detecting dependencies:
>
> 1. Detect all compile time dependences- you detect headers and
> libraries, this is probably the safest way to go, but makes the code
> very complex.
>
> 2. Detect library only - you assume that if library is present, the
> functionality exists, this is what important... no need to check for
> header, most probably this exists as well. This makes the code
> simpler, in the risk of compile failure if header is missing.
>
> In all project I wrote build for (opensc, uswsusp, ntfs3g, ecryptfs)
> we took the 2nd approach, and it is fine.
>
> Alon.
>
> 2012/3/8 Samuli Seppänen :
>> Looks like a cleaner implementation than the earlier one. I take it 
>> AC_CHECK_HEADER is not anymore needed to detect selinux.h, but why exactly?
>>
>> Besides that I give this one an ACK.
>>
>> --
>> Samuli Seppänen
>> Community Manager
>> OpenVPN Technologies, Inc
>>
>> irc freenode net: mattock
>>
>>> Signed-off-by: Alon Bar-Lev 
>>> ---
>>>  configure.ac|   35 +++
>>>  src/openvpn/Makefile.am |1 +
>>>  src/openvpn/init.c  |4 ++--
>>>  src/openvpn/options.c   |6 +++---
>>>  src/openvpn/options.h   |2 +-
>>>  src/openvpn/syshead.h   |2 +-
>>>  6 files changed, 23 insertions(+), 27 deletions(-)
>>>
>>> diff --git a/configure.ac b/configure.ac
>>> index 98615c6..2388f17 100644
>>> --- a/configure.ac
>>> +++ b/configure.ac
>>> @@ -215,7 +215,7 @@ AC_ARG_ENABLE(
>>>
>>>  AC_ARG_ENABLE(
>>>   [selinux],
>>> - [AS_HELP_STRING([--disable-selinux], [disable SELinux support])],
>>> + [AS_HELP_STRING([--enable-selinux], [enable SELinux support])],
>>>   ,
>>>   [enable_selinux="no"]
>>>  )
>>> @@ -619,6 +619,13 @@ AC_CHECK_LIB(
>>>  )
>>>  AC_SUBST([SOCKETS_LIBS])
>>>
>>> +AC_CHECK_LIB(
>>> + [selinux],
>>> + [setcon],
>>> + [SELINUX_LIBS="-lselinux"]
>>> +)
>>> +AC_SUBST([SELINUX_LIBS])
>>> +
>>>  case "${with_mem_check}" in
>>>   valgrind)
>>>   AC_CHECK_HEADER(
>>> @@ -826,25 +833,6 @@ if test "${enable_crypto}" = "yes"; then
>>> fi
>>>  fi
>>>
>>> -dnl
>>> -dnl check for SELinux library and headers
>>> -dnl
>>> -if test "${enable_selinux}" = "yes"; then
>>> - AC_CHECK_HEADER(
>>> - [selinux/selinux.h],
>>> - [AC_CHECK_LIB(
>>> - [selinux],
>>> - [setcon],
>>> - [
>>> - LIBS="${LIBS} -lselinux"
>>> - AC_DEFINE(HAVE_SETCON, 1, [SELinux support])
>>> - ],
>>> - [AC_MSG_RESULT([SELinux library not found.])]
>>> - )],
>>> - [AC_MSG_ERROR([SELinux headers not found.])]
>>> - )
>>> -fi
>>> -
>>>  if test -n "${SP_PLATFORM_WINDOWS}"; then
>>>   AC_DEFINE_UNQUOTED([PATH_SEPARATOR], [''], [Path separator]) #"
>>>   AC_DEFINE_UNQUOTED([PATH_SEPARATOR_STR], [""], [Path separator]) 
>>> #"
>>> @@ -896,6 +884,12 @@ else
>>>   fi
>>>  fi
>>>
>>> +if test "${enable_selinux}" = "yes"; then
>>> + test -z "${SELINUX_LIBS}" && AC_MSG_ERROR([libselinux required but 
>>> missing])
>>> + OPTIONAL_SELINUX_LIBS="${SELINUX_LIBS}"
>>> + AC_DEFINE([ENABLE_SELINUX], [1], [SELinux support])
>>> +fi
>>> +
>>>  if test "${enable_pedantic}" = "yes"; then
>>>   enable_strict="yes"
>>>   CFLAGS="${CFLAGS} -ansi -pedantic"
>>> @@ -922,6 +916,7 @@ AC_SUBST([TAP_WIN_MIN_MAJOR])
>>>  AC_SUBST([TAP_WIN_MIN_MINOR])
>>>
>>>  AC_SUBST([OPTIONAL_DL_LIBS])
>>> +AC_SUBST([OPTIONAL_SELINUX_LIBS])
>>>
>>>  AM_CONDITIONAL([WIN32], [test "${WIN32}" = "yes"])
>>>
>>> diff --git a/src/openvpn/Makefile.am b/src/openvpn/Makefile.am
>>> index 86abd09..a3f8b3a 100644
>>> --- a/src/openvpn/Makefile.am
>>> +++ b/src/openvpn/Makefile.am
>>> @@ -97,6 +97,7 @@ openvpn_SOURCES = \
>>>   cryptoapi.h cryptoapi.c
>>>  openvpn_LDADD = \
>>>   $(SOCKETS_LIBS) \
>>> + $(OPTIONAL_SELINUX_LIBS) \
>>>   $(OPTIONAL_DL_LIBS)
>>>  if WIN32
>>>  openvpn_SOURCES += openvpn_win32_resources.rc
>>> diff --git a/src/openvpn/init.c b/src/openvpn/init.c
>>> index b8f57b2..0c995ff 100644
>>> --- a/src/openvpn/init.c
>>> +++ b/src/openvpn/init.c
>>> @@ -1038,7 +1038,7 @@ do_uid_gid_chroot (struct context *c, bool no_delay)
>>>   mstats_open(c->options.memstats_fn);
>>>  #endif
>>>
>>> -#ifdef HAVE_SETCON
>>> +#ifdef ENABLE_SELINUX
>>>/* Apply a SELinux context in order to restrict what OpenVPN can do
>>> * to _only_ what it is supposed to do after initialization is 
>>> complete
>>> * (basically just network I/O operations). Doing it after chroot
>>> @@ -2465,7 +2465,7 @@ do_option_warnings (struct context *c)
>>>  msg 

Re: [Openvpn-devel] [PATCH 35/52] build: proper selinux detection and usage

[Alon Bar-Lev]

I wrote this in the introduction of the patch set.

There are two approaches to detecting dependencies:

1. Detect all compile time dependences- you detect headers and
libraries, this is probably the safest way to go, but makes the code
very complex.

2. Detect library only - you assume that if library is present, the
functionality exists, this is what important... no need to check for
header, most probably this exists as well. This makes the code
simpler, in the risk of compile failure if header is missing.

In all project I wrote build for (opensc, uswsusp, ntfs3g, ecryptfs)
we took the 2nd approach, and it is fine.

Alon.

2012/3/8 Samuli Seppänen :
> Looks like a cleaner implementation than the earlier one. I take it 
> AC_CHECK_HEADER is not anymore needed to detect selinux.h, but why exactly?
>
> Besides that I give this one an ACK.
>
> --
> Samuli Seppänen
> Community Manager
> OpenVPN Technologies, Inc
>
> irc freenode net: mattock
>
>> Signed-off-by: Alon Bar-Lev 
>> ---
>>  configure.ac            |   35 +++
>>  src/openvpn/Makefile.am |    1 +
>>  src/openvpn/init.c      |    4 ++--
>>  src/openvpn/options.c   |    6 +++---
>>  src/openvpn/options.h   |    2 +-
>>  src/openvpn/syshead.h   |    2 +-
>>  6 files changed, 23 insertions(+), 27 deletions(-)
>>
>> diff --git a/configure.ac b/configure.ac
>> index 98615c6..2388f17 100644
>> --- a/configure.ac
>> +++ b/configure.ac
>> @@ -215,7 +215,7 @@ AC_ARG_ENABLE(
>>
>>  AC_ARG_ENABLE(
>>       [selinux],
>> -     [AS_HELP_STRING([--disable-selinux], [disable SELinux support])],
>> +     [AS_HELP_STRING([--enable-selinux], [enable SELinux support])],
>>       ,
>>       [enable_selinux="no"]
>>  )
>> @@ -619,6 +619,13 @@ AC_CHECK_LIB(
>>  )
>>  AC_SUBST([SOCKETS_LIBS])
>>
>> +AC_CHECK_LIB(
>> +     [selinux],
>> +     [setcon],
>> +     [SELINUX_LIBS="-lselinux"]
>> +)
>> +AC_SUBST([SELINUX_LIBS])
>> +
>>  case "${with_mem_check}" in
>>       valgrind)
>>               AC_CHECK_HEADER(
>> @@ -826,25 +833,6 @@ if test "${enable_crypto}" = "yes"; then
>>     fi
>>  fi
>>
>> -dnl
>> -dnl check for SELinux library and headers
>> -dnl
>> -if test "${enable_selinux}" = "yes"; then
>> -     AC_CHECK_HEADER(
>> -             [selinux/selinux.h],
>> -             [AC_CHECK_LIB(
>> -                     [selinux],
>> -                     [setcon],
>> -                     [
>> -                             LIBS="${LIBS} -lselinux"
>> -                             AC_DEFINE(HAVE_SETCON, 1, [SELinux support])
>> -                     ],
>> -                     [AC_MSG_RESULT([SELinux library not found.])]
>> -             )],
>> -             [AC_MSG_ERROR([SELinux headers not found.])]
>> -     )
>> -fi
>> -
>>  if test -n "${SP_PLATFORM_WINDOWS}"; then
>>       AC_DEFINE_UNQUOTED([PATH_SEPARATOR], [''], [Path separator]) #"
>>       AC_DEFINE_UNQUOTED([PATH_SEPARATOR_STR], [""], [Path separator]) #"
>> @@ -896,6 +884,12 @@ else
>>       fi
>>  fi
>>
>> +if test "${enable_selinux}" = "yes"; then
>> +     test -z "${SELINUX_LIBS}" && AC_MSG_ERROR([libselinux required but 
>> missing])
>> +     OPTIONAL_SELINUX_LIBS="${SELINUX_LIBS}"
>> +     AC_DEFINE([ENABLE_SELINUX], [1], [SELinux support])
>> +fi
>> +
>>  if test "${enable_pedantic}" = "yes"; then
>>       enable_strict="yes"
>>       CFLAGS="${CFLAGS} -ansi -pedantic"
>> @@ -922,6 +916,7 @@ AC_SUBST([TAP_WIN_MIN_MAJOR])
>>  AC_SUBST([TAP_WIN_MIN_MINOR])
>>
>>  AC_SUBST([OPTIONAL_DL_LIBS])
>> +AC_SUBST([OPTIONAL_SELINUX_LIBS])
>>
>>  AM_CONDITIONAL([WIN32], [test "${WIN32}" = "yes"])
>>
>> diff --git a/src/openvpn/Makefile.am b/src/openvpn/Makefile.am
>> index 86abd09..a3f8b3a 100644
>> --- a/src/openvpn/Makefile.am
>> +++ b/src/openvpn/Makefile.am
>> @@ -97,6 +97,7 @@ openvpn_SOURCES = \
>>       cryptoapi.h cryptoapi.c
>>  openvpn_LDADD = \
>>       $(SOCKETS_LIBS) \
>> +     $(OPTIONAL_SELINUX_LIBS) \
>>       $(OPTIONAL_DL_LIBS)
>>  if WIN32
>>  openvpn_SOURCES += openvpn_win32_resources.rc
>> diff --git a/src/openvpn/init.c b/src/openvpn/init.c
>> index b8f57b2..0c995ff 100644
>> --- a/src/openvpn/init.c
>> +++ b/src/openvpn/init.c
>> @@ -1038,7 +1038,7 @@ do_uid_gid_chroot (struct context *c, bool no_delay)
>>       mstats_open(c->options.memstats_fn);
>>  #endif
>>
>> -#ifdef HAVE_SETCON
>> +#ifdef ENABLE_SELINUX
>>        /* Apply a SELinux context in order to restrict what OpenVPN can do
>>         * to _only_ what it is supposed to do after initialization is 
>> complete
>>         * (basically just network I/O operations). Doing it after chroot
>> @@ -2465,7 +2465,7 @@ do_option_warnings (struct context *c)
>>      msg (M_WARN, "WARNING: --ping should normally be used with 
>> --ping-restart or --ping-exit");
>>
>>    if (o->username || o->groupname || o->chroot_dir
>> -#ifdef HAVE_SETCON
>> +#ifdef ENABLE_SELINUX
>>        || o->selinux_context
>>  #endif
>>        )
>> diff --git a/src/openvpn/options.c 

Re: [Openvpn-devel] [PATCH 35/52] build: proper selinux detection and usage

[Samuli Seppänen]

Looks like a cleaner implementation than the earlier one. I take it 
AC_CHECK_HEADER is not anymore needed to detect selinux.h, but why exactly?

Besides that I give this one an ACK.

-- 
Samuli Seppänen
Community Manager
OpenVPN Technologies, Inc

irc freenode net: mattock

> Signed-off-by: Alon Bar-Lev 
> ---
>  configure.ac|   35 +++
>  src/openvpn/Makefile.am |1 +
>  src/openvpn/init.c  |4 ++--
>  src/openvpn/options.c   |6 +++---
>  src/openvpn/options.h   |2 +-
>  src/openvpn/syshead.h   |2 +-
>  6 files changed, 23 insertions(+), 27 deletions(-)
>
> diff --git a/configure.ac b/configure.ac
> index 98615c6..2388f17 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -215,7 +215,7 @@ AC_ARG_ENABLE(
>  
>  AC_ARG_ENABLE(
>   [selinux],
> - [AS_HELP_STRING([--disable-selinux], [disable SELinux support])],
> + [AS_HELP_STRING([--enable-selinux], [enable SELinux support])],
>   ,
>   [enable_selinux="no"]
>  )
> @@ -619,6 +619,13 @@ AC_CHECK_LIB(
>  )
>  AC_SUBST([SOCKETS_LIBS])
>  
> +AC_CHECK_LIB(
> + [selinux],
> + [setcon],
> + [SELINUX_LIBS="-lselinux"]
> +)
> +AC_SUBST([SELINUX_LIBS])
> +
>  case "${with_mem_check}" in
>   valgrind)
>   AC_CHECK_HEADER(
> @@ -826,25 +833,6 @@ if test "${enable_crypto}" = "yes"; then
> fi
>  fi
>  
> -dnl
> -dnl check for SELinux library and headers
> -dnl
> -if test "${enable_selinux}" = "yes"; then
> - AC_CHECK_HEADER(
> - [selinux/selinux.h],
> - [AC_CHECK_LIB(
> - [selinux],
> - [setcon],
> - [
> - LIBS="${LIBS} -lselinux"
> - AC_DEFINE(HAVE_SETCON, 1, [SELinux support])
> - ],
> - [AC_MSG_RESULT([SELinux library not found.])]
> - )],
> - [AC_MSG_ERROR([SELinux headers not found.])]
> - )
> -fi
> -
>  if test -n "${SP_PLATFORM_WINDOWS}"; then
>   AC_DEFINE_UNQUOTED([PATH_SEPARATOR], [''], [Path separator]) #"
>   AC_DEFINE_UNQUOTED([PATH_SEPARATOR_STR], [""], [Path separator]) #"
> @@ -896,6 +884,12 @@ else
>   fi
>  fi
>  
> +if test "${enable_selinux}" = "yes"; then
> + test -z "${SELINUX_LIBS}" && AC_MSG_ERROR([libselinux required but 
> missing])
> + OPTIONAL_SELINUX_LIBS="${SELINUX_LIBS}"
> + AC_DEFINE([ENABLE_SELINUX], [1], [SELinux support])
> +fi
> +
>  if test "${enable_pedantic}" = "yes"; then
>   enable_strict="yes"
>   CFLAGS="${CFLAGS} -ansi -pedantic"
> @@ -922,6 +916,7 @@ AC_SUBST([TAP_WIN_MIN_MAJOR])
>  AC_SUBST([TAP_WIN_MIN_MINOR])
>  
>  AC_SUBST([OPTIONAL_DL_LIBS])
> +AC_SUBST([OPTIONAL_SELINUX_LIBS])
>  
>  AM_CONDITIONAL([WIN32], [test "${WIN32}" = "yes"])
>  
> diff --git a/src/openvpn/Makefile.am b/src/openvpn/Makefile.am
> index 86abd09..a3f8b3a 100644
> --- a/src/openvpn/Makefile.am
> +++ b/src/openvpn/Makefile.am
> @@ -97,6 +97,7 @@ openvpn_SOURCES = \
>   cryptoapi.h cryptoapi.c
>  openvpn_LDADD = \
>   $(SOCKETS_LIBS) \
> + $(OPTIONAL_SELINUX_LIBS) \
>   $(OPTIONAL_DL_LIBS)
>  if WIN32
>  openvpn_SOURCES += openvpn_win32_resources.rc
> diff --git a/src/openvpn/init.c b/src/openvpn/init.c
> index b8f57b2..0c995ff 100644
> --- a/src/openvpn/init.c
> +++ b/src/openvpn/init.c
> @@ -1038,7 +1038,7 @@ do_uid_gid_chroot (struct context *c, bool no_delay)
>   mstats_open(c->options.memstats_fn);
>  #endif
>  
> -#ifdef HAVE_SETCON
> +#ifdef ENABLE_SELINUX
>/* Apply a SELinux context in order to restrict what OpenVPN can do
> * to _only_ what it is supposed to do after initialization is complete
> * (basically just network I/O operations). Doing it after chroot
> @@ -2465,7 +2465,7 @@ do_option_warnings (struct context *c)
>  msg (M_WARN, "WARNING: --ping should normally be used with 
> --ping-restart or --ping-exit");
>  
>if (o->username || o->groupname || o->chroot_dir
> -#ifdef HAVE_SETCON
> +#ifdef ENABLE_SELINUX
>|| o->selinux_context
>  #endif
>)
> diff --git a/src/openvpn/options.c b/src/openvpn/options.c
> index d7f848e..4e95b83 100644
> --- a/src/openvpn/options.c
> +++ b/src/openvpn/options.c
> @@ -316,7 +316,7 @@ static const char usage_message[] =
>"--user user : Set UID to user after initialization.\n"
>"--group group   : Set GID to group after initialization.\n"
>"--chroot dir: Chroot to this directory after initialization.\n"
> -#ifdef HAVE_SETCON
> +#ifdef ENABLE_SELINUX
>"--setcon context: Apply this SELinux context after initialization.\n"
>  #endif
>"--cd dir: Change to this directory before initialization.\n"
> @@ -1477,7 +1477,7 @@ show_settings (const struct options *o)
>SHOW_STR (groupname);
>SHOW_STR (chroot_dir);
>SHOW_STR (cd_dir);
> -#ifdef HAVE_SETCON
> +#ifdef ENABLE_SELINUX
>SHOW_STR (selinux_context);
>