Skip to content
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: T4186: Correct icmp type-name options for firewall rules #1173

Closed
wants to merge 3 commits into from

Conversation

nicolas-fort
Copy link
Contributor

@nicolas-fort nicolas-fort commented Jan 16, 2022

Change Summary

In firewall, there were options for icmp type-name that are not supported in nft. Those options were removed, and helpers for options that remains were also changed.
In corcondancy, same corrections are applied for icmpv6

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes)
  • Migration from an old Vyatta component to vyos-1x, please link to related PR inside obsoleted component
  • Other (please describe):

Related Task(s)

Component(s) name

Firewall

Proposed changes

Reduce options for icmp type-name to the ones that are supported in nft. this options are available here
Also description for this options where modified, giving a clearer information of what type of icmp message is being matched.

Same applies to icmpv6 options and helpers. Wifi reference icmpv6

How to test

vyos@vyos# set firewall name FOO rule 10 icmp type-name 
Possible completions:
   echo-reply   ICMP type 0: echo-reply
   destination-unreachable
                ICMP type 3: destination-unreachable
   source-quench
                ICMP type 4: source-quench
   redirect     ICMP type 5: redirect
   echo-request ICMP type 8: echo-request
   router-advertisement
                ICMP type 9: router-advertisement
   router-solicitation
                ICMP type 10: router-solicitation
   time-exceeded
                ICMP type 11: time-exceeded
   parameter-problem
                ICMP type 12: parameter-problem
   timestamp-request
                ICMP type 13: timestamp-request
   timestamp-reply
                ICMP type 14: timestamp-reply
   info-request ICMP type 15: info-request
   info-reply   ICMP type 16: info-reply
   address-mask-request
                ICMP type 17: address-mask-request
   address-mask-reply
                ICMP type 18: address-mask-reply
                
[edit]

vyos@vyos# set firewall ipv6-name FOO-v6 rule 10 icmpv6 
Possible completions:
   code         ICMPv6 code (0-255)
   type         ICMPv6 type (0-255)
   type-name    ICMPv6 type-name

vyos@vyos# set firewall ipv6-name FOO-v6 rule 10 icmpv6 type-name 
Possible completions:
   destination-unreachable
                ICMPv6 type 1: destination-unreachable
   packet-too-big
                ICMPv6 type 2: packet-too-big
   time-exceeded
                ICMPv6 type 3: time-exceeded
   echo-request ICMPv6 type 128: echo-request
   echo-reply   ICMPv6 type 129: echo-reply
   mld-listener-query
                ICMPv6 type 130: mld-listener-query
   mld-listener-report
                ICMPv6 type 131: mld-listener-report
   mld-listener-reduction
                ICMPv6 type 132: mld-listener-reduction
   nd-router-solicit
                ICMPv6 type 133: nd-router-solicit
   nd-router-advert
                ICMPv6 type 134: nd-router-advert
   nd-neighbor-solicit
                ICMPv6 type 135: nd-neighbor-solicit
   nd-neighbor-advert
                ICMPv6 type 136: nd-neighbor-advert
   nd-redirect  ICMPv6 type 137: nd-redirect
   parameter-problem
                ICMPv6 type 4: parameter-problem
   router-renumbering
                ICMPv6 type 138: router-renumbering

Checklist:

  • I have read the CONTRIBUTING document
  • I have linked this PR to one or more Phabricator Task(s)
  • I have run the components SMOKETESTS if applicable
  • My commit headlines contain a valid Task id
  • My change requires a change to the documentation
  • I have updated the documentation accordingly

@nicolas-fort nicolas-fort changed the title T4186 Firewall: T4186: Correct icmp type-name options for firewall rules Jan 16, 2022
Copy link
Member

@c-po c-po left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You have dropped the any type option without a migration script or alternative - this will break existing configurations.

The type explanations are nice - thank you for this. Maybe split this PR in two parts - lave the any option intact and fix the valueHelp, and another commit that does the any cleanup with a migrator.

@nicolas-fort
Copy link
Contributor Author

Now that you pointed out, I can see that without migration script, not only "any" options will break existing configuration. Also all other options that were removed will break existing config.
So, without migration script, this PR makes no sense.

@nicolas-fort
Copy link
Contributor Author

@sarthurdev Think it's ready for adding migration scripts.

@c-po
Copy link
Member

c-po commented Jan 22, 2022

Superseeded by #1184

@c-po c-po closed this Jan 22, 2022
@nicolas-fort nicolas-fort deleted the T4186 branch March 21, 2024 15:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
2 participants