Skip to content

Commit

Permalink
daemon: cache GPG commit verification
Browse files Browse the repository at this point in the history
In Fedora today, we ship 51 GPG pubkeys in `/etc/pki/rpm-gpg`. These
keys are used to verify RPM packages, but also OSTree commits. But the
sheer number of keys makes actually loading them and verifying
signatures costly. rpm-ostree pays this price at startup when creating
variants for its D-Bus properties describing the deployments.

Multiple things make this even costlier in rpm-ostree:
1. by default we auto-exit after a certain period of time, which means
   that on the next startup we have to pay the verification price again
2. the same deployed commit may be re-verified up to 3 times as the
   different D-Bus properties may refer to the same deployment, and we
   dumbly regenerate its `GVariant` each time

This results in a noticeable delay in rpm-ostree startup:
coreos/fedora-coreos-tracker#761

I believe also this is the root cause for the `ostree.hotfix` FCOS test
flaking: coreos/fedora-coreos-tracker#942. My
theory is that when this test runs on nodes with contended I/O (e.g.
with many other tests running in parallel), GPG verification can get
slow enough that the daemon doesn't finish in time to answer back the
the D-Bus call from the client, which then times out. That test creates
a new deployment using `ostree admin unlock --hotfix` which multiples
the cost.

This patch adds caching of verification results as suggested in the
tracker issue. This makes rpm-ostree startup *noticeably* faster and
should also fix the `ostree.hotfix` flake.

I think though we should still do $something about those keys, ideally
at the Fedora level if not in FCOS/FSB/FIoT.

Closes: coreos/fedora-coreos-tracker#761
  • Loading branch information
jlebon committed Feb 10, 2022
1 parent 4493630 commit eb0a665
Showing 1 changed file with 82 additions and 15 deletions.
97 changes: 82 additions & 15 deletions src/daemon/rpmostreed-deployment-utils.cxx
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@
#include "rpmostree-cxxrs.h"
#include "rpmostreed-errors.h"

#define RPM_OSTREED_COMMIT_VERIFICATION_CACHE "/run/rpm-ostree/gpgcheck-cache"

gboolean
rpmostreed_deployment_get_for_id (OstreeSysroot *sysroot,
const gchar *deploy_id,
Expand Down Expand Up @@ -103,6 +105,77 @@ rpmostreed_deployment_get_for_index (OstreeSysroot *sysroot,
return (OstreeDeployment*)g_object_ref (deployments->pdata[deployment_index]);
}

static char*
get_path_for_cached_signatures_variant (const gchar *remote,
const gchar *checksum)
{
if (remote == NULL)
return g_build_filename (RPM_OSTREED_COMMIT_VERIFICATION_CACHE, "local", checksum, NULL);
return g_build_filename (RPM_OSTREED_COMMIT_VERIFICATION_CACHE, "remote", remote, checksum, NULL);
}

static GVariant*
get_cached_signatures_variant (OstreeRepo *repo,
const gchar *remote,
const gchar *checksum,
GError **error)
{
gboolean cache_hit = FALSE;

struct stat stbuf;
g_autofree char *path = get_path_for_cached_signatures_variant (remote, checksum);
if (!glnx_fstatat_allow_noent (AT_FDCWD, path, &stbuf, AT_SYMLINK_NOFOLLOW, error))
return (GVariant*)glnx_prefix_error_null (error, "querying cached signature variant");
else if (errno == 0)
{
if (stbuf.st_mode == (S_IFREG | 0600) && stbuf.st_uid == 0 && stbuf.st_gid == 0)
cache_hit = TRUE;
}
else if (errno != ENOENT)
{
g_assert_not_reached ();
}

if (!cache_hit)
{
if (!glnx_shutil_mkdir_p_at (AT_FDCWD, dirname (strdupa (path)), 0700, NULL, error))
return NULL;
if (TEMP_FAILURE_RETRY (unlink (path)) && errno != ENOENT)
return (GVariant*)glnx_null_throw_errno_prefix (error, "unlink(%s)", path);

g_autoptr(OstreeGpgVerifyResult) verify_result =
ostree_repo_verify_commit_for_remote (repo, checksum, remote, NULL, error);
if (!verify_result)
return NULL;

g_auto(GVariantBuilder) builder;
g_variant_builder_init (&builder, G_VARIANT_TYPE ("av"));
guint n_sigs = ostree_gpg_verify_result_count_all (verify_result);
for (guint i = 0; i < n_sigs; i++)
g_variant_builder_add (&builder, "v", ostree_gpg_verify_result_get_all (verify_result, i));

g_autoptr(GVariant) v = g_variant_builder_end (&builder); /* keep floating */
if (!glnx_file_replace_contents_with_perms_at (AT_FDCWD, path,
static_cast<const guint8*>(g_variant_get_data (v)),
g_variant_get_size (v), 0600, 0, 0,
(GLnxFileReplaceFlags)0, NULL, error))
return NULL;

g_debug ("successfully cached signatures for %s", checksum);
return util::move_nullify(v);
}

glnx_autofd int fd = -1;
if (!glnx_openat_rdonly (AT_FDCWD, path, FALSE, &fd, error))
return NULL;
g_autoptr(GBytes) data = glnx_fd_readall_bytes (fd, NULL, error);
if (!data)
return NULL;

g_debug ("signature cache hit for %s", checksum);
return g_variant_new_from_bytes (G_VARIANT_TYPE ("av"), data, FALSE);
}

static gboolean
variant_add_remote_status (OstreeRepo *repo,
const gchar *origin_refspec,
Expand Down Expand Up @@ -138,24 +211,18 @@ variant_add_remote_status (OstreeRepo *repo,
if (!gpg_verify)
return TRUE; /* Note early return; no need to verify signatures! */

g_autoptr(OstreeGpgVerifyResult) verify_result =
ostree_repo_verify_commit_for_remote (repo, checksum, remote, NULL, NULL);
if (!verify_result)
GVariant *signatures = get_cached_signatures_variant (repo, remote, checksum, &local_error); /* floating */

if (signatures)
g_variant_dict_insert_value (dict, "signatures", signatures);
else
{
/* Somehow, we have a deployment which has gpg-verify=true, but *doesn't* have a valid
* signature. Let's not just bomb out here. We need to return this in the variant so
* that `status` can show the appropriate msg. */
return TRUE;
/* Somehow, we have a deployment which has gpg-verify=true, but we couldn't
* verify its signature. Let's not just bomb out here. We need to return this
* in the variant so that `status` can show the appropriate msg. */
g_debug ("failed to get cached signature variant: %s", local_error->message);
}

g_auto(GVariantBuilder) builder;
g_variant_builder_init (&builder, G_VARIANT_TYPE ("av"));

guint n_sigs = ostree_gpg_verify_result_count_all (verify_result);
for (guint i = 0; i < n_sigs; i++)
g_variant_builder_add (&builder, "v", ostree_gpg_verify_result_get_all (verify_result, i));

g_variant_dict_insert_value (dict, "signatures", g_variant_builder_end (&builder));
return TRUE;
}

Expand Down

0 comments on commit eb0a665

Please sign in to comment.