ostree commit: Add option --use-bare-user-xattrs

[wmanley]

For our build-system we have a patched version of fakeroot which
understands the user.ostreemeta xattr. It's intended for use with
ostree checkout --user-mode --require-hardlinks. When programs are run
with LD_PRELOAD=libfakeroot... they see the ownership and permissions
that ostree understands, rather than the real ones.

Our build system mostly consists of calls to:

ostree checkout --user-mode --require-hardlinks $COMMIT root
LD_PRELOAD=fakeroot <make some changes>
ostree commit --tree=dir=root --link-checkout-speedup --devino-canonical

in the past this seemed to work fine but at some point I started losing the
setuid bit on sudo. I suspect the previous behaviour was a bug. This
commit makes the ostreemeta preserving behaviour explicit.

--use-bare-user-xattrs will currently be ignored when loading a commit
from a tarball. This can be fixed in the future.

I originally considered implementing this inside _ostree_repo_commit_modifier_apply but we don't have access to the file fd to retrieve the xattrs from in there. I considered using g_file_info_get_attribute_data (info, "xattr::ostreemeta", ...) but it GLib has no way of retrieving an attribute that may contain embedded NULs.

This may need a little bit more work (see TODO list below) but I wanted to raise it sooner rather than later to receive feedback.

I'm not super happy with the name

TODO:

• Test xattrs in xattrs
• Rebase and test for conflicts with checkout: Ensure copies of unreadable usermode checkouts are readable #2199 and checkout: Don't hardlink zero sized files #2198 - check that empty files and files with 0 permissions have xattrs set on checkout.

[cgwalters]

LD_PRELOAD=fakeroot

Yeah. For gnome-continuous I basically ended up maintaining the set of non-root or setuid things out of band. What happens with rpm-ostree is pretty similar; the canonical permissions are maintained in the RPM spec %files section - it's also out-of-band.

Did you evaluate just how many packages/components/sources have non-root-owned files or setuid files? I suspect it's similar.

Anyways, let me see if I understand this bug correctly - your patched version of fakeroot is writing out its own user.ostreemeta xattrs, and the current commit process effectively only uses that if the file is already hardlinked?

Offhand...I feel like we could probably just make this the default - if we detect a user.ostreemeta xattr, we adopt it? The downside of this level of magic would be that it becomes impossible to commit a filesystem tree that happens to contain a bare-user ostree repository. I'm not sure how much I care about that though - what would be the use case?

[wmanley]

Did you evaluate just how many packages/components/sources have non-root-owned files or setuid files? I suspect it's similar.

I've not looked into this explicitly. I'm installing stock Ubuntu 14.04 and 16.04 using multistrap and then bwrap, so I want to avoid modifying the upstream packages. /usr is mostly root:root, but some files have a different gid, perticularly setgid executables like wall. /etc has more variety with ssl certificates stored as user ssl-cert, and the shadow, etc. files as group shadow.

Anyways, let me see if I understand this bug correctly - your patched version of fakeroot is writing out its own user.ostreemeta xattrs, and the current commit process effectively only uses that if the file is already hardlinked?

Exactly

Offhand...I feel like we could probably just make this the default - if we detect a user.ostreemeta xattr, we adopt it? The downside of this level of magic would be that it becomes impossible to commit a filesystem tree that happens to contain a bare-user ostree repository. I'm not sure how much I care about that though - what would be the use case?

I can't think of any use-case, I implemented it this way because I thought the change of behaviour would be more acceptable if it we're behind an explicit option. I'd be happy to make it the default.

One open question then would be how to deal with files that don't have user.ostreemeta xattrs set. As I've been using this new code I've found that I want non-xattr files to be interpreted as root:root 0644. fakeroot doesn't seem to catch all the file-accesses - it doesn't interpose creat for example - but I don't want these files to take on the uid from the build environment - I want them to be root. So I've made ostree commit fall-back to root:root. I'm not sure of an elegant way of solving this problem without an explicit flag.

I've just pushed two more commits that:

• Make uid:gid default to 0:0
• Make ostree checkout also write ostreemeta xattrs for directories that it creates.

I'm not sure what you think of these ideas?

[rh-atomic-bot]

☔ The latest upstream changes (presumably 1c0933b) made this pull request unmergeable. Please resolve the merge conflicts.

[openshift-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: wmanley
To complete the pull request process, please assign pwithnall
You can assign the PR to them by writing /assign @pwithnall in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[wmanley]

/test sanity

[openshift-ci-robot]

@wmanley: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/test sanity

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.

[wmanley]

I've rebased this and reworked it a bit fixing the xattrs in xattrs case. It's a bit more general than before because it shares the xattr loading code with get_final_xattrs.

I seem to have lost the ability to get the bot to do my bidding. I guess that's related to my name being commented out in OWNERS:

# Does not currently have affiliation public
# - wmanley

@cgwalters: What is affiliation in this context?, and how does one make it public?

/assign @pwithnall

[cgwalters]

@cgwalters: What is affiliation in this context?, and how does one make it public?

https://docs.github.com/en/github/setting-up-and-managing-your-github-user-account/publicizing-or-hiding-organization-membership

The bot needs to know you're a member of the organization.

(Plus, you can think of it like collecting flair! )

[wmanley]

The bot needs to know you're a member of the organization.

Great, thanks. Let's give it a go:

/test sanity

(Plus, you can think of it like collecting flair! )

😄

[wmanley]

TODO: Rebase and test for conflicts with #2199 and #2198 - check that empty files and files with 0 permissions have xattrs set on checkout.

[openshift-ci-robot]

@wmanley: PR needs rebase.

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.

[openshift-ci]

@wmanley: 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/sanity	a9ca968	link	true	/test sanity
ci/prow/images	a9ca968	link	true	/test images
ci/prow/fcos-e2e	a9ca968	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.