Add FreeBSD Jail execution environment support #224

ihoro · 2024-02-03T16:59:52Z

The main work and review was conducted here: https://reviews.freebsd.org/D42350.
The respective mailing list discussion: https://lists.freebsd.org/archives/freebsd-hackers/2024-February/003032.html.

ihoro · 2024-03-22T17:41:27Z

The following topics are closed by current state of the patch:

FreeBSD specifics are separated with conditional build.
The existing tests, broken by the change, are fixed.
The documentation update is added. kyua.conf.5 and kyuafile.5 man pages got the respective upgrades.

ngie-eign

As much as I'm not a fan of the term "container" because it focuses on the Linux/Solaris technological implementations, I believe that it would be wise to use "container" in lieu of "jail". "jail"s are a concept that are very FreeBSD specific and don't exist outside of FreeBSD/FreeBSD derivatives. I realize this project is hosted in the freebsd GitHub org, but it's a good idea to keep things as generic as possible to avoid having to rearchitect things later if someone contributes an actual Linux or Solaris container implementation (for instance).

Cross-references to jail(8), et al, are missing from the manpage edits (this seems to be where most of the jail kernel subsystem documentation lives). This is important because you're adding a feature which isn't fully documented and not providing enough information for more casual FreeBSD users to use the feature.

ngie-eign · 2024-05-04T21:44:40Z

doc/kyua.conf.5.in

+.It Va execenv
+Whitespace-separated list of execution environment names.
+.Pp
+Only tests which require one of the given execution environments will be run.


Mentioning what execenv's are supported would be a good idea -- otherwise it's unclear (without looking at the source code) what is or isn't supported.
Referencing alternative documentation is a good thing to do in place of hardcoding the values, but I think being explicit avoids the need for the documentation potentially becoming out of sync/less implicit.

It's already covered in kyuafile.5: https://github.com/ihoro/kyua/blob/9721be7abbd48f2ee020e5deaee1e1daa44c0087/doc/kyuafile.5.in#L181

I guess we could simply make a reference to the kyuafile.5 for the possible values.

I've added the mentioned reference.

ngie-eign · 2024-05-04T21:46:38Z

doc/kyuafile.5.in

@@ -1,4 +1,4 @@
-.\" Copyright 2012 The Kyua Authors.
+.\" Copyright 2024 The Kyua Authors.


Suggested change

.\" Copyright 2024 The Kyua Authors.

.\" Copyright 2015-2024 The Kyua Authors.

Copyright's should always represent the full period that they cover.

ngie-eign · 2024-05-04T21:50:35Z

doc/kyuafile.5.in

+It makes sense only if execenv is set to
+.Sq jail .
+.sp
+Kyua implicitly adds


Is "adds" the appropriate concept to use here? I will look at the surrounding documentation to compare/contrast.
Also: kyua(1) should be used here:

Suggested change

Kyua implicitly adds

.Xr kyua 1

implicitly adds

Fixed. Plus a little rephrasing with s/adds/passes/.

ngie-eign · 2024-05-04T21:57:36Z

doc/kyuafile.5.in

+.It jail
+The
+.Fx
+jail environment.


Whenever mentioning jail, I would probably reference other documentation.
I'm reaching out to other developers to get input, as it's not clear if .Xr jail 2 or .Xr jail 3 should be used here (I'm leaning towards the former).
Again, this should only be discussed in the actual implementation -- the general documentation should use the term, "containers".

.Xr jail 8 feels like the best one due to kyua users are expected to be usual users of jail(8), instead of the system calls and the jail library. And the execenv.jail parameters are actually parameters for jail(8). What do you think?

I've put .Xr jail 8 for now.

ngie-eign · 2024-05-04T21:58:14Z

doc/kyuafile.5.in

+It creates a temporary jail to run the test and its optional cleanup logic
+within.
+.El
+.It Va execenv_jail


What kind of parameters are you envisioning adding for host vs container style execenv's?

For now, there are no extra things for the existing default env, it works the same way without change. A new metadata property like execenv.host.* could be added in future if there is a production need, probably to pass some env variables (just thinking out loud).

ngie-eign · 2024-05-04T23:12:39Z

model/metadata.hpp

+    const std::string& execenv(void) const;
+    bool has_execenv(void) const;
+    const std::string& execenv_jail(void) const;


This should be put directly below description to be sorted alphanumerically.

ngie-eign · 2024-05-04T23:13:30Z

utils/config/nodes.ipp

-    const int /* value_index */)
+    lutok::state& state,


This change seems superfluous.

What do you mean? It adds a capability to read a string set config values which are needed for the execenv engine config variable. It looks the function was left as a stub for future implementation when it's really needed. The patch adds string set support.

ngie-eign · 2024-05-04T23:13:53Z

utils/process/executor.cpp

@@ -689,6 +689,34 @@ struct utils::process::executor::executor_handle::impl : utils::noncopyable {
                data._pimpl->state_owners,
                all_exec_handles)));
    }
+
+    executor::exit_handle
+    reap(const int original_pid)


This should use pid_t, not int.

ngie-eign · 2024-05-04T23:15:51Z

utils/process/executor.cpp

+    executor::exit_handle
+    reap(const int original_pid)
+    {
+        const exec_handles_map::iterator iter = all_exec_handles.find(


Error handling is missing for values like 0, -1, etc. These values have special meanings and can result in the process either committing suicide OR a system-wide reboot/shutdown/etc. Neither case is desired.
From kill(2):

If pid is zero: The sig signal is sent to all processes whose group ID is equal to the process group ID of the sender, and for which the process has permission; this is a variant of killpg(2). If pid is -1: If the user has super-user privileges, the signal is sent to all processes excluding system processes (with P_SYSTEM flag set), process with ID 1 (usually init(8)), and the process sending the signal. If the user is not the super user, the signal is sent to all processes which the caller has permissions to, excluding the process sending the signal. No error is returned if any process could be signaled.

It looks the function name is misleading, it's not the same reaping as in the general process management context. During my work on this patch back in Oct, I found a defect in the existing Kyua mechanism of abrupt termination handling -- it was not collecting all exit handles of its children. It was easier to reproduce with parallelism>1, and the parallelism is what I was actively working on. The pid passed here as a parameter is a pid of a spawn child previously saved by kyua, the kyua simply expects to see an exit handle object per each child spawn and this "kyua specifics" reaping process helps with that. That is, it's named after the same term of the general context, but this is about kyua internals.

ngie-eign · 2024-05-04T23:17:15Z

freebsd/utils/jail.cpp

+    // invoke jail
+    std::auto_ptr< process::child > child = child::fork_capture(
+        run(fs::path("/usr/sbin/jail"), av));


It seems like sanity checking that the jail(8) program exists and skipping with a helpful message would be wise--instead of just marking all of the tests broken.
This is especially important for systems where WITHOUT_JAIL= is specified in src.conf(5).

I was working on a concern from Olivier: https://lists.freebsd.org/archives/freebsd-hackers/2024-February/003016.html. Currently it's covered such way that for the WITHOUT_JAIL system kyua is built without execenv=jail support. It makes kyua config to report execenv list with a single host item. As a result it triggers the existing mechanism of requirements checking and marks execenv=jail based tests as skipped with execenv="jail" requires FreeBSD with jail feature. message. It looks to be simpler, on the higher level, without additional checks of the system consistency in cases like WITH_JAIL system + missing binary, or WITHOUT_JAIL system + kyua binary copied from WITH_JAIL system, etc. Also, the same mechanism and message is used in case of execenv=jail tests running on Linux and other systems. Any not-covered inconsistency will be reported in a test log with messages like /usr/sbin/jail: file not found and so on.
What do you think?

ngie-eign

Seems like the code warrants adding or modifying isolation_test.cpp, et al.

In general, it seems like the PR is proposing a number of changes without providing sufficient numbers of negative tests and integration tests. This is concerning from a code maintenance/support perspective.

Also, the jail feature creates an exploitable vector for security issues which doesn't seem to have an adequate disclaimer regarding its use. Although it's an implied bad practice to run tests from a potentially untrusted sources as root on production systems, this concern isn't called out in the documentation. In particular, this feature requires root to function, even if required.user='unprivileged' is implemented such that jails are run as unprivileged users. jail(8) on FreeBSD always requires superuser privileges, whereas other container implementations, such as docker, allow specific groups to run administrative commands.

Another warning should be added noting that multiple parallel kyua processes can conflict with one another. This is true if is_exclusive = true is not set, but also if multiple kyua instances are run in parallel, despite other guarantees (sqlite3 db locking) which prevents multiple processes for conflicting with one another.

It would be really good to call this out (as a warning) too.

ngie-eign · 2024-05-04T23:33:10Z

doc/kyuafile.5.in

+atf_test_program{name='network_test',
+                 execenv='jail',
+                 execenv_jail='vnet allow.raw_sockets',
+                 required_user='root'}


(picking a line)
The code below doesn't explicitly check to make sure the user has permissions to create jails.
atf_test_program allows a config file to incorrectly specify execenv='jail' and required_user='unprivileged_user'.

I was thinking about this and it looks to be simpler to keep the mechanism without additional built-in policy. Who knows, probably in future we will have a capability to create some sort of jails by non-superuser.

For now I propose to improve the documentation (kyuafile.5). I've added the key message that root is needed and the examples section was extended with extra explanation. The non-working jail+unprivileged combination is mentioned explicitly. What do you think?

Anyway, explicit documentation is a good change here.

ihoro · 2024-05-05T19:12:38Z

@ngie-eign, thank you very much for your time. Going to work on the outcome of the review.

ihoro · 2024-05-06T20:24:45Z

As much as I'm not a fan of the term "container" because it focuses on the Linux/Solaris technological implementations, I believe that it would be wise to use "container" in lieu of "jail". "jail"s are a concept that are very FreeBSD specific and don't exist outside of FreeBSD/FreeBSD derivatives. I realize this project is hosted in the freebsd GitHub org, but it's a good idea to keep things as generic as possible to avoid having to rearchitect things later if someone contributes an actual Linux or Solaris container implementation (for instance).

Sounds interesting to consider. I have concerns that two levels of abstraction could add extra misunderstanding from end user perspective and extra interface limitations for the possible future execution environments added. The execenv is already meant to be the first abstraction added. It's abstracted from the concepts and details of a specific execution environment, i.e. it does not imply a containerization only. And currently it's designed as a switch, it could be anything (just thinking out loud): execenv=host, execenv=jail, execenv=bhyve, execenv=docker, execenv=kubernetes, execenv=python-virtualenv, execenv=aws, etc. And the key abstraction is that parameters are not generalized, due to it can be hard to do and it may introduce limitations only. For example, jail has a single metadata property -- execenv.jail, what is a string of parameters passed to the jail(8) program. Docker could have multiple properties like execenv.docker.imageurl and execenv.docker.runparams. AWS could have more execenv.aws.* properties to cover authorization and details of the exact service to be used.

There was no intention to provide an automatic containerization feature which uses the local capabilities like jail for FreeBSD or cgroups et al. for Linux. It simply provides a new concept, execution environment, and implements the first non-default (aka host) environment -- FreeBSD jail based one. It's merely an outcome that it can be used to add extra isolation of some FreeBSD tests to improve parallelism of the test suite. A test author is expected to understand the background if jail execution environment is picked for a test.

This is a short elaboration of the current vision. What do you think? Probably I have not caught accurately the idea with execenv=container and execenv.container.params.

ihoro · 2024-05-07T07:45:18Z

Anyway, now I think that execenv.jail could be re-worked into execenv.jail.params to keep the "namespace" for possible future improvements of jail execenv with extra execenv.jail.* metadata.

ihoro · 2024-05-09T12:28:57Z

@ngie-eign, I'm leaving the "Resolve conversation" button for you. I guess it must be convenient for you.

A new Kyua concept is added -- "execution environment". A test can be configured to be run within a specific environment. The test case lifecycle is extended respectively: - execenv init (creates a jail or does nothing for default execenv="host") - test exec - cleanup exec (optional) - execenv cleanup (removes a jail or does nothing for default execenv="host") The following new functionality is provided, from bottom to top: 1 ATF based tests - The new "execenv" metadata property can be set to explicitly ask for an execution environment: "host" or "jail". If it's not defined, as all existing tests do, then it implicitly means "host". - The new "execenv.jail" metadata property can be optionally defined to ask Kyua to use specific jail(8) parameters during creation of a temporary jail. An example is "vnet allow.raw_sockets". Kyua implicitly adds "children.max" to "execenv_jail" parameters with the maximum possible value. A test case can override it. 2 Kyuafile - The same new metadata properties can be defined on Kyuafile level: "execenv" and "execenv_jail". - Note that historically ATF uses dotted style of metadata naming, while Kyua uses underscore style. Hence "execenv.jail" vs. "execenv_jail". 3 kyua.conf, kyua CLI - The new "execenv" engine configuration variable can be set to a list of execution environments to run only tests designed for. Tests of not listed environments are skipped. - By default, this variable lists all execution environments supported by a Kyua binary, e.g. execenv="host jail". - This variable can be changed via "kyua.conf" or via kyua CLI's "-v" parameter. For example, "kyua -v execenv=host test" will run only host-based tests and skip jail-based ones. - Current value of this variable can be examined with "kyua config". Signed-off-by: Igor Ostapenko <pm@igoro.pro>

ihoro · 2024-05-13T08:49:19Z

@ngie-eign, I think it's ready for the next round of your review.

This comment was marked as outdated.

Sign in to view

ihoro marked this pull request as draft February 25, 2024 14:45

ihoro closed this Mar 22, 2024

ihoro force-pushed the freebsd-jail-execenv branch from 99692a8 to 0a43bb8 Compare March 22, 2024 16:53

ihoro reopened this Mar 22, 2024

ihoro marked this pull request as ready for review March 22, 2024 17:41

ihoro force-pushed the freebsd-jail-execenv branch from a33e429 to 08699b3 Compare March 23, 2024 11:28

ihoro force-pushed the freebsd-jail-execenv branch from 08699b3 to 9721be7 Compare April 1, 2024 13:05

ngie-eign requested changes May 4, 2024

View reviewed changes

ngie-eign reviewed May 4, 2024

View reviewed changes

ihoro force-pushed the freebsd-jail-execenv branch 2 times, most recently from 10330c3 to 6553ba0 Compare May 9, 2024 12:17

ihoro force-pushed the freebsd-jail-execenv branch 3 times, most recently from 0bfed6a to 2b9f217 Compare May 10, 2024 14:08

ihoro force-pushed the freebsd-jail-execenv branch from 2b9f217 to 4fe2a93 Compare May 10, 2024 15:53

ihoro requested a review from ngie-eign May 16, 2024 11:59

		@@ -1,4 +1,4 @@
		.\" Copyright 2012 The Kyua Authors.
		.\" Copyright 2024 The Kyua Authors.

	.\" Copyright 2024 The Kyua Authors.
	.\" Copyright 2015-2024 The Kyua Authors.

Add FreeBSD Jail execution environment support #224

Are you sure you want to change the base?

Add FreeBSD Jail execution environment support #224

Conversation

ihoro commented Feb 3, 2024 • edited

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

ihoro commented Mar 22, 2024

ngie-eign left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ngie-eign left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ihoro commented May 5, 2024

ihoro commented May 6, 2024

ihoro commented May 7, 2024

ihoro commented May 9, 2024

ihoro commented May 13, 2024

ihoro commented Feb 3, 2024 •

edited

ngie-eign left a comment •

edited

ngie-eign left a comment •

edited