Implementing a better ICMP selector in filter rules

[Stilez]

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.)

[AdSchellevis]

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

[Stilez]

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.

[hbc]

Great. Excited to test it.

[Stilez]

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.

[bsiege]

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.

[chrcoluk]

So what happened with this? is a bit bizarre each icmp type needs its own rule.