[iptables,v3] iptables: accept lock file name at runtime

allow users to override at runtime the lock file to use through the
XTABLES_LOCKFILE environment variable.

It allows to use iptables when the user has granted enough
capabilities (e.g. a user+network namespace) to configure the network
but that lacks access to the XT_LOCK_NAME (by default placed under
/run).

$ XTABLES_LOCKFILE=/tmp/xtables unshare -rn iptables ...

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
---
 configure.ac           |  1 +
 iptables/iptables.8.in |  8 ++++++++
 iptables/xshared.c     | 11 ++++++++---
 3 files changed, 17 insertions(+), 3 deletions(-)

On Fri, Jul 17, 2020 at 10:39:40AM +0200, Giuseppe Scrivano wrote:
> allow users to override at runtime the lock file to use through the
> XTABLES_LOCKFILE environment variable.
> 
> It allows to use iptables when the user has granted enough
> capabilities (e.g. a user+network namespace) to configure the network
> but that lacks access to the XT_LOCK_NAME (by default placed under
> /run).
> 
> $ XTABLES_LOCKFILE=/tmp/xtables unshare -rn iptables ...
> 
> Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
> ---
>  configure.ac           |  1 +
>  iptables/iptables.8.in |  8 ++++++++
>  iptables/xshared.c     | 11 ++++++++---
>  3 files changed, 17 insertions(+), 3 deletions(-)
> 
> diff --git a/configure.ac b/configure.ac
> index 31a8bb26..d37752a2 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -219,6 +219,7 @@ AC_SUBST([libxtables_vmajor])
>  
>  AC_DEFINE_UNQUOTED([XT_LOCK_NAME], "${xt_lock_name}",
>  	[Location of the iptables lock file])
> +AC_SUBST([XT_LOCK_NAME], "${xt_lock_name}")
>  
>  AC_CONFIG_FILES([Makefile extensions/GNUmakefile include/Makefile
>  	iptables/Makefile iptables/xtables.pc
> diff --git a/iptables/iptables.8.in b/iptables/iptables.8.in
> index 054564b3..999cf339 100644
> --- a/iptables/iptables.8.in
> +++ b/iptables/iptables.8.in
> @@ -397,6 +397,14 @@ corresponding to that rule's position in the chain.
>  \fB\-\-modprobe=\fP\fIcommand\fP
>  When adding or inserting rules into a chain, use \fIcommand\fP
>  to load any necessary modules (targets, match extensions, etc).
> +
> +.SH LOCK FILE
> +iptables uses the \fI@XT_LOCK_NAME@\fP file to take an exclusive lock at
> +launch.
> +
> +The \fBXTABLES_LOCKFILE\fP environment variable can be used to override
> +the default setting.
> +
>  .SH MATCH AND TARGET EXTENSIONS
>  .PP
>  iptables can use extended packet matching and target modules.
> diff --git a/iptables/xshared.c b/iptables/xshared.c
> index c1d1371a..7d97637f 100644
> --- a/iptables/xshared.c
> +++ b/iptables/xshared.c
> @@ -249,15 +249,20 @@ void xs_init_match(struct xtables_match *match)
>  static int xtables_lock(int wait, struct timeval *wait_interval)
>  {
>  	struct timeval time_left, wait_time;
> +	const char *lock_file;
>  	int fd, i = 0;
>  
>  	time_left.tv_sec = wait;
>  	time_left.tv_usec = 0;
>  
> -	fd = open(XT_LOCK_NAME, O_CREAT, 0600);
> +	lock_file = getenv("XTABLES_LOCKFILE");
> +	if (lock_file == NULL || lock_file[0] == '\0')

Probably remove the check for lock_file[0] == '\0'

Or is this intentional?

git grep getenv in iptables does not show any similar handling for
getenv().

Thanks.

Hi Pablo,

Pablo Neira Ayuso <pablo@netfilter.org> writes:

> Probably remove the check for lock_file[0] == '\0'
>
> Or is this intentional?

I've added it intentionally as I think it is safer to ignore an empty
string.  The programs I've checked, GNU coreutils and GNU grep, have the
same check.

The empty string will likely fail on open(2), at least on tmpfs
it does with ENOENT.  If you want though, I can drop the check.

> git grep getenv in iptables does not show any similar handling for
> getenv().

not sure if that is the desired behaviour as it will be processed as
an empty string.

Regards,
Giuseppe

On Fri, Jul 17, 2020 at 12:12:10PM +0200, Giuseppe Scrivano wrote:
> Hi Pablo,
> 
> Pablo Neira Ayuso <pablo@netfilter.org> writes:
> 
> > Probably remove the check for lock_file[0] == '\0'
> >
> > Or is this intentional?
> 
> I've added it intentionally as I think it is safer to ignore an empty
> string.  The programs I've checked, GNU coreutils and GNU grep, have the
> same check.

Thanks for explaining.

> The empty string will likely fail on open(2), at least on tmpfs
> it does with ENOENT.  If you want though, I can drop the check.

No worries, patch description did not explain this and quick git grep
on iptables was showing no check for \0 in other spots in the tree. No
need to drop the check.

On Fri, Jul 17, 2020 at 10:39:40AM +0200, Giuseppe Scrivano wrote:
> allow users to override at runtime the lock file to use through the
> XTABLES_LOCKFILE environment variable.
> 
> It allows to use iptables when the user has granted enough
> capabilities (e.g. a user+network namespace) to configure the network
> but that lacks access to the XT_LOCK_NAME (by default placed under
> /run).
> 
> $ XTABLES_LOCKFILE=/tmp/xtables unshare -rn iptables ...

Applied, thanks.