Implement secure temporary file handling

[TobyGiacometti]

Is your feature request related to a problem? Please describe

I have noticed that the temporary files created by pyinfra are world-readable and often have a predictable name. For example, when using pyinfra.operations.server.script(), the uploaded script is stored in the temporary directory with permission bits set to 755. This has security implications if the script contains sensitive information. Another example would be pyinfra.operations.apt.deb() which downloads data and stores it in a temporary file with a predictable name (SHA-1 hash of the download URL). In this example, a malicious actor could simply place a Debian package with the same name in the temporary directory, and because the internal call to pyinfra.operations.files.download() does not use the force parameter, the malicious actor's Debian package would be installed.

Describe the solution you'd like

In my opinion, the most robust solution would be to store all temporary deploy files in a secure temporary directory managed by pyinfra. I have addressed the issue in pyinfra 3.0b0 using a config file (config.py) that looks as follows:

"""The pyinfra config file."""

import shlex
import sys
import typing
import uuid

import pyinfra
import pyinfra.api.host
import pyinfra.connectors.ssh
import pyinfra.facts.server
import pyinfra.operations.python


def pyinfra_temp_path(self: pyinfra.api.host.Host, *_: typing.Any) -> str:
    """Override for ``pyinfra.api.host.Host.get_temp_filename``.

    pyinfra creates predictably named world-readable temporary files.
    This override introduces more secure handling.
    """

    def create_temp_dir(user: str) -> str:
        class TempDir(typing.TypedDict):  # pylint: disable=missing-class-docstring
            path: str
            created: bool
            queued: bool

        temp_dirs = vars(pyinfra_temp_path).setdefault(self.name, {})
        temp_dir: TempDir = temp_dirs.setdefault(
            user, {"path": "", "created": False, "queued": False}
        )

        # One temporary directory per host and user combination should
        # be created.
        if not temp_dir["path"]:
            temp_dir["path"] = "/tmp/iac." + uuid.uuid4().hex[:10]

        # Temporary directories should only be created when needed
        # during the execution stage.
        if pyinfra.state.is_executing:
            if not temp_dir["created"]:
                # A fact is used to execute commands so that operation
                # parameters are automatically inherited.
                self.get_fact(
                    pyinfra.facts.server.Command,
                    (
                        shlex.join(["mkdir", "-m", "700", "--", temp_dir["path"]])
                        + " && "
                        + shlex.join(["chown", "--", user, temp_dir["path"]])
                    ),
                )
                temp_dir["created"] = True
        elif not self.in_op:
            # If this function is not called as part of an operation,
            # an operation has to be queued to ensure that the directory
            # creation code is executed during the execution stage.
            if not temp_dir["queued"]:
                pyinfra.operations.python.call(
                    name=f'Create directory "{temp_dir["path"]}"',
                    function=create_temp_dir,
                    user=user,
                )
                temp_dir["queued"] = True

        return temp_dir["path"]

    # SFTP uploads are executed with the user specified in the connector.
    # If an operation uses privilege escalation, any uploads end up in
    # the temporary directory and are moved to the desired location with
    # escalated privileges. By using a separate temporary directory for
    # this process, the connected user does not have to receive access
    # to the superuser's temporary directory.
    user = None
    if isinstance(self.connector, pyinfra.connectors.ssh.SSHConnector):
        # Using stack frame objects to check whether this function is
        # being called as part of the put operation is simpler than
        # monkey patching operation functions.
        # pylint: disable-next=protected-access
        caller_frame = sys._getframe().f_back
        if caller_frame:
            if (
                caller_frame.f_code.co_filename.endswith("/pyinfra/operations/files.py")
                and caller_frame.f_code.co_name == "put"
            ):
                user = self.get_fact(
                    pyinfra.facts.server.User, _sudo=False, _su_user=None, _doas=False
                )
    if user is None:
        user = self.get_fact(pyinfra.facts.server.User)

    # Any arguments passed to the function are ignored because it should
    # always return a random path to prevent collisions.
    return create_temp_dir(user) + "/" + uuid.uuid4().hex[:14]


pyinfra.api.host.Host.get_temp_filename = pyinfra_temp_path  # type: ignore

With this approach, pyinfra creates a temporary directory with a random name and permission bits set to 700 (/tmp is used statically because I don't require $TMPDIR evaluation). All temporary file/directory paths generated by pyinfra are scoped to this directory. The temporary directory is created when needed during the execution stage to prevent needless creation of temporary directories. If an SFTP upload is executed with an unprivileged user, an additional temporary directory for that user is created. In addition, all arguments passed to pyinfra.api.host.Host.get_temp_filename() are ignored, because the returned path should always be random to prevent collisions.

[sudoBash418]

I'd love to see this implemented, as it can a pretty big security hole in my opinion (depending on use-case).

FWIW, I tried implementing a similar feature against pyinfra 2.4 a couple years ago, but didn't have time to polish it for upstreaming - I might pick it back up and post a PR.

One notable issue I ran into was that using sudo/su/doas with non-privileged target users tends to be incompatible with this feature. This is because basic Unix permissions don't allow you to assign read permissions to an arbitrary user (root can usually bypass FS access controls, but other users usually cannot).
POSIX ACLs might be a solution to this, but setfacl isn't guaranteed to exist and neither is filesystem support. I suppose pyinfra could just fallback to either raising an error, or setting files world-readable, depending on a config flag? That way there's an escape hatch for weird situations, but pyinfra would become more secure-by-default.

Edit: somehow I missed the fact that setfacl is already used whenever a user is specified 😄

[TobyGiacometti]

Good point about using unprivileged users with sudo/su/doas! The workaround I shared does not account for this as I don't need it, but it wouldn't complicate things too much: Multiple temporary directories for each SFTP user and sudo/su/doas user combination could be created, each with an ACL entry for the sudo/su/doas user. If ACLs are not supported, I agree that raising an error would be the safest solution, while at the same time offering an opt-in solution to still create world-readable files (for affected user combinations). As such files are moved right after upload, potentially sensitive information is not world-readable for long.