New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

eliminate file duplication during a local untrusted pull #1723

Open
ramcq opened this Issue Sep 14, 2018 · 19 comments

Comments

Projects
None yet
6 participants
@ramcq
Contributor

ramcq commented Sep 14, 2018

As mentioned in #1720 (comment), we have a problem in Endless OS using Flatpak apps for some pretty large apps. During an install/upgrade of an app, Flatpak runs the networking code and does the pull as the user (for various good reasons, proxies, auth, etc) into a child repo in /var/tmp, and then uses the priveleged system helper to do a local filesystem pull from the user-owner child repo into the real one.

Because a malicious user could keep FDs open and modify the child repo, this has to be an untrusted pull where every file is copied and verified against the configured GPG key of the system remote. On btrfs this uses reflinks, but everywhere else the consequence of this double pull is that twice the disk space is required during an app install or upgrade.

We've got some 5-6GB apps and some systems which ship with eg 32GB MMCs, so this is a very real problem for us - so we're having to use heuristics to disable automatic upgrades when we are low on disk space - but we'd like to come up with a way to mitigate or eliminate this duplication entirely. As far as I'm aware there are no current kernel APIs to revoke an FD or seal a file.

Some approaches we've thought about:

  • We thought about running the networking code as a depriveleged and/or sandboxed system user which can only be accessed by the flatpak system helper - as we have bwrap etc - but this doesn't allow normal access to the user's desktop config (proxies, env vars, etc).
  • Implement a "destructive pull" which unlinked each source file as it went. This one seems not too bad although we would probably need some help mapping out exactly how/where to do it. It has the downside that we're not able to entirely eliminate the duplication, merely reduce it to per-file rather than the whole ref. Still, 1GB is a lot better than 6GB.
  • "Stealing" the file somehow - mv to a root-owned temp folder, chown to root:root, scanning /proc to look for open fds to the file, then proceeding.
  • A variant of the above which used the immutable flag to ensure that after verification, no changes could be made...? The problem is that we wouldn't know when we'd be able to clear that immutable flag, potentially not until a reboot.
  • Ask the system helper to create the user child for us, and it does so by creating an unaccessible folder and then bind mounting it so that it's accessible to the user to do the pull. It could then unmount the user bind mount to enforce that no FDs are open, before proceeding to verify and hardlink in-place. (Kudos to @wjt for this suggestion!)

/cc @uajain @wjt @pwithnall @mwleeds

@wjt

This comment has been minimized.

Contributor

wjt commented Sep 14, 2018

It could then unmount the user bind mount to enforce that no FDs are open, before proceeding to verify and hardlink in-place.

The problem I ran into here is that if there is an FD open there, the bind mount will just be leaked, because I couldn't see a way to forcibly unmount it (and break any open FDs). Presumably providing a way for unprivileged users to create unbounded numbers of bind mounts would be bad :)

@ramcq

This comment has been minimized.

Contributor

ramcq commented Sep 14, 2018

It could then unmount the user bind mount to enforce that no FDs are open, before proceeding to verify and hardlink in-place.

The problem I ran into here is that if there is an FD open there, the bind mount will just be leaked, and I couldn't see a way to forcibly unmount it, breaking any open FDs. Presumably providing a way for unprivileged users to create unbounded numbers of bind mounts would be bad :)

Could the helper not enforce a limit per user? It can stay running as long as there are any mounts, or use a predictable path to re-use the same mount point between invocations, and try to clean/unmount/remount when preparing the operation, and fail if you /still/ have FDs open at that point.

@wjt

This comment has been minimized.

Contributor

wjt commented Sep 14, 2018

(On Endless we have the additional quirk that /var/tmp is on the /var bind-mount whereas /var/lib/flatpak is a symlink into the /sysroot bind-mount (so that the ostree and flatpak repos can be shared). It's the same physical filesystem but you can't hardlink across a bind mount. But this could be addressed by arranging for the temporary directory to be below /var/lib/flatpak/repo/tmp which will always be on the same mount as /var/lib/flatpak[/repo].)

@pwithnall

This comment has been minimized.

Member

pwithnall commented Sep 14, 2018

* "Stealing" the file somehow - mv to a root-owned temp folder, chown to root:root, scanning /proc to look for open fds to the file, then proceeding.

I think smcv shot this one down by pointing out that a malicious process could race to re-open a file from /proc/*/fd/$fd after the scanning process had passed over that $fd, essentially making it impossible to guarantee that a process can’t keep an FD around after you move the original file to an inaccessible path.

tl;dr: We couldn’t think of a way to make this particular option secure.

@alexlarsson

This comment has been minimized.

Member

alexlarsson commented Sep 14, 2018

Yeah, the bind mount is not gonna work. Maybe we can create a fuse filesystem that is a pure forwarding system + revoke.

@pwithnall

This comment has been minimized.

Member

pwithnall commented Sep 14, 2018

Could the helper not enforce a limit per user?

Sounds reasonable to me. The limit could even be 1.

I guess one potential downside there is that it means a malicious app could DoS the user’s access to updates for other apps, which provides a nice avenue for a malicious app to concrete itself in place. If the user were made aware of the stuck-mount issue, they might be able to deal with the issue (or, more likely, not understand what’s going on).

If a bind mount is stuck, we could fall back to the current code path of using double the disk space. That would avoid the DoS issue.

@alexlarsson

This comment has been minimized.

Member

alexlarsson commented Sep 14, 2018

Then the system helper could mount that, and revoke before applying.

Hmm, actually, that will probably not work, because fuse file systems are only accessible to one uid.

@pwithnall

This comment has been minimized.

Member

pwithnall commented Sep 14, 2018

Yeah, the bind mount is not gonna work.

Because…?

@alexlarsson

This comment has been minimized.

Member

alexlarsson commented Sep 14, 2018

@pwithnall Its possible to have a host namespace unmount "succeed", yet the filesystem could still be mounted in a different namespace.

@cgwalters

This comment has been minimized.

Member

cgwalters commented Sep 14, 2018

On btrfs this uses reflinks

Random side note, multiple kernel filesystems now support reflinks/copy_file_range() - most notably XFS, but also including NFS.

a malicious process could race to re-open a file from /proc/*/fd/$fd after the scanning process had passed over that $fd

If the system helper is running as root, an unprivileged process isn't going to be able to open its /proc/<pid> dir.

BTW see also https://marc.info/?l=linux-xfs&m=150151699914687&w=2

@ramcq

This comment has been minimized.

Contributor

ramcq commented Sep 14, 2018

On btrfs this uses reflinks

Random side note, multiple kernel filesystems now support reflinks/copy_file_range() - most notably XFS, but also including NFS.

Selfishly also very concerned about ext4... :)

a malicious process could race to re-open a file from /proc/*/fd/$fd after the scanning process had passed over that $fd

If the system helper is running as root, an unprivileged process isn't going to be able to open its /proc/<pid> dir.

Ie there is no attack from the unpriveleged process being able to re-open anything provided it's first moved into a path inaccessible to anyone but root? So "steal the entire repo, scan for open FDs, ..." option might not be so absurd?

BTW see also https://marc.info/?l=linux-xfs&m=150151699914687&w=2

How was that received? :)

@ramcq

This comment has been minimized.

Contributor

ramcq commented Sep 14, 2018

Then the system helper could mount that, and revoke before applying.

Hmm, actually, that will probably not work, because fuse file systems are only accessible to one uid.

Does this apply to root as well? Stuff like ntfs-3g runs whole "normal" filesystems as FUSE. Are they all tied to one UID?

@alexlarsson

This comment has been minimized.

Member

alexlarsson commented Sep 14, 2018

@ramcq I tried bindfs, and it did allow a root-mounted fuse fs to be accessible by the user, so clearly there is some way to do this. For further security we could run the fuse fs as some special (non-root) uid, exposing it only to the right user (and then getting rid of the fuser mount, or making it forced readonly) when revoking.

@cgwalters

This comment has been minimized.

Member

cgwalters commented Sep 14, 2018

How was that received? :)

Well, the most recent update is https://marc.info/?l=linux-kernel&m=153515073024956&w=2 - so far the fsverity proposal only denies truncate which I don't think is a good idea. I just followed up.

@ramcq

This comment has been minimized.

Contributor

ramcq commented Sep 15, 2018

There was some follow-up discussion on #flatpak about using a FUSE filesystem as a way to basically make a revokable bind mount. (Although use of FUSE is hardly unprecedented in the OSTree stack, it seems sad to me that the only way we have to achieve this is by introducing another process with all of the complexity, fragility and overhead.)

@alexlarsson found http://bindfs.org which in terms of exporting a root owned dir so that a user can write to it, and then ungracefully kill the mount. It seemed to work in his quick test, so rather than recurse into writing a new FUSE filesystem, I think we can prototype something in Flatpak using this. Then with a bit more practical experience of it, decide if it's acceptable to bring in as an optional external dependency, bundle or rewrite as needed.

The steps to me seem like:

  • add a method to the system helper to start a pull, which creates a temp folder under repo/tmp and returns it to the user, keep a list of these
  • keep the helper process alive if the list is non empty
  • take them off the list if the dir is given back to us for a pull
  • check the UID of the requesting user and implement a limit of the number open per user
  • implement a timeout after which the pull is killed anyway (previously, tmpfiles would come knocking after 30 days...)
  • after all that set up, we can change the requesting method to do a bindfs instead, keep the process id in the structure, and the apply method to kill it and proceed
  • profit!

Thoughts? I guess that makes this more of a Flatpak ticket than an OSTree one at this point.

@uajain

This comment has been minimized.

Contributor

uajain commented Nov 14, 2018

This issue is being resolved as a part of flatpak instead of OSTree. OSTree admins @cgwalters @jlebon Can you help me by transferring this issue to flatpak github repo so that I can link that in my commit messages?

Transferring instructions are here.

Thank you.

@cgwalters

This comment has been minimized.

Member

cgwalters commented Nov 14, 2018

To transfer an open issue to another repository, you must have admin permissions on the repository the issue is in and the repository you're transferring the issue to.

Is not true for me; I am not sure if there is currently such a person. To help unblock this I have made @pwithnall an Owner in the ostreedev organization.

@uajain

This comment has been minimized.

Contributor

uajain commented Nov 14, 2018

Is not true for me

Maybe it's in beta that's why? 🤔

https://blog.github.com/changelog/2018-10-30-issue-transfer/

@pwithnall

This comment has been minimized.

Member

pwithnall commented Nov 15, 2018

It’s not possible:

If you're transferring an issue from a repository that's owned by an organization you're a member of, you must transfer it to another repository within your organization.

I get options only to transfer the issue to other repositories within the ostreedev project.

uajain added a commit to uajain/flatpak that referenced this issue Nov 21, 2018

system-helper: Add GetChildRepoForPull D-Bus method
There occurs a situation in --system installation where free disk space
should be atleast 2x of the size of the pull(for it to succeed). Apart
from the double disk space requirement of the pull, there is a lot of
I/O activity involved while pulling a system child repo into the parent
one. Therefore, this D-Bus method tries to address these issues by
creating a child repo at the place (inside repo/tmp) from where pulling
into the parent is just a matter of hardlinking. This also makes the pull
disk-space efficient.

This method primarily tries to create a $REPO/tmp/flatpak-cache-$REF-XXXXXX
directory containing a OSTree child repo. This directory is bind-mounted
using bindfs which additionally enforces relevant file-ownership and
file-creation policies. The path of the child repo in the mirrored
directory is returned back to the client for pulling the object from
remote. The client sees the mirrored directory as writable and starts
the pull. As the pull proceeds, actual files that are being created on
the disk have the ownership of the mounter(i.e. in this case, root) but
as long as the mirrored directory is mounted, the client is able to write
to it.

After the pull is completed, we unmount the mirrored directory just
before Deploy so the client would not be able to modify or write to it.
This enables us to perform a "trusted pull" from the child repo into the
system one.

The mounting of the mirrored directory takes place as a subprocess via
bindfs. If bindfs is not found on the system, we fallback on the
existing codepath of doing a --system pull.

In case of pull interruption, this method can find the partially pulled
child repo in repo/tmp, verify the child repo and re-use the cache
directory to pull the remaining objects.

Each ongoing pull is tracked using a helper struct FlatpakSystemHelperOngoingPull.
A GPtrArray ongoing_pulls keeps the track of all ongoing pulls in
the system helper.

ostreedev/ostree#1723

uajain added a commit to uajain/flatpak that referenced this issue Nov 21, 2018

system-helper: Add CancelPull D-Bus method
If there is a pull failure in a child repo created by GetChildRepoForPull
system helper method, there is no way to go back to the system helper and
notify it to cleanup. Therefore, CancelPull is required on the pull failure
error path, so that the ongoing pull can be cleaned up nicely and prevent
any dangling mounts and subprocesses.

ostreedev/ostree#1723

uajain added a commit to uajain/flatpak that referenced this issue Nov 21, 2018

common: Use system-helper's GetChildRepoForPull method during install
The current --system pull requires atleast twice the free space of the
total size of the pull. This is due to the fact that the pull needs to
be verified first. Hence, it is first fetched from the remote into a
child repo, verified and pull in the system's repo. This temporary
duplication for security reasons brings in a double-disk space
requirement which hurt systems with tight disk space and trying
to install a relatively large ref.

The approach to solve this issue is basically to create a child repo
at a place so that it could use hardlinks to pull into the system repo.
The client asks the system helper to create a child repo inside a
cache directory in repo/tmp and bind mount the cache directory using
bindfs. The client will be able to pull into the child repo(as long as
the cache directory is mounted) from the remote and just before deploying,
unmount the cache directory to prevent any modification in the child repo.
This lets us to do a trusted pull from the child repo to the system one
using hardlinks.

On failure, CancelPull will cleanup relevant bits of the system-helper.
The pull will now happen with the codepath that exists currently
(fallback).

ostreedev/ostree#1723

uajain added a commit to uajain/flatpak that referenced this issue Nov 27, 2018

system-helper: Add GetChildRepoForPull D-Bus method
There occurs a situation in --system installation where free disk space
should be atleast 2x of the size of the pull(for it to succeed). Apart
from the double disk space requirement of the pull, there is a lot of
I/O activity involved while pulling a system child repo into the parent
one. Therefore, this D-Bus method tries to address these issues by
creating a child repo at the place (inside repo/tmp) from where pulling
into the parent is just a matter of hardlinking. This also makes the pull
disk-space efficient.

This method primarily tries to create a $REPO/tmp/flatpak-cache-$REF-XXXXXX
directory containing a OSTree child repo. This directory is bind-mounted
using bindfs which additionally enforces relevant file-ownership and
file-creation policies. The path of the child repo in the mirrored
directory is returned back to the client for pulling the object from
remote. The client sees the mirrored directory as writable and starts
the pull. As the pull proceeds, actual files that are being created on
the disk have the ownership of the mounter(i.e. in this case, root) but
as long as the mirrored directory is mounted, the client is able to write
to it.

After the pull is completed, we unmount the mirrored directory just
before Deploy so the client would not be able to modify or write to it.
This enables us to perform a "trusted pull" from the child repo into the
system one.

The mounting of the mirrored directory takes place as a subprocess via
bindfs. If bindfs is not found on the system, we fallback on the
existing codepath of doing a --system pull.

In case of pull interruption, this method can find the partially pulled
child repo in repo/tmp, verify the child repo and re-use the cache
directory to pull the remaining objects.

Each ongoing pull is tracked using a helper struct FlatpakSystemHelperOngoingPull.
A GPtrArray ongoing_pulls keeps the track of all ongoing pulls in
the system helper.

ostreedev/ostree#1723

uajain added a commit to uajain/flatpak that referenced this issue Nov 27, 2018

system-helper: Add CancelPull D-Bus method
If there is a pull failure in a child repo created by GetChildRepoForPull
system helper method, there is no way to go back to the system helper and
notify it to cleanup. Therefore, CancelPull is required on the pull failure
error path, so that the ongoing pull can be cleaned up nicely and prevent
any dangling mounts and subprocesses.

ostreedev/ostree#1723

uajain added a commit to uajain/flatpak that referenced this issue Nov 27, 2018

common: Use system-helper's GetChildRepoForPull method during install
The current --system pull requires atleast twice the free space of the
total size of the pull. This is due to the fact that the pull needs to
be verified first. Hence, it is first fetched from the remote into a
child repo, verified and pull in the system's repo. This temporary
duplication for security reasons brings in a double-disk space
requirement which hurt systems with tight disk space and trying
to install a relatively large ref.

The approach to solve this issue is basically to create a child repo
at a place so that it could use hardlinks to pull into the system repo.
The client asks the system helper to create a child repo inside a
cache directory in repo/tmp and bind mount the cache directory using
bindfs. The client will be able to pull into the child repo(as long as
the cache directory is mounted) from the remote and just before deploying,
unmount the cache directory to prevent any modification in the child repo.
This lets us to do a trusted pull from the child repo to the system one
using hardlinks.

On failure, CancelPull will cleanup relevant bits of the system-helper.
The pull will now happen with the codepath that exists currently
(fallback).

ostreedev/ostree#1723

uajain added a commit to uajain/flatpak that referenced this issue Nov 28, 2018

system-helper: Add GetChildRepoForPull D-Bus method
There occurs a situation in --system installation where free disk space
should be atleast 2x of the size of the pull(for it to succeed). Apart
from the double disk space requirement of the pull, there is a lot of
I/O activity involved while pulling a system child repo into the parent
one. Therefore, this D-Bus method tries to address these issues by
creating a child repo at the place (inside repo/tmp) from where pulling
into the parent is just a matter of hardlinking. This also makes the pull
disk-space efficient.

This method primarily tries to create a $REPO/tmp/flatpak-cache-$REF-XXXXXX
directory containing a OSTree child repo. This directory is bind-mounted
using bindfs which additionally enforces relevant file-ownership and
file-creation policies. The path of the child repo in the mirrored
directory is returned back to the client for pulling the object from
remote. The client sees the mirrored directory as writable and starts
the pull. As the pull proceeds, actual files that are being created on
the disk have the ownership of the mounter(i.e. in this case, root) but
as long as the mirrored directory is mounted, the client is able to write
to it.

After the pull is completed, we unmount the mirrored directory just
before Deploy so the client would not be able to modify or write to it.
This enables us to perform a "trusted pull" from the child repo into the
system one.

The mounting of the mirrored directory takes place as a subprocess via
bindfs. If bindfs is not found on the system, we fallback on the
existing codepath of doing a --system pull.

In case of pull interruption, this method can find the partially pulled
child repo in repo/tmp, verify the child repo and re-use the cache
directory to pull the remaining objects.

Each ongoing pull is tracked using a helper struct FlatpakSystemHelperOngoingPull.
A GPtrArray ongoing_pulls keeps the track of all ongoing pulls in
the system helper.

ostreedev/ostree#1723

uajain added a commit to uajain/flatpak that referenced this issue Nov 28, 2018

system-helper: Add CancelPull D-Bus method
If there is a pull failure in a child repo created by GetChildRepoForPull
system helper method, there is no way to go back to the system helper and
notify it to cleanup. Therefore, CancelPull is required on the pull failure
error path, so that the ongoing pull can be cleaned up nicely and prevent
any dangling mounts and subprocesses.

ostreedev/ostree#1723

uajain added a commit to uajain/flatpak that referenced this issue Nov 28, 2018

common: Use system-helper's GetChildRepoForPull method during install
The current --system pull requires atleast twice the free space of the
total size of the pull. This is due to the fact that the pull needs to
be verified first. Hence, it is first fetched from the remote into a
child repo, verified and pull in the system's repo. This temporary
duplication for security reasons brings in a double-disk space
requirement which hurt systems with tight disk space and trying
to install a relatively large ref.

The approach to solve this issue is basically to create a child repo
at a place so that it could use hardlinks to pull into the system repo.
The client asks the system helper to create a child repo inside a
cache directory in repo/tmp and bind mount the cache directory using
bindfs. The client will be able to pull into the child repo(as long as
the cache directory is mounted) from the remote and just before deploying,
unmount the cache directory to prevent any modification in the child repo.
This lets us to do a trusted pull from the child repo to the system one
using hardlinks.

On failure, CancelPull will cleanup relevant bits of the system-helper.
The pull will now happen with the codepath that exists currently
(fallback).

ostreedev/ostree#1723

uajain added a commit to uajain/ostree that referenced this issue Dec 2, 2018

lib/repo-pull: Add OSTREE_REPO_PULL_FLAGS_IMMUTABLE_FILES
In the pull code path, if the caller can guarantee that the files
in the local repo are immutable and checksum-verified, then use the
_OSTREE_REPO_IMPORT_FLAGS_TRUSTED flag to try hardlink or reflink
while importing.

This mainly helps flatpak for enabling a hardlink-able local pull
in the --system case during deploy. If the child repo is owned by root
and is not world-writable, one can pass this to flag solve the double
space requirement problem (i.e. copying each object during import) by
using a hardlink-able(or reflink) pull.

ostreedev#1723

uajain added a commit to uajain/ostree that referenced this issue Dec 3, 2018

lib/repo-pull: Add OSTREE_REPO_PULL_FLAGS_IMMUTABLE_FILES
In the pull code path, if the caller can guarantee that the files
in the local repo are immutable and checksum-verified, then use the
_OSTREE_REPO_IMPORT_FLAGS_TRUSTED flag to try hardlink or reflink
while importing.

This mainly helps flatpak[1] for enabling a hardlink-able local pull
during deploy in the --system case. If the files are immutable and
and are not world-writable, the caller can set this to flag solve the
double disk space requirement problem (i.e. copying each object during
import) by using a hardlink-able(or reflink) pull while importing
into the system's repo.

[1] flatpak/flatpak#2342

ostreedev#1723

uajain added a commit to uajain/ostree that referenced this issue Dec 3, 2018

lib/repo-pull: Add OSTREE_REPO_PULL_FLAGS_IMMUTABLE_FILES
In the pull code path, if the caller can guarantee that the files
in the local repo are immutable and will be checksum verified, then
use the _OSTREE_REPO_IMPORT_FLAGS_TRUSTED flag to try to hardlink or
reflink while importing.

This mainly helps flatpak[1] for enabling a hardlink-able local pull
during deploy in the --system case. If the files are immutable and
and are not world-writable, the caller can set this to flag solve the
double disk space requirement problem (i.e. copying each object during
import) by using a hardlink-able(or reflink) pull while importing
into the system's repo.

[1] flatpak/flatpak#2342

ostreedev#1723

uajain added a commit to uajain/ostree that referenced this issue Dec 3, 2018

lib/repo-pull: Add OSTREE_REPO_PULL_FLAGS_IMMUTABLE_FILES
In the pull code path, if the caller can guarantee that the files
in the local repo are immutable and will be checksum verified, then
use the _OSTREE_REPO_IMPORT_FLAGS_TRUSTED flag to try to hardlink or
reflink while importing.

This mainly helps flatpak[1] for enabling a hardlink-able local pull
during deploy in the --system case. If the files are immutable and
and are not world-writable, the caller can set this to flag solve the
double disk space requirement problem (i.e. copying each object during
import) by using a hardlink-able(or reflink) pull while importing
into the system's repo.

[1] flatpak/flatpak#2342

ostreedev#1723

uajain added a commit to uajain/ostree that referenced this issue Dec 3, 2018

lib/repo-pull: Add OSTREE_REPO_PULL_FLAGS_IMMUTABLE_FILES
In the pull code path, if the caller can guarantee that the files
in the local repo are immutable and will be checksum verified, then
use the _OSTREE_REPO_IMPORT_FLAGS_TRUSTED flag to try to hardlink or
reflink while importing.

This mainly helps flatpak[1] for enabling a hardlink-able local pull
during deploy in the --system case. If the files are immutable and
and are not world-writable, the caller can set this to flag solve the
double disk space requirement problem (i.e. copying each object during
import) by using a hardlink-able(or reflink) pull while importing
into the system's repo.

[1] flatpak/flatpak#2342

ostreedev#1723

uajain added a commit to uajain/ostree that referenced this issue Dec 4, 2018

lib/commit: Try checksum+hardlink for untrusted local same-uid repos
This mainly helps flatpak[1] for enabling a hardlink-able local pull
during deploy in the --system case.  We assume the files are immutable
when owned by the same uid.

[1] flatpak/flatpak#2342

ostreedev#1723

cgwalters added a commit to uajain/ostree that referenced this issue Dec 4, 2018

lib/commit: Try checksum+hardlink for untrusted local same-uid repos
This mainly helps flatpak for enabling a hardlink-able local pull
during deploy in the --system case.  We assume the files are immutable
when owned by the same uid.

See ostreedev#1723
and flatpak/flatpak#2342

rh-atomic-bot added a commit that referenced this issue Dec 4, 2018

lib/commit: Try checksum+hardlink for untrusted local same-uid repos
This mainly helps flatpak for enabling a hardlink-able local pull
during deploy in the --system case.  We assume the files are immutable
when owned by the same uid.

See #1723
and flatpak/flatpak#2342

Closes: #1776
Approved by: uajain

cgwalters added a commit to uajain/ostree that referenced this issue Dec 4, 2018

lib/commit: Try checksum+hardlink for untrusted local same-uid repos
This mainly helps flatpak for enabling a hardlink-able local pull
during deploy in the --system case.  We assume the files are immutable
when owned by the same uid.

See ostreedev#1723
and flatpak/flatpak#2342

rh-atomic-bot added a commit that referenced this issue Dec 4, 2018

lib/commit: Try checksum+hardlink for untrusted local same-uid repos
This mainly helps flatpak for enabling a hardlink-able local pull
during deploy in the --system case.  We assume the files are immutable
when owned by the same uid.

See #1723
and flatpak/flatpak#2342

Closes: #1776
Approved by: uajain

uajain added a commit to uajain/flatpak that referenced this issue Dec 4, 2018

system-helper: Add GetChildRepoForPull D-Bus method
There occurs a situation in --system installation where free disk space
should be atleast 2x of the size of the pull(for it to succeed). Apart
from the double disk space requirement of the pull, there is a lot of
I/O activity involved while pulling a system child repo into the parent
one. Therefore, this D-Bus method tries to address these issues by
creating a child repo at the place (inside repo/tmp) from where pulling
into the parent is just a matter of hardlinking (instead of copy). This
also makes the pull disk-space efficient.

This method primarily tries to create a $REPO/tmp/flatpak-cache-$REF-XXXXXX
directory containing a OSTree child repo. It is bind-mounted with "-bindfs"
suffixed directory using bindfs which additionally enforces relevant file-ownership
and file-creation policies. The bindfs-mounted path of the child repo is returned
back to the client for pulling the repo from the remote. The client sees the
bind mounted directory as writable (because bindfs enforced relevant permissions)
and starts the pull. As the pull proceeds, actual files that are being created on
the disk have the ownership of the mounter(i.e. in this case, root) but
as long as the bind mount exists, the client(non-privileged) is able to write to it.

After the pull is completed, we unmount the bind mount just before Deploy so the
client would not be able to modify or write to it. This now enables the helper
to verify and hardlink the child repo(as it's owned by root) into the system one[1].

If bindfs is not found on the system, we fallback on the existing codepath of
doing a --system pull (that involves copying each object).

In case of pull interruption, this method can find the partially pulled
child repo in repo/tmp, verify the child repo and re-use the cache
directory to pull the remaining objects.

Each ongoing pull is tracked using a helper struct FlatpakSystemHelperOngoingPull.
A GPtrArray ongoing_pulls keeps the track of all ongoing pulls in
the system helper.

[1] ostreedev/ostree#1776

ostreedev/ostree#1723

uajain added a commit to uajain/flatpak that referenced this issue Dec 4, 2018

system-helper: Add CancelPull D-Bus method
If there is a pull failure in a child repo created by GetChildRepoForPull
system helper method, there is no way to go back to the system helper and
notify it to cleanup. Therefore, CancelPull is required on the pull failure
error path, so that the ongoing pull can be cleaned up nicely and prevent
any dangling mounts and subprocesses. We probably want to reuse the partial
pull in future so we don't delete the pull cache directory.

Some future work is needed to specify the expiration or explicit cleanup
of these stale pulls directories. At one end we want to be disk space
efficient and at another end, we do not want to waste user's network
bandwidth.

ostreedev/ostree#1723

uajain added a commit to uajain/flatpak that referenced this issue Dec 4, 2018

common: Use system-helper's GetChildRepoForPull method during install
The current --system pull requires atleast twice the free space of the
total size of the pull. This is due to the fact that the pull needs to
be copied over and cannot be hardlinked due to different repo-ownership[1].
This temporary duplication brings in a double-disk space requirement which
hurt systems with low disk space and trying to install a relatively large ref.

The approach to solve this issue is basically to create a child repo
at a place so that it could use hardlinks to pull into the system repo.
The client asks the system helper to create a child repo inside a
cache directory in repo/tmp and bind mount the cache directory using
bindfs. The client will be able to pull into the child repo(as long as
the cache directory is mounted) from the remote and just before deploying,
unmount the cache directory to prevent any modification in the child repo.
Now that the files are immutable and ownership of both repos are the same,
these files are checksum-verified and then hardlinked into the system's repo.

On failure, CancelPull will cleanup relevant bits of the system-helper.
The pull will now happen with the codepath that exists currently
(fallback).

[1] ostreedev/ostree#1776

ostreedev/ostree#1723
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment