[Pulp-dev] Proposal: Changing in 3.1 that Artifact.save() will hard-link/copy, not move

[Simon Baatz]

On Tue, Jan 07, 2020 at 04:45:54PM -0500, Brian Bouterse wrote:
>    We had two bugs filed recently [0][1] which suggest that when using the
>    default backend for Pulp, i.e. pulpcore.app.models.storage.FileSystem
>    Pulp should not be "moving" files. This is the default behavior Django
>    gives us, and it destroys data when sync'ed from file:/// for example
>    [1].
>    I propose that with 3.1 we fix this bug by switching
>    pulpcore.app.models.storage.FileSystem to leave files in place and
>    either hard-link (same filesystem) or copy (different filesystem).
>    This will require files to be cleaned up where before the were "moved"
>    so they didn't need cleanup. This will include worker temp directories,
>    uploaded files, uploaded files w/ the chunked API, and downloaded files
>    during sync. I believe this is all straightforward, but an important
>    side-effect of this change to identify. Plugin writers that manage
>    files would also need to handle this, but can mostly rely on pulpcore
>    cleaning up the worker temp directories and user-uploaded files.
>    What do you think about this? Do you have concerns? Is there something
>    better we can do?

Yes, I have concerns:

- Aren't there files that are readable for the worker ("pulp" user), but
  may be not for the user doing the import?  The user could try to
  import them.  "file:///etc/pulp/settings.py" comes to mind as a
  juicy target (Pulp pun intended).

  Can we ensure that this can't happen? And which part of Pulp must
  ensure this?

- In general, we must not trust paths supplied by the user; things
  like missing path normalization and path comparisons using
  startswith() make me nervous (https://access.redhat.com/security/cve/cve-2018-10917
  comes to mind).