firewalld: close mdns port in public (default) zone #15740
Conversation
ddstreet
requested review from
christopherco,
ddstreetmicrosoft and
reubeno
as code owners
| @@ -0,0 +1,11 @@ | |||
| diff -urpN a/config/zones/public.xml firewalld-2.4.0/config/zones/public.xml | |||
There was a problem hiding this comment.
Remind me -- is the base Fedora version of this XML file a loose file in the dist-git repo for firewalld, or does it come upstream-upstream from the repo housing firewalld's code base? (Or are they one and the same?)
If it's a loose file in the package (and not buried in a tarball), we have 2 choices:
- Apply a patch like this.
- Use an overlay to directly edit the file (e.g., textually remove the mdns entry).
If it's not, this is really the only option.
There was a problem hiding this comment.
ok clearly I didn't spend enough time actually looking at what I was doing here...as upstream firewalld actually doesn't include mdns in the public zone, it's added by fedora:
https://src.fedoraproject.org/rpms/firewalld/blob/rawhide/f/fedora-only-MDNS-default.patch
So if we do decide to remove it, we should simply remove fedora's patch, instead of adding our own.
It is good to see that fedora provided analysis/justification for including mdns in their patch; I'm inclined to follow their lead on this one, unless someone with more security expertise on our team thinks we should close the mdns port.
|
|
||
| [[components.firewalld.overlays]] | ||
| type = "spec-add-tag" | ||
| description = "Close mdns port in public (i.e. default) zone" |
There was a problem hiding this comment.
Something that I think is important is documenting the why/rationale for changes made via overlays or patches so that the current state of this repo is sufficient to understand the deviations made from upstream.
What do you think would be a good pattern for doing this? Do you think we should add a required reason field?
There was a problem hiding this comment.
I think the best place for a more detailed description of whatever change we are making is the actual commit message in git. I don't think putting it into the toml file is the best place for it.
There was a problem hiding this comment.
the exception is an added patch file; i think all patch files we add should ideally be full git format-patch format (preferably actually generated by git) and should include the commit description (which should be a complete description/rationale in every patch)
There was a problem hiding this comment.
If it's an added patch, I 100% agree that the detailed description belongs in the patch itself, with a short hint in the TOML file.
Do you know if there are any scripts/tools out there that can validate if a patch is a well-formatted git format-patch style patch? That seems like a linter we'd want to enable -- or at least something we'd want AI code reviewing to "watch out for".
There was a problem hiding this comment.
Do you know if there are any scripts/tools out there that can validate if a patch is a well-formatted git format-patch style patch?
I am (yet again) only really familiar with the deb-world tooling for this, and over there patches are supposed to conform to DEP-3, and there is a patchedit tool that can check them (as well as some of the build tooling including built-in checking, IIRC).
There was a problem hiding this comment.
Good to know. I see https://docs.fedoraproject.org/en-US/packaging-guidelines/PatchUpstreamStatus/ but couldn't quickly find guidelines on patch formats, nor could I find clear evidence as to whether rpmlint inspects patches. (It does look some at how the patches are referenced in .spec files.) Possibly an opportunity for contribution?
ddstreet
force-pushed
the
firewalld
branch
2 times, most recently
from
97c5fad to
fc314d9
Compare
Upstream firewalld does not open the mdns port by default, while Fedora does with reasoning: https://fedoraproject.org/wiki/Desktop/Whiteboards/AvahiDefault However, the use cases for mdns do not seem to apply for AZL4 whose primary use case is enterprise systems, mainly in the cloud. So we remove the Fedora patch and revert to upstream configuration without mdns exposed.
|
Ok, I updated this to remove the fedora patch, instead of adding our own patch. The git commit has a bit of reasoning on why. |
| tag = "Patch0" | ||
|
|
||
| [[components.firewalld.overlays]] | ||
| type = "file-remove" |
There was a problem hiding this comment.
this is potentially a policy choice...when we "remove" existing upstream patches, should we simply remove their PatchN: line in the spec and leave the actual patch file? Or remove the patch file as well?
Also, this policy decision could be moved into azldev if a spec-remove-patch command type is added.
There was a problem hiding this comment.
Agreed on spec-remove-patch abstracting that. Would you mind filing an issue on the azldev repo to request that?
It could also be less brittle -- being able to, say, match a patch by filename pattern. That would be resilient against the patch being renamed to, say, Patch1.
reubeno
left a comment
reubeno
left a comment
There was a problem hiding this comment.
I think this is a reasonable way to disable the patch for now, with some clear follow-ups to extend azldev to simplify adding/removing patches.
2 participants
This closes the
mdnsport in thepubliczone, which is the default zone.