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

T6188: add description to show firewall #3219

Merged
merged 4 commits into from
Apr 6, 2024
Merged

T6188: add description to show firewall #3219

merged 4 commits into from
Apr 6, 2024

Conversation

l0crian1
Copy link
Contributor

@l0crian1 l0crian1 commented Mar 30, 2024

Change Summary

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)

https://vyos.dev/T6188

Related PR(s)

Component(s) name

src/op_mode/firewall.py

Proposed changes

This change adds the Firewall Description that exists under the rules config as a column in "show firewall" op-mode commands.

Description column was added for these commands and their subsections:

show firewall statistics
show firewall groups
show firewall <family>

Detail view was added for these commands:

show firewall bridge forward filter detail
show firewall bridge forward filter rule <rule#> detail
show firewall bridge name <chain> detail
show firewall bridge name <chain> rule <rule#> detail

show firewall ipv4 forward filter detail
show firewall ipv4 forward filter rule <rule#> detail
show firewall ipv4 input filter detail
show firewall ipv4 input filter rule <rule#> detail
show firewall ipv4 output filter detail
show firewall ipv4 output filter rule <rule#> detail
show firewall ipv4 name <chain> detail
show firewall ipv4 name <chain> rule <rule#> detail

show firewall ipv6 forward filter detail
show firewall ipv6 forward filter rule <rule#> detail
show firewall ipv6 input filter detail
show firewall ipv6 input filter rule <rule#> detail
show firewall ipv6 output filter detail
show firewall ipv6 output filter rule <rule#> detail
show firewall ipv6 name <chain> detail
show firewall ipv6 name <chain> rule <rule#> detail

show firewall group detail
show firewall group <group> detail

How to test

Type "show firewall" command(s):

l0crian@R86S:~$ show firewall ipv4 forward filter rule 20
Rule Information

---------------------------------
ipv4 Firewall "forward filter"

Rule     Description    Action    Protocol      Packets    Bytes  Conditions
-------  -------------  --------  ----------  ---------  -------  -----------------------------------------------------------------------
20       Bogons         drop      all                 0        0  ip daddr @N_BOGONS oifname "eth0.4040"  prefix "[ipv4-FWD-filter-20-D]"
default                 drop      all                 0        0

Type "show firewall" command with detail option available:

l0crian@R86S:~$ show firewall ipv4 forward filter rule 20 detail
Rule Information

---------------------------------
ipv4 Firewall "forward filter"

 Rule        | 20
 Description | Bogons
 Action      | drop
 Protocol    | all
 Packets     | 0
 Bytes       | 0
 Conditions  | ip daddr @N_BOGONS oifname "eth0.4040"  prefix "[ipv4-FWD-filter-20-D]"

-->

Smoketest result

  • 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

@vyosbot vyosbot requested review from a team, dmbaturin, sarthurdev, zdc, jestabro, sever-sever and c-po and removed request for a team March 30, 2024 15:49
@Apachez-
Copy link
Contributor

Since descriptions can be very long I assume there will be a linewrap at the end?

Would it be possible to add the method MySQL is using where you can have some terminator of the show command to get it in lines instead of columns (in MySQL case its the use of "\G" instead of ";")?

Like regular output is:

Rule     Description    Action    Protocol      Packets    Bytes  Conditions
-------  -------------  --------  ----------  ---------  -------  -----------------------------------------------------------------------
20       Bogons         drop      all                 0        0  ip daddr @N_BOGONS oifname "eth0.4040"  prefix "[ipv4-FWD-filter-20-D]"
default                 drop      all                 0        0

While with a "I want this in lines instead of columns" terminator the output would be something like this instead:

Rule:        20
Description: Bogons
Action:      drop
Protocol:    all
Packets:     0
Bytes:       0
Conditions:  ip daddr @N_BOGONS oifname "eth0.4040" prefix "[ipv4-FWD-filter-20-D]"

Rule:        default
Description: 
Action:      drop
Protocol:    all
Packets:     0
Bytes:       0
Conditions:  

@l0crian1
Copy link
Contributor Author

Since descriptions can be very long I assume there will be a linewrap at the end?

It does not currently wrap, though that is something I was hoping to get feedback on. If the desire is to wrap, after how many characters should it wrap? My immediate thoughts were 50. I set this to 30 just to show how it would look in general:

Rule     Description                  Action    Protocol      Packets    Bytes  Conditions
-------  ---------------------------  --------  ----------  ---------  -------  ------------------------------------------------------------------------------
5        Allow return traffic from    offload   all              3612   227256  ct state { established, related }  flow add @VYOS_FLOWTABLE_FLOW1
         inside network

The other output should be possible, I could do something like this:

def show_firewall_vertical(rules):
    headers = ["Rule", "Description", "Action", "Protocol", "Packets", "Bytes", "Conditions"]
    
    max_header_length = max(len(header) for header in headers)
    
    for rule in rules:
        for header, item in zip(headers, rule):
            formatted_header = header.ljust(max_header_length)
            print(f"{formatted_header}  : {item}")
        print()

def output_firewall_name(family, hook, priority, firewall_conf, single_rule_id=None):
    if rows:
        if <some arg is passed>:
            show_firewall_vertical(rows)
        else:
            header = ['Rule', 'Description', 'Action', 'Protocol', 'Packets', 'Bytes', 'Conditions']
            print(tabulate.tabulate(rows, header) + '\n')

This is the output when I run it:

root@R86S:/usr/libexec/vyos/op_mode# show firewall ipv4 forward filter rule 20
Rule Information

---------------------------------
ipv4 Firewall "forward filter"

Rule         : 20
Description  : Bogons
Action       : drop
Protocol     : all
Packets      : 0
Bytes        : 0
Conditions   : ip daddr @N_BOGONS oifname "eth0.4040"  prefix "[ipv4-FWD-filter-20-D]"

Rule         : default
Description  :
Action       : drop
Protocol     : all
Packets      : 0
Bytes        : 0

We'd need a useful switch in the op-definition structure to call it for passing the arg. Maybe something like "fieldview"? Definitely open to suggestions there.

@Apachez-
Copy link
Contributor

I think the wrapping should be left for the output to select since you can either be in a regular serialconsole of 80x25 or some highresmode which brings more characters per line or even through SSH with a 4k monitor which will be plenty of lines.

Suggestion regarding command in op mode:

show firewall mode vertical

Which would allow for looking at a specific rule aswell like so:

show firewall chain input rule 20 mode vertical

"mode" could of course be called "display" or "view" or something proper to be reused for other commands where an vertical output could be something good to be able to choose.

@sever-sever
Copy link
Member

I like the idea of the output.

root@R86S:/usr/libexec/vyos/op_mode# show firewall ipv4 forward filter rule 20
Rule Information

---------------------------------
ipv4 Firewall "forward filter"

Rule         : 20
Description  : Bogons
Action       : drop
Protocol     : all
Packets      : 0
Bytes        : 0
Conditions   : ip daddr @N_BOGONS oifname "eth0.4040"  prefix "[ipv4-FWD-filter-20-D]"

Rule         : default
Description  :
Action       : drop
Protocol     : all
Packets      : 0
Bytes        : 0

But it should show only rule 20 in this case :)
What about one of them?

show firewall ipv4 forward filter rule 20 detail
show firewall ipv4 forward filter rule 20 verbose

@l0crian1
Copy link
Contributor Author

But it should show only rule 20 in this case :)

I'm currently just taking the 'rows' list that is created in all of the different 'output_firewall' functions and reformatting it to that view. That way it will work for all of the different "show firewall" op commands. I can pop the second element in the list if the rules argument is present to remove the default action from that view:

def output_firewall_vertical(rules, headers):
    if args.rule:
        rules.pop()

    max_header_length = max(len(header) for header in headers)
    for rule in rules:
        for header, item in zip(headers, rule):
            formatted_header = header.ljust(max_header_length)
            print(f"{formatted_header}  : " + item.replace("\n"," "))
        print()

That generates this output:

l0crian@R86S# run show firewall ipv4 forward filter rule 20 detail
Rule Information

---------------------------------
ipv4 Firewall "forward filter"

Rule         : 20
Description  : Bogons
Action       : drop
Protocol     : all
Packets      : 0
Bytes        : 0
Conditions   : ip daddr @N_BOGONS oifname "eth0.4040"  prefix "[ipv4-FWD-filter-20-D]"

But it should show only rule 20 in this case :) What about one of them?

show firewall ipv4 forward filter rule 20 detail
show firewall ipv4 forward filter rule 20 verbose

I like the simplicity of show firewall ipv4 forward filter rule 20 detail.

@l0crian1
Copy link
Contributor Author

@sever-sever

The normal command when looking at rules also shows the default rule:

vyos@vyos# run show firewall ipv4 forward filter rule 10
Rule Information

---------------------------------
ipv4 Firewall "forward filter"

Rule     Description    Action    Protocol      Packets    Bytes  Conditions
-------  -------------  --------  ----------  ---------  -------  ------------
10                      accept    all                 0        0  accept
default                 accept    all                 0        0

Do you want me to make that change in general, where the default rule isn't listed when looking at a specific rule? So it would look like this whenever trying to look at a specific rule:

vyos@vyos# run show firewall ipv4 forward filter rule 10
Rule Information

---------------------------------
ipv4 Firewall "forward filter"

Rule     Description    Action    Protocol      Packets    Bytes  Conditions
-------  -------------  --------  ----------  ---------  -------  ------------
10                      accept    all                 0        0  accept

@sever-sever
Copy link
Member

Do you want me to make that change in general, where the default rule isn't listed when looking at a specific rule? So it would look like this whenever trying to look at a specific rule:

If you know how to fix it, then fix it :)

What I expect we'll have include the leaf-node detail

show firewall detail
show firewall ipv4 detail
show firewall ipv4 forward filter detail
show firewall ipv4 forward filter rule 20 detail

Or start with only the rule, then we'll think about if we want to see it in other places

l0crian1 added 2 commits April 1, 2024 11:14
      - Added show firewall <sections> detail paths
	modified:   src/op_mode/firewall.py
      - Added Description as a header to normal "show firewall" commands
      - Added 'detail' view which shows the output in a list key-pair format
Description column was added for these commands and their subsections:
show firewall statistics
show firewall groups
show firewall <family>

Detail view was added for these commands:
show firewall bridge forward filter detail
show firewall bridge forward filter rule <rule#> detail
show firewall bridge name <chain> detail
show firewall bridge name <chain> rule <rule#> detail

show firewall ipv4 forward filter detail
show firewall ipv4 forward filter rule <rule#> detail
show firewall ipv4 input filter detail
show firewall ipv4 input filter rule <rule#> detail
show firewall ipv4 output filter detail
show firewall ipv4 output filter rule <rule#> detail
show firewall ipv4 name <chain> detail
show firewall ipv4 name <chain> rule <rule#> detail

show firewall ipv6 forward filter detail
show firewall ipv6 forward filter rule <rule#> detail
show firewall ipv6 input filter detail
show firewall ipv6 input filter rule <rule#> detail
show firewall ipv6 output filter detail
show firewall ipv6 output filter rule <rule#> detail
show firewall ipv6 name <chain> detail
show firewall ipv6 name <chain> rule <rule#> detail

show firewall group detail
show firewall group <group> detail
      - modified:   src/op_mode/firewall.py
Changed behavior of "show firewall" for specific rule to only show rule and not also default-action
@l0crian1
Copy link
Contributor Author

l0crian1 commented Apr 1, 2024

If you know how to fix it, then fix it :)

I didn't see this before pushing the commit right now. I can just move the popping of the list under the output_firewall_name function instead of just in the output_firewall_vertical function.

I just pushed that commit, this should be done at this point.

Here is a summary of the additional show capabillities.

Description column was added for these commands and their subsections. Descriptions are wrapped at 50 characters for readability with long descriptions:

show firewall statistics
show firewall groups
show firewall <family>

Detail view was added for these commands. Values are wrapped to 100 characters for readability:

show firewall bridge forward filter detail
show firewall bridge forward filter rule <rule#> detail
show firewall bridge name <chain> detail
show firewall bridge name <chain> rule <rule#> detail

show firewall ipv4 forward filter detail
show firewall ipv4 forward filter rule <rule#> detail
show firewall ipv4 input filter detail
show firewall ipv4 input filter rule <rule#> detail
show firewall ipv4 output filter detail
show firewall ipv4 output filter rule <rule#> detail
show firewall ipv4 name <chain> detail
show firewall ipv4 name <chain> rule <rule#> detail

show firewall ipv6 forward filter detail
show firewall ipv6 forward filter rule <rule#> detail
show firewall ipv6 input filter detail
show firewall ipv6 input filter rule <rule#> detail
show firewall ipv6 output filter detail
show firewall ipv6 output filter rule <rule#> detail
show firewall ipv6 name <chain> detail
show firewall ipv6 name <chain> rule <rule#> detail

show firewall group detail
show firewall group <group> detail

show firewall statistics detail

I didn't add a general detail output for show firewall <family> since I worried it would just too much output in larger deployments. I think it's better to make the user call a more explicit section like show firewall ipv4 forward filter detail or show firewall ipv4 forward filter rule 20 detail

@Apachez-
Copy link
Contributor

Apachez- commented Apr 1, 2024

Even if "rule 20 detail" should show just rule 20 I think its handy to also always display what is the default.

On the other hand there can be other rules that will get a hit so always displaying default might be a false sense of security as in "nice, this packet wont match rule 20 so it should hit the default" while in fact you might have for example rule 30 (which isnt displayed) which would also match and use its action before it hits the default.

So I would probably vote for to not display the default unless no rules are specified (or "rule any" is specified) or the "rule default" is specified.

@sever-sever
Copy link
Member

Can we see the description only for detail?
For example, this config

set firewall ipv4 forward filter rule 10 action 'accept'
set firewall ipv4 forward filter rule 10 description 'Allow transport protocol TCP'
set firewall ipv4 forward filter rule 10 protocol 'tcp'
set firewall ipv4 forward filter rule 20 action 'accept'
set firewall ipv4 forward filter rule 20 description 'Allow transport protocol UDP and something else 12345 because test reason'
set firewall ipv4 forward filter rule 20 protocol 'udp'

It looks ok if we check firewall rules via SSH
det01
It looks completely unreadable if you connect via the console

det02

For readability in console sessions, moved the description column to only be shown in the detail view.

Changed wrapping in the detail view for description to 65 characters to prevent full line wrapping in console sessions.
@l0crian1
Copy link
Contributor Author

l0crian1 commented Apr 5, 2024

Can we see the description only for detail?

Done!

I also changed the wrapping for the description column in the detail view to 65 characters to keep the overall line below 80 characters.

@sever-sever sever-sever merged commit df2add5 into vyos:current Apr 6, 2024
5 of 7 checks passed
@c-po
Copy link
Member

c-po commented Apr 6, 2024

@Mergifyio backport sagitta

Copy link
Contributor

mergify bot commented Apr 6, 2024

backport sagitta

✅ Backports have been created

dmbaturin added a commit that referenced this pull request Apr 6, 2024
T6188: add description to show firewall (backport #3219)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

4 participants