Skip to content

fix: configurable temp file directory#638

Merged
kommendorkapten merged 2 commits intotheupdateframework:masterfrom
mrjoelkamp:fix-configurable-working-dir
Jun 17, 2024
Merged

fix: configurable temp file directory#638
kommendorkapten merged 2 commits intotheupdateframework:masterfrom
mrjoelkamp:fix-configurable-working-dir

Conversation

@mrjoelkamp
Copy link
Copy Markdown
Contributor

@mrjoelkamp mrjoelkamp commented Jun 11, 2024 •
edited
Loading

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

@mrjoelkamp mrjoelkamp requested a review from a team as a code owner June 11, 2024 16:03
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Jun 11, 2024 •
edited
Loading

⚠️ 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 ⚠️

❗ 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.

@mrjoelkamp mrjoelkamp marked this pull request as draft June 11, 2024 16:59
@mrjoelkamp mrjoelkamp force-pushed the fix-configurable-working-dir branch 2 times, most recently from e7bb364 to 1a40475 Compare June 11, 2024 17:26
@mrjoelkamp mrjoelkamp changed the title fix: configurable temp file directory fix: read-only temp file directory Jun 11, 2024
@mrjoelkamp mrjoelkamp marked this pull request as ready for review June 11, 2024 17:31
@trishankatdatadog
Copy link
Copy Markdown
Contributor

trishankatdatadog commented Jun 11, 2024 •
edited
Loading

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
Copy link
Copy Markdown
Contributor Author

mrjoelkamp commented Jun 12, 2024 •
edited
Loading

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
Copy link
Copy Markdown
Contributor

trishankatdatadog commented Jun 12, 2024

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
Copy link
Copy Markdown
Member

JustinCappos commented Jun 12, 2024

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
Copy link
Copy Markdown
Contributor Author

mrjoelkamp commented Jun 12, 2024 •
edited
Loading

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
Copy link
Copy Markdown
Contributor Author

mrjoelkamp commented Jun 12, 2024

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.

@mrjoelkamp mrjoelkamp force-pushed the fix-configurable-working-dir branch from 1a40475 to 5eb28f2 Compare June 12, 2024 21:50
@mrjoelkamp
fix: windows rename error
bba12ec 
Signed-off-by: mrjoelkamp <[email protected]>
@mrjoelkamp mrjoelkamp force-pushed the fix-configurable-working-dir branch from 5eb28f2 to bba12ec Compare June 12, 2024 21:51
@mrjoelkamp mrjoelkamp mentioned this pull request Jun 12, 2024
Closed
@JustinCappos
Copy link
Copy Markdown
Member

JustinCappos commented Jun 12, 2024

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

trishankatdatadog
trishankatdatadog reviewed Jun 13, 2024
View reviewed changes
@mrjoelkamp
Copy link
Copy Markdown
Contributor Author

mrjoelkamp commented Jun 13, 2024

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.

@mrjoelkamp mrjoelkamp changed the title fix: read-only temp file directory fix: configurable temp file directory Jun 13, 2024
@trishankatdatadog
Copy link
Copy Markdown
Contributor

trishankatdatadog commented Jun 13, 2024

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

@mrjoelkamp
Copy link
Copy Markdown
Contributor Author

mrjoelkamp commented Jun 14, 2024

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
Copy link
Copy Markdown
Contributor

trishankatdatadog commented Jun 14, 2024

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
kommendorkapten approved these changes Jun 17, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@kommendorkapten kommendorkapten left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

rdimitrov
rdimitrov approved these changes Jun 17, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@rdimitrov rdimitrov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Copy Markdown
Member

kommendorkapten commented Jun 17, 2024

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.

@kommendorkapten kommendorkapten merged commit 4186614 into theupdateframework:master Jun 17, 2024
@rdimitrov
Copy link
Copy Markdown
Contributor

rdimitrov commented Jun 17, 2024

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

@mrjoelkamp
Copy link
Copy Markdown
Contributor Author

mrjoelkamp commented Jun 17, 2024

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.

@mrjoelkamp mrjoelkamp deleted the fix-configurable-working-dir branch June 17, 2024 19:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kommendorkapten kommendorkapten kommendorkapten approved these changes

@rdimitrov rdimitrov rdimitrov approved these changes

+2 more reviewers

@trishankatdatadog trishankatdatadog trishankatdatadog left review comments

@whalelines whalelines whalelines left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@mrjoelkamp @codecov-commenter @trishankatdatadog @JustinCappos @kommendorkapten @rdimitrov @whalelines