Add the ability have executable jail.conf

[crest_freebsd_rlwinm.de]

Summary

Jails often have repetitive boilerplate that can't be factored out of jail.conf using .include "<glob>"; or requires external lookups that require wrappers around jail(8) e.g. network interface creation or IP allocation, database lookups.

This patch checks if a jail.conf(5) is executable by trying to reopen its file descriptor for execution in which case it runs it as child process of the jail command and parses the child's standard output.

Test Plan

• Write example configurations e.g. create a jail from each sub-directory in a directory of jail names.
• Emit invalid jail.conf output.
• Emit valid jail.conf, but exit with status != 0

[kevans]

I'm a little torn on the idea itself, but here's some review

usr.sbin/jail/config.c

320

Please move side-effecty initialization like this out of the declarations

321

__DECONST(char *, cfname) gets the point across a little better imo

323

These should be converted to /* single line comments */

354

Extra spacing inside the if should be removed (just after and just before opening and closing parentheses)

373

file should be declared at the top

374

file == NULL

414

Should be initialized to 0 or so to be safe.

433

Space after the above declarations

435

Extraneous spacing again around the parentheses

[crest_freebsd_rlwinm.de]

Using this /etc/jail.conf:

.include "/usr/local/etc/jail[.]conf";
.include "/etc/jail.d/*.conf";
.include "/usr/local/etc/jail.d/*.conf";

Place this script into /usr/local/etc/jail.d/dirs.conf:

#!/bin/sh
set -Ceu
cd "${1%/*}"
for dir in *.d; do
        [ -d "$dir" ] || continue
        name="${dir%.d}"
        printf '%s {\n\t.include "%s/*.conf";\n}\n' "$name" "$dir"
done

To write a block like this for each ${dir}.d jail directory, deriving the jail name from the directory name (stripping the .d suffix):

jail_name { # variable references aren't allowed in jail names
    .include "$name.d/*.conf";
}

[crest_freebsd_rlwinm.de]

usr.sbin/jail/config.c

414

open_config() sets it to -1 before attempting to fork. It's an out parameter not an in-out parameter so imo it doesn't have to be initialised, but there is no harm in initialising it.

[crest_freebsd_rlwinm.de]

Address style feedback.

[jamie]

It would make sense for the exec.clean parameter to apply to the config execution. Bit of a chicken and egg problem there, but there's still the "-l" flag.

But I'm still just not convinced this is a good idea. It's pretty far outside the norm of how configuration files are expected to work.

[crest_freebsd_rlwinm.de]

Put the executable jail.conf(5) behind the -x option (as chicken bit).

[jamie]

I had considered that the -l (exec clean) flag should be considered, but decided it really only makes sense for keeping the jail environment clean.

[crest_freebsd_rlwinm.de]

Document new -x flag in jail(8).

[jamie]

usr.sbin/jail/config.c

317

"pid_t* pid" is much more intuitive and readable than "pid_t pid[static 1]".

331

I know you were told to use single line comments, but that only counts when not overrunning 80 columns.

337

The point of the -x flag was so existing configurations that happen to have that bit set on their config files "just work." While failing with an appropriate error message is better than trying to execute a config file, it's not just working. I was expecting -x to just enable the whole attempting to execute the file.

That does mean that if someone uses an actual executable script (or even binary) as a config file without adding -x, the error message may be non-intuitive. But that's the price of POLA for existing users.

347

The code lines themselves should be limited to 80 colums. There are are a number of lines that are just a little longer, but line breaks and with proper indentation are much more readable than run-ons, at least for my archaic 80-coumn emacs windows.

347

This isn't always the current jail. If the .include line is withing a jail definition, this will be the name of that jail. But if the .include line is after a jail definition was completed, this will still name that jail.

This may take add an actual global current_jail variable that can make this distinction.

348

Do we really need to tell the config file what its name is? I suppose that could be useful, so you can use the same script file to act differently in multiple locations. Still, that seems the config script/program's

But do we really need to tell it in so many different ways? Surely if the config script wants to break apart its pathname into so many pieces, it's welcome to do so.

371

Again, if the config script wants environment variables that are the same as its arguments, it can do that itself. This is just oddly specific corner-case work for running a script.

Also, error messages are nice, but the setenv can all be collapsed into a single setenv error. Either there's enough memory for environment variables or there isn't. Of one of them is NULL, which is programmer error and doesn't appear to be the case anyway. But then, that's unimportant if you don't use environment variables to tell the script what its argv is.

389

Turns out fexecve of a script requires fdescfs to be mounted. That's not always the case (it's not on my own system). A core system component shouldn't depend on this.

Instead of exec_fd and fexecve, just regular execve should work just fine. Perhaps you need exec_fd just to check the executable permission, but it's not suitable for actual execution.