mrjoelkamp commented 5 months ago

Summary

When cache is enabled, using the current working directory (cwd) for temporary file storage has been a bit problematic for container based workflows and various operating systems.

This is especially true for k8s workloads that have readOnlyRootFilesystem set, which is a security best practice. The creation of the temporary file in the working directory is not permitted with a read-only root filesystem.

Changes

use update.cfg.LocalMetadataDir for temporary file storage, so that it can be configured to a writable volume.
remove moveFile as it is no longer needed
- temp file and destination file are in the same directory
fixes open file handle in repository_similator_setup.go leading to os.Rename error on windows

fixes

639

codecov-commenter commented 5 months ago

:warning: Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 60.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 73.27%. Comparing base (14cf073) to head (415898f). Report is 20 commits behind head on master.

Files	Patch %	Lines
metadata/updater/updater.go	60.00%	1 Missing and 1 partial :warning:

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## master #638 +/- ## ========================================== + Coverage 70.51% 73.27% +2.75% ========================================== Files 10 10 Lines 2123 1635 -488 ========================================== - Hits 1497 1198 -299 + Misses 505 322 -183 + Partials 121 115 -6 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

trishankatdatadog commented 5 months ago

I can see where you're coming from, but before we remove it, do we know why it is there in the first place? We wouldn't want to accidentally run into the risk of removing a Chesterton fence...

mrjoelkamp commented 5 months ago

I can see where you're coming from, but before we remove it, do we know why it is there in the first place? We wouldn't want to accidentally run into the risk of removing a Chesterton fence...

@trishankatdatadog it appears that the temp file and rename logic is here to produce an atomic file write so that there is never a partially written or 0-byte file for persisted metadata. Although, the go-tuf updater logic and TUF framework is robust enough to recover from incomplete/corrupt persisted metadata.

edit: opened https://github.com/theupdateframework/go-tuf/issues/639 and moved summary there

trishankatdatadog commented 5 months ago

I am in favor of 2 but willing to refactor the PR to implement 1 if desired.

Excellent summary, thanks very much, @mrjoelkamp! I suspect you are right that this is a reliability but not a security issue, and so I don't mind Option 2.

@JustinCappos do you see any security issues? If not, we should be able to simplify the code.

JustinCappos commented 5 months ago

I'm more concerned about a file being verified, being overwritten, and then being re-read with the assumption it was verified. Could this occur?

mrjoelkamp commented 5 months ago

Excellent summary, thanks very much, @mrjoelkamp! I suspect you are right that this is a reliability but not a security issue, and so I don't mind Option 2.

I just looked to python-tuf for consistency between the two clients, which leads me to think that option 1 is probably best to keep them the same, see https://github.com/theupdateframework/go-tuf/issues/639#issuecomment-2163948854

I also think I found the culprit for the windows os.Rename failing, so this might be a pretty easy fix

mrjoelkamp commented 5 months ago

I'm more concerned about a file being verified, being overwritten, and then being re-read with the assumption it was verified. Could this occur?

No, whenever the metadata files are read from persisted storage in updater they are verified against the root to be "trusted" and the trusted metadata is kept in volatile memory.

JustinCappos commented 5 months ago

I'm more concerned about a file being verified, being overwritten, and then being re-read with the assumption it was verified. Could this occur?

No, whenever the metadata files are read from persisted storage in updater they are verified against the root to be "trusted" and the trusted metadata is kept in volatile memory.

Okay, and one other related question. Could one possibly use this to overwrite old, cached files in some way? (I don't see how, but wanted to ask.)

If not then I don't have any concerns...

mrjoelkamp commented 5 months ago

Okay, and one other related question. Could one possibly use this to overwrite old, cached files in some way? (I don't see how, but wanted to ask.)

If not then I don't have any concerns...

persistMetadata is only called here when the roles are updated from the remote server. From what I can tell, the role metadata has to pass verification before persistMetadata is called. No changes to the behavior here for that and it matches the python-tuf client implementation as well.

trishankatdatadog commented 5 months ago

This looks good to me. Do you think we need any regression test at least for this behaviour?

mrjoelkamp commented 5 months ago

This looks good to me. Do you think we need any regression test at least for this behaviour?

Before these changes I'd say yes, but now that this removes moveFile and sets the temp dir to the same as the local metadata dir we have test coverage in the existing tests.

trishankatdatadog commented 5 months ago

Before these changes I'd say yes, but now that this removes moveFile and sets the temp dir to the same as the local metadata dir we have test coverage in the existing tests.

Got it. I'll let @kommendorkapten or @rdimitrov approve this PR since they have more SITG than I do here 🙂