pull: complete detached meta fetch before scanning

If somehow a repo has gpg verification on but doesn't have signatures
present for the existing commit, ostree would error out if it needs to
scan the commit object (e.g. if there are no updates available).

An instance of this is currently happening in Fedora AH, in which
signatures are not shipped in the ISO due to filesystem restrictions.
Another possible scenario is if a content provider switches from not
signing commits to signing them; even if older commits are retroactively
signed, clients' local commit objects would error out if they needed
scanning.

This patch adds a check to ensure that we always attempt to fetch the
detached metadata and wait for its result (whether it exists or not)
before moving on to scan their corresponding commit objects.

See also: https://github.com/projectatomic/rpm-ostree/issues/630

Closes: #873
Approved by: cgwalters

src/libostree/ostree-repo-pull.c

@@ -80,6 +80,7 @@ typedef struct {
   GHashTable       *expected_commit_sizes; /* Maps commit checksum to known size */
   GHashTable       *commit_to_depth; /* Maps commit checksum maximum depth */
   GHashTable       *scanned_metadata; /* Maps object name to itself */
+  GHashTable       *fetched_detached_metadata; /* Set<checksum> */
   GHashTable       *requested_metadata; /* Maps object name to itself */
   GHashTable       *requested_content; /* Maps checksum to itself */
   GHashTable       *requested_fallback_content; /* Maps checksum to itself */
@@ -912,8 +913,15 @@ meta_fetch_on_complete (GObject           *object,
             {
               /* There isn't any detached metadata, just fetch the commit */
               g_clear_error (&local_error);
+
+              /* Now that we've at least tried to fetch it, we can proceed to
+               * scan/fetch the commit object */
+              g_hash_table_add (pull_data->fetched_detached_metadata, g_strdup (checksum));
+
               if (!fetch_data->object_is_stored)
                 enqueue_one_object_request (pull_data, checksum, objtype, fetch_data->path, FALSE, FALSE);
+              else
+                queue_scan_one_metadata_object (pull_data, checksum, objtype, fetch_data->path, 0);
             }
 
           /* When traversing parents, do not fail on a missing commit.
@@ -960,8 +968,12 @@ meta_fetch_on_complete (GObject           *object,
                                                        pull_data->cancellable, error))
         goto out;
 
+      g_hash_table_add (pull_data->fetched_detached_metadata, g_strdup (checksum));
+
       if (!fetch_data->object_is_stored)
         enqueue_one_object_request (pull_data, checksum, objtype, fetch_data->path, FALSE, FALSE);
+      else
+        queue_scan_one_metadata_object (pull_data, checksum, objtype, fetch_data->path, 0);
     }
   else
     {
@@ -1377,15 +1389,20 @@ scan_one_metadata_object_c (OtPullData         *pull_data,
     }
   else if (is_stored && objtype == OSTREE_OBJECT_TYPE_COMMIT)
     {
-      /* For commits, always refetch detached metadata. */
+      /* Even though we already have the commit, we always try to (re)fetch the
-      enqueue_one_object_request (pull_data, tmp_checksum, objtype, path, TRUE, TRUE);
+       * detached metadata before scanning it, in case new signatures appear.
+       * https://github.com/projectatomic/rpm-ostree/issues/630 */
+      if (!g_hash_table_contains (pull_data->fetched_detached_metadata, tmp_checksum))
+        enqueue_one_object_request (pull_data, tmp_checksum, objtype, path, TRUE, TRUE);
+      else
+        {
+          if (!scan_commit_object (pull_data, tmp_checksum, recursion_depth,
+                                   pull_data->cancellable, error))
+            goto out;
 
-      if (!scan_commit_object (pull_data, tmp_checksum, recursion_depth,
+          g_hash_table_add (pull_data->scanned_metadata, g_variant_ref (object));
-                               pull_data->cancellable, error))
+          pull_data->n_scanned_metadata++;
-        goto out;
+        }
-
-      g_hash_table_add (pull_data->scanned_metadata, g_variant_ref (object));
-      pull_data->n_scanned_metadata++;
     }
   else if (is_stored && objtype == OSTREE_OBJECT_TYPE_DIR_TREE)
     {
@@ -2787,6 +2804,8 @@ ostree_repo_pull_with_options (OstreeRepo             *self,
                                                                (GDestroyNotify)g_free);
   pull_data->scanned_metadata = g_hash_table_new_full (ostree_hash_object_name, g_variant_equal,
                                                        (GDestroyNotify)g_variant_unref, NULL);
+  pull_data->fetched_detached_metadata = g_hash_table_new_full (g_str_hash, g_str_equal,
+                                                       (GDestroyNotify)g_free, NULL);
   pull_data->requested_content = g_hash_table_new_full (g_str_hash, g_str_equal,
                                                         (GDestroyNotify)g_free, NULL);
   pull_data->requested_fallback_content = g_hash_table_new_full (g_str_hash, g_str_equal,
@@ -3509,6 +3528,7 @@ ostree_repo_pull_with_options (OstreeRepo             *self,
   g_clear_pointer (&pull_data->commit_to_depth, (GDestroyNotify) g_hash_table_unref);
   g_clear_pointer (&pull_data->expected_commit_sizes, (GDestroyNotify) g_hash_table_unref);
   g_clear_pointer (&pull_data->scanned_metadata, (GDestroyNotify) g_hash_table_unref);
+  g_clear_pointer (&pull_data->fetched_detached_metadata, (GDestroyNotify) g_hash_table_unref);
   g_clear_pointer (&pull_data->summary_deltas_checksums, (GDestroyNotify) g_hash_table_unref);
   g_clear_pointer (&pull_data->requested_content, (GDestroyNotify) g_hash_table_unref);
   g_clear_pointer (&pull_data->requested_fallback_content, (GDestroyNotify) g_hash_table_unref);

tests/pull-test.sh

@@ -24,7 +24,7 @@ function repo_init() {
     rm repo -rf
     mkdir repo
     ostree_repo_init repo
-    ${CMD_PREFIX} ostree --repo=repo remote add --set=gpg-verify=false origin $(cat httpd-address)/ostree/gnomerepo
+    ${CMD_PREFIX} ostree --repo=repo remote add origin $(cat httpd-address)/ostree/gnomerepo "$@"
 }
 
 function verify_initial_contents() {
@@ -35,10 +35,10 @@ function verify_initial_contents() {
     assert_file_has_content baz/cow '^moo$'
 }
 
-echo "1..16"
+echo "1..18"
 
 # Try both syntaxes
-repo_init
+repo_init --no-gpg-verify
 ${CMD_PREFIX} ostree --repo=repo pull origin main
 ${CMD_PREFIX} ostree --repo=repo pull origin:main
 ${CMD_PREFIX} ostree --repo=repo fsck
@@ -128,7 +128,7 @@ assert_file_has_content main.txt ${rev}
 echo "ok pull specific commit"
 
 cd ${test_tmpdir}
-repo_init
+repo_init --no-gpg-verify
 ${CMD_PREFIX} ostree --repo=repo pull origin main
 ${CMD_PREFIX} ostree --repo=repo fsck
 # Generate a delta from old to current, even though we aren't going to
@@ -153,7 +153,7 @@ ${CMD_PREFIX} ostree --repo=ostree-srv/gnomerepo summary -u
 # Explicitly test delta fetches via ref name as well as commit hash
 for delta_target in main ${new_rev}; do
 cd ${test_tmpdir}
-repo_init
+repo_init --no-gpg-verify
 ${CMD_PREFIX} ostree --repo=repo pull origin main@${prev_rev}
 ${CMD_PREFIX} ostree --repo=repo pull --dry-run --require-static-deltas origin ${delta_target} >dry-run-pull.txt
 # Compression can vary, so we support 400-699
@@ -166,7 +166,7 @@ done
 # Explicitly test delta fetches via ref name as well as commit hash
 for delta_target in main ${new_rev}; do
 cd ${test_tmpdir}
-repo_init
+repo_init --no-gpg-verify
 ${CMD_PREFIX} ostree --repo=repo pull origin main@${prev_rev}
 ${CMD_PREFIX} ostree --repo=repo pull --require-static-deltas origin ${delta_target}
 if test ${delta_target} = main; then
@@ -179,7 +179,7 @@ ${CMD_PREFIX} ostree --repo=repo fsck
 done
 
 cd ${test_tmpdir}
-repo_init
+repo_init --no-gpg-verify
 ${CMD_PREFIX} ostree --repo=repo pull origin main@${prev_rev}
 ${CMD_PREFIX} ostree --repo=repo pull --disable-static-deltas origin main
 ${CMD_PREFIX} ostree --repo=repo fsck
@@ -197,7 +197,7 @@ cd ${test_tmpdir}
 ${CMD_PREFIX} ostree --repo=ostree-srv/gnomerepo static-delta generate --swap-endianness main
 ${CMD_PREFIX} ostree --repo=ostree-srv/gnomerepo summary -u
 
-repo_init
+repo_init --no-gpg-verify
 ${CMD_PREFIX} ostree --repo=repo pull origin main@${prev_rev}
 ${CMD_PREFIX} ostree --repo=repo pull --require-static-deltas --dry-run origin main >byteswapped-dry-run-pull.txt
 ${CMD_PREFIX} ostree --repo=repo fsck
@@ -211,7 +211,7 @@ echo "ok pull byteswapped delta"
 cd ${test_tmpdir}
 rm ostree-srv/gnomerepo/deltas -rf
 ${CMD_PREFIX} ostree --repo=ostree-srv/gnomerepo summary -u
-repo_init
+repo_init --no-gpg-verify
 if ${CMD_PREFIX} ostree --repo=repo pull --require-static-deltas origin main 2>err.txt; then
     assert_not_reached "--require-static-deltas unexpectedly succeeded"
 fi
@@ -219,7 +219,7 @@ assert_file_has_content err.txt "deltas required, but none found"
 ${CMD_PREFIX} ostree --repo=repo fsck
 
 # Now test with a partial commit
-repo_init
+repo_init --no-gpg-verify
 ${CMD_PREFIX} ostree --repo=repo pull --commit-metadata-only origin main@${prev_rev}
 if ${CMD_PREFIX} ostree --repo=repo pull --require-static-deltas origin main 2>err.txt; then
     assert_not_reached "--require-static-deltas unexpectedly succeeded"
@@ -227,7 +227,7 @@ fi
 assert_file_has_content err.txt "deltas required, but none found"
 echo "ok delta required but don't exist"
 
-repo_init
+repo_init --no-gpg-verify
 ${CMD_PREFIX} ostree --repo=repo pull origin main@${prev_rev}
 if ${CMD_PREFIX} ostree --repo=repo pull --require-static-deltas origin ${new_rev} 2>err.txt; then
     assert_not_reached "--require-static-deltas unexpectedly succeeded"
@@ -294,3 +294,27 @@ fi
 assert_file_has_content err.txt "ONE BILLION DOLLARS"
 
 echo "ok unconfigured"
+
+cd ${test_tmpdir}
+repo_init --set=gpg-verify=true
+${CMD_PREFIX} ostree --repo=ostree-srv/gnomerepo commit \
+  --gpg-homedir=${TEST_GPG_KEYHOME} --gpg-sign=${TEST_GPG_KEYID_1} -b main \
+  -s "A signed commit" --tree=ref=main
+${CMD_PREFIX} ostree --repo=ostree-srv/gnomerepo summary -u
+# make sure gpg verification is correctly on
+csum=$(${CMD_PREFIX} ostree --repo=ostree-srv/gnomerepo rev-parse main)
+objpath=objects/${csum::2}/${csum:2}.commitmeta
+remotesig=ostree-srv/gnomerepo/$objpath
+localsig=repo/$objpath
+mv $remotesig $remotesig.bak
+if ${CMD_PREFIX} ostree --repo=repo --depth=0 pull origin main; then
+    assert_not_reached "pull with gpg-verify unexpectedly succeeded?"
+fi
+# ok now check that we can pull correctly
+mv $remotesig.bak $remotesig
+${CMD_PREFIX} ostree --repo=repo pull origin main
+echo "ok pull signed commit"
+rm $localsig
+${CMD_PREFIX} ostree --repo=repo pull origin main
+test -f $localsig
+echo "ok re-pull signature for stored commit"

tests/test-remote-headers.sh

@@ -44,9 +44,9 @@ ${CMD_PREFIX} ostree --repo=repo remote add --set=gpg-verify=false origin $(cat
 # Sanity check the setup, without headers the pull should fail
 assert_fail ${CMD_PREFIX} ostree --repo=repo pull origin main
 
-echo "ok, setup done"
+echo "ok setup done"
 
 # Now pull should succeed now
 ${CMD_PREFIX} ostree --repo=repo pull --http-header foo=bar --http-header baz=badger origin main
 
-echo "ok, pull succeeded"
+echo "ok pull succeeded"