Like every other error return path in this function, jump to the `out`
label on error here. Returning directly will cause leaks.
Spotted by reading the code, not actually necessarily encountered in the
wild.
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
This patch makes it so that we mark the .commit file from a static delta
as partial before writing the commit to the staging directory. This
exactly mirrors what we do in meta_fetch_on_complete() when writing the
commit on that codepath, which should lend some credibility to the
correctness of this patch.
I have checked that this fixes an issue Flatpak users have been
encountering (https://github.com/flatpak/flatpak/issues/3479) which
results in error messages like "error: Failed to install
org.freedesktop.Sdk.Extension.texlive: Failed to read commit
c7958d966cfa8b80a42877d1d6124831d7807f93c89461a2a586956aa28d438a: No
such metadata object
8bdaa943b957f3cf14d19301c59c7eec076e57389e0fbb3ef5d30082e47a178f.dirtree"
Here's the sequence of events that lead to the error:
1. An install operation is started that fetches static deltas.
2. The fetch is interrupted for some reason such as network connectivity
dropping.
3. The .commit and .commitmeta files for the commit being pulled are
left in the staging dir, e.g.
"~/.local/share/flatpak/repo/tmp/staging-dfe862b2-13fc-49a2-ac92-5a59cc0d8e18-RURckd"
4. There is no `.commitpartial` file for the commit in
"~/.local/share/flatpak/repo/state/"
5. The next time the user attempts the install, libostree reuses the
existing staging dir, pulls the commit and commitmeta objects into
the repo from the staging dir on the assumption that it's a complete
commit.
6. Flatpak then tries to deploy the commit but fails in
ostree_repo_read_commit() in flatpak_dir_deploy(), leading to the
error message "Failed to read commit ..."
7. This happens again any subsequent time the user attempts the install,
until the incomplete commit is removed with "flatpak repair --user".
I will try to also add a workaround in Flatpak so this is fixed even
when Flatpak links against affected versions of libostree.
This will be helpful for the "ostree native container" work in
https://github.com/ostreedev/ostree-rs-ext/
Basically in order to reuse GPG/signapi verification, we need
to support adding a remote, even though it can't be used via
`ostree pull`. (At least, not until we merge ostree-rs-ext into ostree, but
even then I think the principle stands)
for deltafiles the legacy_transaction_resuming flag is not used,
which will mark the commit as done, even if files are missing.
using already existing commitstate_is_partial function as fix
Previous to this we'd trip an assertion `abort()` deep in the curl code if e.g.
a user did `ostree remote add foo htttp://...` etc.
Motivated by considering supporting "external remotes" where code outside
ostree does a pull, but we want to reuse the signing verification infrastructure.
Currently if a file path contains a special character such as '\', and
that character is encoded into a file:// URI that is passed to
ostree_repo_pull_with_options(), the percent encoding will remain in the
path passed to g_file_new() (in the case of backslash %5C) and the pull
will then fail with a file not found error. This is an important edge
case to handle because by default on many Linux distributions a
filesystem with no label is mounted at a path based on its UUID, and
this is then passed to systemd-escape by Flatpak (when
--enable-auto-sideloading was used at compile time) to create a symbolic
link such as this which contains backslashes:
$ ls -l /run/flatpak/sideload-repos/
total 0
lrwxrwxrwx 1 mwleeds mwleeds 55 Mar 9 14:21
'automount-run-media-mwleeds-29419e8f\x2dc680\x2d4e95\x2d9a31\x2d2cc907d421cb'
-> /run/media/mwleeds/29419e8f-c680-4e95-9a31-2cc907d421cb
And Flatpak then passes libostree a file:// URI containing that path, to
implement sideloading (pulling content from the USB drive).
This results in an error like:
Error: While pulling app/org.videolan.VLC/x86_64/stable from remote
flathub:
/run/flatpak/sideload-repos/automount-run-media-mwleeds-29419e8f%5Cx2dc680%5Cx2d4e95%5Cx2d9a31%5Cx2d2cc907d421cb/.ostree/repo:
opendir(/run/flatpak/sideload-repos/automount-run-media-mwleeds-29419e8f%5Cx2dc680%5Cx2d4e95%5Cx2d9a31%5Cx2d2cc907d421cb/.ostree/repo):
No such file or directory
This patch avoids such errors by using g_file_new_for_uri() instead of
g_file_new_for_path(), so that GLib handles the %-decoding for us.
Bug report by user:
https://community.endlessos.com/t/can-not-install-vlc-from-usb-drive-3-9-3/16353
If the `summary_sig_not_modified` branch is taken above, both
`signatures` and `summary` are loaded from the cache. This makes the
`_ostree_repo_load_cache_summary_if_same_sig()` call below redundant (it
checks `signatures` matches the file it was just loaded from, and then
loads `summary` again) — but that call also currently overwrites
`summary` without clearing the old value.
Fix this by only making that call if `signatures` was retrieved, but the
server said the local `summary` cache was invalid.
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
The local pull path was erroring on any missing commit, but that
prevents a depth pull where the source repo has truncated history. As in
the remote case, this also tries to pull in a tombstone commit if the
source repo supports it.
Fixes: #2266
When pulling with depth, missing parent commits are ignored. However,
the check was applying to any commit, which means that it would succeed
even if the requested commit was missing. This might happen on a
corrupted remote repo or when using ref data from a stale summary.
To achieve this, the semantics of the `commit_to_depth` hash table is
changed slightly to only ever includes parent commits. This makes it
easy to detect when a parent commit is being referenced (although there
is a minor bug there when multiple refs are being pulled) while keeping
references to commits that need their `commitpartial` files cleaned up.
It also means that the table is only populated on depth pulls, which
saves some memory and processing in the common depth=0 case.
Fixes: #2265
In some cases such as backups or mirroring you may want to pull commits
from one repo to another even if there commits that have incorrect
bindings. Fixing the commits in the source repository to have correct
bindings may not be feasible, so provide a pull option to disable
verification.
For Endless we have several repositories that predate collection IDs and
ref bindings. Later these repositories gained collection IDs to support
the features they provide and ref bindings as the ostree tooling was
upgraded. These repositories contain released commits that were valid to
the clients they were targeting at the time. Correcting the bindings is
not really an option as it would mean invalidating the repository
history.
The cache shouldn't be affected by the user passing in some other
summary as it may not be the "official one".
I ran into this in flatpak where the passed summary was correct, but
the re-saving of the cache updated the mtime of the cached file which
led to later http If-Modified-Since calls failing to update.
If we have a commit id for all the refs we're pulling, and if we
don't need the summary to list all the refs when mirroring then the
only reason to download the summary is for the list of deltas.
With the new "indexed-deltas" property in the config file (and mirrored
to the summary file) we can detect when we don't need the summary for
deltas and completely avoid downloading it then.
As `summary` and `summary.sig` aren’t immutable, HTTP requests to
download them can be optimised by sending the `If-None-Match` and
`If-Modified-Since` headers to avoid unnecessarily re-downloading them
if they haven’t changed since last being checked.
Hook them up to the new support for that in the fetcher.
The `ETag` and `Last-Modified` for each file in the cache are stored as
the `user.etag` xattr and the mtime, respectively. For flatpak, for
example, this affects the cached files in
`~/.local/share/flatpak/repo/tmp/cache/summaries`.
If xattrs aren’t supported, or if the server doesn’t support the caching
headers, the pull behaviour is unchanged from before.
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
Add support in the soup and curl fetchers to send the `If-None-Match`
and `If-Modified-Since` request headers, and pass on the `ETag` and
`Last-Modified` response headers.
This currently introduces no functional changes, but once call sites
provide the appropriate integration, this will allow HTTP caching to
happen with requests (typically with metadata requests, where the data
is not immutable due to being content-addressed). That should reduce
bandwidth requirements.
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
Otherwise, fall back to downloading and reading them from the `config`
file. See the previous commit for details.
Signed-off-by: Philip Withnall <withnall@endlessm.com>
Fixes: #2165
This was only used in one place, and (especially with the simplification
with GMainContextPopDefault) and the one caller doesn't really do
much more than call the helper. Additionally, what little it does (saving
the result in the cache) is inherently tied to how the helper work,
and will become even more so when we support summary indexes.
This is a preparatory cleanup for supporting summary indexes. It
doesn't change any behaviour and passes make check on its own.
The change in cbf1aca1d5c08d2f40832d16670484ba878d95fb actually
only mmaps the signature file, not the summary. This change makes
use mmap both, as well as extract the cache loading into a helper
function that we will later use in more places.
These allow the `summary` and `summary.sig` files to be cached at a
higher layer (for example, flatpak) between related pull operations (for
example, within a single flatpak transaction). This avoids
re-downloading `summary.sig` multiple times throughout a transaction,
which increases the transaction’s latency and introduces the possibility
for inconsistency between parts of the transaction if the server changes
its `summary` file part-way through.
In particular, this should speed up flatpak transactions on machines
with high latency network connections, where network round trips have a
high impact on the latency of an overall operation.
Signed-off-by: Philip Withnall <withnall@endlessm.com>
This is the dual of 1f3c8c5b3d
where we output more detail when signapi fails to validate.
Extend the API to return a string for success, which we output
to stdout.
This will help the test suite *and* end users validate that the expected
thing is happening.
In order to make this cleaner, split the "verified commit" set
in the pull code into GPG and signapi verified sets, and have
the signapi verified set contain the verification string.
We're not doing anything with the verification string in the
pull code *yet* but I plan to add something like
`ostree pull --verbose` which would finally print this.
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.
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.
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.
The way `timestamp-check` works might be too restrictive in some
situations. Essentially, we need to support the case where users want to
pull an older commit than the current tip, but while still guaranteeing
that it is newer than some even older commit.
This will be used in Fedora CoreOS. For more information see:
https://github.com/coreos/rpm-ostree/pull/2094https://github.com/coreos/fedora-coreos-tracker/issues/481
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
Philip Withnall
Dan Nicholson
Daniel Kolesa
Phaedrus Leeds
Joseph Marrero
Colin Walters
Buddelmann, Richard RB
Jonathan Lebon
Phaedrus Leeds
Alexander Larsson
OpenShift Merge Robot
Philip Withnall
Phaedrus Leeds