pull: complete detached meta fetch before scanning

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 */
@@ -965,8 +966,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.
@@ -1013,8 +1021,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
     {
@@ -1030,7 +1042,7 @@ meta_fetch_on_complete (GObject           *object,
           if (!write_commitpartial_for (pull_data, checksum, error))
             goto out;
         }
-      
+
       ostree_repo_write_metadata_async (pull_data->repo, objtype, checksum, metadata,
                                         pull_data->cancellable,
                                         on_metadata_written, fetch_data);
@@ -1430,15 +1442,20 @@ scan_one_metadata_object_c (OtPullData         *pull_data,
     }
   else if (is_stored && objtype == OSTREE_OBJECT_TYPE_COMMIT)
     {
-      /* For commits, always refetch detached metadata. */
-      enqueue_one_object_request (pull_data, tmp_checksum, objtype, path, TRUE, TRUE);
-
-      if (!scan_commit_object (pull_data, tmp_checksum, recursion_depth,
-                               pull_data->cancellable, error))
-        goto out;
+      /* Even though we already have the commit, we always try to (re)fetch the
+       * 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;
 
-      g_hash_table_add (pull_data->scanned_metadata, g_variant_ref (object));
-      pull_data->n_scanned_metadata++;
+          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)
     {
@@ -2840,6 +2857,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,
@@ -3562,6 +3581,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,23 +211,23 @@ 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
 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"
 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"