Merge pull request #2063 from cgwalters/sig-cleanups-2

pull: Cleanup signature verification functions

src/libostree/ostree-repo-pull.c

@@ -1470,11 +1470,23 @@ process_gpg_verify_result (OtPullData            *pull_data,
 }
 #endif /* OSTREE_DISABLE_GPGME */
 
+static gboolean
+get_signapi_remote_option (OstreeRepo *repo,
+                           OstreeSign *sign,
+                           const char *remote_name,
+                           const char *keysuffix,
+                           char      **out_value,
+                           GError    **error)
+{
+  g_autofree char *key = g_strdup_printf ("verification-%s-%s", ostree_sign_get_name (sign), keysuffix);
+  return ostree_repo_get_remote_option (repo, remote_name, key, NULL, out_value, error);
+}
+
 /* _load_public_keys:
  *
  * Load public keys according remote's configuration:
- * inlined key passed via config option `verification-key` or
+ * inlined key passed via config option `verification-<signapi>-key` or
- * file name with public keys via `verification-file` option.
+ * file name with public keys via `verification-<signapi>-file` option.
  *
  * If both options are set then load all all public keys
  * both from file and inlined in config.
@@ -1493,15 +1505,10 @@ _load_public_keys (OstreeSign *sign,
   gboolean loaded_from_file = TRUE;
   gboolean loaded_inlined = TRUE;
 
-  ostree_repo_get_remote_option (repo,
+  if (!get_signapi_remote_option (repo, sign, remote_name, "file", &pk_file, error))
-                                 remote_name,
+    return FALSE;
-                                 "verification-file", NULL,
+  if (!get_signapi_remote_option (repo, sign, remote_name, "key", &pk_ascii, error))
-                                 &pk_file, NULL);
+    return FALSE;
-
-  ostree_repo_get_remote_option (repo,
-                                 remote_name,
-                                 "verification-key", NULL,
-                                 &pk_ascii, NULL);
 
   /* return TRUE if there is no configuration for remote */
   if ((pk_file == NULL) &&(pk_ascii == NULL))
@@ -1571,9 +1578,10 @@ _sign_verify_for_remote (OstreeRepo *repo,
 {
   /* list all signature types in detached metadata and check if signed by any? */
   g_auto (GStrv) names = ostree_sign_list_names();
-  g_autoptr (GError) verification_error = NULL;
+  guint n_invalid_signatures = 0;
-
+  guint n_unknown_signatures = 0;
-  glnx_throw (&verification_error, "signed with unknown key");
+  g_autoptr (GError) last_sig_error = NULL;
+  gboolean found_sig = FALSE;
 
   for (char **iter=names; iter && *iter; iter++)
     {
@@ -1581,10 +1589,12 @@ _sign_verify_for_remote (OstreeRepo *repo,
       g_autoptr (GVariant) signatures = NULL;
       const gchar *signature_key = NULL;
       GVariantType *signature_format = NULL;
-      g_autoptr (GError) local_error = NULL;
 
-      if ((sign = ostree_sign_get_by_name (*iter, &local_error)) == NULL)
+      if ((sign = ostree_sign_get_by_name (*iter, NULL)) == NULL)
-        continue;
+        {
+          n_unknown_signatures++;
+          continue;
+        }
 
       signature_key = ostree_sign_metadata_key (sign);
       signature_format = (GVariantType *) ostree_sign_metadata_format (sign);
@@ -1598,24 +1608,37 @@ _sign_verify_for_remote (OstreeRepo *repo,
         continue;
 
       /* Try to load public key(s) according remote's configuration */
-      if (_load_public_keys (sign, repo, remote_name, &local_error))
+      if (!_load_public_keys (sign, repo, remote_name, error))
-        {
+        return FALSE;
-          /* Return true if any signature fit to pre-loaded public keys.
-           * If no keys configured -- then system configuration will be used */
-          if (ostree_sign_data_verify (sign,
-                                       signed_data,
-                                       signatures,
-                                       &local_error))
-            return TRUE;
-        }
 
-      /* Save error message for better reason detection later if needed */
+      found_sig = TRUE;
-      glnx_prefix_error (&verification_error, "%s", local_error->message);
+
+        /* Return true if any signature fit to pre-loaded public keys.
+          * If no keys configured -- then system configuration will be used */
+      if (!ostree_sign_data_verify (sign,
+                                    signed_data,
+                                    signatures,
+                                    last_sig_error ? NULL : &last_sig_error))
+        {
+          n_invalid_signatures++;
+          continue;
+        }
+      /* Accept the first valid signature */
+      return TRUE;
     }
 
-  /* In case if there were no signatures of known type
+  if (!found_sig)
-   * or metadata contains invalid data */
+    {
-  return glnx_throw (error, "%s", verification_error->message);
+      if (n_unknown_signatures > 0)
+        return glnx_throw (error, "No signatures found (%d unknown type)", n_unknown_signatures);
+      return glnx_throw (error, "No signatures found");
+    }
+
+  g_assert (last_sig_error);
+  g_propagate_error (error, g_steal_pointer (&last_sig_error));
+  if (n_invalid_signatures > 1)
+    glnx_prefix_error (error, "(%d other invalid signatures)", n_invalid_signatures-1);
+  return FALSE;
 }
 
 static gboolean
@@ -2001,39 +2024,46 @@ scan_commit_object (OtPullData                 *pull_data,
   if (pull_data->sign_verify &&
       !g_hash_table_contains (pull_data->verified_commits, checksum))
     {
-      gboolean ret = FALSE;
+      g_autoptr(GError) last_verification_error = NULL;
-      g_autoptr (GError) verification_error = NULL;
+      gboolean found_any_signature = FALSE;
+      gboolean found_valid_signature = FALSE;
 
       /* list all signature types in detached metadata and check if signed by any? */
       g_auto (GStrv) names = ostree_sign_list_names();
-      for (char **iter=names; !ret && iter && *iter; iter++)
+      for (char **iter=names; iter && *iter; iter++)
         {
           g_autoptr (OstreeSign) sign = NULL;
-          g_autoptr (GError) local_error = NULL;
 
-          if ((sign = ostree_sign_get_by_name (*iter, &local_error)) == NULL)
+          if ((sign = ostree_sign_get_by_name (*iter, NULL)) == NULL)
             continue;
 
           /* Try to load public key(s) according remote's configuration */
-          if (_load_public_keys (sign, pull_data->repo, pull_data->remote_name, &local_error))
+          if (!_load_public_keys (sign, pull_data->repo, pull_data->remote_name, error))
+            return FALSE;
+
+          found_any_signature = TRUE;
+
+          /* Set return to true if any sign fit */
+          if (ostree_sign_commit_verify (sign,
+                                          pull_data->repo,
+                                          checksum,
+                                          cancellable,
+                                          last_verification_error ? NULL : &last_verification_error))
             {
-
+              found_valid_signature = TRUE;
-              /* Set return to true if any sign fit */
+              break;
-              if (ostree_sign_commit_verify (sign,
-                                             pull_data->repo,
-                                             checksum,
-                                             cancellable,
-                                             &local_error))
-                ret = TRUE;
             }
-
-          /* Save error message for better reason detection later if needed */
-          if (!ret)
-            glnx_prefix_error (&verification_error, "%s", local_error->message);
          }
 
-      if (!ret)
+      if (!found_any_signature)
-        return glnx_throw (error, "Can't verify commit %s: %s", checksum, verification_error->message);
+        return glnx_throw (error, "No signatures found for commit %s", checksum);
+
+      if (!found_valid_signature)
+        {
+          g_assert (last_verification_error);
+          g_propagate_error (error, g_steal_pointer (&last_verification_error));
+          return glnx_prefix_error (error, "Can't verify commit %s", checksum);
+        }
     }
 
   /* If we found a legacy transaction flag, assume we have to scan.

src/libostree/ostree-sign-ed25519.c

@@ -242,7 +242,7 @@ gboolean ostree_sign_ed25519_data_verify (OstreeSign *self,
         }
     }
 
-  return glnx_throw (error, "Not able to verify: no valid signatures found");
+  return glnx_throw (error, "no valid ed25519 signatures found");
 #endif /* HAVE_LIBSODIUM */
 
   return FALSE;

tests/test-local-pull.sh

@@ -121,27 +121,29 @@ if has_libsodium; then
 
     mkdir repo8
     ostree_repo_init repo8 --mode="archive"
-    ${CMD_PREFIX} ostree --repo=repo8 remote add --set=verification-key="${ED25519PUBLIC}" origin repo
+    ${CMD_PREFIX} ostree --repo=repo8 remote add --set=verification-ed25519-key="${ED25519PUBLIC}" origin repo
     cat repo8/config
 
-    if ${CMD_PREFIX} ostree --repo=repo8 pull-local --remote=origin --sign-verify repo test2 2>&1; then
+    if ${CMD_PREFIX} ostree --repo=repo8 pull-local --remote=origin --sign-verify repo test2 2>err.txt; then
         assert_not_reached "Ed25519 signature verification unexpectedly succeeded"
     fi
+    assert_file_has_content err.txt 'ed25519: commit have no signatures of my type'
     echo "ok --sign-verify with no signature"
 
     ${OSTREE} sign test2 ${ED25519SECRET}
 
     mkdir repo9
     ostree_repo_init repo9 --mode="archive"
-    ${CMD_PREFIX} ostree --repo=repo9 remote add --set=verification-key="$(gen_ed25519_random_public)" origin repo
+    ${CMD_PREFIX} ostree --repo=repo9 remote add --set=verification-ed25519-key="$(gen_ed25519_random_public)" origin repo
-    if ${CMD_PREFIX} ostree --repo=repo9 pull-local --remote=origin --sign-verify repo test2 2>&1; then
+    if ${CMD_PREFIX} ostree --repo=repo9 pull-local --remote=origin --sign-verify repo test2 2>err.txt; then
         assert_not_reached "Ed25519 signature verification unexpectedly succeeded"
     fi
+    assert_file_has_content err.txt 'no valid ed25519 signatures found'
     echo "ok --sign-verify with wrong signature"
 
     mkdir repo10
     ostree_repo_init repo10 --mode="archive"
-    ${CMD_PREFIX} ostree --repo=repo10 remote add --set=verification-key="${ED25519PUBLIC}" origin repo
+    ${CMD_PREFIX} ostree --repo=repo10 remote add --set=verification-ed25519-key="${ED25519PUBLIC}" origin repo
     ${CMD_PREFIX} ostree --repo=repo10 pull-local --remote=origin --sign-verify repo test2
     echo "ok --sign-verify"
 else

tests/test-signed-pull-summary.sh

@@ -115,7 +115,7 @@ do
     ${OSTREE} --repo=${test_tmpdir}/ostree-srv/gnomerepo summary -u ${COMMIT_SIGN}
 
     cd ${test_tmpdir}
-    repo_reinit --set=verification-key=${PUBLIC_KEY}
+    repo_reinit --set=verification-${engine}-key=${PUBLIC_KEY}
     ${OSTREE} --repo=repo pull origin main
     assert_has_file repo/tmp/cache/summaries/origin
     assert_has_file repo/tmp/cache/summaries/origin.sig
@@ -136,7 +136,7 @@ do
     echo "ok ${engine} prune summary cache"
 
     cd ${test_tmpdir}
-    repo_reinit --set=verification-key=${PUBLIC_KEY}
+    repo_reinit --set=verification-${engine}-key=${PUBLIC_KEY}
     mkdir cachedir
     ${OSTREE} --repo=repo pull --cache-dir=cachedir origin main
     assert_not_has_file repo/tmp/cache/summaries/origin
@@ -152,13 +152,13 @@ do
     echo "ok ${engine} pull with signed summary and cachedir"
 
     cd ${test_tmpdir}
-    repo_reinit --set=verification-key=${PUBLIC_KEY}
+    repo_reinit --set=verification-${engine}-key=${PUBLIC_KEY}
     mv ${test_tmpdir}/ostree-srv/gnomerepo/summary.sig{,.good}
     echo invalid > ${test_tmpdir}/ostree-srv/gnomerepo/summary.sig
     if ${OSTREE} --repo=repo pull origin main 2>err.txt; then
         assert_not_reached "Successful pull with invalid ${engine} signature"
     fi
-    assert_file_has_content err.txt "signed with unknown key"
+    assert_file_has_content err.txt "No signatures found"
     mv ${test_tmpdir}/ostree-srv/gnomerepo/summary.sig{.good,}
     echo "ok ${engine} pull with invalid ${engine} summary signature fails"
 
@@ -167,7 +167,7 @@ do
     ${OSTREE} --repo=${test_tmpdir}/ostree-srv/gnomerepo summary -u ${COMMIT_SIGN}
 
     cd ${test_tmpdir}
-    repo_reinit --set=verification-key=${PUBLIC_KEY}
+    repo_reinit --set=verification-${engine}-key=${PUBLIC_KEY}
     ${OSTREE} --repo=repo pull origin main
     echo "ok ${engine} pull delta with signed summary"
 
@@ -212,7 +212,7 @@ cd ${test_tmpdir}
 # Reset to the old valid summary and pull to cache it
 cp ${test_tmpdir}/ostree-srv/gnomerepo/summary{.1,}
 cp ${test_tmpdir}/ostree-srv/gnomerepo/summary.sig{.1,}
-repo_reinit --set=verification-key=${PUBLIC_KEY}
+repo_reinit --set=verification-ed25519-key=${PUBLIC_KEY}
 ${OSTREE} --repo=repo pull origin main
 assert_has_file repo/tmp/cache/summaries/origin
 assert_has_file repo/tmp/cache/summaries/origin.sig
@@ -226,7 +226,7 @@ cp ${test_tmpdir}/ostree-srv/gnomerepo/summary.sig{.2,}
 if ${OSTREE} --repo=repo pull origin main 2>err.txt; then
     assert_not_reached "Successful pull with old summary"
 fi
-assert_file_has_content err.txt "signed with unknown key"
+assert_file_has_content err.txt "no valid ed25519 signatures found"
 assert_has_file repo/tmp/cache/summaries/origin
 assert_has_file repo/tmp/cache/summaries/origin.sig
 cmp repo/tmp/cache/summaries/origin ${test_tmpdir}/ostree-srv/gnomerepo/summary.1 >&2
@@ -246,7 +246,7 @@ echo "ok ${engine} pull with signed summary remote old summary"
 # Reset to the old valid summary and pull to cache it
 cp ${test_tmpdir}/ostree-srv/gnomerepo/summary{.1,}
 cp ${test_tmpdir}/ostree-srv/gnomerepo/summary.sig{.1,}
-repo_reinit --set=verification-key=${PUBLIC_KEY}
+repo_reinit --set=verification-ed25519-key=${PUBLIC_KEY}
 ${OSTREE} --repo=repo pull origin main
 assert_has_file repo/tmp/cache/summaries/origin
 assert_has_file repo/tmp/cache/summaries/origin.sig

tests/test-signed-pull.sh

@@ -80,22 +80,22 @@ if ${CMD_PREFIX} ostree --repo=repo pull origin main; then
 fi
 echo "ok pull failure without keys preloaded"
 
-${CMD_PREFIX} ostree --repo=repo config set 'remote "origin"'.verification-key "somewrongkey"
+${CMD_PREFIX} ostree --repo=repo config set 'remote "origin"'.verification-dummy-key "somewrongkey"
 if ${CMD_PREFIX} ostree --repo=repo pull origin main; then
     assert_not_reached "pull with unknown key unexpectedly succeeded"
 fi
 echo "ok pull failure with incorrect key option"
 
-${CMD_PREFIX} ostree --repo=repo config unset 'remote "origin"'.verification-key
+${CMD_PREFIX} ostree --repo=repo config unset 'remote "origin"'.verification-dummy-key
-${CMD_PREFIX} ostree --repo=repo config set 'remote "origin"'.verification-file "/non/existing/file"
+${CMD_PREFIX} ostree --repo=repo config set 'remote "origin"'.verification-dummy-file "/non/existing/file"
 if ${CMD_PREFIX} ostree --repo=repo pull origin main; then
     assert_not_reached "pull with unknown keys file unexpectedly succeeded"
 fi
 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 set 'remote "origin"'.verification-dummy-key "${DUMMYSIGN}"
-${CMD_PREFIX} ostree --repo=repo config unset 'remote "origin"'.verification-file
+${CMD_PREFIX} ostree --repo=repo config unset 'remote "origin"'.verification-dummy-file
 test_signed_pull "dummy" ""
 
 if ! has_libsodium; then
@@ -117,7 +117,7 @@ SECRET=${ED25519SECRET}
 COMMIT_ARGS="--sign=${SECRET} --sign-type=ed25519"
 
 repo_init --set=sign-verify=true
-${CMD_PREFIX} ostree --repo=repo config set 'remote "origin"'.verification-key "${PUBLIC}"
+${CMD_PREFIX} ostree --repo=repo config set 'remote "origin"'.verification-ed25519-key "${PUBLIC}"
 test_signed_pull "ed25519" "key"
 
 # Prepare files with public ed25519 signatures
@@ -130,13 +130,13 @@ for((i=0;i<100;i++)); do
 done > ${PUBKEYS}
 
 # Test case with the file containing incorrect signatures and with the correct key set
-${CMD_PREFIX} ostree --repo=repo config set 'remote "origin"'.verification-file "${PUBKEYS}"
+${CMD_PREFIX} ostree --repo=repo config set 'remote "origin"'.verification-ed25519-file "${PUBKEYS}"
 test_signed_pull "ed25519" "key+file"
 
 # Add correct key into the list
 echo ${PUBLIC} >> ${PUBKEYS}
 
 repo_init --set=sign-verify=true
-${CMD_PREFIX} ostree --repo=repo config set 'remote "origin"'.verification-file "${PUBKEYS}"
+${CMD_PREFIX} ostree --repo=repo config set 'remote "origin"'.verification-ed25519-file "${PUBKEYS}"
 test_signed_pull "ed25519" "file"