Fix: Re-apply custom firewall post-rules after VPN reconnection and rule updates#2821
Fix: Re-apply custom firewall post-rules after VPN reconnection and rule updates#2821atasmohammadi wants to merge 6 commits intoqdm12:masterfrom
Conversation
qdm12
left a comment
qdm12
left a comment
There was a problem hiding this comment.
Thanks for the PR! Can you please move things around as suggested in comments, in order to have an easier PR review? Thanks!
| VERSION=${{ fromJSON(steps.meta.outputs.json).labels['org.opencontainers.image.version'] }} | ||
| tags: ${{ steps.meta.outputs.tags }} | ||
| push: true | ||
| push: ${{ github.event_name != 'pull_request' }} |
There was a problem hiding this comment.
please revert this file for the time being, this is out of scope.
| release_name: Release ${{ github.ref }} | ||
| body: ${{ steps.changelog.outputs.changelog }} | ||
| draft: false | ||
| prerelease: false No newline at end of file |
There was a problem hiding this comment.
same, revert this. Happy to get another PR eventually though.
| _ = c.runIptablesInstruction(ctx, flipRule(rule)) | ||
| } | ||
| }() | ||
|
|
There was a problem hiding this comment.
nit unneeded newline change here
| func (c *Config) disable(ctx context.Context) (err error) { | ||
| // Clear applied post-rules when disabling | ||
| if err = c.clearAppliedPostRules(ctx); err != nil { | ||
| c.logger.Warn("failed to clear post-rules during disable: " + err.Error()) | ||
| } | ||
|
|
||
| if err = c.clearAllRules(ctx); err != nil { | ||
| return fmt.Errorf("clearing all rules: %w", err) | ||
| } | ||
| if err = c.setIPv4AllPolicies(ctx, "ACCEPT"); err != nil { | ||
| return fmt.Errorf("setting ipv4 policies: %w", err) | ||
| } | ||
| if err = c.setIPv6AllPolicies(ctx, "ACCEPT"); err != nil { | ||
| return fmt.Errorf("setting ipv6 policies: %w", err) | ||
| } | ||
|
|
||
| const remove = true | ||
| err = c.redirectPorts(ctx, remove) | ||
| if err != nil { | ||
| return fmt.Errorf("removing port redirections: %w", err) | ||
| } | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| // To use in defered call when enabling the firewall. | ||
| func (c *Config) fallbackToDisabled(ctx context.Context) { | ||
| if ctx.Err() != nil { | ||
| return | ||
| } | ||
| if err := c.disable(ctx); err != nil { | ||
| c.logger.Error("failed reversing firewall changes: " + err.Error()) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
move these blocks where they previously were to see diffs more clearly.
qdm12
force-pushed
the
master
branch
3 times, most recently
from
27f74e4 to
fe3d4a9
Compare
qdm12
force-pushed
the
master
branch
2 times, most recently
from
d0247a1 to
0eeee5c
Compare
qdm12
left a comment
qdm12
left a comment
There was a problem hiding this comment.
I don't see the point of this pull request, please elaborate on how this helps
Also
Observed that without this fix, rules were lost after VPN reconnection
That is not true, rules are persisted already as far as I'm aware.
Plus modifications to SetVPNConnection and SetOutboundSubnets are missing.
| // Check if this rule was already applied (avoid duplicates) | ||
| if !remove { |
There was a problem hiding this comment.
Why do we track duplicates? That should be up to the user to ensure they don't have duplicate iptables commands
| // Apply post-rules only once after adding the port, and only if not already applied | ||
| if err := c.applyPostRulesOnce(ctx); err != nil { | ||
| c.logger.Warn("failed to apply post-rules after adding port: " + err.Error()) | ||
| } | ||
|
|
There was a problem hiding this comment.
How is this different from just running the post rules once at firewall enabling? Especially given applyPostRulesOnce is not even aware of the newly allowed port.
| // Clear any previously applied post-rules | ||
| if err = c.clearAppliedPostRules(ctx); err != nil { | ||
| c.logger.Warn("failed to clear previous post-rules: " + err.Error()) | ||
| } | ||
|
|
There was a problem hiding this comment.
this is not needed, given the firewall is only enabled once on a clear set of rules. Plus disable does remove all rules anyway, in case the firewall would get disabled then re-enabled.
| // Clear applied post-rules when disabling | ||
| if err = c.clearAppliedPostRules(ctx); err != nil { | ||
| c.logger.Warn("failed to clear post-rules during disable: " + err.Error()) | ||
| } |
There was a problem hiding this comment.
not needed because of existing clearAllRules
2 participants
Problem
When using custom firewall post-rules in
/iptables/post-rules.txt, these rules are only applied during the initial firewall setup but are not re-applied when:This causes custom rules to be lost during normal operation, requiring manual re-application of rules (e.g., via SSH or exec into the container).
Solution
This PR adds a helper method
applyUserPostRules()and ensures it's called after any significant firewall rule changes in:SetVPNConnection()in vpn.goSetAllowedPort()in ports.goSetOutboundSubnets()in outboundsubnets.goThe changes are minimal and follow the existing code patterns.
Testing
I've tested this fix with a custom Gluetun setup using WireGuard and the following scenario:
/iptables/post-rules.txtto allow specific trafficiptables -Lthat rules remain in place after reconnection eventsUse Case
This is particularly important for users who:
Changes
applyUserPostRules()helper method in firewall.goSetVPNConnection()to re-apply post-rules after VPN changesSetAllowedPort()to re-apply post-rules after port changesSetOutboundSubnets()to re-apply post-rules after subnet changesrunUserPostRules()(optional)