-
Notifications
You must be signed in to change notification settings - Fork 3.1k
ipfw: Add option for firewall_type to be a directory #1828
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
base: main
Are you sure you want to change the base?
Conversation
If specified, this will run rcorder over a directory. A small subset of examples is also provided Signed-off-by: Dan Mahoney <[email protected]>
|
IIRC this was discussed a bit at BSDCan with @allanjude . |
|
|
@allanjude Can you take care of landing this (either Dan's version or the ScaleEngine one)? As release engineer I'm fine with this going into 15.0 since it won't change the behaviour for existing configurations. |
|
I put in a few files to go into the examples directory, but realistically the whole point of this is that users are going to drop their own in. But because the stock firewall scripts have some good examples for things that you want to do like Bogan blocking, and the rest of that it felt like that was useful and also an example of how a good way to set up pseudo services is. Just like the regular RC.D system has pseudo services. |
|
And yes, the two examples I didn't mention, but did in my talk or: if you're using something like puppet to deploy services, it means that one file can come in with the service then it needs, and also that now since you don't have a model at the firewall file, if a rule fails to load, it doesn't crash the entire stack. |
| do | ||
| ipfw -q $fwfile; | ||
| done | ||
| fi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The indentation looks wonky here, or is that just github rendering it incorrectly somehow?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I may have messed it up. The style in this file is odd -- things are indented with hard-tabs inside a function (but only sometimes?), but two-spaces inside a for loop? I had used a tab inside the for
Here's a screenshot in Nano with whitespace display turned on. (Also look at the function above this one)
But go look at the original file, it's just as weird and other than the "for" loop it's all hard tabs. This file is very inconsistent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I think this is fine then.
| if [ -f "${firewall_type}" ]; then | ||
| ${fwcmd} ${firewall_flags} ${firewall_type} | ||
| else | ||
| if [ -d "${firewall_type}" ]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to update share/man/man5/rc.conf.5 to note that firewall_type can be a directory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That I can do -- this change came out of the way we had modded this file to meet our needs, and obviously, we didn't tweak the docs for our own use. Coming shortly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done (modulo some fighting with github)
|
Thank you for taking the time to contribute to FreeBSD!
Please review CONTRIBUTING.md, then update and push your branch again. |
|
I've added the manpage changes. I don't know how to "sign off" on where I synced the branch with upstream, or if I should remove that from the pull, somehow? |
39f5060 to
5d8c0ea
Compare
Signed-off-by: Dan Mahoney <[email protected]>
|
At this point, with the suggested man changes, I believe this can be merged, please? |
Co-authored-by: Alexander Ziaee <[email protected]>
|
Okay, I've used the Github web UI to accept those changes into my branch. I couldn't figure out how to view those suggestions with the gh command line, and I was confused about adding a signed-off-by line on those since those lines of code are your work, not mine. But either way, the corrected man pages should now be part of the source. |
|
Approved from manpages, cc @allanjude, @markjdb |
|
|
||
| # allow bsd-standard-port traceroutes | ||
| add allow udp from me to any 33434-33600 // traceroute in | ||
| add allow udp from any to me 33434-33600 // traceroute out |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer:
add unreach port udp from any to me dst-port 33434-33626
traceroute expects the host to return a destination port unreachable (ICMP type 3, code 3), by default starting on port net.inet.ip.ttl (64 by default) hops, each with 3 probes by default (

If specified, this will run rcorder over a directory. A small subset of examples is also provided for /usr/share/examples