syslogd: Refresh configuration using libcasper

[jfree]

Summary

When a SIGHUP signal is sent to syslogd, the configuration is reparsed,
leading to new resource acquisition.

If syslogd is running in capability mode and a SIGHUP is received, new
resources cannot be acquired. To mitigate this issue, libcasper is used
to parse the configuration.

The libcasper process runs outside of capability mode and is capable of
parsing syslogd's configuration and obtaining new resources. These
resources are then sent to the syslogd process via nvlist.

[jfree]

Use logerror() to log errors instead of exiting. This makes debugging significantly easier when something goes wrong during configuration parsing.

[jfree]

Update to avoid rebase conflicts

[markj]

usr.sbin/syslogd/syslogd_cap_config.c

74

What's the point of having filed_count as an nvlist element? Doesn't nvlist_get_nvlist_array() return the size of the array in filed_count?

130

Per my comment in D41463, we should continue factoring this code out to make it less confusing. In an ideal design, IMO, readconfigfile() would return the nvlist instead of mucking with global variables.

That doesn't need to happen in this revision, it's just a comment about the direction I think we should go.

[jfree]

Don't manually add the filed count to the nvlist. The filed count is fetched when getting the nvlist filed array.

[jfree]

Fix minor mistyping in populate_config(). filed_count should be declared as size_t, not uint64_t.

[markj]

usr.sbin/syslogd/syslogd_cap_config.c

82

Does the casper service do any validation of the config file path? Doesn't this interface allow syslogd to parse any file as a syslogd configuration file and derive capabilities from it?

Most likely we should handle this by making the configuration service take a limit, set by cap_limit_set(). During initialization, syslogd would limit the service to only /etc/syslog.conf (or whatever is specified by -f). An attempt to parse any other file should be rejected by the casper service.

[jfree]

Create filed nvlist directly from readconfigfile() and address Mark's comments.

[jfree]

usr.sbin/syslogd/syslogd_cap_config.c

82

Does the casper service do any validation of the config file path? Doesn't this interface allow syslogd to parse any file as a syslogd configuration file and derive capabilities from it?

Most likely we should handle this by making the configuration service take a limit, set by cap_limit_set(). During initialization, syslogd would limit the service to only /etc/syslog.conf (or whatever is specified by -f). An attempt to parse any other file should be rejected by the casper service.

I made ConfFile a global variable and did a strcmp() to verify that the passed-in path matched libcasper's original copy.

[jfree]

Bumping this

[markj]

This broadly looks ok to me. I'm a bit worried about resource leaks in the routines which (de)serialize nvlists. It's quite difficult to account for everything and be confident that we're not leaking descriptors or memory somewhere.

usr.sbin/syslogd/syslogd_cap_config.c

56

In general, I'd like for us to move towards copying structures using nvlist_add_binary(), to try and simplify this tedious serialization code. Of course, special care is needed for pointers and file descriptors, but most of these fields don't belong in those two categories.

Once the patch set has landed, we should try and simplify this code further.

239

Can we not use nvlist_take_descriptor() here?

[markj]

usr.sbin/syslogd/syslogd_cap_config.c

146

This is maybe a painful request, but I wonder if it's possible to avoid having to serialize addrinfo here at all. syslogd really just wants the socket addr and address family. If we go down this path, it can be done in a separate patch.

[jfree]

usr.sbin/syslogd/syslogd_cap_config.c

146

As you pointed out in another comment, nvlist_add_binary() would probably be less tedious since a lot of struct filed members are inline (not pointers or descriptors).

I think I originally went with this approach because I didn't like the idea of storing a binary blob with invalid descriptor numbers and pointers; I feel like doing that generally leads to confusion for future contributors that are not completely familiar with which specific members should be xfered outside of the binary blob.

239

This function does not modify the state of nvl_filed. Using take_descriptor() would remove (and free) the descriptor from nvl_filed. We just get it and dup(2) it instead