Skip to content

Conversation

@cyphar
Copy link
Member

@cyphar cyphar commented Nov 6, 2016

According to the runtime-spec, some filesystems must be mounted in all
containers'. So, add verification to make sure someone doesn't ask us to
create a rootfs that is missing key features.

Signed-off-by: Aleksa Sarai [email protected]

According to the runtime-spec[1], some filesystems must be mounted in
all containers'. So, add verification to make sure someone doesn't ask
us to create a rootfs that is missing key features.

[1]: https://github.com/opencontainers/runtime-spec/blob/v1.0.0-rc2/config-linux.md#default-filesystems

Signed-off-by: Aleksa Sarai <[email protected]>
@cyphar
Copy link
Member Author

cyphar commented Nov 6, 2016

Also, apparently runc will not complain if you have mounts set but don't have the right namespace setup. We should probably error out validation in that case too.

{Destination: "/sys"},
{Destination: "/dev/shm"},
{Destination: "/dev/pts"},
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more test to make it failed with invalid mount info?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. I'll do it tomorrow (have an exam tomorrow morning).

dests[filepath.Clean(mount.Destination)] = true
}

// From the spec.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: i think this comment is redundant.

@dqminh
Copy link
Contributor

dqminh commented Nov 7, 2016

do we want to swap this validator package with https://github.com/opencontainers/runtime-tools. They are mostly the same .

@cyphar
Copy link
Member Author

cyphar commented Nov 7, 2016

@dqminh We validate configs.Config, while the runtime-tools validator validates spec configs. Not to mention that AFAIK we do more comprehensive testing of edge cases than runtime-tools. We should eventually upstream our validation, but we have to switch away from the legacy configs.Config first.

@dqminh
Copy link
Contributor

dqminh commented Nov 7, 2016

@cyphar yah i supposed that's fair.

@mrunalp
Copy link
Contributor

mrunalp commented Nov 9, 2016

LGTM

Approved with PullApprove

@crosbymichael
Copy link
Member

crosbymichael commented Nov 28, 2016

I don't know who added a MUST to the spec but these are not required, they are just things that most applications expect.

Things like /sys are not required and you can leave it out for added security. Also you don't need /dev/shm at all for most applications and PTS is only required when you want a PTY.

This is also weird because if we say that they are required then why even give the user the option to mess up.

I would rather leave this as is and change the spec to a SHOULD.

@crosbymichael
Copy link
Member

@philips Could you take a look at this and let me know what you think since you added the section for these required filesystems?

@justincormack
Copy link
Contributor

Yes I have containers that really don't need or want these filesystems. They are required by (for example) most libc implementations, but eg only if you use shm do you need /dev/shm. runc is a low level tool and should allow for weird use cases. In particular if you want to totally lock down ability to write to files by having no writeable mounts you want to remove /dev/shm.

@cyphar
Copy link
Member Author

cyphar commented Jan 23, 2017

In which case, can you please make a PR against the runtime specification to switch things to SHOULD?

@crosbymichael
Copy link
Member

We changed this in the spec to SHOULDs so we don't need this PR, it would have created many issues if we did validate this in the first place

@cyphar cyphar deleted the validate-default-filesystems branch March 4, 2017 13:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants