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.

Sign up for GitHub

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

config-linux: Forbid the empty string for mountLabel #843

Closed
wants to merge 1 commit into from
Closed

config-linux: Forbid the empty string for mountLabel #843

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions config-linux.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -642,6 +642,7 @@ The values MUST be absolute paths in the [container namespace](glossary.md#conta
## <a name="configLinuxMountLabel" />Mount Label

**`mountLabel`** (string, OPTIONAL) will set the Selinux context for the mounts in the container.
The value MUST NOT be an empty string.
Copy link
Contributor

Choose a reason for hiding this comment

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

I dont think this has to be too restrictive. Probably something equivalent to an empty value will not be applied / have no effect is good enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably something equivalent to an empty value will not be applied / have no effect is good enough.

Runtimes don't have to validate this if they don't want. But when we are validating, I think we want to treat empty-string mountLabels as errors for the same reasons that we treat nulls as errors in a number of places (see here and #662). Why would a config author who understood that empty strings are ignored set an empty-string value instead of leaving the property unset? I think it's much more likely that config authors explicitly setting an empty-string value actually expect it to do something, and a “that's not valid” error will help correct their misunderstanding.

Copy link
Contributor

Choose a reason for hiding this comment

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

So i dont think opencontainers/runtime-tools#386 that you referred to has anything to do with empty string clarification here. If we want opencontainers/runtime-tools#386 to seems correct, then we would have to include some current selinux validation code here i.e.

if strings.Contains(label, "z") && strings.Contains(label, "Z") {
   return ErrIncompatibleLabel
}

and i think it doesnt make sense for the spec to chase how selinux validates the options.

But when we are validating, I think we want to treat empty-string mountLabels as errors for the same reasons that we treat nulls as errors in a number of places

I dont think this is correct. IIRC we just remove null / pointers in many fields because it's replaceable in many cases with the usages of default value which is simpler to use ( in this case an empty string ).

I think it's much more likely that config authors explicitly setting an empty-string value actually expect it to do something

I'm not sure if this is true, but even if it is, clarification on the default value ( empty string ) seems like a more reasonable option for me than enforcing that it always needs to have an non-empty value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So i dont think opencontainers/runtime-tools#386 that you referred to has anything to do with empty string clarification here.

The connection is the line I linked in the initial comment.

if v.spec.Linux.MountLabel != "" {
    …validate the value
}

That only makes sense if we have some reason to know that either:

a. the empty string is valid in its own right (in which case we'd probably want to patch the Go type to use *string and patch the validation to accept the empty string), or
b. the empty string is invalid, so we can use something like opencontainers/runtime-tools#197 if we want to catch those invalid ,"mountLabel": "" configs, and everybody else can do whatever undefined thing they want them (including treating them like configs that don't set mountLabel at all, which is what the string Go type does).

This PR goes with (b), but I'm fine going with (a) if folks feel like that's more appropriate.

…i think it doesnt make sense for the spec to chase how selinux validates the options…

I think this part of validation is #844.

IIRC we just remove null / pointers in many fields because it's replaceable in many cases with the usages of default value which is simpler to use ( in this case an empty string ).

There's a distinction between “the default is {whatever}”, where the runtime still does something with that default (e.g. here), and “the default is to not do anything”, where you might as well leave the property unset. We have lots of cases where leaving the property unset means “do nothing”, and I don't see the point of defining (and requiring support for) synonyms like setting an explicit null or empty-string value.

I'm not sure if this is true, but even if it is, clarification on the default value ( empty string ) seems like a more reasonable option for me than enforcing that it always needs to have an non-empty value.

That's more work for runtimes (now you have to handle both unset and empty-string values), although it's not much more work in Go (where using a string type and the default deserializer collapse those two cases for you). And it's more work for compliance testing, which now needs to check that the runtime does the right thing for both the unset and empty-string cases. And it's an unnecessary decision for config authors, who need to figure out which of the two ways to say “don't do anything” they'd like to use. What's the upside?


### Example

Expand Down
3 changes: 2 additions & 1 deletion schema/config-linux.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -264,7 +264,8 @@
},
"mountLabel": {
"id": "https://opencontainers.org/schema/bundle/linux/mountLabel",
"type": "string"
"type": "string",
"minLength": 1
}
}
}
Expand Down