config-linux: Forbid the empty string for mountLabel

[wking]

minLength is documented here.

The spec is currently not clear about how values for this property should be used, and after this commit it is still not clear. But the Linux.MountLabel property is not a *string, so distinguishing between “unset” and “set to the empty string” would be awkward in Go. I'm not familiar enough with the backing kernel API to be able to put RFC 2119 teeth into how the value should be used (#746), but I'm pretty sure we either want this commit (forbidding the empty string) or a *string in the Go type.

Related to opencontainers/runtime-tools#386, which is attempting to validate this property and using “is the Go property different from the empty string?” to mean “does the config set this property?”. Without this commit, those statements are not interchangeable.

[dqminh]

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.

[wking]

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.

[dqminh]

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.

[wking]

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?

[crosbymichael]

REJECT as the field is optional and its expected that the field is empty when no mount labels are set.

[wking]

On Wed, May 24, 2017 at 08:24:48AM -0700, Mrunal Patel wrote: Closed #843.

Note that the current wording in #844 (as of 203473c) has [1]: If `mountLabel` is specified and SELinux is enabled, the runtime MUST set `mountLabel` for the mounts in the container. There is nothing in that requirement about “an empty value will not be applied / have no effect” [2]. So if maintainers really want to make explicitly-set empty-string values a no-op here (and for hostname? And for process.cwd? And for process.apparmorProfile? etc.), they should probably land something explicit in the spec to that effect. [1]: 203473c#diff-428eec4013a655816cdefafd5d3505f1R645 [2]: #843 (comment)