Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rebase #36720 "Add exec option to tmpfs" #46809

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

dperny
Copy link
Contributor

@dperny dperny commented Nov 13, 2023

I'm eliding the typical format for PRs because this PR is simply a rebase of an existing PR, #36720 onto the current master.

Closes #36720.

@@ -434,6 +434,9 @@ definitions:
Mode:
description: "The permission mode for the tmpfs mount in an integer."
type: "integer"
Options:
description: "The list of options to be passed to the tmpfs mount in a string."
type: "string"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ISTR there was some discussion whether this should be a string, a []string (or a map[string]string, but I think we didn't want a map), did we ever reach full consensus on that?

Copy link
Member

@cpuguy83 cpuguy83 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to be a little pedantic on having a TmpfsOptions struct with a generic Options field inside of it.
I don't want to block this on pedantry, especially since noexec is a good thing to add so happy to bow out if others think this is fine.

@@ -118,7 +118,8 @@ type TmpfsOptions struct {
SizeBytes int64 `json:",omitempty"`
// Mode of the tmpfs upon creation
Mode os.FileMode `json:",omitempty"`

// Options passed directly to the tmpfs mount
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like we only plan to have a very small subset of options here.
Does it make sense to promote those optioons to fields on this struct?
The ones we have are difficult to represent accurately, I admit (default exec vs explicit exec vs noexec).

UID/GID are probably worth adding at some point... if someone asks for it (this may have already happened).
I'm not sure I agree with the comment about implications in the cluster.

I'm not sure the other options would be worthwhile.

@dperny dperny force-pushed the add-exec-option-to-tmpfs branch 4 times, most recently from c86d6d4 to c58464a Compare April 3, 2024 14:04
adshmh and others added 4 commits April 4, 2024 12:09
Signed-off-by: Arash Deshmeh <adeshmeh@ca.ibm.com>
Signed-off-by: Arash Deshmeh <adeshmeh@ca.ibm.com>
Signed-off-by: Drew Erny <derny@mirantis.com>
Signed-off-by: Drew Erny <derny@mirantis.com>
@corhere
Copy link
Contributor

corhere commented Apr 18, 2024

We discussed this during today's maintainer call. The main sticking point is that the daemon is tokenizing and parsing the options string. Parsing would not be needed if the options were encoded as structured data in the API request. Nor would parsing be needed if the daemon didn't validate the options against an allowlist: it would simply concatenate the user options to the end of the daemon-generated options string and pass that along to the kernel. No matter which decision we make on validating the options against an allowlist, there is no need for the daemon to parse a mount options string.

Consensus was in favour of keeping validation in, based on the "yes is forever, no is temporary" principle of API design. We can always expand the allowlist or remove the validation in the future, but we can't walk it back. (We would also like to add a few more obviously safe options to the allowlist, such as nosuid, nosymfollow, dev/nodev, suid/nosuid.) That means the options need to be encoded as structured data in the API so the daemon can validate them without parsing. For future-proofing, I would ideally like the structure to map cleanly onto the new fsconfig syscall API.

  • Key and value are separate in the structured data
  • Ordered list, in case the order that parameters are supplied may be significant

The outer data structure has to be a JSON Array. The only remaining question is how to encode individual parameters. At the very least we have to distinguish between flags and string parameters in the API as some flags will not be accepted through the fsconfig_set_string command.

If we limit ourselves to making only fsconfig_set_flag and fsconfig_set_string parameters representable in the API, it could be as simple as encoding flags as bare strings and string parameters as a 2-tuple of (string, string), encoded in JSON as a two-element array.

[ "noexec", ["uid", "12345"] ]

Or use 2-tuples consistently, but use null in the value position to signify a flag?

[ ["noexec", null], ["uid", "12345"] ]

If they're all 2-tuples, they could simply be flattened into an array where even indices are keys and odd indices are values.

["noexec", null, "uid", "12345"]

If we really want to future-proof for the other kinds of parameter, we could (also?) accept a more verbose struct (JSON Object) representation, e.g.:

[
  {"kind": "flag", "key": "noexec"},
  {"kind": "string", "key": "uid", "value": "12345"},
  {"kind": "binary-base64", "key": "some-binary-blob", "value": "eQo="},
  {"kind": "host-path", "key": "lowerdir", "value": "/path/to/thing"}
]

Or only be verbose in the value position.

[
  "noexec", null,
  "uid", "12345",
  "gid": {"kind": "string", "value": "23456"},
  "rw", {"kind": "flag"},
  "some-binary-blob": {"kind": "binary", "content-encoding": "base64", "value": "eQo="}
]

The tmpfs filesystem currently only accepts flag and string parameters (and no in-tree filesystem accepts binary parameters as of Linux v6.8) so we don't need to implement support for the other kinds yet. We only need to ensure that whatever shape we choose can afford being extended to support expressing the other kinds of parameter when the time comes.


We also considered adding the mount options to the outer Mount struct, but have decided not to pursue that for now. We could add one in the future without conflicting with the TmpfsOptions mount options, so long as we concatenate the options (or apply the fsconfig) from the different sources in a consistent order.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants