Page MenuHomeFreeBSD

Add basic blacklist build support
ClosedPublic

Authored by lidl on Apr 11 2016, 2:37 PM.

Details

Summary

Add support for building NetBSD's blacklist library, blacklistd daemon
and blacklistctl control program to FreeBSD. Enhance the 520.pfdenied
periodic script to report on the automatically generated blacklist
tables. Add a startup script for blacklistd.

Depends on D5912

Sponsored by: The FreeBSD Foundation

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

lidl updated this revision to Diff 15081.Apr 11 2016, 2:37 PM
lidl retitled this revision from to Add basic blacklist build support.
lidl updated this object.
lidl edited the test plan for this revision. (Show Details)
lidl added reviewers: rpaulo, emaste, gnn.
lidl added reviewers: bapt, sjg.Apr 11 2016, 4:19 PM
bapt added inline comments.Apr 11 2016, 8:08 PM
etc/periodic/security/520.pfdenied
51 ↗(On Diff #15081)

Why enforcing nawk and not using the regular awk?

lib/libblacklist/Makefile
9 ↗(On Diff #15081)

I do not think it is necessary to add it to /lib instead of the regular /usr/lib given there is "nothing vital about this lib"

sbin/blacklistctl/Makefile
13 ↗(On Diff #15081)

why not adding in a single line?

BTW the LDFLAGS is not necessary here given this is already default

sbin/blacklistd/Makefile
3 ↗(On Diff #15081)

Useless inclusion

15 ↗(On Diff #15081)

Same as above

emaste added inline comments.Apr 11 2016, 8:16 PM
lib/libblacklist/Makefile
9 ↗(On Diff #15081)

Do we have daemons in /bin or /sbin that might (later on) depend on blacklistd, and could start before /usr is mounted?

lidl added inline comments.Apr 11 2016, 9:45 PM
etc/periodic/security/520.pfdenied
51 ↗(On Diff #15081)

I am just using what was there before - I don't know why it is currently specified as 'nawk'.

lib/libblacklist/Makefile
9 ↗(On Diff #15081)

OK, I'll move it to /usr/lib. I was thinking that sshd did not have a requirement for /usr/lib, but checking shows that it does require /usr/lib.

sbin/blacklistctl/Makefile
13 ↗(On Diff #15081)

I looked at a few different Makefiles that use LIBADD, and the ones already in the tree do it both ways - listing on a single line, and listing on multiple lines. I'd be happy to make it a single line.

As for the LDFLAGS settings, it doesn't compile on a system that doesn't already have it installed without setting LDFLAGS. Once the library is already installed, the LDFLAGS isn't needed. Until the library is installed, it needs LDFLAGS to be set, to find the library file in the OBJDIR.

Since the introduction of this will be a new feature, not setting LDFLAGS would break everyone's build.

sbin/blacklistd/Makefile
3 ↗(On Diff #15081)

I will fix this.

15 ↗(On Diff #15081)

I'll smash the LIBADD lines into one.

Same response for why LDFLAGS needs to be set.

lidl added a comment.Apr 11 2016, 10:02 PM

In light of the request to move the library into /usr/lib, rather than /lib, I wonder if the binaries and helper program ought to be installed into /usr/sbin (rather than /sbin) and the helper into /usr/libexec too?

bapt edited edge metadata.Apr 12 2016, 7:41 AM
In D5913#126142, @lidl wrote:

In light of the request to move the library into /usr/lib, rather than /lib, I wonder if the binaries and helper program ought to be installed into /usr/sbin (rather than /sbin) and the helper into /usr/libexec too?

Yes

emaste added inline comments.Apr 12 2016, 1:24 PM
etc/periodic/security/520.pfdenied
51 ↗(On Diff #15081)

It came from rS138061. This one is the only case of nawk in etc/ (vs. 31 awk). It probably makes sense to switch it in a separate commit.

lib/libblacklist/Makefile
9 ↗(On Diff #15081)

Note that since sshd itself is in /usr/sbin /usr must be mounted for it to start anyhow :-)

share/mk/bsd.libnames.mk
25–27 ↗(On Diff #15081)

I'd just set the variable unconditionally.

lidl updated this revision to Diff 15135.Apr 13 2016, 5:35 AM
lidl edited edge metadata.

Address code review comments: move lib to /usr/lib, move binaries
into /usr/sbin, add separate config file, add additional man pages
to the OptionalObsoleteFiles.inc file.

wblock added a subscriber: wblock.Apr 13 2016, 3:34 PM
sjg edited edge metadata.Apr 14 2016, 2:44 AM

Does FreeBSD have style guide for makefiles?

VAR+= \
<TAB>value \
<TAB>value2 \

etc is both more efficient than lots of VAR+= and also aids review when changes are made (assuming the list is kept sorted)

lib/libblacklist/Makefile
3 ↗(On Diff #15135)

FWIW ${.CURDIR:H:H} results in much neater paths than
${.CURDIR}/../..

lidl updated this revision to Diff 16967.May 27 2016, 4:08 AM
lidl updated this object.
lidl edited edge metadata.
lidl marked an inline comment as done.

Updated to address review comments.

Add copyright headers to FreeBSD Foundation where needed.

rpaulo accepted this revision.May 27 2016, 7:20 PM
rpaulo edited edge metadata.
rpaulo added inline comments.
libexec/Makefile
8 ↗(On Diff #16967)

I think we tend to avoid '-' in variable names.

share/mk/src.libnames.mk
182 ↗(On Diff #16967)

Remove empty space.

This revision is now accepted and ready to land.May 27 2016, 7:20 PM
lidl added inline comments.May 27 2016, 8:06 PM
libexec/Makefile
8 ↗(On Diff #16967)

It's not my exact preference either, but there's the existence proof of both "_rtld-elf" and "_tftp-proxy" in the same list of variables. So it's already being done. (Perhaps it should be undone, but that would be a different set of patches.)

I was pretty careful to make sure that I wasn't adding something that did not match the existing code.

share/mk/src.libnames.mk
182 ↗(On Diff #16967)

I'm torn about doing this. The code above (setting _LIBRARIES) ends the same way, with a trailing backslash, and the obligatory blank line. As does the code below my addition (appending to _LIBRARIES for MK_OFED). Another trailing backslash and followed by a blank line.

So, violate the style of the surrounding code, or leave the blank line and match the style? I left the blank line to match the existing style.

sjg added inline comments.May 28 2016, 2:00 AM
etc/periodic/security/520.pfdenied
48 ↗(On Diff #16967)

fwiw there is no need to comment blacklistd

lidl added inline comments.May 28 2016, 4:11 AM
etc/periodic/security/520.pfdenied
48 ↗(On Diff #16967)

Fixed in my local repository, will be fixed when I post the next review update / commit the code.

This revision was automatically updated to reflect the committed changes.