Page MenuHomeFreeBSD

[NEW PORT] security/snort2pfcd
ClosedPublic

Authored by woodsb02 on Aug 30 2016, 12:39 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, May 17, 5:36 AM
Unknown Object (File)
Wed, May 15, 10:25 PM
Unknown Object (File)
Sun, May 12, 1:42 AM
Unknown Object (File)
Sun, May 12, 1:42 AM
Unknown Object (File)
Sun, May 12, 1:42 AM
Unknown Object (File)
Sun, May 12, 1:42 AM
Unknown Object (File)
Sun, May 12, 1:42 AM
Unknown Object (File)
Sun, May 12, 1:42 AM
Subscribers
None

Details

Summary

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

Test Plan

portlint: OK (looks fine.)
testport: OK (poudriere: 9+10, i386+amd64, with MANPAGES option enabled and disabled)

Diff Detail

Repository
rP FreeBSD ports repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

woodsb02 retitled this revision from to [NEW PORT] security/snort2pfcd.
woodsb02 updated this object.
woodsb02 edited the test plan for this revision. (Show Details)
woodsb02 added reviewers: adamw, mat, koobs.
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?

6.27. Starting and Stopping Services (rc Scripts)

2 ↗(On Diff #19842)

Make this optional (OPTIONS_DEFINE=MAN) if you can

security/snort2pfcd/Makefile
19 ↗(On Diff #19842)

Since we're installing manually, this is a good place for the conditional MAN thing

security/snort2pfcd/pkg-plist
1 ↗(On Diff #19842)

Could use PLIST_FILES for a pkg-plist this short

@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

woodsb02 edited edge metadata.
  • 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.

woodsb02 added inline comments.
security/snort2pfcd/Makefile
6 ↗(On Diff #19842)

There are no other MASTER_SITES that I know of.

  • 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.

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

In D7712#160084, @koobs wrote:
  • 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.

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 (why I added as a comment, not (necessarily) requesting changes)

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>
koobs requested changes to this revision.Aug 30 2016, 2:05 PM
koobs edited edge metadata.
In D7712#160084, @koobs wrote:
  • 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.

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 (why I added as a comment, not (necessarily) requesting changes)

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>

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 :)

This revision now requires changes to proceed.Aug 30 2016, 2:05 PM
woodsb02 edited edge metadata.
woodsb02 edited edge metadata.
  • 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.
woodsb02 edited edge metadata.
  • Add the change to security/Makefile back into the review
koobs edited edge metadata.

@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

This revision is now accepted and ready to land.Aug 30 2016, 4:31 PM

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

2016-08-31 02_36_34-_snort2pfcd_ - Google Search - Firefox Developer Edition.png (382×427 px, 12 KB)

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 :-)

This revision was automatically updated to reflect the committed changes.