Allow disabling pulling from LAN/USB/Internet #1758

mwleeds · 2018-10-18T00:25:31Z

This branch has two patches, one to add a repo config option that can be used to disable OstreeRepoFinders, and one to disable pulling from the LAN unless explicitly enabled.

mwleeds · 2018-10-18T00:27:55Z

cc @pwithnall @dbnicholson

dbnicholson

I think this makes sense in general but found a couple issues.

dbnicholson · 2018-10-18T11:34:47Z

src/libostree/ostree-repo-pull.c


-          default_finders[2] = NULL;
-          g_clear_object (&finder_avahi);
+              default_finders[2] = NULL;


The index is only 2 if repo-finders contains both config and mount. I think you need to track what index avahi was inserted at in default_finders.

dbnicholson · 2018-10-18T11:39:19Z

src/libostree/ostree-repo-pull.c

@@ -4970,6 +4970,8 @@ ostree_repo_find_remotes_async (OstreeRepo                     *self,
  g_autoptr(GTask) task = NULL;
  g_autoptr(FindRemotesData) data = NULL;
  OstreeRepoFinder *default_finders[4] = { NULL, };
+  OstreeRepoFinder *sanitized_finders[4] = { NULL, };
+  guint finder_index = 0;


Both default_finders and sanitized_finders are local to blocks below and don't need to be declared at this level. Likewise, I probably wouldn't reuse finder_index between both loops. It's currently used locally to each block, and declaring it there reduces the chance of using it incorrectly later on some refactoring.

Are you sure it would be correct to declare default_finders/sanitized_finders in the respective blocks? Once they go out of scope wouldn't finders point to memory that has been freed?

Good point, I didn't think of that.

dbnicholson · 2018-10-18T11:41:24Z

src/libostree/ostree-repo.c

+    if (g_error_matches (local_error, G_KEY_FILE_ERROR, G_KEY_FILE_ERROR_KEY_NOT_FOUND))
+      g_clear_error (&local_error);
+    else if (local_error)
+      return FALSE;


Don't you want to propagate the error here so it's returned to the caller?

yes, thanks

dbnicholson · 2018-10-18T11:43:58Z

src/libostree/ostree-repo.c

+                                                     NULL, &local_error);
+    if (g_error_matches (local_error, G_KEY_FILE_ERROR, G_KEY_FILE_ERROR_KEY_NOT_FOUND))
+      g_clear_error (&local_error);
+    else if (local_error)


I think it's more typical ostree style to explicitly check against NULL.

dbnicholson · 2018-10-18T11:46:19Z

src/libostree/ostree-repo.c

+      return FALSE;
+
+    g_clear_pointer (&self->repo_finders, g_strfreev);
+    self->repo_finders = g_steal_pointer (&configured_finders);


I think it would be a little more efficient to validate configured_finders before freeing the existing repo_finders.

dbnicholson · 2018-10-18T11:47:15Z

src/libostree/ostree-repo.c

+          return glnx_throw (error, "Invalid empty repo-finders configuration");
+
+        repo_finders_iter = self->repo_finders;
+        for (;repo_finders_iter && *repo_finders_iter; repo_finders_iter++)


Not that it matters, but is there a reason the assignment isn't done in the loop conditions?

I just copied that from somewhere else in the codebase but yeah we can do it the more normal way

dbnicholson · 2018-10-18T12:01:42Z

src/libostree/ostree-repo-pull.c

@@ -5013,7 +5013,8 @@ ostree_repo_find_remotes_async (OstreeRepo                     *self,
        default_finders[finder_index++] = finder_mount = OSTREE_REPO_FINDER (ostree_repo_finder_mount_new (NULL));

 #ifdef HAVE_AVAHI
-      if (self->repo_finders == NULL || g_strv_contains ((const char * const *)self->repo_finders, "lan"))
+      /* Only check the LAN if it was explicitly enabled (it's more expensive) */
+      if (self->repo_finders != NULL && g_strv_contains ((const char * const *)self->repo_finders, "lan"))


Would it make more sense to have self->repo_finders default to config and mount instead of treating lan specially here? Because this is inconsistent with what ostree_repo_get_repo_finders() would say.

Well ostree_repo_get_repo_finders() would return NULL which means "the default set which is up to libostree". If we were to set repo->repo_finders = "config,mount" as a default, the user couldn't do ostree find-remotes --finders=lan ... (and similar for the API that lets you pass finders in)

Well ostree_repo_get_repo_finders() would return NULL which means "the default set which is up to libostree".

Right, but that default policy is now inconsistent with this change.

If we were to set repo->repo_finders = "config,mount" as a default, the user couldn't do ostree find-remotes --finders=lan ... (and similar for the API that lets you pass finders in)

I think it might be better to change the policy that the config key is implementing. Right now the key is both the default set of finders and the bounding set of finders. If it was just the default set (with config and mount), then ostree find-remotes --finders=lan would be allowed, it just wouldn't be the default. I think this makes the policy handled by that option clearer.

If you want to have an option that controls the bounding set (for someone that wants to actively disallow a finder), then I think that should be a different option. core.allowed-repo-finders or something.

Sure that makes sense. I think having core.repo-finders be the default set not the bounding set will serve the right purpose. Then Flatpak can use the value of ostree_repo_get_repo_finders in the implementation of flatpak_installation_list_remotes_by_type, which is the only place Flatpak calls ostree_repo_find_remotes_async with a non-NULL set of finders.

mwleeds · 2018-10-18T19:04:17Z

Pushed a fixup to address the feedback. It will need to be squashed manually.

Currently libostree essentially has two modes when it's pulling refs: the "legacy" code paths pull only from the Internet, and the code paths that are aware of collection IDs try to pull from the Internet, the local network, and mounted filesystems (such as USB drives). The problem is that while we eventually want to migrate everyone to using collection IDs, we don't want to force checking LAN and USB sources if the user just wants to pull from the Internet, since the LAN/USB code paths can have privacy[1], security[2], and performance[3] implications. So this commit implements a new repo config option called "repo-finders" which can be configured to, for example, "config;lan;mount;" to check all three sources or "config;mount;" to disable searching the LAN. The set of values mirror those used for the --finders option of the find-remotes command. This configuration affects pulls in three places: 1. the ostree_repo_find_remotes_async() API, regardless of whether or not the user of the API provided a list of OstreeRepoFinders 2. the ostree_repo_finder_resolve_async() / ostree_repo_finder_resolve_all_async() API 3. the find-remotes command This feature is especially important right now since we soon want to have Flathub publish a metadata key which will have Flatpak clients update the remote config to add a collection ID.[4] This effectively fixes flatpak/flatpak#1863 but I'll patch Flatpak too, so it doesn't pass finders to libostree only to then have them be removed. [1] flatpak/flatpak#1863 (comment) [2] ostreedev#1527 [3] Based on how long the "ostree find-remotes" command takes to complete, having the LAN finder enabled slows down that step of the pull process by about 40%. See also flatpak/flatpak#1862 [4] flathub/flathub#676

This commit disables searching on the local network for refs, unless explicitly requested by the user either by changing the value of the "core.repo-finders" config option, or by passing an OstreeRepoFinderAvahi to ostree_repo_find_remotes_async() / ostree_repo_finder_resolve_async(), or by specifying "lan" in the --finders option of the find-remotes command. The primary reason for this is that ostree_repo_find_remotes_async() takes about 40% longer to complete with the LAN finder enabled, and that API is used widely (e.g. in every flatpak operation). It's also probable that some users don't want ostree doing potentially unexpected traffic on the local network, even though everything pulled from a peer is GPG verified. Flathub will soon deploy collection IDs to everyone[1] so these code paths will soon see a lot more use and that's why this change is being made now. Endless is the only potential user of the LAN updates feature, and we can revert this patch on our fork of ostree. For it to be used outside Endless OS we will need to upstream eos-updater-avahi and eos-update-server into ostree. [1] flathub/flathub#676

mwleeds · 2018-10-20T00:34:31Z

This is CR Ready again and the semantics of it are now that it's a "default set" not a "whitelist" (it can be overridden when specific finders are passed to library API or specified on the command line).

OSTree now has a repo config option to set which repo finders (that is, sources for refs) will be used, and API which exposes the default set (which was either configured by the user or decided by libostree). So this commit changes flatpak_installation_list_remotes_by_type() to respect that configuration, since that's the only place flatpak passes a set of OstreeRepoFinder instances to libostree. See also ostreedev/ostree#1758

cgwalters · 2018-10-21T19:11:40Z

@rh-atomic-bot r+ c04d61c

Looks good to me; if there's anything dbnicholson wants to change here I think we can do it as a followup. Thanks for the patch!

rh-atomic-bot · 2018-10-21T19:11:46Z

⚡ Test exempted: merge already tested.

This commit disables searching on the local network for refs, unless explicitly requested by the user either by changing the value of the "core.repo-finders" config option, or by passing an OstreeRepoFinderAvahi to ostree_repo_find_remotes_async() / ostree_repo_finder_resolve_async(), or by specifying "lan" in the --finders option of the find-remotes command. The primary reason for this is that ostree_repo_find_remotes_async() takes about 40% longer to complete with the LAN finder enabled, and that API is used widely (e.g. in every flatpak operation). It's also probable that some users don't want ostree doing potentially unexpected traffic on the local network, even though everything pulled from a peer is GPG verified. Flathub will soon deploy collection IDs to everyone[1] so these code paths will soon see a lot more use and that's why this change is being made now. Endless is the only potential user of the LAN updates feature, and we can revert this patch on our fork of ostree. For it to be used outside Endless OS we will need to upstream eos-updater-avahi and eos-update-server into ostree. [1] flathub/flathub#676 Closes: #1758 Approved by: cgwalters

mwleeds · 2018-10-22T16:36:08Z

Looks like I forgot to update the commit message after the last changes so it's a bit inaccurate in saying the config option affects ostree_repo_finder_resolve_async and ostree find-remotes. Sorry about that.

pwithnall · 2018-10-22T21:35:39Z

I left some post-hoc review since I wasn’t around last week. Sorry for the delay.

mwleeds · 2018-10-22T22:17:25Z

I addressed your feedback in #1763

OSTree now has a repo config option to set which repo finders (that is, sources for refs) will be used, and API which exposes the default set (which was either configured by the user or decided by libostree). So this commit changes flatpak_installation_list_remotes_by_type() to respect that configuration, since that's the only place flatpak passes a set of OstreeRepoFinder instances to libostree. See also ostreedev/ostree#1758

OSTree now has a repo config option to set which repo finders (that is, sources for refs) will be used, and API which exposes the default set (which was either configured by the user or decided by libostree). So this commit changes flatpak_installation_list_remotes_by_type() to respect that configuration, since that's the only place flatpak passes a set of OstreeRepoFinder instances to libostree. See also ostreedev/ostree#1758 Closes: #2264 Approved by: pwithnall

mwleeds force-pushed the allow-disabling-p2p branch from cb4f236 to b568259 Compare October 18, 2018 00:27

mwleeds force-pushed the allow-disabling-p2p branch from b568259 to a10a38f Compare October 18, 2018 02:52

dbnicholson requested changes Oct 18, 2018

View reviewed changes

mwleeds changed the title ~~Allow disabling pulling from LAN/USB/Internet~~ [WIP] Allow disabling pulling from LAN/USB/Internet Oct 19, 2018

mwleeds added 2 commits October 19, 2018 16:35

mwleeds force-pushed the allow-disabling-p2p branch from a6c021f to c04d61c Compare October 20, 2018 00:33

mwleeds changed the title ~~[WIP] Allow disabling pulling from LAN/USB/Internet~~ Allow disabling pulling from LAN/USB/Internet Oct 20, 2018

mwleeds mentioned this pull request Oct 20, 2018

installation: Respect configured set of repo finders flatpak/flatpak#2264

Closed

cgwalters added the homu/approved label Oct 21, 2018

rh-atomic-bot closed this in 3956fc8 Oct 21, 2018

mwleeds deleted the allow-disabling-p2p branch October 22, 2018 16:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow disabling pulling from LAN/USB/Internet #1758

Allow disabling pulling from LAN/USB/Internet #1758

mwleeds commented Oct 18, 2018

mwleeds commented Oct 18, 2018

dbnicholson left a comment

dbnicholson Oct 18, 2018

mwleeds Oct 18, 2018

dbnicholson Oct 18, 2018

mwleeds Oct 18, 2018

dbnicholson Oct 18, 2018

dbnicholson Oct 18, 2018

mwleeds Oct 18, 2018

dbnicholson Oct 18, 2018

mwleeds Oct 18, 2018

dbnicholson Oct 18, 2018

mwleeds Oct 18, 2018

dbnicholson Oct 18, 2018

mwleeds Oct 18, 2018

dbnicholson Oct 18, 2018

mwleeds Oct 18, 2018

dbnicholson Oct 18, 2018

mwleeds Oct 19, 2018

mwleeds commented Oct 18, 2018

mwleeds commented Oct 20, 2018

cgwalters commented Oct 21, 2018

rh-atomic-bot commented Oct 21, 2018

mwleeds commented Oct 22, 2018

pwithnall commented Oct 22, 2018

mwleeds commented Oct 22, 2018

Allow disabling pulling from LAN/USB/Internet #1758

Allow disabling pulling from LAN/USB/Internet #1758

Conversation

mwleeds commented Oct 18, 2018

mwleeds commented Oct 18, 2018

dbnicholson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mwleeds commented Oct 18, 2018

mwleeds commented Oct 20, 2018

cgwalters commented Oct 21, 2018

rh-atomic-bot commented Oct 21, 2018

mwleeds commented Oct 22, 2018

pwithnall commented Oct 22, 2018

mwleeds commented Oct 22, 2018