Repository navigation
The init fence closes a port it cannot redirect - #18
Conversation
iptables_add_redirect skipped the REDIRECT rule for an address family whose nat table cannot be used, warned, and carried on; bin/turnkey-init-fence then started the mini server and reported success, with the ports the fence was asked to stand in front of reaching the application over that family. In an LXC container whose host has not loaded (ip6)table_nat that is the normal path, and the warning lands in the journal of a first boot nobody is watching. A mitigation that declines and reports success is worse than one that is absent, because everything else is written as though it worked. Going back to what #3 changed is not the answer: before it, the fence exited 3 on the first family it could not install a rule for, the mini server never started and 29tagid failed reloading it. Nor is a louder warning, which would still leave the port open. So the fence now closes what it cannot redirect. fence_close_port refuses the port in the filter table, which a container does have, inserted at the head of the INPUT chain so that it precedes the ACCEPT rules the appliance firewall installs for the same ports. What the fence promises holds either way: the application behind a fenced port does not answer, and the fence page stays reachable on its own ports, which is what #3 was for. A port that can be neither redirected nor refused is fatal, with a message, because then nothing at all is fencing it. fence_open_port undoes it, and iptables_delete_redirect calls it for both families, so stop and the stop-post cleanup after a failed run leave no port closed behind them. Four new tests, 31 in the file and 116 in the suite: the refusal for one family and for both, the insertion at the head of the chain, the removal loop, and the fatal path both through the library and through the script, the second because a library that returns non-zero as an error only stops its caller if the caller runs under errexit, and bats does not. kcov: lib/init-fence.sh 78 of 79 lines, 98.73 percent, total 99.57. The uncovered line is line 154, the first line of the multi-line simplehttpd.py invocation that kcov attributes to a later line, uncovered before this branch too and not part of it. .gitignore gains the default output directory of tests/coverage.sh. Closes #17.
|
Reviewed alongside Keel-Linux/common#12. Coverage reproduced exactly: 116 HIGH — MEDIUM — MEDIUM — LOW — LOW — LOW — landing order. This branch and #16 collide on exactly one hunk of The reasoning for not reverting #3 is correct, and closing what cannot be Warning |
The fence tests stub iptables and assert the commands issued. That is asserting the configuration: every review finding on this branch was about what the rules do, and none of it was visible to the stubs. tests/netns-sandbox runs lib/init-fence.sh against the real iptables and ip6tables in a network namespace of its own (an unprivileged user namespace where the kernel allows one, 'sudo -n unshare' otherwise, as on the CI runner), with a client namespace on the other end of a veth pair. A listener answers "app" on the fenced ports and "fence" on the fence's own, and a probe connects over IPv6, IPv4 or loopback and reports what it got: app, fence, refused, or silent. The only stand-in is the answer "no nat table" (or "the filter insert is refused") for one family; what the rules do to a packet is never modelled. tests/fixtures/webmin-fw.rules.v4 and .v6 are the files common's webmin-fw writes, rendered with its defaults. tests/test-init-fence-netfilter.bats, 8 tests. Against the branch as reviewed, 4 fail, and they are the findings: - without the IPv6 nat table the fenced ports over IPv6 are silent, not refused: a REJECT with the default icmp6-port-unreachable leaves an IPv6 client retrying its SYN until it times out (IPv4 is refused); - from the appliance itself, a refused port refuses loopback as well, where a redirected port never touched it; - the fatal path ends with the application answering on every port, not with the port closed; - a restore of the appliance firewall while the fence is up removes the fence, redirects and refusals alike. The stub tests change to the rule the next commit installs, and gain two assertions: the journal never says a port is refused when the insert failed, and the unit is ordered after every service that restores a saved firewall at boot. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SfWQScmDZ94KCS5DMYrfe6
…restore does Review of this branch, reproduced against the real netfilter by the previous commit, and what each finding became: - HIGH, a restore flushes the chain. Confirmed, and wider than the refusal: iptables-restore of the files common's webmin-fw writes flushes nat as well, so it takes the REDIRECT rules too, which has been true since before this branch. The unit is now ordered after netfilter-persistent, webmin-iptables and webmin-ip6tables, the services that apply a saved firewall at boot (Webmin creates the last two when its Firewall module is set to activate at boot; a built core has neither unit today). A restore while the fence is up cannot be ordered against, and fence_close_port's comment and the changelog now say so instead of implying position 1 holds. - MEDIUM, the fatal path ends unfenced. It does; the comment and the changelog say so, and a netfilter test shows it. - MEDIUM, no interface match. The refusal is now '! -i lo', so a refused port behaves like a redirected one for anything on the appliance. - Found by measuring, not in the review: the refusal is now '--reject-with tcp-reset'. With the default ICMP error an IPv6 client waited for its connect timeout, so over IPv6, which is the family this fallback exists for, the port looked silent rather than refused. - LOW, the message precedes the rule: it follows the insert now. - LOW, fence_open_port cannot tell "nothing left" from "cannot ask": said. The rule is built in one place, fence_refusal, so the insert and the delete cannot drift apart. 125 bats; kcov lib/init-fence.sh 83/84 (98.81), total 99.58, the same line uncovered as before the branch. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SfWQScmDZ94KCS5DMYrfe6
…inherited ones A change of approach within this branch, so the three parts: 1. Why stripping does not work. tagid_strip_remote_scripts removed <script> elements whose src was double or single quoted and remote. Review measured an unquoted src, a stylesheet, an image and a frame getting past it, and a script with a body leaving its body behind as page text. Each is a regular expression away from being handled, and the next form is another. 2. Whether it can be made to work. Only as far as a parser agrees with a browser, and none does on malformed markup: Python's HTMLParser, the best candidate here, does not see an image after '<!-->' or after '</script x>', and a browser loads both. A defence built on it would have the detector's blind spots, which is what review found the first time. 3. Why rebuilding is better. It needs no judgement of the page at all. Nothing in the tree writes a page into the /var copy but this hook, so replacing that copy with the packaged pages loses nothing, and a page the package did not ship is not served, however it is written. The packaged pages are what the tests judge, by two detectors. What 29tagid does now, on every run, under turnkey-init as well: - builds the pages in a directory beside HTDOCS (mktemp in its parent), never inside the directory the fence serves, which review noted the temporary index was; - names the appliance in the copy, then swaps it in with two renames and removes the old one; - on any failure, removes HTDOCS: the fence serves the packaged htdocs when the writable copy is missing, so a failure serves the unnamed page rather than an inherited one. It exits 1 with a <3> message; it used to exit 0 and keep the old page; - stops before touching anything when mktemp fails (an empty directory name would have sent the copy to /) or when the default file sets no HTDOCS (the reviewed hook then copied the pages into its working directory, which the new test caught by doing it). Under turnkey-init it used to exit at once. That exit was for the naming half; the pages are what an operator whose first boot did not complete gets served, so it runs. lib/tagid.sh is down to tagid_app_name. The changelog entry is 2.3.6+keel7: this lands after #18, which has keel6. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SfWQScmDZ94KCS5DMYrfe6
…-skip master gained #16, which rewrote the bare negations in tests/test-init-fence.bats. The one hunk both branches touched is the "both nat tables" test, where #16 made 'grep -q WARNING' a 'run !' and this branch had replaced it with counts of the inserts and of every prefixed journal line; the counts are kept, as review of both asked. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SfWQScmDZ94KCS5DMYrfe6
|
Review addressed in 36663c5 and 7cc4139, then master merged in (d24ca3e). The fence is now measured against real iptables in a network namespace. The unit starts after the services that restore a saved firewall at boot, and the comment says an on-demand restore unfences every port. The fatal path is documented as open. The refusal skips loopback and is now a tcp-reset, because the default ICMP error left IPv6 clients hanging until they timed out. |
…-skip master gained #19 (keel-host-keys), which took 2.3.6+keel6. This branch's entry becomes 2.3.6+keel7, above it, with the date of this merge; COVERAGE.md keeps both sections. No code overlaps: keel-host-keys.service is ordered before turnkey-init-fence.service and this branch only adds After= lines to the latter. 147 bats, shell total 99.67. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SfWQScmDZ94KCS5DMYrfe6
…script master gained #19 (keel-host-keys, 2.3.6+keel6) and #18 (the fence, 2.3.6+keel7). This branch's entry becomes 2.3.6+keel8, above both, with the date of this merge. No code overlaps: #18 changes lib/init-fence.sh and the fence unit, and fence_htdocs, which this branch's failure path relies on to serve the packaged pages, is unchanged. Shell total 99.68, 29tagid 29/29; Python 196 passed, 3 skipped. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SfWQScmDZ94KCS5DMYrfe6
…inherited ones A change of approach within this branch, so the three parts: 1. Why stripping does not work. tagid_strip_remote_scripts removed <script> elements whose src was double or single quoted and remote. Review measured an unquoted src, a stylesheet, an image and a frame getting past it, and a script with a body leaving its body behind as page text. Each is a regular expression away from being handled, and the next form is another. 2. Whether it can be made to work. Only as far as a parser agrees with a browser, and none does on malformed markup: Python's HTMLParser, the best candidate here, does not see an image after '<!-->' or after '</script x>', and a browser loads both. A defence built on it would have the detector's blind spots, which is what review found the first time. 3. Why rebuilding is better. It needs no judgement of the page at all. Nothing in the tree writes a page into the /var copy but this hook, so replacing that copy with the packaged pages loses nothing, and a page the package did not ship is not served, however it is written. The packaged pages are what the tests judge, by two detectors. What 29tagid does now, on every run, under turnkey-init as well: - builds the pages in a directory beside HTDOCS (mktemp in its parent), never inside the directory the fence serves, which review noted the temporary index was; - names the appliance in the copy, then swaps it in with two renames and removes the old one; - on any failure, removes HTDOCS: the fence serves the packaged htdocs when the writable copy is missing, so a failure serves the unnamed page rather than an inherited one. It exits 1 with a <3> message; it used to exit 0 and keep the old page; - stops before touching anything when mktemp fails (an empty directory name would have sent the copy to /) or when the default file sets no HTDOCS (the reviewed hook then copied the pages into its working directory, which the new test caught by doing it). Under turnkey-init it used to exit at once. That exit was for the naming half; the pages are what an operator whose first boot did not complete gets served, so it runs. lib/tagid.sh is down to tagid_app_name. The changelog entry is 2.3.6+keel7: this lands after #18, which has keel6.
The init fence closes a port it cannot redirect
…script master gained #19 (keel-host-keys, 2.3.6+keel6) and #18 (the fence, 2.3.6+keel7). This branch's entry becomes 2.3.6+keel8, above both, with the date of this merge. No code overlaps: #18 changes lib/init-fence.sh and the fence unit, and fence_htdocs, which this branch's failure path relies on to serve the packaged pages, is unchanged. Shell total 99.68, 29tagid 29/29; Python 196 passed, 3 skipped.
1 participant
Closes #17.
The problem
iptables_add_redirectskipped the REDIRECT rule for an address familywhose nat table cannot be used, warned, and carried on.
bin/turnkey-init-fencethen started the mini server and reported success, and the unit sat
active, while the ports the fence was asked to stand in front of reachedthe application over that family.
In an LXC container whose host has not loaded
ip6table_natthat is thenormal path, not an edge case, and the warning lands in the journal of a
first boot nobody is watching. A mitigation that declines and reports
success is worse than one that is not there, because the rest of the system
is written as though it worked.
Why the skip is not simply reverted
#3 put it there deliberately. Before it, the fence exited 3 on the first
family it could not install a rule for: the unit failed, the mini server
never started and
29tagidthen failed reloading it. Going back to thattrades one wrong answer for another, and a louder warning would still leave
the port open.
The container case #3 was serving is legitimate. What was wrong was the
conclusion: if the port cannot be redirected to the fence, the answer is
that the application behind it must not answer either, not that the fence
should shrug.
The change
fence_close_portrefuses the port in the filter table, which a containerdoes have, inserted at the head of the INPUT chain so that it precedes the
ACCEPT rules
webmin-fwinstalls for the same ports. Appending would haveput it after them and matched nothing. The rule is
! -i lo ... -j REJECT --reject-with tcp-reset: loopback is spared, as aREDIRECT in PREROUTING always spared it, and the reset is because an IPv6
client given the default ICMP error waits for its timeout instead of
failing (measured, below).
The head of the chain is the chain as it stands when the fence starts. A
restore of a saved firewall flushes the tables it names, and with them the
refusal and the REDIRECT rules alike. The unit is ordered after the
services that restore one at boot (
netfilter-persistent, Webmin'swebmin-iptablesandwebmin-ip6tables); a restore while the fence is upstill unfences every port, and the comment and changelog say so.
What the fence promises then holds either way:
HTTP_FENCE_PORTandHTTPS_FENCE_PORT, which keeps turnkey-init-fence: skip a REDIRECT rule when its nat table is unavailable #3's outcome, including the29tagidreload;
<3>message, because then nothing at all is fencing it. That is the only case
left where the unit fails, and it is a case where failing is the truth.
Failing does not mean closed:
stop-postthen removes every rule thefence installed and the application answers on every port. It
does not cascade the way the pre-turnkey-init-fence: skip a REDIRECT rule when its nat table is unavailable #3 failure did:
29tagidhas askedsystemctl is-activebefore reloading since 2.3.6+keel1, so a failed fenceis skipped rather than fatal to the hook.
fence_open_portundoes it, andiptables_delete_redirectcalls it for bothfamilies, so
stopand thestop-postcleanup after a failed run leave noport closed behind them. That keeps the existing policy: a failed fence
cleans up rather than locking the appliance out, and the port is open again
because the fence is gone, not because it pretended.
The message moved from
WARNING:to the<4>prefix the rest of the fileuses for a journal priority.
Tests
31 in the file, 116 in the suite: the refusal for one family and for both,
the insertion at the head of the chain, the removal loop, and the fatal path
twice, through the library and through the script. The second one matters on
its own: a library function that returns non-zero as an error only stops its
caller if the caller runs under errexit, and
bats runturns errexit off, soa test that only drives the library cannot see it (
docs/traps.md, "A batssuite cannot see a library that kills its caller").
One assertion in the pre-existing "both nat tables" test was a bare
!before a later command, so it asserted nothing; it is now a count that does.
#16 is rewriting the other negations in this file, so
expect a conflict in
tests/test-init-fence.batsbetween the two.tests/test-init-fence-netfilter.bats, 8 tests, asks the real netfilter:tests/netns-sandboxruns the library against realiptablesandip6tablesin a network namespace of its own, with a client namespace onthe other end of a veth pair, and connects over IPv6, IPv4 and loopback.
The only stand-in is the answer "no nat table" for one family. Against the
branch as first reviewed it fails 4, which are the review's findings plus
the silent IPv6 refusal.
kcov (first version):
lib/init-fence.sh78 of 79 lines, 98.73 percent,total 99.57; now 83 of 84, 98.81, total 99.58, 125 bats. Over
the 98 gate and decision 0003's 95 percent bar. The uncovered line is line
154, the first line of the multi-line
simplehttpd.pyinvocation that kcovattributes to a later line; it was uncovered before this branch and is not
part of it. Every line this branch adds is covered.
What this does not do
Nothing is built or published and no build lock was taken, so this has not
run on a booted container; the rules are measured in a network namespace,
with the nf_tables backend where the appliance uses iptables-legacy.
2.3.6+keel6is this one's; #14 has moved tokeel7and lands after it.