Page MenuHomeFreeBSD

Revisit blacklistd support in ftpd
ClosedPublic

Authored by lidl on Oct 28 2016, 2:40 PM.

Details

Summary

Enhance blacklistd support to not log anything by default,
unless blacklistd support is enabled on the command line.
Document new flag in man page, cleanup patches to be less
intrusive in code.

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 21769.Oct 28 2016, 2:40 PM
lidl retitled this revision from to Revisit blacklistd support in ftpd.
lidl updated this object.
lidl edited the test plan for this revision. (Show Details)
lidl added a reviewer: emaste.
lidl set the repository for this revision to rS FreeBSD src repository.
cem accepted this revision.Oct 28 2016, 4:45 PM
cem added a reviewer: cem.
cem added a subscriber: cem.
cem added inline comments.
libexec/ftpd/ftpd.c
335 ↗(On Diff #21769)

Should this be err() instead?

This revision is now accepted and ready to land.Oct 28 2016, 4:45 PM
lidl added inline comments.Oct 28 2016, 6:33 PM
libexec/ftpd/ftpd.c
335 ↗(On Diff #21769)

I would argue 'no'.

The blacklist support is just supposed to notify the blacklist daemon, not alter the flow of execution of the augmented program.

If someone specified the -B flag in their config, but then managed to get a binary installed without blacklist support, I don't think it is appropriate to start failing just because the configuration via rc.conf or inetd.conf has gone stale.

cem added inline comments.Oct 28 2016, 6:38 PM
libexec/ftpd/ftpd.c
335 ↗(On Diff #21769)

The counterargument might be: The user has elected to start the daemon with a security mitigation enabled. If we can't support that, we have no business silently running without the mitigation. If we abort, the user can fix their configuration (if it is a stale configuration problem).

emaste added inline comments.Nov 1 2016, 1:41 AM
libexec/ftpd/blacklist.c
39 ↗(On Diff #21769)

you can rely on the global being initialized to 0

40 ↗(On Diff #21769)

probably no need for extra blank line

emaste accepted this revision.Nov 1 2016, 1:44 AM
emaste edited edge metadata.

I have a slight preference for @cem's approach but am fine with it either way.

lidl marked 2 inline comments as done.Nov 1 2016, 6:14 PM
This revision was automatically updated to reflect the committed changes.