Add new port security/snort2pfcd, which analyzes snort alert output and
blocks ip addresses using pf for a specified period of time.
PR: 211813
Submitted by: onestsam@gmail.com
Details
Details
portlint: OK (looks fine.)
testport: OK (poudriere: 9+10, i386+amd64, with MANPAGES option enabled and disabled)
Diff Detail
Diff Detail
- Repository
- rP FreeBSD ports repository
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| security/snort2pfcd/Makefile | ||
|---|---|---|
| 6 ↗ | (On Diff #19842) | Add second MASTER_SITES if you can find one |
| 14 ↗ | (On Diff #19842) | Tip: Prefer using ./configure or other methods like --libfoo-prefix=${LOCALBASE}/lib or similarly narrower-scoped methods (if possible) over widely-scoped global *FLAG modification like USES=localbase does. Some implicit stuff can happen with the latter (like autodetection of other libraries that are supported). |
| security/snort2pfcd/pkg-plist | ||
| 1 ↗ | (On Diff #19842) | Missing corresponding USE_RC_SUBR in Makefile? |
| 2 ↗ | (On Diff #19842) | Make this optional (OPTIONS_DEFINE=MAN) if you can |
Comment Actions
@woodsb02 For review TEST PLAN sections, something like the following provides better clarity and more than just 'what test steps were run' which has greater review(er) value, particularly if certain things fail but are 'ok' (eg: unit tests with upstream issue reports, a certain arch/version failure, etc)
* portlint: OK (looks fine.) * testport: OK (poudriere: <versions>, <archs>, <OPTIONS> tested) * maketest: OK (15231 tests PASSED in 21 seconds)
I reckon Phabricator (or we) should rename TEST PLAN to TEST RESULTS
Comment Actions
- Change from USES=localbase to CFLAGS+=-I${LOCALBASE}/include Since this port does not have a configure script (or similar), there is no way to pass a narrower scoped method for detailing where to find the headers installed by the devel/libcidr port.
- Add USE_RC_SUBR, and provide a local copy of the rc script in files to allow /usr/local to be replaced with %%LOCALBASE%%.
- Added MANPAGES option, enabled by default.
- Move from pkg-plist to PLIST_FILES
| security/snort2pfcd/Makefile | ||
|---|---|---|
| 19 ↗ | (On Diff #19842) | Why put it behind a knob? The MANPAGES knob is generally for when large external dependencies are required to build the manpages. |
| security/snort2pfcd/Makefile | ||
|---|---|---|
| 6 ↗ | (On Diff #19842) | There are no other MASTER_SITES that I know of. |
Comment Actions
If there are no lib-specific wrksrc/Makefile variables you can use (LIBFOO_CFLAGS|LDFLAGS|LIBS) and if all it needs is the header location (and not the lib location as well), then this is fine, otherwise USES=localbase is ok.
I added this as a comment (as I did not inspect the sources), not as a blocking review change.
| security/snort2pfcd/Makefile | ||
|---|---|---|
| 19 ↗ | (On Diff #19842) | Generally yes, but also principle: user choice (we <3 it), other ports do it and users (un)set it not expecting man pages. It should of course be on by default (packages), and this was only a suggested change, not a required one |
Comment Actions
The lib location does not need to be provided, since the ${WRKSRC}/Makefile already has LDFLAGS -L${LOCALBASE} set for that.
It might help to know that this port does not have any configure script, cmake or similar... it simply has the following ${WRKSRC}/Makefile:
PROG= snort2pfcd
SRCS= main.c parser.c kevent.c spfc.c ioctl_helpers.c
MAN= ${PROG}.8
CFLAGS+=-Wall -Werror -pedantic
LDFLAGS+=-lutil -L${LOCALBASE}/lib -lcidr
.include <bsd.prog.mk>Comment Actions
Weird (why -L but not -I?) but lovely nonetheless: what you have now (CFLAGS only in port Makefile) is the minimal, most specifically-scoped thing you can do. Good.
By the way, you want to patch out -Werror :)
Tell upstream not to include it in release builds/distributions
| security/snort2pfcd/Makefile | ||
|---|---|---|
| 6 ↗ | (On Diff #19842) | Tick "Done" next to review items you address :) |
Comment Actions
- Patch -Werror out of ${WRKSRC}/Makefile
- While here, move CFLAGS+=-I${LOCALBASE}/include into ${WRKSRC}/Makefile, in the hope that the patch will get upstreamed.
Comment Actions
@woodsb02 Looks immaculate.
I'm a pedant, so would have CFLAGS+=-Wno-error in the port Makefile instead of the patch, though having said that, an annoying patch is always a good reminder/nudge to get it upstreamed, which is a good indicator of a quality committer/contributor.
Approved, nice work
Comment Actions
Just a note, phabricator is showing two differing file/path modifications for this changeset, which is odd since they're both for the same port, and which I'm guessing is not reflected in an svn status at your end.
Worth checking just in case pre-commit
| security/snort2pfcd/Makefile | ||
|---|---|---|
| 17–19 ↗ | (On Diff #19848) | Is there a good reason *not* to install the man page ? I mean, if it took all of doxygen and texlive to build, yeah maybe, but, mmm, it's already there... This is FreeBSD, not Linux, we install the documentation. |
| security/snort2pfcd/pkg-descr | ||
| 1–3 ↗ | (On Diff #19848) | This could grow a WWW line, if a web site exist. |
| security/snort2pfcd/Makefile | ||
|---|---|---|
| 17–19 ↗ | (On Diff #19848) | Indeed, I will remove the MANPAGES option pre-commit. |
| security/snort2pfcd/pkg-descr | ||
| 1–3 ↗ | (On Diff #19848) | No website exists for this, as far as I can tell. I will prompt the maintainer to possibly create one or submit to a source code repository as a future improvement (I believe the maintainer may also be the sole developer). |
| security/snort2pfcd/pkg-descr | ||
|---|---|---|
| 1–3 ↗ | (On Diff #19848) | If there is no web site, don't bother, it was just in case there was already one :-) |