-
Notifications
You must be signed in to change notification settings - Fork 726
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Firewall: fix issues with antilockout firewall rule #7381
base: master
Are you sure you want to change the base?
Conversation
src/etc/inc/filter.lib.inc
Outdated
@@ -90,13 +90,23 @@ function filter_core_get_antilockout() | |||
{ | |||
global $config; | |||
|
|||
// Migration from old noantilockout option |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assumes the default (new) configuration contains a fixed choice, which circles back to a static lan
in config.xml.sample
, when not persisting lan, initial boot after install will likely always persist lan
here anyway, which might then be altered by an interface mismatch later on.
When removing the side affect write_config();
, a migration also will assume a static situation, which past experiences learn has its limitations.
It's also good to remember, you're only able to cause your issue by removing existing interfaces, which almost sound like if we didn't let you remove "WAN", you didn't have an issue either..... (probably not true, but similar to the changes proposed)
Just my two cents on the subject.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's what I wrote in the related issue of this PR: The place and the way of the migration doesn't feel right, which is due to my lack of knowledge about the internal workings of the config system. A one-time migration is most likely the better choice, but I'd need guidance as how to do that.
And it is not the removal of the WAN interface that causes trouble, it is the removal of the LAN interface (more precisely: the interface that in the config is named "lan"). Once that is gone, the opt with the lowest number is chosen:
If that opt happens to act as a WAN => antilockout rule opens ports on that WAN
If that opt happens to be disabled => antilockout rule doesn't work at all
src/etc/inc/console.inc
Outdated
@@ -384,6 +384,7 @@ EOD; | |||
$config['interfaces']['lan']['if'] = $lanif; | |||
$config['interfaces']['lan']['enable'] = true; | |||
|
|||
$config['system']['webgui']['antilockout_interface'] = 'lan'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as said in the issue I'd prefer a per-interface field instead of a loose list maintained somewhere else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
User view: I only use WAN and/or OPTX and I still want anti-lockout. What do I do on the console? A single WAN will also lock itself out...
src/etc/inc/filter.lib.inc
Outdated
if (isset($config['system']['webgui']['noantilockout'])) { | ||
return []; | ||
unset($config['system']['webgui']['noantilockout']); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is no reason to interfere with the state of this setting
src/www/system_advanced_firewall.php
Outdated
if (!empty($pconfig['noantilockout'])) { | ||
$config['system']['webgui']['noantilockout'] = true; | ||
} elseif (isset($config['system']['webgui']['noantilockout'])) { | ||
unset($config['system']['webgui']['noantilockout']); | ||
} | ||
$config['system']['webgui']['antilockout_interface'] = $pconfig['antilockout_interface']; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
historically these constructs exist to avoid PHP warnings and notices (check development mode)
src/www/system_advanced_firewall.php
Outdated
@@ -732,19 +728,31 @@ | |||
</div> | |||
</td> | |||
</tr> | |||
<tr> | |||
<td><a id="help_for_noantilockout" href="#" class="showhelp"><i class="fa fa-info-circle"></i></a> <?=gettext("Disable anti-lockout"); ?></td> | |||
<tr id="antilockout_interface"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be frank adding a better way but removing the old way to make it "safe" is questionable at best.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok there is the thing:
Redesigning the feature and implementing it in a complimentary way is probably easier, but I see potential for hours of work and going to the drawing board to actually come up with a better system that prevents a similar amount of lockout-protection that the current system is offering.
A flaw in the system doesn't make the whole system worthless and that's why removing it will just create friction with existing users that I'm not going to debate including.
Maybe complications are unavoidable, but we better know all of them. See my "user view" questions.
src/etc/inc/filter.lib.inc
Outdated
unset($config['system']['webgui']['noantilockout']); | ||
} | ||
if (!isset($config['system']['webgui']['antilockout_interface'])) { | ||
$lockout_if = get_primary_interface_from_list(array_keys(legacy_config_get_interfaces(['virtual' => false, 'enabled' => true]))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
User view: Why are we adding compatibility but don't provide a way to turn this off anymore? I just want the old setting to remain and work as before.
src/etc/inc/filter.lib.inc
Outdated
if (!isset($config['system']['webgui']['antilockout_interface'])) { | ||
$lockout_if = get_primary_interface_from_list(array_keys(legacy_config_get_interfaces(['virtual' => false, 'enabled' => true]))); | ||
if (empty($lockout_if)) { | ||
$config['system']['webgui']['antilockout_interface'] = 'None'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Storing arbitrary values like "None" is a bad idea.
if (isset($config['system']['webgui']['noantilockout'])) { | ||
unset($config['system']['webgui']['noantilockout']); | ||
if ($config['system']['webgui']['antilockout_interface'] ?? 'None' != $interface) { | ||
$config['system']['webgui']['antilockout_interface'] = $interface; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
User view: huh, why does it reset the interface to WAN if I edit the address?
src/www/interfaces_assign.php
Outdated
@@ -51,6 +51,13 @@ function link_interface_to_group($int) | |||
return $result; | |||
} | |||
|
|||
function link_interface_to_antilockout($int) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we please avoid legacy glue constructs like this?
src/www/interfaces_assign.php
Outdated
@@ -155,6 +162,8 @@ interface_sync_wireless_clones($config['interfaces'][$newifname], false); | |||
$input_errors[] = gettext("The interface is part of a gre tunnel. Please delete the tunnel to continue"); | |||
} elseif (link_interface_to_gif($id)) { | |||
$input_errors[] = gettext("The interface is part of a gif tunnel. Please delete the tunnel to continue"); | |||
} elseif (link_interface_to_antilockout($id)) { | |||
$input_errors[] = gettext("The interface is used by the anti lockout firewall rule. Please select a different one to continue"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a silly constraint.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
User view: ok now I need to go to some other page to set a new anti-lockout interface to be able to delete this and then I'll just forget to set anti-lockout I just want to create a new interface...
8917923
to
5beb223
Compare
Yes, I do like this idea much better than the old way. Also, it was much easier to implement this than the old way. So I have updated (force-pushed) this PR implementing the way you proposed, see commit 5beb223. Screenshots: Resetting to defaults will enable the anti lockout on the LAN interface. One thing is missing, because I couldn't find out how to do it: |
5beb223
to
5549a52
Compare
src/www/system_advanced_firewall.php
Outdated
@@ -43,7 +43,6 @@ | |||
$pconfig['maximumfrags'] = isset($config['system']['maximumfrags']) ? $config['system']['maximumfrags'] : null; | |||
$pconfig['adaptivestart'] = isset($config['system']['adaptivestart']) ? $config['system']['adaptivestart'] : null; | |||
$pconfig['adaptiveend'] = isset($config['system']['adaptiveend']) ? $config['system']['adaptiveend'] : null; | |||
$pconfig['noantilockout'] = isset($config['system']['webgui']['noantilockout']); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if I said this earlier, but it's not a good idea to aim for a replacement at this point without a migration strategy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, now I got what you meant. You still want to keep a global on/off switch for the whole feature. Yes, that makes sense.
So I have updated the PR once again and now the changes are as minimal as possible:
Interfaces can have an attribute "antilockout". Interfaces with this attribute take precedence when trying to find interfaces to activate the anti lockout rule for. If no such interfaces are found, the old (buggy) logic applies.
If the "Disable anti-lockout" setting in Firewall/Settings/Advanced is checked, no anti lockout rules will be generated, regardless of the new attribute.
Resetting to defaults (or generating the default configuration) will set the new antilockout attribute on the LAN interface.
This way there is also no need to migrate anything for now.
This doesn't fully solve the issue, but it makes it even less likely and now there is actually an option to define the interfaces for which the anti lockout rule shall apply for.
78845fc
to
8ba454a
Compare
db7fdef
to
b78c5f8
Compare
src/www/system_advanced_firewall.php
Outdated
"in place that allows you in, or you will lock yourself out."), | ||
count($config['interfaces']) == 1 && !empty($config['interfaces']['wan']['if']) ? | ||
gettext('WAN') : gettext('LAN')) ?> | ||
<?= gettext(<<<EOD |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this breaks translations. not sure why that's a good idea
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reverted
src/www/interfaces_assign.php
Outdated
<i class="fa fa-trash fa-fw"></i> | ||
</button> | ||
<?php endif ?> | ||
<?php |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should couple functionalities here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reverted
@@ -542,6 +542,10 @@ function console_configure_dhcpd($version = 4) | |||
unset($config['system']['webgui']['noantilockout']); | |||
$restart_webgui = true; | |||
} | |||
if (empty($config['interfaces'][$interface]['antilockout'])) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
eh, huh, why? you can use it to configure a WAN too...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assumed the user would not choose "yes" here when configuring a WAN interface, but yes, the user could. It will configure antilockout for the LAN interface now and if no LAN is present, print a warning about it.
src/etc/inc/filter.lib.inc
Outdated
@@ -556,7 +558,7 @@ function filter_core_rules_system($fw, $defaults) | |||
'to' => '(self)', | |||
'to_port' => implode(' ', $lockoutprts), | |||
'descr' => 'anti-lockout rule', | |||
'#ref' => 'system_advanced_firewall.php#noantilockout' | |||
'#ref' => 'interfaces.php?if=' . $lockoutif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that makes sense as both could be valid.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
true, it was a left over from previous work. reverted
@@ -383,6 +383,7 @@ EOD; | |||
config_read_array('interfaces', 'lan'); | |||
$config['interfaces']['lan']['if'] = $lanif; | |||
$config['interfaces']['lan']['enable'] = true; | |||
$config['interfaces']['lan']['antilockout'] = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
config.xml.sample needs this too, but you have to consider if people skip LAN the unfavourable (as per your view) implementation wins and keeps doing what it always did.
it might just be better to stay clear and leave the new antilockout as an opt-in for the forseeable future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added. But it's a sample config file only and it is already now missing a couple of other stuff, like "lock" for example. So I don't think adding it there is too confusing. Shows the option, which IMHO is fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
think more about it it would make sense to simply ask the user if they want to participate in the anti-lockout group with this interface. It should be safe, because in this assignment run we assign all ports so there is no ambiguity about it.
The old antilockout rule logic had two problems: * the rule may randomly open ports on the WAN under certain circumstances * the rule may select inactive interfaces The new logic has an antilockout setting per interface. It also prevents the deletion of interface assignments if: * the interface is locked * the anti lockout rule is active on this interface
b78c5f8
to
cda1e98
Compare
@@ -542,6 +542,12 @@ function console_configure_dhcpd($version = 4) | |||
unset($config['system']['webgui']['noantilockout']); | |||
$restart_webgui = true; | |||
} | |||
if (isset($config['interfaces']['lan'])) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What this code tries to achieve is go back to the factory defaults. We are not the LAN interface... we are any interface here and reverting to defaults attempts to restore GUI access, but not for a definitive "lan" interface. This script allows the user to select the interface to set up prior (and asking for anti-lockout there might be more sensible).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(more to your original point the 'lan' key could have been repurposed as the WAN interface in the meantime)
$lockout_if = get_primary_interface_from_list(array_keys(legacy_config_get_interfaces(['virtual' => false]))); | ||
if (empty($lockout_if)) { | ||
$lockout_ifs = array_keys(legacy_config_get_interfaces(['virtual' => false, 'enable' => true, 'antilockout' => true])); | ||
if (empty($lockout_ifs)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still split on this fallback. It could also be considered to move noantilockout
to only refer to the automatic anti-lockout behaviour and never mess with the interface-based setting.
Uh, now I feel this has become like a moving target and I don't really know anymore where this should go. I'd like a clear path forward. So I propose the following strategy to continue on this topic:
I am happy to do all these steps, so here is my proposal for step 1, the goals: Can we agree on this strategy to go forward and on these goals? If not, please propose other clear and reachable goals that fix the problem at hand. |
Skipping the design phase of this is exactly what makes this a moving target and I said so initially:
Your goals are ok, but you have to keep regressions in mind, offering a complimentary system, namely:
Yes, but now you already manoeuvred yourself into a dead-end because this will not work without breaking other people's setup. You will lock someone out and I don't think it's worth it.
This is unrealistic because the user can delete interfaces (if not from the GUI then from the console) and then he/she locked himself/herself out.
This clearly conflicts with goal 1. The per interface setting presented here is looking promising, but it's not done if you want people to accept and use the new way of doing it. You've discarded a number of my concerns and requests that were in line with your desire for everyone to use it (adding the new setting to the default config.xml) and you touched code where otherwise not relevant or dangerous because you missed the scope of the script (like setaddr.sh). I know this stuff is hard, but I've done it for 10 years and if something doesn't look good enough it will certainly not be for the users when this will be included in a release it must be as good as you can get it and it has to make sense for everyone who didn't follow this discussion too. I think for now it just makes sense for you and that's what you aim for, which is a fine enough motivation to get started, but not something that can be included without severe repercussions, post-release fixes and maybe 1-2 years of further tinkering and the bad feeling that it should not have been merged permaturely. Take it for what it is. I'm only here to guide and shape the submission if you are willing. Cheers, |
You said (highlighting by me):
And this is exactly what I have in mind. A complimentary feature. So either the old logic is active, or the new one. Never both. The user can choose. But that's already discussing step 2 (measures). I'd like to discuss step 1 (goals, and only the goals) first before going into step 2.
I totally disagree. I do not want to break existing configurations (goal 3) and I do not want to lock anyone out (goal 2).
Then let's prevent that from happening! But that's already discussing an implementation detail. I want to focus on the goal itself:
No. See above, complimentary feature.
I am willing if we can reach an agreement on the goals. I kept them very simple on purpose. If they are not clear enough, let me know. Only in the next step I would like to discuss measures how to reach them. And then we will see what implications those measures are bringing in and if those implications are acceptable or not. But not before we have an agreement (or not) on the goals. So let's sort that out first, please. Cheers, Curly060 =;-> |
Is that why you removed the other feature in the first submission? Sorry I don't want to nitpick but you have to be realistic. Goal 1 is not well-defined. "The router" in the factory config will do this in the current state of the PR. Hence I'm pressing on the right approach for the factory reset, whatever it is going to be. The goal is not reached. Goal 2 also needs to predict user mistakes. We don't have to prevent anything and I don't think we can with the per-interface default. If the user wants to delete an interface he can and will. If the strategy for new installs as per Goal 1 is to not offer the old fallback (no anti-lockout enabled) then there is no way we can add another interface in anti-lockout. Goal 3 fair enough but now you need to make sure the expectation matches the reality. There is a knob to turn of anti-lockout but you've bound your new feature to it. The user using the new anti-lockout will lock itself out by enabling this option. I don't think that is expected or wanted because it fails to adhere to a simple and fool-proof approach. Together we defined a per interface feature that works quite well from the looks of it, but you haven't exposed it to the user in all the cases in the right manner where it matters:
Cheers, |
I removed stuff because I think you did not want them. It was not clear to me what you actually want, because except for the good idea to make it interface based you only ever said what you don't want. That's why I suggested to start from scratch. But now the same thing happens again:
How is that not well defined? I want a firewall to never ever open ports in an unexpected way, esp not on unexpected interfaces. That's one of the most basic expectations I have on a firewall. How else to formulate it?
Then let's not make the "per interface" the default, see below.
I will not bind the new feature to that switch. Ok, even though we haven't agreed on the goals yet, here is my proposal for step 2, the measures to reach the goals I had proposed. Maybe that makes the goals more clear to you: Measures:
The advantage of this is: That's my proposal. If you don't like it, then please come up with a different proposal (clear goals and measures).
I think I have. But maybe not in the way you had expected it. The mistake was to set the anti lockout flag as default. Cheers, Curly060 =;-> |
Isn't the feature that is tried to be added here, actually just the same as this:
And all without adding any new code. |
No, not at all, see below.
But I do not want to opt in to create my own firewall rules for this! I quite like the anti lockout feature, esp. when refactoring the rules. In fact, I would like to always have access on LAN, no matter how badly I mess up my firewall rules. I just don't want the system to guess and eventually guess wrong. That's why I like the new interface based approach very much: Cheers, Curly060 =;-> |
You can actually do that by creating one "anti lockout rule" in "Floating" that has all the interfaces assigned where you don't want to lock yourself out on. Floating always matches first. I'm just saying that the overlay you will create for the current anti-lockout system is also "opt in" as would be checking that one option in Firewall Advanced. Creating a single "Floating" rule with the destination "This Firewall" from Source "Any" and adding all interfaces to it where the anti lockout should match, would serve the same purpose without adding new code. In my opinion simplicity wins. I just wanted to write my opinion on this. I am not the guy who can decide what gets added or not. I think if you can counter my argument here you have a pretty good chance to get further with this. |
There is a huge difference between:
The whole point of the anti lockout feature is that it works despite of any user defined rules! It has to be able to always override whatever the user does. And it currently does, except in a few rare cases. And these rare cases come from the fact that the current system is trying to "guess" the interface to work on, based on how they are named inside the config. And sometimes it "guesses" wrong. The proposal to set a checkbox per interface removes the need to guess. But if the "guess" way shall be fully replaced by the new way, then yes indeed, that's not so easy. Hence my latest proposal. |
The problem is that in terms of actions for the user it’s roughly the same as @Monviech mentioned, the outcome in terms of rules is the same. I think I already mentioned this a long time ago, but we’re really trying to over-engineer a relatively small feature here, where in reality this is all about visibility. (Something we may be able to further improve in the future, but not now) |
This commit fixes two issues with the antilock firewall rule:
The issue is fixed by giving the user a choice for which interface the rule should apply for, default being LAN. The user is also being prevented from deleting/unassigning/disabling the currently selected interface.