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

lib: unify user-mode canonical mask to 0775 #2420

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

lucab
Copy link
Member

@lucab lucab commented Aug 25, 2021

This aligns all canonical permissions masks to 0775, which is relevant
for bare-user-only mode.
Such mask is already used for user-mode pulls and checkouts; however
the commit logic/modifier was using a different and smaller mask (0755),
which resulted in asymmetries across operations.


Context: I noticed some misalignment around this while touching #2410.
I guess it was supposed to be done in #913, but somehow it didn't happen there.
I did some history reading and found flatpak/flatpak#837 (comment) which seems to suggest this larger mask would be beneficial for some flatpak content.

This aligns all canonical permissions masks to 0775, which is relevant
for bare-user-only mode.
Such mask is already used for user-mode pulls and checkouts; however
the commit logic/modifier was using a different and smaller mask (0755),
which resulted in asymmetries across operations.
@lucab
Copy link
Member Author

lucab commented Aug 25, 2021

/cc @alexlarsson @cgwalters

@@ -298,7 +298,7 @@ commit_loose_regfile_object (OstreeRepo *self,
*/
if (S_ISREG (mode))
{
const mode_t content_mode = (mode & (S_IFREG | 0775)) | S_IRUSR;
const mode_t content_mode = (mode & USERMODE_CANONICAL_MASK) | S_IFREG | S_IRUSR;
Copy link
Member

Choose a reason for hiding this comment

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

Hmm. There's a really important difference between bare-user and bare-user-only, which is that bare-user is intended to be losslessly convertible to-from bare. Now, we store the mode as an xattr, but it kind of intentional that the checked out tree mostly resembles what's in the bare.

I'd be happier I think if this change only touched code paths involved in bare-user-only. Then the messaging is clearer.

@cgwalters
Copy link
Member

I would also like to have an ack from the flatpak maintainers before merging because this will basically mainly affect that. Looks like that may be @smcv ?

@smcv
Copy link
Contributor

smcv commented Aug 27, 2021

Sorry, I am not in a position to make this decision on behalf of Flatpak. I'm doing some release-management stuff at the moment, and I've been working with the containerization side of Flatpak, but don't understand the "big picture" of how Flatpak interacts with libostree.

@@ -1318,7 +1318,7 @@ adopt_and_commit_regfile (OstreeRepo *self,
if (self->mode == OSTREE_REPO_MODE_BARE_USER_ONLY)
{
const guint32 src_mode = g_file_info_get_attribute_uint32 (finfo, "unix::mode");
if (fchmod (fd, src_mode & 0755) < 0)
if (fchmod (fd, src_mode & USERMODE_CANONICAL_MASK) < 0)
Copy link
Member

Choose a reason for hiding this comment

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

Hmm. I was surprised this didn't seem to break any tests that looked at the commit permissions. That seems to be a bit lacking in the tests. There are several that look at the permissions after a checkout, but only a few that look at the permissions of the committed objects with ostree ls.

@alexlarsson
Copy link
Member

From the flatpak side this should be fine. The only difference is that a new build could produce a file that has a group writeable bit set (where it was stripped before), but those would be allowed already by older flatpak versions (by the 775 validation), so should be fine.

Security wise this would mean potentially leaving a root-group writable directory in the app checkouts, which should be fine too.

@openshift-ci
Copy link

openshift-ci bot commented Jun 29, 2023

@lucab: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/images 02f86aa link true /test images
ci/prow/fcos-e2e 02f86aa link true /test fcos-e2e

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

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.

5 participants