From 47539874b8e5259040a11a79b3c155e633fcae34 Mon Sep 17 00:00:00 2001
From: Colin Walters <walters@verbum.org>
Date: Sun, 5 Apr 2020 18:22:49 +0000
Subject: [PATCH] pull: Update key loading function to match error style

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.
---
 src/libostree/ostree-repo-pull.c | 23 +++++++----------------
 tests/test-signed-pull.sh        |  1 +
 2 files changed, 8 insertions(+), 16 deletions(-)

diff --git a/src/libostree/ostree-repo-pull.c b/src/libostree/ostree-repo-pull.c
index 454eaf8ac5..a5e3a72936 100644
--- a/src/libostree/ostree-repo-pull.c
+++ b/src/libostree/ostree-repo-pull.c
@@ -1488,14 +1488,10 @@ _load_public_keys (OstreeSign *sign,
                    const gchar *remote_name,
                    GError **error)
 {
-
   g_autofree gchar *pk_ascii = NULL;
   g_autofree gchar *pk_file = NULL;
   gboolean loaded_from_file = TRUE;
   gboolean loaded_inlined = TRUE;
-  g_autoptr (GError) verification_error = NULL;
-
-  glnx_throw (&verification_error, "no public keys loaded");
 
   ostree_repo_get_remote_option (repo,
                                  remote_name,
@@ -1536,10 +1532,8 @@ _load_public_keys (OstreeSign *sign,
         loaded_from_file = TRUE;
       else
         {
-          g_debug("Unable to load public keys for '%s' from file '%s': %s",
-                  ostree_sign_get_name(sign), pk_file, local_error->message);
-          /* Save error message for better reason detection later if needed */
-          glnx_prefix_error (&verification_error, "%s", local_error->message);
+          return glnx_throw (error, "Failed loading '%s' keys from '%s",
+                             ostree_sign_get_name (sign), pk_file);
         }
     }
 
@@ -1556,19 +1550,16 @@ _load_public_keys (OstreeSign *sign,
 
       if (!loaded_inlined)
         {
-          g_debug("Unable to load public key '%s' for '%s': %s",
-                  pk_ascii, ostree_sign_get_name (sign), local_error->message);
-
-          /* Save error message for better reason detection later if needed */
-          glnx_prefix_error (&verification_error, "%s", local_error->message);
+          return glnx_throw (error, "Failed loading '%s' keys from inline `verification-key`",
+                             ostree_sign_get_name (sign));
         }
     }
 
   /* Return true if able to load from any source */
-  if (loaded_from_file || loaded_inlined)
-    return TRUE;
+  if (!(loaded_from_file || loaded_inlined))
+    return glnx_throw (error, "No keys found");
 
-  return glnx_throw (error, "%s", verification_error->message);
+  return TRUE;
 }
 
 static gboolean
diff --git a/tests/test-signed-pull.sh b/tests/test-signed-pull.sh
index f222db4fb3..2c677d46fb 100755
--- a/tests/test-signed-pull.sh
+++ b/tests/test-signed-pull.sh
@@ -93,6 +93,7 @@ echo "ok pull failure with incorrect keys file option"
 
 # Test with correct dummy key
 ${CMD_PREFIX} ostree --repo=repo config set 'remote "origin"'.verification-key "${DUMMYSIGN}"
+${CMD_PREFIX} ostree --repo=repo config unset 'remote "origin"'.verification-file
 test_signed_pull "dummy" ""
 
 if ! has_libsodium; then