Home
Help
Search
Login
Register
OPNsense Forum
»
English Forums
»
Development and Code Review
(Moderator:
fabian
) »
Implementing a better ICMP selector in filter rules
« previous
next »
Print
Pages: [
1
]
Author
Topic: Implementing a better ICMP selector in filter rules (Read 6664 times)
Stilez
Newbie
Posts: 27
Karma: 1
Implementing a better ICMP selector in filter rules
«
on:
November 06, 2019, 03:16:59 pm »
I'd like to enhance the filter rules page to allow (1) multi-select of ICMP subtypes within a filter rule, and (2) where an ICMP subtype can validly apply to both IPv4 and IPv6, such as echo request/reply, to allow that subtype to be used in rules with protocol = "IPv4+IPv6".
The aim is to allow a router admin to avoid pointless and distracting multiplicity of rules whereby several ICMP subtypes must be given in many otherwise-identical rules just because they can't go in one rule, and then *all* of these rules must be repeated a *second* time for both v4 and v6 because ICMP filters can't currently be used within IPv4+IPv6 rules.
I already wrote this code
for pfSense a while back, before I moved to OPNSense, so I know it's easy, and the code works. It's been part of that project for years. But I'm not yet familiar with the way mvc works in the codebase (despite reading the wiki) - sorry, the details within OPNSense is just extremely unfamiliar - so I'd like to check "what will go where" with a view to writing and testing an OPNSense PR for this enhancement.
The working code I'm starting with looks like this:
firewall_rules_edit.php (protocols list):
ICMP and ICMP6 are replaced by a single entry "ICMP v4+v6". This is similar to how we have IPv4+IPv6 in the protocol field. (The
pf.conf
generated by filter.inc still of course contains
icmp
and
icmp6
as needed). There are 2 copies of these tables in the codebase so they can probably be replaced by a single copy in some appropriate file.
firewall_rules_edit.php + firewall_rules_edit.php (tables of ICMP+ICMP6 subtypes):
ICMP subtypes tables for v4+v6 are merged into a single table, which lists all subtypes, with each flagged as "valid for v4? valid for v6?". These tables are only used very few times in the codebase, so consequential changes are trivial.
firewall_rules_edit.php
:
- the merged ICMP subtypes table is used to build a table with 3 subarrays giving the valid data for IPv4, IPv6 and IPv4+6 rules respectively
- if $proto == 'icmp', the validation code checks that the selected ICMP subtypes are all valid for the IPv* selected
- a multi-select rather than dropdown field is used for ICMP subtypes. Its contents+selection are initially populated by Javascript. They are also updated by Javascript, if IPv4/6/46 is changed by the user, to ensure the ICMP subtypes displayed+selected remain compatible.
firewall_rules.php
: the "long format" rules table lists all ICMP subtypes selected, whethere one or multiple. The "short format" table just shows the number of subtypes against ICMP rules (e.g:
"ICMP (4)"
) which can be hovered/expanded to view the specific list of subtypes.
FilterRule.php (rule generation for rules with protocol=ICMP)
:
- As a first step, builds a subtype-string giving the actionable subtypes in
pf.conf
format. If an ICMP rule contained just one subtype, then this string would look like
"subtype"
. If the rule contained multiple subtypes, the string would look like
"{ subtype, subtype, subtype, ...}"
.
- ICMP and ICMP6 are then handled trivially as part of IPv4 and IPv6 handling. Meaning: as the code stands,
inet46
rules result in two
pf
rules being created, one for IPv4 and one for IPv6. So for ICMP rules, if the IP type is
inet
then an
"icmp-type [subtype-string]"
clause will be generated. If the IP type is
inet6
then an
"icmp6-type [subtype-string]"
clause will be generated. If the IP type is
inet46
then an
"icmp-type [subtype-string]"
clause will be used in the resulting
inet
rule and an
"icmp6-type [subtype-string]"
clause will be used in the resulting
inet6
rule.
This all seems pretty simple. What I'm not clear about is this:
The code I created is 100% PHP with some HTML+JS. I can find equivalent PHP in OPNSense and adapt the existing code to work properly. That's trivial. But some of the above, mainly related to the GUI, its fields, and their validation, I'm not at all clear what's required.
Can someone kind, please help me a bit, by running through the 8 code bullets above, and let me know which bullets are a matter of finding+editing the equivalent PHP in OPNSense and just need pure PHP code work to bring into OPNSense, and which bullets will require totally reimplementing in OPNSense because they work totally differently
(XML/Phalcon/validation/mvc/HTML/JS/??)
.
(And the bigger picture is, I have a number of enhancements I've done, and I want to continbute PRs more actively. But I feel stalled by this aspect. Hopefully what I learn from this small port will mean I can PR other useful work I've done, without needing to ask as much.)
«
Last Edit: November 06, 2019, 03:20:06 pm by Stilez
»
Logged
AdSchellevis
Administrator
Hero Member
Posts: 905
Karma: 183
Re: Implementing a better ICMP selector in filter rules
«
Reply #1 on:
November 06, 2019, 07:56:13 pm »
The firewall code is refactored legacy code, which doesn't align very well with the development documents in place.
Since we don't offer support for migrating legacy configurations, we rather keep the stored structure as is for as far as possible. The specific change your proposing might therefor be challenging.
Personally I've never been a great fan of these combined rules, but that doesn't mean you can't suggest a change, as long as it's small and manageable.
If you make sure
icmptype
only lists the items applicable to both, it probably already generates the correct ruleset:
https://github.com/opnsense/core/blob/eeae08415038e80285c100c5e4c425830adc40b3/src/opnsense/mvc/app/library/OPNsense/Firewall/FilterRule.php#L223-L230
I would suggest to keep IPv6 and the related
icmp6-type
as is to limit the scope of the change.
It would probably be possible to fix this in javascript, since the items in
icmptype
should exist in
icmp6-type
when proto IPv4+IPv6 is selected.
https://github.com/opnsense/core/blob/6bb03c18068b816f75d4e40fbee6887886733e5a/src/www/firewall_rules_edit.php#L872-L897
Best regards,
Ad
Logged
Stilez
Newbie
Posts: 27
Karma: 1
Re: Implementing a better ICMP selector in filter rules
«
Reply #2 on:
November 10, 2019, 08:12:31 am »
Done and working really smoothly and nice
Not much code needed to do it, either.
Got about 3 PRs already waiting review, holding off on more just to avoid too much at once. Also some of them fix issues in the rules pages and I'll rebase if they're accepted.
Logged
hbc
Hero Member
Posts: 501
Karma: 47
Re: Implementing a better ICMP selector in filter rules
«
Reply #3 on:
November 11, 2019, 08:01:05 am »
Great. Excited to test it.
Logged
Intel(R) Xeon(R) Silver 4116 CPU @ 2.10GHz (24 cores)
256 GB RAM, 300GB RAID1, 3x4 10G Chelsio T540-CO-SR
Stilez
Newbie
Posts: 27
Karma: 1
Re: Implementing a better ICMP selector in filter rules
«
Reply #4 on:
November 11, 2019, 05:02:23 pm »
Quote from: hbc on November 11, 2019, 08:01:05 am
Great. Excited to test it.
I'm letting a current batch of underlying PRs get sorted out before I PR it. Shouldnt be long with luck. Meantime Im playing round with another project I've wanted for a while - giving Monit the ability to send notifications via other common routes. Making quick progress, its proving pretty simple. Basically making it ready to accept new alert methods people might want, as right now its totally geared to email, rather than a choice of a few delivery methods. And adding at least 1 other method to show it works, however simple - whether IRC, telegram, slack, growl, GSM card via serial, whatever. A lot of ways people might use to get alerts these days, all come down to a very similar function: URL + API key/login + appropriately formatted URL/HTTPS/POST/headers + cURL. So making Monit able to choose between say email and some one other, for an alert, is more about enabling other devs to easily add delivery methods they want in future, if they need to, than about any one specific delivery method.
«
Last Edit: November 11, 2019, 05:07:44 pm by Stilez
»
Logged
bsiege
Newbie
Posts: 8
Karma: 0
Re: Implementing a better ICMP selector in filter rules
«
Reply #5 on:
December 18, 2022, 05:35:16 pm »
Found two topics about this.
One with 1.5k reads, the other with 3k. Maybe there is a need for this indeed ;-) ?
Try to implement this with actual possibilities:
https://www.rfc-editor.org/rfc/rfc4890
Multiple one-liners for reaching this is a pain.
Logged
chrcoluk
Newbie
Posts: 24
Karma: 2
Re: Implementing a better ICMP selector in filter rules
«
Reply #6 on:
March 05, 2024, 09:14:05 am »
So what happened with this? is a bit bizarre each icmp type needs its own rule.
Logged
OPNsense 24.1
Print
Pages: [
1
]
« previous
next »
OPNsense Forum
»
English Forums
»
Development and Code Review
(Moderator:
fabian
) »
Implementing a better ICMP selector in filter rules