From 749ba26800e1db65a783b72eeb414aabefe393ac Mon Sep 17 00:00:00 2001 From: Jonathan Lebon Date: Fri, 4 Feb 2022 17:01:15 -0500 Subject: [PATCH] daemon: cache GPG commit verification 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: https://github.com/coreos/fedora-coreos-tracker/issues/761 I believe also this is the root cause for the `ostree.hotfix` FCOS test flaking: https://github.com/coreos/fedora-coreos-tracker/issues/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: https://github.com/coreos/fedora-coreos-tracker/issues/761 --- src/daemon/rpmostreed-deployment-utils.cxx | 95 ++++++++++++++++++---- 1 file changed, 80 insertions(+), 15 deletions(-) diff --git a/src/daemon/rpmostreed-deployment-utils.cxx b/src/daemon/rpmostreed-deployment-utils.cxx index e366cc2bd7..339d43d38f 100644 --- a/src/daemon/rpmostreed-deployment-utils.cxx +++ b/src/daemon/rpmostreed-deployment-utils.cxx @@ -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, @@ -103,6 +105,75 @@ 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) +{ + return g_build_filename (RPM_OSTREED_COMMIT_VERIFICATION_CACHE, 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)); + + GVariant *v = g_variant_builder_end (&builder); /* keep floating */ + if (!glnx_file_replace_contents_with_perms_at (AT_FDCWD, path, + static_cast(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, %s)", remote, checksum); + return 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, %s)", remote, 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, @@ -138,24 +209,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; }