Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ngclient: throws securesystemslib errors #1761

Closed
jku opened this issue Jan 10, 2022 · 5 comments · Fixed by #1799
Closed

ngclient: throws securesystemslib errors #1761

jku opened this issue Jan 10, 2022 · 5 comments · Fixed by #1799
Assignees
Labels
1.0.0 blockers To be addressed before 1.0.0 release bug ngclient
Milestone

Comments

@jku
Copy link
Member

jku commented Jan 10, 2022

If ngclient fails to write a target file to disk, we let securesystemslib.exceptions.StorageError propagate.

This seems like a bug: we should either handle this error, or possibly not use the securesystemslib.util.persist_temp_file() at all (it's not super useful to us as we're not using the storage abstraction and the rest is a few lines of code that actually hides the error we'd really like to get: OSError which we already use for metadata files in the same situation).

@jku
Copy link
Member Author

jku commented Jan 10, 2022

also I think the persist_temp_file() is partly cargo cult at least for the purposes of the client: it does three things: write, flush, fsync. I have tried to imagine a case where we actually did need to flush and fsync and cannot think of one -- when TUF is being used correctly. I think a simple shutil.copyfileobj() probably does the job.

What I mean with when TUF is being used correctly:

  • Target file write failing (even half way through in a power outage) is not a major issue. When the client intends to use a target file it will first validate it with TUF. A corrupt file will not validate and will have to be re-downloaded.
  • This is in contrast with metadata write failing mid way through: in the case of root metadata this can be a catastrophic failure that prevents all future updates. This is what ngclient tries to avoid with metadata persistence using atomic-replace.

@jku jku added the 1.0.0 blockers To be addressed before 1.0.0 release label Jan 12, 2022
@MVrachev MVrachev self-assigned this Jan 12, 2022
@MVrachev MVrachev added this to the Sprint 15 milestone Jan 12, 2022
@MVrachev
Copy link
Collaborator

This is in contrast with metadata write failing mid way through: in the case of root metadata this can be a catastrophic failure that prevents all future updates. This is what ngclient tries to avoid with metadata persistence using atomic-replace.

That sounds bad and we should make sure we do atomic operations.
Does that mean this is an argument for using securesystemslib.util.persist_temp_file()?
I didn't understand what you tried to support.

@jku
Copy link
Member Author

jku commented Jan 21, 2022

in context of this issue and target persistence: I wanted to say that it seems like it's not actually critical how target files are written to disk. A plain shutil.copyfileobj() instead of persist_temp_file() seems fine to me (and I'll always prefer simplicity...).

If someone has a counter argument to that I'd be interested in hearing it.

@lukpueh
Copy link
Member

lukpueh commented Jan 21, 2022

cc @joshuagl, author of secure-systems-lab/securesystemslib#181 and secure-systems-lab/securesystemslib#232

@joshuagl
Copy link
Member

joshuagl commented Jan 21, 2022

Filesystem abstraction was implemented for repository related code, I agree with the assertion that this seems unnecessary in the context of the client. Simpler use of standard file move methods seems like a reasonable decision.

Separately, it would be worth investigating whether the write/flush/fsync in securesystemslib is even necessary for the modern Python versions we support (it was copy/pasted from old code and written when we were supporting Python 2.7). The Python standard library is constantly evolving and, for example, since Python 3.8 a file copy may happen entirely within the OS kernel (avoiding user space buffers).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.0.0 blockers To be addressed before 1.0.0 release bug ngclient
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants