Skip to content
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

Fix permissions issues with sandbox mounts #1211

Merged

Conversation

katiewasnothere
Copy link
Contributor

@katiewasnothere katiewasnothere commented Oct 28, 2021

An issue was reported where when a user went to write files as a non-privileged user in a sandboxMount mounted with r/w in a container, they'd get a permission denied error. This was because the underlying directories in the UVM were created with 0755 permissions so only a privileged user in the container could write to a r/w sandboxMount mounted in. This PR changes that behavior by creating the underlying directories with 0777 permissions.

Signed-off-by: Kathryn Baldauf kabaldau@microsoft.com

// Temporarily set the umask of this process to 0 so that we can actually
// make all dirs with os.ModePerm permissions.
savedUmask := unix.Umask(0)
if err := os.MkdirAll(sandboxSource, os.ModePerm); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

This will not reset the umask if MkdirAll returns non-nil error. Can we change this to something like:

savedumask := unix.Umask(0)
defer unix.Umask(savedUmask)
err := os.MkdirAll(...)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

Signed-off-by: Kathryn Baldauf <kabaldau@microsoft.com>
@katiewasnothere
Copy link
Contributor Author

@dcantah and @ambarve ptal

Copy link
Contributor

@ambarve ambarve left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants