Uh oh!
There was an error while loading. Please reload this page.
runtimetest: add validateSeccomp - #514
Conversation
0abf4c1 to
3a9ec3aCompare| if name == "getcwd" { | ||
| _, err := os.Getwd() | ||
| if err == nil { | ||
| return fmt.Errorf("when action is %v, return value should being passed to user space as the errno value without executing the system call", sys.Action) |
There was a problem hiding this comment.
I don't see any runtime-oriented RFC 2119 requirements for linux.seccomp, which means we only have the config-oriented SeccSyscallsNamesRequired in specerror, which means we have no grounds for making this a fatal error in compliance testing. I think the best we can do (at least for 1.0.x configs) is warn the caller that the runtime is skipping some not-strictly-required-but-probably-expected functionality.
There was a problem hiding this comment.
Before spec adds RFC requirements, we'd better just give a warn instead of error
| syscallArgs := seccomp.SyscallOpts{ | ||
| Action: "errno", | ||
| Syscall: "getcwd", | ||
| Index: "0", |
There was a problem hiding this comment.
what are Index, Value used for in this case?
There was a problem hiding this comment.
According to the code in parse_action.go:
action, _ := parseAction(arguments[0])
if action == config.DefaultAction && args.argsAreEmpty() {
// default already set, no need to make changes
return nil
}
If args is empty, you can't add syscall.
There was a problem hiding this comment.
If you really want to test the Action, you should change the defautAction not equal to the specified action.
Then, empty args will not be a problem.
4b330b9 to
59c6c2dCompare59c6c2d to
2138ac8Compare| if name == "getcwd" { | ||
| _, err := os.Getwd() | ||
| if err == nil { | ||
| logrus.Warnf("when action is %v, return value should being passed to user space as the errno value without executing the system call", sys.Action) |
There was a problem hiding this comment.
I think the warning is not suitable, that's not what we really want to tell users.
We want to tell them even seccomp is configured but this runtime can not apply it or similar warning
3f1f3ca to
f363ae3CompareLGTM |
0a6e9ae to
bd222f9Compare| for _, sys := range spec.Linux.Seccomp.Syscalls { | ||
| if sys.Action == "SCMP_ACT_ERRON" { | ||
| for _, name := range sys.Names { | ||
| if name == "getcwd" { |
There was a problem hiding this comment.
Can we write t.Skips for other names and actions? I don't want to give users the false impression that we're testing them by silently ignoring other values.
There was a problem hiding this comment.
Can we write
t.Skips…
This is probably blocking on passing the TAP instance down into the validators, which I do in #308. I don't see a clean way to handle this case before #308 lands; if maintainers want a stub in the meantime that's fine, and I don't really care if/how this gets handled in the stub.
There was a problem hiding this comment.
Does that mean that all the action and name combinations will be exported as skipped information? Maybe I misunderstood.
There was a problem hiding this comment.
Does that mean that all the action and name combinations will be exported as skipped information?
Yes, but only if a configuration uses them. That would make it easy for folks writing tests in validation to see that runtimetest was ignoring something which the configuration had requested.
| if name == "getcwd" { | ||
| _, err := os.Getwd() | ||
| if err == nil { | ||
| logrus.Warnf("Syscall action %v can not be properly applied in the runtime", sys.Action) |
There was a problem hiding this comment.
This is a non-TAP warning that will be gobbled by RuntimeInsideValidate. I think you want something like:
t.Skip(1, fmt.Sprintf("%s syscall returns errno", sys.Name))
iferr==nil {
t.Diagnostic("did not return an error")
} else {
t.Diagnosticf("returned %d (%s)", err.WhateverOtherErrno(), err.Error())
}There was a problem hiding this comment.
I tried to modify it, what do you think?
| return nil | ||
| } | ||
| for _, sys := range spec.Linux.Seccomp.Syscalls { | ||
| if sys.Action == "SCMP_ACT_ERRON" { |
87a2e04 to
4907259Compare| if spec.Linux == nil || spec.Linux.Seccomp == nil { | ||
| return nil | ||
| } | ||
| t := tap.New() |
There was a problem hiding this comment.
This TAP-within-a-TAP approach is a bit strange, but I think it might work… We can drop this once #308 lands and the t instance is getting passed around.
There was a problem hiding this comment.
Some other modifications can wait until #308 land. Thank you for your advice.
| if name == "getcwd" { | ||
| _, err := os.Getwd() | ||
| if err == nil { | ||
| t.Diagnostic("Syscall action ERRNO can not be properly applied in the runtime") |
There was a problem hiding this comment.
Maybe the diagnostic could be:
getcwd did not return an error
Once #308 lands with a local test, that test will be the %s syscall returns errno you have in the skip below, so the total output will be:
not ok … - getcwd syscall returns errno
# getcwd did not return an error
| t.Diagnostic("Syscall action ERRNO can not be properly applied in the runtime") | ||
| } | ||
| } else { | ||
| t.Skip(i, fmt.Sprintf("%s syscall returns errno", name)) |
There was a problem hiding this comment.
This should be t.Skip(1, …), since we're only skipping one test.
| t.Skip(i, fmt.Sprintf("%s syscall returns errno", name)) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
And here we want an else case saying that we're skipping the action:
} else {
t.Skip(1, fmt.Sprintf("syscall action %s", sys.Action))
}
or similar.
Signed-off-by: Zhou Hao <zhouhao@cn.fujitsu.com>
4907259 to
5bb8754Comparezhouhao3
commented
Jan 2, 2018
LGTM |
zhouhao3
commented
Jan 3, 2018
ping @liangchenye |
LGTM |
Signed-off-by: Zhou Hao zhouhao@cn.fujitsu.com