dir: Try to delete the remote if we failed to add it entirely

Ideally, we would be able to atomically add and remove remotes, but
we're very far from that ideal state. The current behavior is really
suboptimal and leaves the remotes in a inconsistent state if
initialization failed. We can at least make it better by trying to clean
up the half-initialized mess we're currently in. It does however not
protect against SIGKILL-like aborts, as that would require it to be
atomic.

Closes: #6449
Co-authored-by: craftyguy "Clayton Craft" <clayton@craftyguy.net>

common/flatpak-dir.c

@@ -4618,10 +4618,12 @@ apply_new_flatpakrepo (const char *remote_name,
   g_autoptr(GKeyFile) group_config = NULL;
   g_autoptr(GKeyFile) keyfile = g_key_file_new ();
   g_autoptr(GError) local_error = NULL;
+  g_autoptr(GKeyFile) old_config = NULL;
   g_autoptr(GKeyFile) new_config = NULL;
   g_auto(GStrv) old_applied_remotes = NULL;
   g_autoptr(GPtrArray) new_applied_remotes = NULL;
   int i;
+  gboolean res = FALSE;
 
   if (!g_key_file_load_from_file (keyfile, flatpak_file_get_path_cached (file), 0, &local_error))
     {
@@ -4636,6 +4638,7 @@ apply_new_flatpakrepo (const char *remote_name,
       return FALSE;
     }
 
+  old_config = ostree_repo_copy_config (repo);
   new_config = ostree_repo_copy_config (repo);
 
   old_applied_remotes = g_key_file_get_string_list (new_config, "core", "xa.applied-remotes", NULL, NULL);
@@ -4652,10 +4655,10 @@ apply_new_flatpakrepo (const char *remote_name,
                               (const char * const *) new_applied_remotes->pdata, new_applied_remotes->len);
 
   if (!ostree_repo_write_config (repo, new_config, error))
-    return FALSE;
+    goto out;
 
   if (!ostree_repo_reload_config (repo, NULL, error))
-    return FALSE;
+    goto out;
 
   if (gpg_data != NULL)
     {
@@ -4664,12 +4667,21 @@ apply_new_flatpakrepo (const char *remote_name,
 
       if (!ostree_repo_remote_gpg_import (repo, remote_name, input_stream,
                                           NULL, &imported, NULL, error))
-        return FALSE;
+        goto out;
 
       g_info ("Imported %u GPG key%s to remote \"%s\"", imported, (imported == 1) ? "" : "s", remote_name);
     }
 
-  return TRUE;
+  res = TRUE;
+out:
+  if (!res)
+    {
+      /* Roll back the changes. Ideally they would be atomic, because if the
+       * program terminates before we roll back, we end up in a broken state */
+      ostree_repo_write_config (repo, old_config, NULL);
+      ostree_repo_reload_config (repo, NULL, NULL);
+    }
+  return res;
 }
 
 static gboolean
@@ -16002,9 +16014,11 @@ flatpak_dir_modify_remote (FlatpakDir   *self,
   g_autofree char *group = g_strdup_printf ("remote \"%s\"", remote_name);
   g_autofree char *url = NULL;
   g_autofree char *metalink = NULL;
+  g_autoptr(GKeyFile) old_config = NULL;
   g_autoptr(GKeyFile) new_config = NULL;
   g_autofree gchar *filter_path = NULL;
   gboolean has_remote;
+  gboolean res = FALSE;
 
   if (strchr (remote_name, '/') != NULL)
     return flatpak_fail_error (error, FLATPAK_ERROR_REMOTE_NOT_FOUND, _("Invalid character '/' in remote name: %s"),
@@ -16058,6 +16072,8 @@ flatpak_dir_modify_remote (FlatpakDir   *self,
   if (!flatpak_dir_cleanup_remote_for_url_change (self, remote_name, url, cancellable, error))
     return FALSE;
 
+  old_config = ostree_repo_copy_config (self->repo);
+
   /* Add it if its not there yet */
   if (!ostree_repo_remote_change (self->repo, NULL,
                                   OSTREE_REPO_REMOTE_CHANGE_ADD_IF_NOT_EXISTS,
@@ -16070,21 +16086,7 @@ flatpak_dir_modify_remote (FlatpakDir   *self,
   copy_remote_config (new_config, config, remote_name);
 
   if (!ostree_repo_write_config (self->repo, new_config, error))
-    return FALSE;
+    goto out;
-
-  if (gpg_data != NULL)
-    {
-      g_autoptr(GInputStream) input_stream = g_memory_input_stream_new_from_bytes (gpg_data);
-      guint imported = 0;
-
-      if (!ostree_repo_remote_gpg_import (self->repo, remote_name, input_stream,
-                                          NULL, &imported, cancellable, error))
-        return FALSE;
-
-      /* XXX If we ever add internationalization, use ngettext() here. */
-      g_info ("Imported %u GPG key%s to remote \"%s\"",
-              imported, (imported == 1) ? "" : "s", remote_name);
-    }
 
   filter_path = g_key_file_get_value (new_config, group, "xa.filter", NULL);
   if (filter_path && *filter_path && g_file_test (filter_path, G_FILE_TEST_EXISTS))
@@ -16114,10 +16116,28 @@ flatpak_dir_modify_remote (FlatpakDir   *self,
 
   /* If we e.g. changed url or gpg config the cached summary may be invalid */
   if (!flatpak_dir_remote_clear_cached_summary (self, remote_name, cancellable, error))
-    return FALSE;
+    goto out;
 
-  if (!flatpak_dir_mark_changed (self, error))
+  if (gpg_data != NULL)
-    return FALSE;
+    {
+      g_autoptr(GInputStream) input_stream = g_memory_input_stream_new_from_bytes (gpg_data);
+      guint imported = 0;
+
+      if (!ostree_repo_remote_gpg_import (self->repo, remote_name, input_stream,
+                                          NULL, &imported, cancellable, error))
+        goto out;
+
+      /* XXX If we ever add internationalization, use ngettext() here. */
+      g_info ("Imported %u GPG key%s to remote \"%s\"",
+              imported, (imported == 1) ? "" : "s", remote_name);
+    }
+
+  {
+    g_autoptr(GError) local_error = NULL;
+
+    if (!flatpak_dir_mark_changed (self, &local_error))
+      g_warning ("Failed to mark dir as changed: %s", local_error->message);
+  }
 
   if (has_remote)
     flatpak_dir_log (self, "modify remote", remote_name, NULL, NULL, NULL, url,
@@ -16126,7 +16146,16 @@ flatpak_dir_modify_remote (FlatpakDir   *self,
     flatpak_dir_log (self, "add remote", remote_name, NULL, NULL, NULL, url,
                      "Added remote %s to %s", remote_name, url);
 
-  return TRUE;
+  res = TRUE;
+out:
+  if (!res)
+    {
+      /* Roll back the changes. Ideally they would be atomic, because if the
+       * program terminates before we roll back, we end up in a broken state */
+      ostree_repo_write_config (self->repo, old_config, NULL);
+      ostree_repo_reload_config (self->repo, NULL, NULL);
+    }
+  return res;
 }
 
 gboolean

tests/test-repo.sh

@@ -24,7 +24,7 @@ set -euo pipefail
 skip_without_bwrap
 skip_revokefs_without_fuse
 
-echo "1..45"
+echo "1..47"
 
 #Regular repo
 setup_repo
@@ -134,6 +134,46 @@ if ${FLATPAK} ${U} install -y test-repo org.test.Platform &> install-error-log;
 fi
 ok "failed to install again from different remote"
 
+port=$(cat httpd-port)
+echo "bad key" > badkey
+
+gpg2_repo_url=$(ostree config --repo=$FL_DIR/repo get --group 'remote "test-gpg2-repo"' url)
+gpg2_repo_key=$(cat "$FL_DIR/repo/test-gpg2-repo.trustedkeys.gpg")
+${FLATPAK} ${U} remote-modify --gpg-import=badkey test-gpg2-repo >&2 || true
+gpg2_repo_url2=$(ostree config --repo=$FL_DIR/repo get --group 'remote "test-gpg2-repo"' url)
+gpg2_repo_key2=$(cat "$FL_DIR/repo/test-gpg2-repo.trustedkeys.gpg")
+
+if [[ "${gpg2_repo_url}" != "${gpg2_repo_url2}" || "${gpg2_repo_key}" != "${gpg2_repo_key2}" ]]; then
+  assert_not_reached "remote-modify failed but remote was modified"
+fi
+
+${FLATPAK} ${U} remote-add --gpg-import=badkey test-broken-repo "http://127.0.0.1:${port}/test-broken-repo" >&2 && false
+ostree config --repo=$FL_DIR/repo get --group 'remote "test-broken-repo"' url > /dev/null 2>&1 && \
+    assert_not_reached "Bug #6449: Remote added with broken GPG key"
+[[ -f "$FL_DIR/repo/test-broken-repo.trustedkeys.gpg" ]] && false
+
+ok "fail with broken repo"
+
+cat << EOF > test-broken-repo.flatpakrepo
+[Flatpak Repo]
+Version=1
+Url=http://no.127.0.0.1:$(cat httpd-port)/test-broken/
+Title=The Remote Title
+GPGKey=AAA${FL_GPG_BASE64}AAA
+EOF
+
+mkdir -p $FLATPAK_CONFIG_DIR/remotes.d
+cp test-broken-repo.flatpakrepo $FLATPAK_CONFIG_DIR/remotes.d/
+
+${FLATPAK} ${U} remote-add --from test-broken-repo >&2 && false
+ostree config --repo=$FL_DIR/repo get --group 'remote "test-broken-repo"' url > /dev/null 2>&1 && \
+    assert_not_reached "Bug #6449: Remote added with broken GPG key"
+[[ -f "$FL_DIR/repo/test-broken-repo.trustedkeys.gpg" ]] && false
+
+rm -rf $FLATPAK_CONFIG_DIR/remotes.d/
+
+ok "fail with statically configured broken repo"
+
 ${FLATPAK} ${U} install -y --reinstall test-repo org.test.Platform >&2
 ok "re-install"