validate: increase OS validation for special cases #350
Conversation
validate/validate.go
Outdated
| if err := CapValid(capability, v.HostSpecific); err != nil { | ||
| msgs = append(msgs, fmt.Sprintf("capability %q is not valid, man capabilities(7)", capability)) | ||
| if v.spec.Platform.OS == "linux" { | ||
| for _, capability := range process.Capabilities { |
There was a problem hiding this comment.
process.capabilities is no longer Linux-specific since opencontainers/runtime-spec#673 (which removed this line). I'm not clear on how the capabilities check is supposed to work on other OSes, but unless the spec changes I don't think we can have a Linux-only check here.
There was a problem hiding this comment.
But the validation of capabilities is compared with CAPABILITIES (7), CAPABILITIES(7) is the value under linux.
There was a problem hiding this comment.
So we should add checks for capabilities on other OSes. Or move the Linux-specific cap validation into a CheckLinuxCapabilities and raise not-implemented-yet errors if someone tries to validate a non-Linux config. But silently ignoring process.capabilities in non-Linux configs will give users a false sense of security.
There was a problem hiding this comment.
But it is not clear the value of other systems, it can not be verified.
My idea is to verify the linux system under the capabilities of the special value, not the entire capabilities field validation, so is not the integration of these tests into a function will be better?
There was a problem hiding this comment.
But it is not clear the value of other systems, it can not be verified.
I agree, and think somebody with more weight than me should bring this up among the runtime-spec maintainers before they cut 1.0 (I tried, but didn't have much success).
My idea is to verify the linux system under the capabilities of the special value, not the entire capabilities field validation…
That's fine, but I don't think we should be silently passing on non-Linux OSes. Does moving the Linux-specific cap validation into CheckLinuxCapabilities and raising not-implemented-yet errors for non-Linux configs sound like a bad idea to you?
validate/validate.go
Outdated
| for i := index + 1; i < len(process.Rlimits); i++ { | ||
| if process.Rlimits[index].Type == process.Rlimits[i].Type { | ||
| msgs = append(msgs, fmt.Sprintf("rlimit can not contain the same type %q.", process.Rlimits[index].Type)) | ||
| for index, rlimit := range process.Rlimits { |
There was a problem hiding this comment.
process.rlimits is also no longer Linux-specific since opencontainers/runtie-spec#673, so this shouldn't get moved into a Linux-specific section.
validate/validate.go
Outdated
| } | ||
| } | ||
| } else { | ||
| logrus.Warnf("OS %q has not yet have a special value for capabilities", v.spec.Platform.OS) |
There was a problem hiding this comment.
Maybe reword to:
process.capabilities validation not yet implemented for OS %q
zhouhao3
force-pushed
the
linux-check
branch
2 times, most recently
from
0e058ea
to
0eae9d8
Compare
validate/validate.go
Outdated
| for i := index + 1; i < len(process.Rlimits); i++ { | ||
| if process.Rlimits[index].Type == process.Rlimits[i].Type { | ||
| msgs = append(msgs, fmt.Sprintf("rlimit can not contain the same type %q.", process.Rlimits[index].Type)) | ||
| } |
There was a problem hiding this comment.
duplicate rlimits check should work for all OSs, not just Linux
Signed-off-by: zhouhao <zhouhao@cn.fujitsu.com>
|
LGTM |
|
LGTM |
For some of the linux system under the special value of the verification, should increase the decision on the
os.Signed-off-by: zhouhao zhouhao@cn.fujitsu.com