[bionic/18.04 only - fixed in newer ubuntu] iptables-restore is missing -w option

Seems like the point release would be a reasonable SRU.

Mark

1.6.2 in 18.04 would be a life saver as I have hit this issue and the only resolution is to compile iptables myself. I can verify that 1.6.2 resolves this issue and I havn't had any other problems on 18.04 as far as other tools calling 1.6.2

Look like a potential patchset to backport without having to bump version in stable release:

6e2e169eb iptables: remove duplicated argument parsing code
24f81746 xshared: do not lock again and again if "-w" option is not specified
72bb3dbf0 xshared: using the blocking file lock request when we wait indefinitely
999eaa24 iptables-restore: support acquiring the lock.
65801d02 iptables-restore.8: document -w/-W options
21ba5b38 ip{,6}tables-restore: Don't accept wait-interval without wait

I'll test it, and then will talk w/ the SRU verification team to see if eligible for SRU.

Wearing my SRU-hat here, I think we need to consider a few things:
 * Generally for SRUs we prefer not to pull in new upstream releases if there is no need. So of course, if possible, cherry-picking fixes is preferred.
 * But on the other hand, if the number of changes that need to be performed to get the functionality added (and bug fixed) is too high, then we start getting into the territory of regression-risk if the cherry-picks are incomplete or buggy.

So first thing I'd like to know is how big of a changeset would be needed to get this into the current focal iptables version. Once we have that info, another thing that might be good to do is to contact the Ubuntu Security Team for opinion - this package is managed by their team and I'd like to hear their opinion about which approach they prefer (for maintenance purposes). A review by them of the cherry-picks would be welcome as well.

Generally we try not to introduce new features for stable releases, but we might make an exception here. But I'd certainly like for us to think a bit about our steps forward.

+1 for a backport, I don't think 1.6.2 is suitable for an SRU, specifically about one change I noticed with test packages that I think can break existing firewall scripts.

The locking code is shared between tools, so in 1.6.2, not only do we get iptables-{save,restore} with -w support, but iptables itself changes behavior.

When a lock is held, this is the current behavior in bionic:
root@b1-iptables-restore-wait-lock:~# time iptables -L
Another app is currently holding the xtables lock; still -9s 0us time ahead to have a chance to grab the lock...
Another app is currently holding the xtables lock; still -19s 0us time ahead to have a chance to grab the lock...
Another app is currently holding the xtables lock; still -29s 0us time ahead to have a chance to grab the lock...

Two things:
- there is an implied -w with no value, meaning infinite wait. Perhaps surprising, perhaps not.
- the time countdown is negative (bug)

In 1.6.2 and later, we have:
root@b1-iptables-restore-wait-lock:~# time iptables -L
Another app is currently holding the xtables lock. Perhaps you want to use the -w option?

real 0m0.003s

Focal:
root@f1:~# time iptables -L
Another app is currently holding the xtables lock. Perhaps you want to use the -w option?

real 0m0.003s
user 0m0.004s
sys 0m0.000s
root@f1:~# iptables --version
iptables v1.8.4 (legacy)

It exits immediately. I can see this breaking existing firewall scripts that were up to now relying on the lock even without knowing it. They would be working with the bionic version, perhaps hitting the lock a few times, but with the updated version, as soon as the lock is hit, iptables exits. This means the script would have to be changed to add -w [n] to all iptables invocations, and I think that's unexpected for an update to an LTS release.

Thanks Andreas. I'll be working on a minimalistic patch set (if feasible)

What about "iptables - 1.6.1-2ubuntu2+testpkg20210629b3" ?

Thanks for the b3 version!

It restores the bionic implicit lock behavior (as if -w was given), but when given a specific value, in the end it ignores that it couldn't acquire the lock and moves on:

In all these tests, I have a lock held.

We have a chain called "andreas". See how -L waits 1 second as I requested, but moves on, listing the chain:
root@b1-iptables-restore-wait-lock:~# time iptables -L andreas -w 1
Chain andreas (0 references)
target prot opt source destination

real 0m1.005s
user 0m0.004s
sys 0m0.000s

Now I delete the chain. This shouldn't work because another app is holding the lock:
root@b1-iptables-restore-wait-lock:~# time iptables -X andreas -w 1

real 0m1.006s
user 0m0.005s
sys 0m0.000s

Was it deleted? Let's list again, and it was:
root@b1-iptables-restore-wait-lock:~# time iptables -L andreas -w 1
iptables: No chain/target/match by that name.

real 0m1.005s
user 0m0.004s
sys 0m0.000s

root@b1-iptables-restore-wait-lock:~# apt-cache policy iptables
iptables:
  Installed: 1.6.1-2ubuntu2+testpkg20210629b3
  Candidate: 1.6.1-2ubuntu2+testpkg20210629b3
  Version table:
 *** 1.6.1-2ubuntu2+testpkg20210629b3 500
        500 http://ppa.launchpad.net/slashd/lp1791958/ubuntu bionic/main amd64 Packages

I have a new test pkg:

This pkg keeps the same behavior for 'iptables'

# Hold the lock:
flock /run/xtables.lock sleep 36000

# It holds and wait until flock() finishes or get killed, and then get executed.
 iptables -L

# With 'wait' option, it waits until the wait time is ended:
time iptables -L -w 3
Another app is currently holding the xtables lock. Stopped waiting after 3s.

real 0m3.006s
user 0m0.002s
sys 0m0.001s

---------------
# Hold the lock:
flock /run/xtables.lock sleep 36000

# cat /etc/rules.v4 | iptables-restore -c -w 5
Another app is currently holding the xtables lock. Perhaps you want to use the -w option?

real 0m5.009s
user 0m0.006s
sys 0m0.001s

The 'wait' option instruction is respected.
The message may be confusing as it is suggesting to use '-w' even if it was used.

This is the progress I have made so far.

- Eric

Version in comment #12 is "iptables - 1.6.1-2ubuntu2+testpkg20210706b4"

I'll work to re-arrange the fprintf if 'wait' option is used or not and see the outcome of it.

Both 'iptables' and 'ip*tables-restores' sleep and wait until the lock is released if no 'wait' option. If 'wait' option is set, it wait for the amount of time instructed and stop waiting.

I think the printed message make more sense now :

# time cat /etc/rules.v4 | iptables-restore -c
Another app is currently holding the xtables lock; still -9s 0us time ahead to have a chance to grab the lock...
....

# time cat /etc/rules.v4 | iptables-restore -c -w 2
Another app is currently holding the xtables lock. Stopped waiting after 2s.

# time cat /etc/rules.v4 | ip6tables-restore -c
Another app is currently holding the xtables lock; still -9s 0us time ahead to have a chance to grab the lock...
....

# time cat /etc/rules.v4 | ip6tables-restore -c -w 2
Another app is currently holding the xtables lock. Stopped waiting after 2s.

New testpackage: iptables - 1.6.1-2ubuntu2+testpkg20210706b8

Note: If that works as expected for Andreas. The only things that needed would be to update the manpage, which I didn't do on this current test pkg.

Excellent progress Eric, thanks!

I'll give it a try.

I tested this last change, and it does exactly what we wanted for iptables, the tool. And since that behavior is shared with all tools of the iptables suite, it means iptables-restore got that fix too (good!), but it also introduces a change in behavior for iptables-restore (bad!).

When compared to the bionic 1.6.1 iptables:
(a) straight backport from 1.6.2
- iptables loses the implicit -w parameter, meaning it will fail right away if it encounters the lock
- iptables-restore maintains the behavior, and grows the extra -w option

(b) massaged patches from comment #16
- iptables keeps the same behavior as in 1.6.1
- iptables-restore grows the implicit -w option, meaning it will block until the lock is released

The locking code is shared by all tools in one .c file. Making it behave differently whether it's iptables or iptables-restore being used is kumbersome, and would make ubuntu the only one with this behavior.

Alternatively, this bug has actually a very decent workaround: wrap iptables-restore in flock. That's the same locking mechanism that iptables itself does, just internally.

Quick example: you want iptables-restore -w 2 file.iptables

Use:
flock -w 2 -x /run/xtables.lock iptables-restore file.iptables

You can even augment that a bit with -E <code>, and have flock return <code> if the lock cannot be acquired in the specified amount of time.

Of the two patch sets, it feels like (b) introduces the less worse behavior change. Before iptables-restore would fail right away, now it can get stuck for as long as the lock is held. Which is the iptables behavior already. But it's still a change, and your script could stall for as long as the lock exists. You should change it to use -w <seconds>.

Option (a) has the danger that if you are not checking for errors in your script, one or more iptables calls could fail, and you wouldn't notice, leaving your firewall incomplete. I think this is a dangerous change.

Considering bionic is an LTS, and the existence of the flock workaround which is exactly what the code itself does, what do you guys think about this SRU? Should we pick a patch and go with it, or reject the change and recommend the flock() alternative?

I'm on the same page.

Maybe we should leave the package in -update as is and go with the flock() alternative.

OR .... evaluate if, let's say, the Focal's iptables could be backported (if feasible) in bionic-backports.

That way, one who wants the 'wait' and 'wait-interval' to be available could rely on -backport, while the rest of our Bionic users could remain on the same old iptables behavior provided in -updates.

- Eric

For backports, a straight build of 1.6.2 would perhaps be enough. Might not seem a version change big enough for backports, but as we have seen, it does introduce a change of behavior that impacts existing firewall scripts.

I think 'backports' would have been a great place to have this behavior change in Bionic while keep the current one in -updates.

I just came across an email about 'backports' about to die (from rbasak). Let's see what is the outcome and rely on the flock() alternative for now.

I'll monitor the "-backports" discussion and let's see from there.

- Eric

> I'll monitor the "-backports" discussion and let's see from there.

Looks like the backports pocket will be available again. I added bionic-backports so that this request can be found from that end, since it looks like backports was something being considered. I don't intend to imply any particular opinion about the best route.

Hello,

the backports process has recently been updated, please see the new documentation:
https://wiki.ubuntu.com/UbuntuBackports

I'm closing this bug, but please feel free to open a new bug (or reopen this bug) using the new process, if appropriate.

I updated the title to better reflect the status.