fix: configurable temp file directory

[mrjoelkamp]

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]

⚠️ Please install the 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 ⚠️

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

Additional details and impacted files

@@            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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[trishankatdatadog]

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]

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 #639 and moved summary there

[trishankatdatadog]

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]

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]

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 #639 (comment)

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

[mrjoelkamp]

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]

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]

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]

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

[mrjoelkamp]

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]

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 🙂

[kommendorkapten]

This is awesome, very pleased to se that we are now back to using os.Rename. Thank you so much for this.

[rdimitrov]

Looks good! 🚀

I have to check the code again but for future work I'm thinking that we can get this a step further and use memfs so we don't rely on having any disk permissions at all.

In any case thank you for addressing this @mrjoelkamp 🙏

[kommendorkapten]

Just to clarify, this already exists today: https://github.com/theupdateframework/go-tuf/blob/master/metadata/config/config.go#L42

DisableLocalCache allows for operating on a read only filesystem.

[rdimitrov]

Funny that I added it and then forgot about its existence 😃

[mrjoelkamp]

Funny that I added it and then forgot about its existence 😃

😄 no problem! I did see that option, however we do need cache enabled for performance.