-
Notifications
You must be signed in to change notification settings - Fork 144
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.
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
runtimetest: add masked paths validation #82
runtimetest: add masked paths validation #82
Conversation
return err | ||
} | ||
if fi.Mode()&0444 != 0 { | ||
return fmt.Errorf("%v should be readonly", v) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error message shouldn't be about “readonly”. And this is probably another place where we want to use “just try it” (in this case, read and write) instead of a mode check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wking spec says "mask over the provided paths inside the container so that they cannot be read." it definitely says change paths permission to make them can't be read.
So, I think there is no need to "just try it".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On Fri, May 27, 2016 at 12:30:03AM -0700, Ma Shimiao wrote:
spec says "mask over the provided paths inside the container so that
they cannot be read." it definitely says change paths permission to
make them can't be read.
I don't read “mask over” as clearly “change the permission bits”. I
haven't been able to turn up runC's implementation with grep, but
@mrunalp's original runtime-spec PR was clearly recommending
bind-mounts 1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, maybe we need "just try it". I get from runc code, it implements maskedpaths as bing mount them to /dev/null
It makes a bit hard to validate.
25c9c96
to
b891e5e
Compare
We can add a test that tries to read from a path and gets a EOF. f, err := os.Open(maskedPath)
if err != nil {
return err
}
defer f.Close()
b := make([]byte, 1)
_, err = f.Read(b)
if err == io.EOF {
fmt.Println("Test succeeded")
} else {
fmt.Println("Test failed")
} |
b891e5e
to
74a274e
Compare
@mrunalp I combined your suggested code with mine. If a maskedPath can not be read, an error "permission denied" will occur, I think that is not what we want. |
@Mashimiao Are you seeing a permission denied with runc? I think it shouldn't return that error in any scenario. |
@mrunalp It seems I think too much :-). runc should be run by root. |
Signed-off-by: Ma Shimiao <[email protected]>
74a274e
to
615585a
Compare
LGTM |
Signed-off-by: Ma Shimiao [email protected]