Carry #1111: specs-go/config: add Landlock LSM support #1241
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -352,6 +352,52 @@ For Linux-based systems, the `process` object supports the following process-spe | |
| for `initial`. If omitted or empty, runtime SHOULD NOT change process' | ||
| CPU affinity after the process is moved to container's cgroup, and the | ||
| final affinity is determined by the Linux kernel. | ||
| * **`landlock`** (object, OPTIONAL) specifies the Landlock unprivileged access control settings for the container process. | ||
|
There was a problem hiding this comment. What happens if the runtime doesn't know this field? I guess in runc it will be completely ignored, right? In that case, I think we at least need to expose landlock support via the features command too. There was a problem hiding this comment. Yes, the runtime will ignore the field. I think it's not necessary to add feature command like other optional features. There was a problem hiding this comment. @Zheaoli why not? This is a security feature, I can see use cases where upper layers want to enforce this. There was a problem hiding this comment. Sorry for replying late. I misunderstand something before. I think it's necessary to export the feature we support for landlock in features command There was a problem hiding this comment. I think this doesn't belong to this file. This should be in config-linux. The same with the secion of the struct this is, this should be inside the Linux section, as seccomp is. Not as a general feature, as this is a linux-only feature. There was a problem hiding this comment. Ouch, there is a section for Linux here too. Sorry! |
||
| Note that `noNewPrivileges` must be set to true to use this feature. | ||
| For more information about Landlock, see [Landlock documentation][landlock]. | ||
| `landlock` contains the following properties: | ||
|
|
||
| * **`handledAccess`** (object, OPTIONAL) specifies the access rights that will be restricted by the ruleset. | ||
| The `handledAccess` currently contains the following types: | ||
| * **`handledAccessFS`** (array of strings, OPTIONAL) is an array of FS typed actions that are handled by a ruleset. | ||
|
|
||
| If no rule explicitly allow them, they should then be forbidden. | ||
|
There was a problem hiding this comment. Is this what landlock does? I thought it will allow everything that is not handled here. Otherwise, backwards compatibility (when you add a new access type here, like kernel X added access type Y) will be impossible, right? Doing a local experiment, this is not indeed what landlock does, so we can't promise this. It will be the case if we don't mention it in path_beneath. But if we mention it here, this is not true. Maybe it is just c&p that went unnoticed? |
||
| * **`handledAccessNetwork`** (array of strings, OPTIONAL) is an array of NETWORK typed actions that are handled by a ruleset. (The NETWORK typed actions are available when the ABI version >= 4. The behavior when the ABI version is less than 4 will depend on the **`enableBestEffort`**) | ||
|
There was a problem hiding this comment. I'd remove the last parenthesis. That is the case with all the things, like some handledAccessFs types might not be supported, etc. There was a problem hiding this comment. idem, what about using ints for this too? |
||
| * **`rules`** (object, OPTIONAL) specifies the security policies (i.e., actions allowed on objects) to be enforced. | ||
| The `rules` currently contains the following types: | ||
| * **`pathBeneath`** (array of objects, OPTIONAL) is an array of the file-hierarchy typed rules. | ||
| Entries in the array contain the following properties: | ||
| * **`allowedAccess`** (array of strings, OPTIONAL) is an array of FS typed actions that are allowed by a rule. The actions are grouped by the ABI version in the following description: | ||
|
There was a problem hiding this comment. Do we want strings or ints here? If we use ints, we can handle new values without requiring to add them to the spec. Of course, we will need to provide a friendly name for known things in the go bindings, but until a new release is done, people can use a number and it will just work. Furthermore, what is here is already out of date. ABI 5 now is supported by new kernels (to restrict ioctls). So I think we need a more "future proof" spec :-) What do others think? |
||
| 1. ABI version >= 1: | ||
| 1. execute | ||
| 2. write_file | ||
| 3. read_file | ||
| 4. read_dir | ||
| 5. remove_dir | ||
| 6. remove_file | ||
| 7. make_char | ||
| 8. make_dir | ||
| 9. make_reg | ||
| 10. make_sock | ||
| 11. make_fifo | ||
| 12. make_block | ||
| 13. make_sym | ||
| 2. ABI version >= 2: | ||
| 1. refer | ||
| 3. ABI version >= 3: | ||
| 1. truncate | ||
| * **`paths`** (array of strings, OPTIONAL) is an array of files or parent directories of the file hierarchies to restrict. | ||
| * **`networkPort`** (array of objects, OPTIONAL) is an array of the network socket rules. | ||
| Entries in the array contain the following properties: | ||
| * **`allowedAccess`** (array of strings, OPTIONAL) is an array of NETWORK typed actions that are allowed by a rule. The actions are grouped by the ABI version in the following description: | ||
|
There was a problem hiding this comment. Idem here, we might get away with ints? |
||
| 1. ABI version >= 4: | ||
| 1. bind | ||
| 2. connect | ||
| * **`ports`** (array of strings, OPTIONAL) is an array of network ports to restrict. | ||
|
There was a problem hiding this comment. I guess this is a typo, we want int for ports, right? |
||
| * **`enableBestEffort`** (bool, OPTIONAL) the `enableBestEffort` field disables the best-effort security approach for Landlock access rights. | ||
|
There was a problem hiding this comment. As I mentioned before, without exposing landlock enabled as a bool in the features subcommand, a high level container runtime (like containerd or crio) can't know if the low-level container runtime implementing this (like crun or runc) supports landlock at all. And in that case, the runtimes that don't support landlock will just ignore this whole section and the container will be created just fine, no landlock enforced. Therefore, I think we really need to expose this in the features subcommand too. There was a problem hiding this comment. I agree, we need to be careful to not give users a false sense of security. |
||
| This is for conditions when the Landlock access rights explicitly configured by the container are not supported or available in the running kernel. | ||
| If the best-effort security approach is enabled (`false`), the runtime SHOULD enforce the strongest rules configured up to the current kernel support, and only be [logged as a warning](runtime.md#warnings) for those not supported. | ||
| If disabled (`true`), the runtime MUST [generate an error](runtime.md#errors) if one or more rules specified by the container is not supported. | ||
|
There was a problem hiding this comment. Per the other comment, this only work for runtimes that use this spec version. For runc 1.1 or 1.2 and probably 1.3, it's mandated by the spec to ignore unrecognized fields. Therefore, this is not safe when you take into account that we need to support older runtimes from a high-level container like containerd or crio |
||
| Default is `true`, i.e., following a best-effort security approach. | ||
|
|
||
| ### <a name="configUser" />User | ||
|
|
||
|
|
@@ -397,6 +443,79 @@ _Note: symbolic name for uid and gid, such as uname and gname respectively, are | |
| "class": "IOPRIO_CLASS_IDLE", | ||
| "priority": 4 | ||
| }, | ||
| "landlock": { | ||
| "handledAccess": { | ||
| "handledAccessFS": [ | ||
| "execute", | ||
| "write_file", | ||
| "read_file", | ||
| "read_dir", | ||
| "remove_dir", | ||
| "remove_file", | ||
| "make_char", | ||
| "make_dir", | ||
| "make_reg", | ||
| "make_sock", | ||
| "make_fifo", | ||
| "make_block", | ||
| "make_sym", | ||
| "refer", | ||
| "truncate" | ||
| ], | ||
| "handledAccessNetwork": [ | ||
| "bind", | ||
| "connect" | ||
| ] | ||
| }, | ||
| "rules": { | ||
| "pathBeneath": [ | ||
| { | ||
| "allowedAccess": [ | ||
| "execute", | ||
| "read_file", | ||
| "read_dir" | ||
| ], | ||
| "paths": [ | ||
| "/usr", | ||
| "/bin" | ||
| ] | ||
| }, | ||
| { | ||
| "allowedAccess": [ | ||
| "execute", | ||
| "write_file", | ||
| "read_file", | ||
| "read_dir", | ||
| "remove_dir", | ||
| "remove_file", | ||
| "make_char", | ||
| "make_dir", | ||
| "make_reg", | ||
| "make_sock", | ||
| "make_fifo", | ||
| "make_block", | ||
| "make_sym" | ||
| ], | ||
| "paths": [ | ||
| "/tmp" | ||
| ] | ||
| } | ||
| ], | ||
| "networkPort": [ | ||
| { | ||
| "allowedAccess": [ | ||
| "bind", | ||
| "connect" | ||
| ], | ||
| "ports": [ | ||
| 80, | ||
| 443 | ||
| ] | ||
| } | ||
| ] | ||
| }, | ||
| "enableBestEffort": true | ||
| }, | ||
| "noNewPrivileges": true, | ||
| "capabilities": { | ||
| "bounding": [ | ||
|
|
@@ -1151,7 +1270,8 @@ Here is a full example `config.json` for reference. | |
|
|
||
| [apparmor]: https://wiki.ubuntu.com/AppArmor | ||
| [cgroup-v1-memory_2]: https://www.kernel.org/doc/Documentation/cgroup-v1/memory.txt | ||
| [selinux]:http://selinuxproject.org/page/Main_Page | ||
| [landlock]: https://landlock.io | ||
| [no-new-privs]: https://www.kernel.org/doc/Documentation/prctl/no_new_privs.txt | ||
| [proc_2]: https://www.kernel.org/doc/Documentation/filesystems/proc.txt | ||
| [umask.2]: https://pubs.opengroup.org/onlinepubs/009695399/functions/umask.html | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this PR should be carefully updated following the latest advances of landlock (rather than simply carried) as some of the spec proposals might be stale now. Pls see https://docs.kernel.org/userspace-api/landlock.html and https://github.com/landlock-lsm/go-landlock for details.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you got time to continue this PR or I can continue carry(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pls go ahead updating this PR. I'll need some time to follow up and dive into the updates of landlock (even for review).