I'm thinking about adding an implementation of ed25519 signatures
with OpenSSL (so we can ship the feature with Fedora CoreOS
without requiring an additional library) and in preparation for
that it's essential that we validate that libsodium-generated
signatures and OpenSSL-generated signatures are compatible.
I don't know if they are yet actually, but the goal of this
new test is to add a pre-generated repository with a signed
commit generated by libsodium.
This will catch if e.g. there's ever a change in libsodium,
or if existing libsodium implementation versions (e.g. the
one in Debian) might differ from what we ship here.
Add a standard key for this. We actually had a case in OpenShift
builds recently where a `ppc64le` image was pushed over an `x86_64`
one and this started failing at runtime with a not immediately
obvious error.
I'll probably end up changing rpm-ostree at least to use
the RPM architecture for this key and fail if it doesn't match
the booted value.
Possibly that should live in ostree but it would involve adding
architecture schema here, which gets into a big mess. Let's
just standardize the key.
xref https://github.com/coreos/coreos-assembler/commit/e02ef2683d688607e7b5ad9ea6a0c00c50a682a5
One OpenShift user saw this from rpm-ostree:
```
client(id:cli dbus:1.583 unit:machine-config-daemon-host.service uid:0) added; new total=1
Initiated txn UpdateDeployment for client(id:cli dbus:1.583 unit:machine-config-daemon-host.service uid:0): /org/projectatomic/rpmostree1/rhcos
Txn UpdateDeployment on /org/projectatomic/rpmostree1/rhcos failed: File header size 4294967295 exceeds size 0
```
which isn't very helpful. Let's add some error
prefixing here which would at least tell us which
object was corrupted.
There's a lot going on here. First, this is intended to run
nicely as part of the new [cosa/kola ext-tests](https://github.com/coreos/coreos-assembler/pull/1252).
With Rust we can get one big static binary that we can upload,
and include a webserver as part of the binary. This way we don't
need to do the hack of running a container with Python or whatever.
Now, what's even better about Rust for this is that it has macros,
and specifically we are using [commandspec](https://github.com/tcr/commandspec/)
which allows us to "inline" shell script. I think the macros
could be even better, but this shows how we can intermix
pure Rust code along with using shell safely enough.
We're using my fork of commandspec because the upstream hasn't
merged [a few PRs](https://github.com/tcr/commandspec/pulls?q=is%3Apr+author%3Acgwalters+).
This model is intended to replace *both* some of our
`make check` tests as well.
Oh, and this takes the obvious step of using the Rust OSTree bindings
as part of our tests. Currently the "commandspec tests" and "API tests"
are separate, but nothing stops us from intermixing them if we wanted.
I haven't yet tried to write destructive tests with this but
I think it will go well.
Follow the precedent set in https://github.com/coreos/rpm-ostree/pull/2106
and rename the directory, to more clearly move away from the
"uninstalled" test model. Prep for Rust-based tests.
We recently disabled the read-only /sysroot handling:
https://github.com/ostreedev/ostree/pull/2108/commits/e35b82fb891daee823fcce421ae8f1442b630ea2
The core problem was that a lot of services run early in the
real root and want write access to things like `/var` and `/etc`.
In trying to do remounts while the system is running we introduce
too many race conditions.
Instead, just make the `/etc` bind mount in the initramfs right
after we set up the main root. This is much more natural really,
and avoids all race conditions since nothing is running in the
sysroot yet.
The main awkward part is that since we're not linking
`ostree-prepare-root` to GLib (yet) we have a hacky parser
for the config file. But, this is going to be fine I think.
In order to avoid parsing the config twice, pass state from
`ostree-prepare-root` to `ostree-remount` via a file in `/run`.
The goal here is to move the code towards a model
where the *client* can explicitly specify which signature types
are acceptable.
We retain support for `sign-verify=true` for backwards compatibility.
But in that configuration, a missing public key is just "no signatures found".
With `sign-verify=ed25519` and no key configured, we can
explicitly say `No keys found for required signapi type ed25519`
which is much, much clearer.
Implementation side, rather than maintaining `gboolean sign_verify` *and*
`GPtrArray sign_verifiers`, just have the array. If it's `NULL` that means
not to verify.
Note that currently, an explicit list is an OR of signatures, not AND.
In practice...I think most people are going to be using a single entry
anyways.
In a CI run I think one of these `ostree show` commands is failing.
While that output would be useful, the actual `err.txt` usually
has what we want too.
There's a lot of historical baggage associated with GPG verification
and `ostree pull` versus `ostree pull-local`. In particular nowadays,
if you use a `file://` remote things are transparently optimized
to e.g. use reflinks if available.
So for anyone who doesn't trust the "remote" repository, you should
really go through through the regular
`ostree remote add --sign-verify=X file://`
path for example.
Having a mechanism to say "turn on signapi verification" *without*
providing keys goes back into the "global state" debate I brought
up in https://github.com/ostreedev/ostree/issues/2080
It's just much cleaner architecturally if there is exactly one
path to find keys: from a remote config.
So here in contrast to the GPG code, for `pull-local` we explictily
disable signapi validation, and the `ostree_repo_pull()` API just
surfaces flags to disable it, not enable it.
In https://bugzilla.redhat.com/show_bug.cgi?id=1827712
some OpenShift CI is seeing `/boot` being unmounted before
`ostree-finalize-staged.service` runs or completes.
We finally tracked this down to a bug elsewhere, but
I think we should add this because it clearly shows
our requirements.
Previously in the pull code, every time we went to verify
a commit we would re-initialize an `OstreeSign` instance
of each time, re-parse the remote configuration
and re-load its public keys etc.
In most cases this doesn't matter really because we're
pulling one commit, but if e.g. pulling a commit with
history would get a bit silly.
This changes things so that the pull code initializes the
verifiers once, and reuses them thereafter.
This is continuing towards changing the code to support
explicitly configured verifiers, xref
https://github.com/ostreedev/ostree/issues/2080
This cleans up the verification code; it was weird how
we'd get the list of known names and then try to create
an instance from it (and throw an error if that failed, which
couldn't happen).
Per https://github.com/ostreedev/ostree/issues/2080#issuecomment-623614483
A huge benefit of ed25519 (and ECC in general) is that keys are very
short - short enough that it's completely reasonable to inline
them into a command line argument.
And I think that's a good model; it makes the keys very visible.
For example, someone could easily copy-paste a commandline
argument from a webpage (secured via TLS) that says to run
`ostree remote add --sign-verify=ed25519=inline:KEY`.
With just `After=` we'll still try to run in the scenario
where `sysroot.mount` fails because the rootfs didn't appear.
And this will end up spewing an error which can confuse people
into thinking something is wrong at the ostree level.
This has come up numerous times w/{Fedora,RHEL} CoreOS, most
recently while looking at
https://bugzilla.redhat.com/show_bug.cgi?id=1803130
I think we should encourage removing the writable bits from
executables. This has happened to me:
https://thomask.sdf.org/blog/2019/11/09/take-care-editing-bash-scripts.html
And not having the writable bit may help prevent hardlink
corruption with OSTree in some cases.
We can't do this by default, but add a convenient CLI flag
for it.
Same motivation as
https://github.com/coreos/rpm-ostree/pull/2060
I tried `InaccessiblePaths=/var` first and was very sad to find
out we have one tiny exception that breaks it. Otherwise it'd
be so elegant. Maybe in the future we split out that one thing
to a separate `ostree-finalized-stage-var.service` that's just
`ExecStart=/bin/rm -vf /var/.updated` and is otherwise
`ProtectSystem=strict` etc.
All of the underlying libostree APIs have supported passing `NULL`
for a merge deployment for...a long time. But we never plumbed
it up into the CLI.
Add a `--no-merge` option to aid people who want to do a "factory reset":
https://github.com/ostreedev/ostree/issues/1793
`ostree-repo-pull.c` is rather monstrous; I plan to split it
up a bit. There's actually already a `pull-private.h` but
that's just for the binding verification API. I think that one
isn't really pull specific. Let's move it into the "catchall"
`repo.c`.
As far as I can tell we're not gating on this right now. From
a quick glance, it looks like Debian stable has `libsodium-dev`
but only Ubuntu eoan does which we're not testing right now.
`libsodium` is an implementation detail. In particular, I'd like
to consider using OpenSSL for ed25519 (if libsodium isn't configured
and openssl is).
So switch the name of the exposed feature and adjust the tests.
The goal is to test "no options" build - and eventually tests.
(We're not actually including libsodium in the cosa buildroot right
now, but we may in the future)
Previously we would pass the `verification-key` and `verification-file`
to all backends, ignoring errors from loading keys until we
found one that worked.
Instead, change the options to be `verification-<engine>-key`
and `verification-<engine>-file`, and then
rework this to use standard error handling; barf explicitly if
we can't load the public keys for example. Preserve
the semantics of accepting the first valid signature. The
first signature error is captured, the others are currently
compressed into a `(and %d more)` prefix.
And now that I look at this more closely there's a lot of
duplication between the two code paths in pull.c for verifying;
will dedup this next.
I'm mainly doing this to sanity check the CI state right now.
However, I also want to more cleanly/clearly distinguish
the "sign" code from the "gpg" code.
Rename one function to include `gpg`.
For the other...I think what it's really doing is using the remote
config, so change it to include `remote` in its name.
I was trying to followup the `--selinux-policy-from-base` work
to add a `cosa build --fast=overlay` for coreos-assembler,
but hit on the fact that using e.g. `--owner-uid` disables
commit optimizations.
A while ago, https://github.com/ostreedev/ostree/pull/1643 landed
which optimized this for the case where no modifications are provided.
But, we really need the SELinux policy bits, and it's super convenient
to run `ostree commit` as non-root.
It's fairly surprising actually that it's taken us so long to
iterate on a good interface for this "commit changes on top of a base"
model. In practice, many nontrivial cases really end up needing
to do a (hardlink) checkout, and that case is optimized.
But for this coreos-assembler work I want to directly overlay onto
a commit object another commit object.
That previous PR above added exactly the API we need, so let's
expose it in the CLI.
What you can see happening in the test is that we provide
`--owner-uid 42`, but that only applies to directories/files
that were added in the commit.
And now that I look at this, I think what we really want here
is to avoid changing directories that exist in the base, but
eh; in practice the main use here is for `--owner-uid 0` while
committing as non-root; and that works fine with this since
the baseline uid will be zero as well.
This code wasn't written with idiomatic GError usage; it's not standard
to construct an error up front and continually append to its
message. The exit from a function is usually `return TRUE`,
with error conditions before that.
Updating it to match style reveals what I think is a bug;
we were silently ignoring failure to parse key files.
I am getting SELinux xattrs committed in local development (inside
https://github.com/cgwalters/coretoolbox ), which
throws off the hardcoded sizes in this test.
Noticed this while writing tests for a core `ostree_sysroot_load()`
entrypoint. And decided to do the same for `ostree_repo_open()`,
and while there also noted we had a duplicate error prefixing
for the open (more recently `glnx_opendirat()` automatically
prefixes with the path).
The [dev-overlay](https://github.com/coreos/coreos-assembler/blob/332c6ab3b91778d904224c3c960d9cc4739d60bd/src/cmd-dev-overlay)
script shipped in coreos-assembler mostly exists to deal
with the nontrivial logic around SELinux policy. Let's make
the use case of "commit some binaries overlaying a base tree, using
the base's selinux policy" just require a magical
`--selinux-policy-from-base` argument to `ostree commit`.
A new C API was added to implement this in the case of `--tree=ref`;
when the base directory is already checked out, we can just reuse
the existing logic that `--selinux-policy` was using.
Requires: https://github.com/ostreedev/ostree/pull/2039
This test keeps occasionally failing in CI - as expected, because
we retry 8 times for an object but it's completely possible for
us to hit the <0.5% chance of 50% failure 8 times in a row.
Since the max errors from the server is 100, set retries to the
same thing.
Rework the simple cases of "commit ." and "commit argv[1]" to
generate the more general "--tree=X --tree=Y" path, so that we
only have one primary control flow here.
Prep for a future patch around loading SELinux policy from
the first argument.
Previously we made an effort to use the [Fedora Standard Test Interface](https://docs.fedoraproject.org/en-US/ci/standard-test-interface/).
This effort was not very successful; the primary thing that
it really died on is Ansible just didn't support rebooting
very well. I think that's since gotten better, but even
then, Ansible wasn't the best thing for a test framework
for us anyways.
In the meantime Fedora CoreOS happened emphasizing Ignition
and not "post-hoc reconciliation" models like Ansible over
ssh.
And, [coreos-assembler](https://github.com/coreos/coreos-assembler) happened too.
Furthermore, we really need to test OSTree's interaction
with Ignition as we've invented several special things there.
Then most recently, I've been working on having
cosa/kola support running externally defined tests:
https://github.com/coreos/coreos-assembler/pull/1215
There's a lot of things to clean up after this but at least this
works for me:
```
$ cd /srv/fcos
$ cosa kola run -- --parallel 4 --output-dir tmp/kola -E ~/src/github/ostreedev/ostree/ 'ext.ostree.*'
```
NOTE: This *does not* drop ostree binaries into the target. See:
https://github.com/coreos/coreos-assembler/pull/1252#issuecomment-600623315
This drops our dependency on Python in the installed tests, and
also fixes a few bugs that came up.
I disabled the `itest-bare-user-root.sh` one because it's
entangled with the shell script infrastructure for the unit tests.
This regressed in https://github.com/ostreedev/ostree/commit/2db79fb398fba2f9ab2e05c517ebccfe1834674b
I noticed this while finally getting the installed tests to run
in FCOS via kola and `ostree admin pin 0` is now aborting because
we were returning TRUE, but no error set.
I don't see a reason to try to continue on if we hit an error;
the original reporter was requesting support for multiple arguments,
but not "ignore invalid requests".
See https://github.com/coreos/fedora-coreos-tracker/issues/343
When we added the read-only sysroot support it broke using "raw"
`ostree pull` and `ostree refs --create` and all of the core repo
CLIs that just operate on a repo and not a sysroot.
Fixing this is a bit ugly as it "layer crosses" things even more.
Extract a helper function that works in both cases.
For some reason I haven't fully debugged (probably a recent
kernel change), in the case where the immutable bit isn't set,
trying to call `EXT2_IOC_SETFLAGS` without it set returns `EINVAL`.
Let's avoid calling the `ioctl()` if we don't have anything to do.
This fixes a slew of `make check` failures here in my toolbox
environment.
(kernel is `5.5.0-0.rc6.git0.1.fc32.x86_64` with `xfs`)
Move the alternative builds into the Jenkinsfile.
Update it to do a FCOS build + kola run.
We drop the flatpak/rpm-ostree runs for now; the former
will needs some work to do the automatic virt bits. The
latter I think we can circle back to when we e.g. figure
out how to include rpm-ostree's tests in kola runs.
Using fs-verity is natural for OSTree because it's file-based,
as opposed to block based (like dm-verity). This only covers
files - not symlinks or directories. And we clearly need to
have integrity for the deployment directories at least.
Also, what we likely need is an API that supports signing files
as they're committed.
So making this truly secure would need a lot more work. Nevertheless,
I think it's time to start experimenting with it. Among other things,
it does *finally* add an API that makes files immutable, which will
help against some accidental damage.
This is basic enablement work that is being driven by
Fedora CoreOS; see also https://github.com/coreos/coreos-assembler/pull/876
We want to support extending the read-only state to cover `/sysroot`
and `/boot`, since conceptually all of the data there should only
be written via libostree. Or at least for `/boot` should *mostly*
just be written by ostree.
This change needs to be opt-in though to avoid breaking anyone.
Add a `sysroot/readonly` key to the repository config which instructs
`ostree-remount.service` to ensure `/sysroot` is read-only. This
requires a bit of a dance because `/sysroot` is actually the same
filesystem as `/`; so we make `/etc` a writable bind mount in this case.
We also need to handle `/var` in the "OSTree default" case of a bind
mount; the systemd generator now looks at the writability state of
`/sysroot` and uses that to determine whether it should have the
`var.mount` unit happen before or after `ostree-remount.service.`
Also add an API to instruct the libostree shared library
that the caller has created a new mount namespace. This way
we can freely remount read-write.
This approach extends upon in a much better way previous work
we did to support remounting `/boot` read-write.
Closes: https://github.com/ostreedev/ostree/issues/1265
This has a few fixes, mainly I want to get this in
as prep for fs-verity.
Update submodule: libglnx
```
Alex Kiernan (1):
macros: Add TEMP_FAILURE_RETRY for musl
Alexander Larsson (1):
Add glnx_open_anonymous_tmpfile_full() allowing you to specify the directory
Colin Walters (8):
Merge branch 'shutil-rm-rf-errprefix' into 'master'
Merge branch 'us-temp-failure-retry' into 'master'
Merge branch 'anonymous-tmpfile-dir' into 'master'
Merge branch 'meson-older-compilers' into 'master'
fdio: Add glnx_tmpfile_reopen_rdonly()
Merge branch 'reopen-rdonly' into 'master'
build-sys: Add libglnx-testlib.c to Automake
Merge branch 'testlib-automake' into 'master'
Jonathan Lebon (1):
Merge branch 'uchar' into 'master'
Simon McVittie (5):
missing: Remove unused <uchar.h>
Run the fdio test in its own temporary directory
meson: Define HAVE_DECL_FOO to 0 if foo isn't declared
Make the Meson build work on older compilers
CI: Target a Fedora stable release
Will Thompson (3):
Add meson.build files
Document using this as a Meson subproject
Add GitLab CI
```
Prep for fsverity, where I want to create a new group
`[fsverity]` in the keyfile that has default values. We should
treat the absence of a group the same as absence of a key
in these "with defaults" APIs.
This reverts commit 985a141002.
It turned out that some people have old bootloaders, and hence
get the "no entries" problem. That's much, much much worse
than double entries.
This way it's clearer this bit is only about the CLI entrypoint
also living in `ostree trivial-httpd`, not the underlying
`ostree-trivial-httpd` binary that's separate now.
Delete the automake conditional for this, and make the manpage
conditional use `if USE_LIBSOUP` the same way the C build does.
Suggested-by: Jonathan Lebon <jonathan@jlebon.com>
Same as https://github.com/coreos/rpm-ostree/pull/1923
Quoting that rationale:
> Since we're not using Homu anymore (and Tide instead looks at
> all statuses by default), let's just drop it. This brings down the
> number of statuses on PRs by one more (and so one less context to
> override when needed).
zipl is a bit special in that it parses the BLS config files
directly *but* we need to run the command to update the "boot block".
Hence, we're not generating a separate config file like the other
backends. Instead, extend the bootloader interface with a `post_bls_sync`
method that is run in the same place we swap the `boot/loader` symlink.
We write a "stamp file" in `/boot` that says we need to run this command.
The reason we use stamp file is to prevent the case where the system is
interrupted after BLS file is updated, but before zipl is triggered,
then zipl boot records are not updated.
This opens the door to making things eventually-consistent/reconcilable
by later adding a systemd unit to run `zipl` if we're interrupted via
a systemd unit - I think we should eventually take this approach
everywhere rather than requiring `/boot/loader` to be a symlink.
Author: Colin Walters <walters@verbum.org>
Tested-by: Tuan Hoang <tmhoang@linux.ibm.com>
Co-Authored-By: Tuan Hoang <tmhoang@linux.ibm.com>
More "scan-build doesn't understand GError and our out-param conventions"
AKA "these errors would be impossible with Rust's sum type Result<> approach".
I was trying to fix a clang `scan-build` error that jlebon
ended up tracking down in
https://github.com/ostreedev/ostree/pull/1939/commits/9344de1ce1e8c185e01988277606ba1ed7f9d16b
But in the process of tracing through this I found it
way easier to read as "new style" code, so this also ports the
code.
I added a `g_assert()` in there too to help assert that
`g_key_file_get_value` won't leak in the error path.
Got this error when trying to rebase libostree in RHEL:
```
Error: CLANG_WARNING: [#def1]
libostree-2019.2/src/libostree/ostree-repo-checkout.c:375:21: warning: Access to field 'disable_xattrs' results in a dereference of a null pointer (loaded from variable 'repo')
```
I think what's happening is it sees us effectively testing
`if (repo == NULL)` via the `while (current_repo)`. Let's
tell it we're sure it's non-null right after the loop.
I tried to balance reflecting the reality of who works on libostree
today with keeping some of the existing committers - particularly
committers from multiple organizations.
Part of switching libostree over to OpenShift Prow.
Colin Walters
