temporary file creation for persisted metadata

[mrjoelkamp]

Problem

A go-tuf client is interrupted or errors when writing remote metadata to disk during persistMetadata in updater.go or a multi-client system shares update.cfg.LocalMetadataDir where metadata files are persisted and a client is writing persisted metadata while simultaneously another client reads it.

The risk of a client reading partially written files is low for single client systems, since the file writing error handler removes partially written files, but it can certainly still occur.

Incomplete, corrupted, or invalid persisted metadata can also occur via other means (disk read error, malicious code, bad actor)

Existing solution

Currently, the persistMetadata function writes to a temporary file in the current working directory (CWD). Once the file write is complete, then the temp file is either moved to the configured LocalMetadataDir or renamed if LocalMetadataDir == CWD.

This mechanism should prevent empty or partially written metadata files from ever existing in the persisted metadata directory. Although, the currently implemented moveFile uses io.Copy that could also be interrupted leading to partially written or empty files.

Issues

Use of io.Copy

Copy copies from src to dst until either EOF is reached on src or an error occurs. It returns the number of bytes copied and the first error encountered while copying, if any.

This could lead to partially written or empty files when copying the temp file to the destination, not providing an atomic write.

Use of current working directory

Writing to the current working directory is problematic for containerized workloads (as described in #638).

This should be an easy fix to either add a new configuration parameter for a temp file location or just use the LocalMetadataDir to store temporary files (tuf_tmp). But I ran into problems...

Windows

I attempted to use update.cfg.LocalMetadataDir for the tuf_tmp file location but this produced an error in windows tests (and revealed a bug) here due to os.Rename access denied on windows.

There is a thread in https://github.com/google/renameio that goes into some detail on how atomic writes for windows is problematic.

It appears that an atomic write for windows is mostly not feasible, therefore the persistMetadata function would need to write directly to the target file for windows. Otherwise, the code will always write a temp file and then read the temp file and write the real file (no rename/move) in windows, which is really inefficient and no benefit.

Do we need atomic writes for persisted metadata?

After reviewing the code base and the caveats made for windows OS, I questioned whether or not I should add additional logic to work around the windows bug discovered in the first attempt or eliminate the persisted metadata atomic write complexity completely.

Let's explore the case that there actually is a partially written or empty persisted metadata file:

• In order for this to occur the program must have been interrupted during file write, error handling, two or more clients are sharing the same metadata and running simultaneous updates
• A client attempts online refresh with one or more partially written metadata files
  • the updater loads the partial role metadata and attempts to verify it and it fails
  • the updater downloads the remote metadata due to invalid (or missing) local data
  • the updater persists the remote copy of the metadata (overwriting the partial copy)
• (non-standard) A client uses unsafeLocalRefresh to load cached metadata
  • the partial metadata fails verification and the client returns an error

Solutions

1. Refactor persistMetadata to add additional logic to the temp file rename/move to only provide atomic writes (via temp file) for POSIX systems

2. Remove code complexity via windows workaround and rely on metadata integrity checks within go-tuf updater and the TUF framework to mitigate partially written or empty persisted metadata files

I am in favor of 2 but willing to refactor the PR to implement 1 if desired.
edit: implemented option 1 in #638

[mrjoelkamp]

Looking to the python-tuf client for consistency here and noticed:

• only designed for a single client instance per local metadata

  • Note that applications using Updater should be 'single instance'
    applications: running multiple instances that use the same cache directories at
    the same time is not supported. (source)

• uses temp file and replace for atomic writes

[mrjoelkamp]

I figured out the bug with os.Rename on windows. It ended up being due to a file handle remaining open from the test setup: https://github.com/theupdateframework/go-tuf/pull/638/files#diff-808fee3380ff8b439ffdf8207f292e1c3cce70f766328bcbba4b2cec3a09c800R79

So this simplifies the solution if we make the temp file location the same as update.cfg.LocalMetadataDir. See #638

[kommendorkapten]

Thanks for the very good summary ❤
I'm in favour of option #1, and so keeping the atomic (rename) operation where possible.