Commit Graph

14 Commits

Author SHA1 Message Date
Colin Walters
3441a48c58 checkout: Ensure copies of unreadable usermode checkouts are readable
The extreme special case of "zero mode" files like `/etc/shadow`
comes up again.  What we want is for "user mode" checkouts to
override it to make the file readable; otherwise when operating
as non-root without `CAP_DAC_OVERRIDE` it becomes very difficult
to work with.

Previously, we were hardlinking these files, but then it intersects
with *another* special case around zero sized files, which is
*also* true for `/etc/shadow`.

Trying to avoid hardlinking there unveiled this bug - when
we go to do a copy checkout, we need to override the mode.
2020-09-16 15:14:54 +00:00
Colin Walters
dc2a7de217 lib/commit: Try checksum+hardlink for untrusted local same-uid repos
This mainly helps flatpak for enabling a hardlink-able local pull
during deploy in the --system case.  We assume the files are immutable
when owned by the same uid.

See https://github.com/ostreedev/ostree/issues/1723
and https://github.com/flatpak/flatpak/pull/2342

Closes: #1776
Approved by: uajain
2018-12-04 20:38:41 +00:00
Jonathan Lebon
8eac5be030 lib/commit: Add devino_cache_hits to txn stats
I found this useful while hacking on rpm-ostree but I think it might be
useful enough to upstream. This stat is really helpful for validating
that a pipeline is hitting the devino cache sweet spot.

Closes: #1772
Approved by: cgwalters
2018-11-05 14:08:54 +00:00
Marcus Folkesson
6bf4b3e1d8 Add SPDX-License-Identifier to source files
SPDX License List is a list of (common) open source
licenses that can be referred to by a “short identifier”.
It has several advantages compared to the common "license header texts"
usually found in source files.

Some of the advantages:
* It is precise; there is no ambiguity due to variations in license header
  text
* It is language neutral
* It is easy to machine process
* It is concise
* It is simple and can be used without much cost in interpreted
  environments like java Script, etc.
* An SPDX license identifier is immutable.
* It provides simple guidance for developers who want to make sure the
  license for their code is respected

See http://spdx.org for further reading.

Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>

Closes: #1439
Approved by: cgwalters
2018-01-30 20:03:42 +00:00
Simon McVittie
f63e62fbd2 tests: Don't assume uid == primary gid
Nothing guarantees that each user has a group containing only
themselves. Even if they do, nothing guarantees that its group ID
equals the user ID, particularly if another user earlier in the same
range was created without a corresponding group or vice versa.

Signed-off-by: Simon McVittie <smcv@collabora.com>

Closes: #1390
Approved by: cgwalters
2018-01-02 14:31:36 +00:00
Colin Walters
7c8ea25306 lib/repo: Add a DEVINO_CANONICAL commit modifier flag
I was seeing the `Writing OSTree commit...` phase of rpm-ostree
being very slow lately.  This turns out to be more fallout from
https://github.com/ostreedev/ostree/pull/1170
AKA commit: 8fe4536

Loading the xattrs is slow on my system (F27AW, XFS+LVM, NVMe). I haven't fully
traced through why, but AIUI at least on XFS the xattrs are often stored outside
of the inode so it's a little bit like doing an `open()+read()`. Plus there's
the LSM overhead, etc.

The thing is that for rpm-ostree's package layering use case, we
basically always want to treat the on-disk state as canonical.  (There's
a subtle case here if one does overrides for something that contains
policy but we'll fix that).

Anyways, so we're in a state now where we do the slow but correct thing by
default, which seems sane. But let's allow the app to opt-in to telling us
"really trust devino". The difference between a `stat()` + hash table lookup
versus the full xattr load on my test case of `rpm-ostree install
./tree-1.7.0-10.fc27.x86_64.rpm` is absolutely dramatic; consistently on the
order of 10s without this support, and <1s with (800ms).

Closes: #1357
Approved by: jlebon
2017-12-04 14:42:37 +00:00
Colin Walters
ed15723cd1 lib/commit: Fix hardlink checkout commit with bare-user + mod xattrs
This is more subtle fallout from:
https://github.com/ostreedev/ostree/pull/1170
AKA commit: 8fe4536257

Before, if we found a devino cache hit, we'd use it unconditionally.

Recall that `bare-user` repositories are very special in that they're the only
mode where the on disk state ("physical state") is not the "real" state. The
latter is stored in the `user.ostreemeta` xattr. (`bare-user` repos are also
highly special in that symlinks are regular files physically, but that's not
immediately relevant here).

Since we now have `bare-user-only` for the "pure unprivileged container" case,
`bare-user` should just be used for "OS builds" which have nonzero uids (and
possibly SELinux labels etc.)

In an experimental tool I'm writing "skopeo2ostree" which imports OCI images
into refs, then squashes them together into a single final commit, we lost the
the `81` group ID for `/usr/libexec/dbus-1/dbus-daemon-launch-helper`.

This happened because the commit code was loading the "physical" disk state,
where the uid/gid are zero because that's the uid I happened to be using. We
didn't just directly do the link speedup because I was using `--selinux-policy`
which caused the xattrs to change, which caused us to re-commit objects from the
physical state.

The unit test I added actually doesn't quite trigger this, but I left
it because "why not".  Really testing this requires the installed test
which uses SELinux policy from `/`.

The behavior without this fix looks like:

```
-00755 0 0     12 { [(b'user.ostreemeta', [byte 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x51, 0x00, 0x00, 0x81, 0xed]), (b'security.selinux', b'system_u:object_r:lib_t:s0')] } /usr/lib/dbus-daemon-helper
```

which was obviously totally broken - we shouldn't be picking up the
`user.ostreemeta` xattr and actually committing it of course.

Closes: #1297
Approved by: jlebon
2017-10-23 17:02:28 +00:00
Simon McVittie
e3c3ec5dd9 tests: Reset umask to 022 while creating test repository
In test-basic-root.sh we make assertions about the permissions
of files like baz/cow, which were created without an explicit chmod.
We can't do that unless we control the permissions.

For some reason the "debomatic" autobuilder used to do some Debian
archive rebuilds does the entire build including build-time tests
as uid 0 with umask 002, which broke those assertions. This seems
a weird thing to do, and I've opened a bug, but it also seems
reasonable to fix this test.

This also lets us remove a couple of existing workarounds for the
same issue.

Bug-Debian: https://bugs.debian.org/876138
Signed-off-by: Simon McVittie <smcv@collabora.com>

Closes: #1192
Approved by: cgwalters
2017-09-19 20:41:36 +00:00
Colin Walters
3348baf6eb lib/commit: Ensure bare-user objects are always user-readable
Some of the Jenkins jobs for Fedora Atomic Host broke after updating
to 2017.7, and it turns out that we regressed handling unreadable
files in `bare-user` mode.  An example of this is `/etc/shadow`, which
ends up in the ostree-as-host content as `/usr/etc/shadow`.

Now there are better fixes here; we should probably delete it and create it
during the config merge if it doesn't exist.  In general, having secret files in
ostree really isn't supported, so it doesn't make sense to include them.

But let's fix this regression - when operating as an unprivileged user we don't
have `CAP_DAC_OVERRIDE` and hence will fail to open un-user-readable objects.

(We still preserve the actual `0` mode of course in the xattr and will
 apply it in `bare`)

Closes: #989
Approved by: jlebon
2017-06-30 21:23:48 +00:00
Colin Walters
5913b22944 lib/repo: For bare-user, mask content object modes with 0775
Having every object in a bare-user repo (and checkouts) be executable
is ugly.  I can't think of a good reason to do that; they should only
be executable if their input is.  This does
for `bare-user` what we did for `bare-user-only` in
https://github.com/ostreedev/ostree/pull/909
It's also a stronger version of what we do with `checkout -U` in suppressing
suid - here we also strip world-writable files and the sticky bit (even though
that's meaningless today, it might not be in the future).

Closes: https://github.com/ostreedev/ostree/issues/907

Closes: #908
Approved by: alexlarsson
2017-06-08 06:50:16 +00:00
Simon McVittie
01fb30b839 Fix TAP syntax in test-basic-user.sh, and run it
In its initial commit, Alexander Larsson wrote

    This works standalone, but unfortunately it breaks in
    gnome-desktop-testing-runner as /tmp doesn't support
    xattrs, so it is not installed atm.

but we now (a) use /var/tmp, and (b) explicitly skip the test if
xattr support is unavailable. So it should be OK to run now.

Closes: #652
Approved by: cgwalters
2017-01-19 13:54:59 +00:00
Simon McVittie
3e3755c497 various tests: skip if temp directory lacks xattr support
Some autobuilder environments place the entire build chroot on tmpfs, so
even /var/tmp might not have this.

Signed-off-by: Simon McVittie <smcv@debian.org>

Closes: #232
Approved by: cgwalters
2016-03-31 14:22:06 +00:00
Colin Walters
5ebe43859d tests: Use "bash strict mode"
I noticed in the static deltas tests, there were some tests that
should have been under `-o pipefail` to ensure we properly propagate
errors.

There were a few places where we were referencing undefined variables.

Overall, this is clearly a good idea IMO.
2016-01-27 11:44:10 -05:00
Alexander Larsson
fcd3caf6dd Add test-basic-user.sh testing for bare-user repos
This just does whatever test-basic.sh does, but on a bare-user
repo.

This works standalone, but unfortunately it breaks in
gnome-desktop-testing-runner as /tmp doesn't support
xattrs, so it is not installed atm.

https://bugzilla.gnome.org/show_bug.cgi?id=741125
2014-12-08 10:39:44 +01:00