runtimetest: add readonly path validation #76
Conversation
|
On Tue, May 24, 2016 at 08:52:20PM -0700, Ma Shimiao wrote:
Travis is missing ‘spec’ in main.go around line 194 1. |
| @@ -191,6 +191,20 @@ func validateSysctls(spec *rspec.Spec) error { | |||
| return nil | |||
| } | |||
|
|
|||
| func validateROPaths(spec, *rspec.Spec) error { | |||
Mashimiao
force-pushed
the
runtime-test-readonly-path-validation
branch
from
7a11ef4 to
bee2090
Compare
|
On Tue, May 24, 2016 at 11:11:04PM -0700, Ma Shimiao wrote:
Ah, that's the piece I was forgetting. This looks good to me then. |
| @@ -191,6 +191,20 @@ func validateSysctls(spec *rspec.Spec) error { | |||
| return nil | |||
| } | |||
|
|
|||
| func validateROPaths(spec *rspec.Spec) error { | |||
| fmt.Println("validating Readonly Paths") | |||
There was a problem hiding this comment.
using 'validating readonlyPaths' as Mrunal suggested match what is expected to be the property name.
Mashimiao
force-pushed
the
runtime-test-readonly-path-validation
branch
from
bee2090 to
d55b619
Compare
|
@liangchenye fixed. |
|
LGTM |
| func validateROPaths(spec *rspec.Spec) error { | ||
| fmt.Println("validating readonlyPaths") | ||
| for _, v := range spec.Linux.ReadonlyPaths { | ||
| fi, err := os.Stat(v) |
There was a problem hiding this comment.
This stat-based approach should be changed to an attempted-write approach.
There was a problem hiding this comment.
And it's probably worth making “is this path readonly?” a separate function so it can be shared with the check in #81.
There was a problem hiding this comment.
@wking The spec says "readonlyPaths will set the provided paths as readonly inside the container."
It seems not to remount readonlyPaths and just change their permisson inside container. If this is true, I think current implementation is OK.
There was a problem hiding this comment.
On Thu, May 26, 2016 at 07:30:15PM -0700, Ma Shimiao wrote:
It seems not to remount readonlyPaths and just change their
permisson.
I think the spec wording leaves the implemenation ambiguous. And
attempting a read clearly covers all possible implementations, while
checking the mode only covers a single implementation.
|
This has the same problem as #78 -- mounting something readonly doesn't change the mode. We should check that it is actually readonly by attempting a write and making sure it fails with |
Mashimiao
force-pushed
the
runtime-test-readonly-path-validation
branch
from
d55b619 to
3c250a9
Compare
|
3c250a9 LGTM |
|
IANAM, but LGTM 3c250a9. |
|
Looks good but needs rebase. |
Signed-off-by: Ma Shimiao <[email protected]>
Mashimiao
force-pushed
the
runtime-test-readonly-path-validation
branch
from
3c250a9 to
447a73b
Compare
|
@mrunalp rebased. |
|
LGTM |
Signed-off-by: Ma Shimiao [email protected]