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

[WIP] Openwrt firewall #162

Draft
wants to merge 45 commits into
base: master
Choose a base branch
from
Draft

[WIP] Openwrt firewall #162

wants to merge 45 commits into from

Conversation

jonathanunderwood
Copy link
Collaborator

@jonathanunderwood jonathanunderwood commented Jul 24, 2020

This builds on #116 to complete the work needed. The parser and renderers are largely incomplete, and there aren't really any tests of note. So, breaking the work down:

  • Merge master so that this branch will merge cleanly
  • Add rules parser and tests
  • Add schema for missing rules parameters
  • Add parser, renderer and tests for missing rules parameters
  • Add zones parser and tests
  • Add firewall defaults parser and tests
  • Add forwardings parser and tests
  • Add missing name and enabled fields to forwarding
  • Add redirect schema
  • Add redirect renderer, parser and tests
  • Add includes schema, parser, renderer and tests
  • Define and use a network regex throughout schema
  • Review test coverage, ensure ValidationError tests are included
  • Review schema for opportunities to restrict values
  • Check that all types have name and enabled fields which are tested
  • Add documentation
  • Review required fields for all objects for consistency with upstream (ensure name is required everywhere except defaults)

Note: this reference document is the source of truth for OpenWRT firewall configuration parameters.

Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

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

@jonathanunderwood thanks for getting this started, I'm having a hard time locating the real changes due to the noise, see my comment below.

docs/source/conf.py Outdated Show resolved Hide resolved
@jonathanunderwood jonathanunderwood force-pushed the openwrt-firewall branch 3 times, most recently from 40d0115 to 8e4f186 Compare July 28, 2020 10:20
This commit adds a firewall rule UCI parser to the OpenWRT backend. This commit
includes:
    - Fixing test_default.py to use the new parser
    - New tests in test_firewall.py
@coveralls
Copy link

coveralls commented Jul 28, 2020

Coverage Status

Coverage decreased (-0.2%) to 99.705% when pulling 8c6ac92 on openwrt-firewall into 0b598e1 on master.

@jonathanunderwood
Copy link
Collaborator Author

@nemesisdesign How do you recommend dealing with a UCI parameter that can either be a string or a list? One such example is the proto parameter in a firewall rule. Both of the following rules are legitimate:

config rule 'rule_Allow_Ping'
    option name 'Allow-Ping'
    option src 'wan'
    option proto 'icmp'
    option family 'ipv4'
    list icmp_type 'echo-request'
    option target 'ACCEPT'
    option enabled '0'

config rule 'rule_Allow_Isolated_DHCP'
    option name 'Allow-Isolated-DHCP'
    option src 'isolated'
    list proto 'udp'
    option dest_port '67-68'
    option target 'ACCEPT'

At present, in the schema, this parameter is specified as a string. One option would be to define it to be an array, and always co-erce it to a list when parsing. But perhaps there are other options/precedent?

@jonathanunderwood jonathanunderwood force-pushed the openwrt-firewall branch 2 times, most recently from 2e8f244 to a8117e6 Compare July 28, 2020 17:16
@jonathanunderwood
Copy link
Collaborator Author

@nemesisdesign How do you recommend dealing with a UCI parameter that can either be a string or a list? One such example is the proto parameter in a firewall rule. Both of the following rules are legitimate:
[snip]
At present, in the schema, this parameter is specified as a string. One option would be to define it to be an array, and always co-erce it to a list when parsing. But perhaps there are other options/precedent?

For now, I have gone with this approach.

@nemesifier
Copy link
Member

@nemesisdesign How do you recommend dealing with a UCI parameter that can either be a string or a list? One such example is the proto parameter in a firewall rule. Both of the following rules are legitimate:
[snip]
At present, in the schema, this parameter is specified as a string. One option would be to define it to be an array, and always co-erce it to a list when parsing. But perhaps there are other options/precedent?

For now, I have gone with this approach.

Yes that should work!

This commit:
    - Adds a schema for redirects
    - Adds a renderer and parser for redirects
    - Adds initial tests for redirects
Treat these parameters as an array, and drop support for negating
the values with a "!" in JSON as it is unneeded.
This change enables parsing of the "!" character for monthdays
and weekdays paraemters of a firewall redirect object when present
in UCI configuration.

This change does not add support for negation in NetJSON config.
@jonathanunderwood
Copy link
Collaborator Author

@nemesisdesign any idea what's going on with the test execution - travis/coveralls seems to have been waiting forever?

@nemesifier
Copy link
Member

@jonathanunderwood travis CI does not offer free plans for open source anymore and they're not renewing our OSS credits, we're moving to all the repositories of OpenWISP to Github actions #178, but it's taking time to do it for all the modules.

You can run tests locally for now, when it's ready for review or need feedback let us know.

@jonathanunderwood jonathanunderwood force-pushed the openwrt-firewall branch 2 times, most recently from 53d407c to f0225e9 Compare March 20, 2021 12:34
This removes the remnants of an attempt to manage older firewall
configurations from before the firewall schema was introduced.
However, this handling was incomplete and flawed, and so this code
is pointless.
@ValerioBob
Copy link

Is there any intention to carry on this branch?

@ValerioBob
Copy link

@nemesisdesign I find this feature very useful and I want to contribute. If it is ok I can spend some of my free time on to carry on this work.
I managed to merge it with the master branch: the test coverage is ok but the schema has some problems when used by the frontend, more specifically using 'allOf' and '$ref'. Replacing the '$ref's with the corresponding fragments fixed it, so it's just written in a wrong way.
Please let me know if I can contribute to this branch and any type of information that can be useful

@jonathanunderwood
Copy link
Collaborator Author

@ValerioBob Sadly I ran out of time to work on this back in 2021. There's actually not that much left to do, so please feel free to dig in and add commits. I'll try and find some time to come back to it in the coming weeks as well.

@ValerioBob
Copy link

@jonathanunderwood Hi! It would be awesome! Rn I don't have so much time but I will in some weeks.
On my repositories you can find a fork with my contributions. As I remember with the Firewall I was almost done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: In progress
Development

Successfully merging this pull request may close these issues.

7 participants