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

Clarify effects of some commands in the man page #618

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

WxNzEMof
Copy link

Fixes #617.

RFC on wording, maybe "permanent" is not the best way to describe it, but it would address the confusion I had.

Copy link
Collaborator

@smcv smcv left a comment

Choose a reason for hiding this comment

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

bwrap.xml: Place the --perms details in their own paragraph

This commit looks fine, I might cherry-pick it.

Copy link
Collaborator

@smcv smcv left a comment

Choose a reason for hiding this comment

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

Please add a Signed-off-by to the commits to indicate your acceptance of the Developer Certificate of Origin (basically declaring that we can use your contribution in the obvious ways).

Comment on lines +326 to +327
<para>Note that if <arg choice="plain">DEST</arg> is visible outside
of the created mount namespace, the effect is permanent.</para>
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think it scales very well to repeat this for every option that might alter the filesystem, because there are a lot:

  • everything that mounts a directory will create an empty directory as a mount point if necessary
  • everything that mounts a non-directory will create an empty regular file as a mount point if necessary
  • --symlink will modify the real filesystem if its destination is a writeable file system
  • so will --chmod

This is really a fact about mounting filesystems, not a fact about the command-line options that modify the filesystem. Would it perhaps be clearer what is going on if we say something similar in --bind instead of this? Maybe like this:

        <para>If a process inside the sandbox or a later command-line option
          modifies the contents of <arg choice="plain">DEST</arg>, those
          modifications will affect <arg choice="plain">SRC</arg>
          outside the sandbox.</p>

If --bind said this, would you have assumed that the same was true for --bind-try, --dev-bind and --dev-bind-try without needing it to be said explicitly?

(We certainly don't need to add this to --ro-bind and --ro-bind-try because those are read-only mounts.)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it's valuable to use essentially the same language to talk about what happens if a process inside the sandbox does a filesystem write (whether that's symlink() or chmod() or a file overwrite or whatever), and what happens if a subsequent command-line option tells bwrap to do a filesystem write, because they're fundamentally the same operation.

I wouldn't want anyone to get the mistaken idea that a --symlink or --chmod or --file is permanent, but the COMMAND doing an equivalent symlink() or chmod() or open() somehow wouldn't be permanent.

Comment on lines +341 to +343
<para>Note that, unlike <option>--bind-data</option>, if
<arg choice="plain">DEST</arg> is visible outside of the created mount
namespace, the effect is permanent.</para>
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it's worth calling out --file specifically, because it's the only one that will silently overwrite an existing file, but I'd prefer it more like perhaps this:

        <para>If <arg choice="plain">DEST</arg> already exists, it will be overwritten.
          Unlike <option>--bind-data</option>, if <arg choice="plain">DEST</arg> or
          a parent directory is a bind-mount from outside the sandbox,
          this may overwrite an existing file outside the sandbox.</para>

Would that have given you the right expectations?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, that sounds great. Shall I apply it to this PR?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes please!

@WxNzEMof
Copy link
Author

Please add a Signed-off-by to the commits to indicate your acceptance of the Developer Certificate of Origin (basically declaring that we can use your contribution in the obvious ways).

Is it OK if I do this pseudonymously (using my GitHub username)?

@smcv
Copy link
Collaborator

smcv commented Jan 31, 2024

Is it OK if I do this pseudonymously (using my GitHub username)?

Hmm. I would personally be fine with that, but it isn't my project or my policy, and https://github.com/containers/common/blob/main/CONTRIBUTING.md does say no anonymous or pseudonymous contributions.

@cgwalters, @alexlarsson, do you intend to be enforcing that policy for this project?

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.

not immediately obvious that --file can overwrite a file mounted rw from outside the container
2 participants