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.

Sign up for GitHub

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

Jump to bottom

Implement filesystem abstraction #232

Merged
merged 6 commits into from
May 7, 2020
Merged

Implement filesystem abstraction #232

merged 6 commits into from
May 7, 2020

Conversation

joshuagl
Copy link
Collaborator

@joshuagl joshuagl commented Apr 17, 2020 •
edited
Loading

Note: this branch of securesystemslib doesn't currently work with theupdateframework/tuf – it's very much WIP.

It feels like the shape these changes are taking introduces a conflict between the desire to support abstract filesystem backends vs. reasonable feature enhancements that have been suggested such as #222. It's not yet clear to me how we will support both abstract filesystem backends and creating private key files with restricted privileges.

Fixes issue #: step one, two and four of theupdateframework/python-tuf#1009

Description of the changes being introduced by the pull request:

  • Add an abstract base class defining an interface for storage
  • Add a concrete implementation of the storage interface for accessing the local file system
  • Add abstract storage support to securesystemslib.hash.digest_filename()
  • Add abstract storage support to securesystemslib.util.[get_file_details|ensure_parent_dir|load_json_file]()

Please verify and check that the pull request fulfils the following
requirements:

  • The code follows the Code Style Guidelines
  • Tests have been added for the bug fix or new feature
  • Docs have been added for the bug fix or new feature

@coveralls
Copy link

coveralls commented Apr 17, 2020 •
edited
Loading

Coverage Status

Coverage increased (+0.2%) to 98.939% when pulling c60ef22 on joshuagl:joshuagl/abstract-filesystem into 62936ca on secure-systems-lab:master.

@joshuagl joshuagl force-pushed the joshuagl/abstract-filesystem branch 3 times, most recently from 011c2cb to 7f4b1ab Compare April 20, 2020 13:22
@joshuagl joshuagl mentioned this pull request Apr 21, 2020
Merged
3 tasks
@joshuagl joshuagl force-pushed the joshuagl/abstract-filesystem branch 2 times, most recently from c44e525 to 59e3ada Compare April 22, 2020 16:38
@joshuagl joshuagl changed the title RFC/WIP: implement filesystem abstraction Implement filesystem abstraction Apr 22, 2020
@joshuagl joshuagl marked this pull request as ready for review April 22, 2020 16:47
This was referenced Apr 23, 2020
Merged
Closed
mnm678
mnm678 reviewed Apr 23, 2020
View reviewed changes
Copy link
Contributor

@mnm678 mnm678 left a comment

Choose a reason for hiding this comment

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

This looks good to me, but it would be nice to get a review from a non-TUF ssl user as well.

Regarding #222, would it be possible to require this as part of the abstract class? There could be a put_secure method that uses the permissions you would want for a key file.

securesystemslib/storage.py Show resolved Hide resolved
securesystemslib/util.py Show resolved Hide resolved
@joshuagl
Copy link
Collaborator Author

Thanks for the review @mnm678

This looks good to me, but it would be nice to get a review from a non-TUF ssl user as well.

Agreed. @SantiagoTorres, would you be able to take a look?

Regarding #222, would it be possible to require this as part of the abstract class? There could be a put_secure method that uses the permissions you would want for a key file.

That's an interesting suggestion. @sechkova proposed similar when we discussed #222 offline.
What I don't know is, if we added a put_secure() or similar to StorageBackendInterface, would that make sense on systems like S3 and GCS?
How would it be implemented? Would they store the permission bits as metadata on the file and apply it during get()?

Perhaps @woodruffw can help us understand how this might work on S3 and similar storage?

@joshuagl
Copy link
Collaborator Author

I just tested this branch with the develop branch of in-toto and the tests run without issue.

@joshuagl joshuagl force-pushed the joshuagl/abstract-filesystem branch from 59e3ada to 67a8931 Compare April 30, 2020 21:41
@joshuagl
Define an interface for storage access
0ab77c4 
Implement an abstract base class (ABC) which defined an abstract interface
for storage operations, regardless of backend.

The aim is to enable securesystemslib functions to operate as normal on
local filesystems by implementing the interface for local filesystem
operations within securesystemslib, with users of the library able to
provide implementations for use with their storage backend of choice when
this is not a local filesystem, i.e. S3 buckets as used by Warehouse for
the PEP 458 implementation.

For more context see tuf issue #1009:
theupdateframework/python-tuf#1009

Signed-off-by: Joshua Lock <[email protected]>
@joshuagl joshuagl force-pushed the joshuagl/abstract-filesystem branch from 67a8931 to 4fcbd73 Compare May 4, 2020 22:00
@joshuagl joshuagl requested a review from SantiagoTorres May 4, 2020 22:21
@joshuagl
Copy link
Collaborator Author

joshuagl commented May 4, 2020

I've updated the PR here to note the get() function of the interface should be a context manager and updated the FilesystemBackend implementation to return a ContextManager from get().

I've also addressed other comments from @mnm678 and @woodruffw. I'd very much appreciate some re-review.

mnm678
mnm678 approved these changes May 5, 2020
View reviewed changes
Copy link
Contributor

@mnm678 mnm678 left a comment

Choose a reason for hiding this comment

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

LGTM The context manager makes this much cleaner.

woodruffw
woodruffw reviewed May 5, 2020
View reviewed changes
securesystemslib/storage.py
"""

class GetFile(object):
# Implementing get() as a function with the @contextmanager decorator
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for this explanation!

woodruffw
woodruffw approved these changes May 5, 2020
View reviewed changes
Copy link
Contributor

@woodruffw woodruffw left a comment

Choose a reason for hiding this comment

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

Looks great!

joshuagl added 5 commits May 5, 2020 21:59
@joshuagl
Local filesystem impl of storage interface
5ad9f26 
Add FilesystemBackend implementing StorageBackendInterface using Python
standard library functions to perform operations on local files and
filesystems.

Signed-off-by: Joshua Lock <[email protected]>
@joshuagl
Add abstract storage support to digest_filename()
d409796 
Add an optional 'storage_backend' argument to digest_filename() which
expects an object implementing
securesystemslib.storage.StorageBackendInterface and defaults to None

Signed-off-by: Joshua Lock <[email protected]>
@joshuagl
Add StorageBackendInterface support to util funcs
8deb29f 
modify get_file_details(), ensure_parent_dir(),  load_json_file() and
persist_temp_file() to take an optional storage_backend argument which
defaults to None.

If no storage_backend is defined the functions will instantiate a
FilesystemBackend and use that, otherwise the argument expects an object
implementing securesystemslib.storage.StorageBackendInterface.

persist_temp_file() now takes an additional should_close parameter, which
defaults to True, indicating whether the persisted tempfile should be closed.

This is to better support scenarios where the same temporary file might need
to be persisted/put to storage multiple times under different names, such as
in the case of python-tuf metadata written with consistent snapshots.

Signed-off-by: Joshua Lock <[email protected]>
@joshuagl
Add StorageBackendInterface to interface funcs
32f3394 
Add storage_backend as an optional parameter to functions in the interface
module which read files from the disk. When no argument is provided and
the default value of None is used, instantiate a FilesystemBackend and use
that to access files on the local filesystem.

Signed-off-by: Joshua Lock <[email protected]>
@joshuagl
Mark uncovered path in interface
c60ef22 
Improve the coverage numbers by marking an untested path as
not having coverage.

Signed-off-by: Joshua Lock <[email protected]>
@joshuagl joshuagl force-pushed the joshuagl/abstract-filesystem branch from 4fcbd73 to c60ef22 Compare May 5, 2020 21:00
@woodruffw woodruffw mentioned this pull request May 6, 2020
Closed
SantiagoTorres
SantiagoTorres approved these changes May 7, 2020
View reviewed changes
Copy link
Collaborator

@SantiagoTorres SantiagoTorres left a comment

Choose a reason for hiding this comment

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

LGTM!

@SantiagoTorres SantiagoTorres merged commit 01a0c95 into secure-systems-lab:master May 7, 2020
@joshuagl joshuagl deleted the joshuagl/abstract-filesystem branch May 11, 2020 09:09
@joshuagl joshuagl mentioned this pull request May 12, 2020
Merged
@lukpueh lukpueh mentioned this pull request Jan 21, 2022
Closed
@di di mentioned this pull request Feb 1, 2022
Open
52 tasks
@woodruffw woodruffw mentioned this pull request Jul 18, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mnm678 mnm678 mnm678 approved these changes

@SantiagoTorres SantiagoTorres SantiagoTorres approved these changes

@woodruffw woodruffw woodruffw approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@joshuagl @coveralls @woodruffw @SantiagoTorres @mnm678