Transition to the newest version of TUF #561

renatav · 2024-10-29T02:24:12Z

Description (e.g. "Related to ...", etc.)

Closes #274

Also implemented/addressed:
Closes #501
Closes #555
Closes #560

Code review checklist (for code reviewer to complete)

Pull request represents a single change (i.e. not fixing disparate/unrelated things in a single PR)
Title summarizes what is changing
Commit messages are meaningful (see this for details)
Tests have been included and/or updated, as appropriate
Docstrings have been included and/or updated, as appropriate
Changelog has been updated, as needed (see CHANGELOG.md)

Remove unused pyopenssl Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>

Implements basic primitives, defined by the python-tuf Repository abstraction, to read and edit metadata on disk, handling version and expiry bumps, and signature creation, and facilitating snapshot and timestamp creation. And adds exemplary API methods that use these primitives while preserving consistent repo states: - create - add_target_files - add_keys Can be tested with: ``` PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 pytest --noconftest taf/tests/tuf/ ``` More detailed usage docs + migration path TBD... Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>

The original design aimed at separating the concepts of delegation (adding public keys) and signing (using private keys). Since the MetadataRepository assumes that metadata can be signed rightaway after edit (e.g. after having added a delegation), which in turn requires private keys to be available, we might as well conflate these two concepts. The advantage is that the signer cache does not have to be managed independently and is more likely to stay in sync with the delegations. Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>

Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>

This should really happen upstream (see linked issue) Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>

YkSigner provides a minimal compatibility layer over `taf.yubikey` module functions for use with MetadataRepository. Even though a yubikey signer implementation (HSMSigner) based on pykcs11 is available in securesystemslib, YkSigner was added for the following reasons: - TAF requires rsa support for yubikeys, but HSMSigner only supports ecdsa. Adding rsa support to HSMSigner, or providing a custom pykcs11-based RSAHSMSigner is feasible, and seems desirable, but requires more effort than this YkSigner did. - TAF provides a few additional features, like setting up a Yubikey, changing pins, etc., which will not be added to securesystemslib. This means the current Yubikey infrastructure based on yubikey-manager needs to be preserved for the time being. Thus it made sense to re-use the existing implementation for YkSigner. - YkSigner show-cases the new Signer API and might be used as blue print for future Signer implementations in TAF. This commit adds basic tests with fake and real Yubikey: ``` REAL_YK=1 PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 \ pytest --noconftest taf/tests/tuf/ taf/tests/tuf/test_yk.py -s ``` Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>

This allows running previously added YkSigner tests, but breaks other things, which need change anyway in the course of upgrading to latest tuf/securesystemslib. Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>

Add alternative TUF metadata repo implementation (WIP)

…gated role lookup

…n dates to the new repository class

…y class

…ort for delegations

…he new tuf

…ass and added tests

…ate test

n-dusan

You've done a really great job with this PR. The amount of organization and work are impressive!

As for next steps, I'll be reviewing the tests and testing framework in more depth with @sale3. For now, I've left a handful of mostly non-substantial comments that you can take a look at.

n-dusan · 2024-12-16T14:11:20Z

taf/tuf/repository.py

+                            are many delegations.
+
+        Returns:
+        Defined delegated paths of delegate target role or * in case of targets


question: why is '*' is returned in case of targets? adding a sentence here would be useful

The main targets role is simply a catch-all. This is an old string, but I can update it

n-dusan · 2024-12-16T14:25:09Z

taf/tuf/keys.py

+def create_signer(priv, pub):
+    return CryptoSigner(priv, _from_crypto(pub))


suggestion: adding a docstring would be useful here

This is not even used, I'll remove it

n-dusan · 2024-12-16T15:08:06Z

taf/tuf/repository.py

+    def get_keyids_of_role(self, role_name):
+        role_obj = self._role_obj(role_name)
+        return role_obj.keyids
+
+    def get_delegations_of_role(self, role_name):
+        signed_obj = self._signed_obj(role_name)
+        return signed_obj.delegations.roles


suggestion: adding docstrings for all methods in MetadataRepository class that don't already have it would be useful

n-dusan · 2024-12-16T15:25:11Z

taf/tests/tuf/test_create_edit_repo/conftest.py

+    # Convert to string if necessary, or use it as a Path object
+    return full_path


question(nitpick): Is this inline comment correct? it doesn't seem to convert as the comment says it should?

n-dusan · 2024-12-16T15:52:02Z

taf/tests/tuf/test_query_repo/conftest.py

+    custom1 =  {"custom_attr1": "custom_val1"}
+    custom2 =  {"custom_attr2": "custom_val2"}
+
+    "delegated role's targets"


suggestion: are these strings supposed to be comments?

n-dusan · 2024-12-17T17:11:26Z

taf/tuf/storage.py

+    commit = None
+
+    @contextmanager
+    def get(self, filepath: str):


suggestion: docstring for GitStorageBackend methods would be helpful

n-dusan · 2024-12-17T17:13:59Z

taf/tuf/storage.py

+    def __new__(cls, *args, **kwargs):
+        return super(FilesystemBackend, cls).__new__(cls, *args, **kwargs)  # Bypass singleton
+


suggestion: expanding on the reason why we needed to bypass the singleton as a docstring would be very much useful

n-dusan · 2024-12-17T17:43:44Z

taf/api/api_workflow.py

+@contextmanager
+def transactional_execution(auth_repo):
+    initial_commit = auth_repo.head_commit_sha()
+    try:
+        yield
+    except PushFailedError:
+        pass
+    except Exception:
+        auth_repo.reset_to_commit(initial_commit, hard=True)
+        raise


this function makes a lot of sense!

question: would moving it to auth_repo.py be a better place for it? I could see us wanting to use it in platform at some point

There is not reason why we wouldn't want to import anything from the api package imo

n-dusan · 2024-12-17T17:48:38Z

docs/developers/repository-classes.md

+files are read and written. To instantiate a `MetadataRepository` class with a custom storage interface, use the
+`storage` keyword argument. If not specified, TUF's default `FilesystemBackend` will be used.
+
+This class is used extensively to implement API functions.



suggestion: adding an overview of GitStorageBackend here seems like it would be useful as well.

n-dusan · 2024-12-17T17:49:48Z

docs/developers/repository-classes.md

-inside the `targets` folder are tracked and secured by TUF.
+This class is derived from `GitRepository`, and indirectly from `MetadataRepository`. Authentication repositories are
+expected to contain TUF metadata and target files, but are also Git repositories. It is important to note that only
+files inside the `targets` folder are tracked and secured by TUF.


nitpick: secured by TUF -> secured by both TAF and TUF?

To me, just TUF makes more sense. Storing hashes and lengths of target files in signed metadata files is what TUF does.

lukpueh and others added 30 commits August 28, 2024 12:06

Update tuf, securesystemslib and cryptography deps

60b8fa9

Remove unused pyopenssl Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>

Make sure targets is signed on add key

b46645b

Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>

Assert keytype rsa in taf.tuf.keys helper

33750eb

This should really happen upstream (see linked issue) Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>

Comment out legacy imports (WIP)

25371d7

This allows running previously added YkSigner tests, but breaks other things, which need change anyway in the course of upgrading to latest tuf/securesystemslib. Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>

Merge branch 'feature/tuf-repositoty' into tuf-upgrade

ec294a8

Merge pull request #512 from lukpueh/tuf-upgrade

70927b7

Add alternative TUF metadata repo implementation (WIP)

feat: moved get threshold to the new repository class, implement dele…

9bf3fb9

…gated role lookup

refact: move get expiration date, get all targets and check expiratio…

c7be3cb

…n dates to the new repository class

refact: move get role paths and all target files to the new repositor…

7601b9b

…y class

refact: work on making the create repository method more flexible

b8d247e

refact: extend creation of repositories using the new TUF, added supp…

ade9af1

…ort for delegations

test: add create repository with delegations test

f1c1b6c

test: use repository created using create in tests

ea05769

test, refact: reorganize tests, implement keyid-roles mapping using t…

c9857d0

…he new tuf

test: re-enabled add target test

b7eb34a

refact: moved modify targets to the new repository class

ccb94b8

test: create test repos with target files and custom data

43af588

refac, test: move get_all_target_files_state to the new repository cl…

feb7b53

…ass and added tests

refact: move generate_roles_description to the new repository class

3202900

test, refact: minor tests refactoring, reimplement is_valid_metadata_key

af59138

refact: test: reimplement add metadata keys, enable keys tests

51d63c9

feat, test: implement revoke key

e57b8e3

test, fix: minor add and revoke key improvements

abeaa2c

refact, test: initial work on reworking signing, add set expiration d…

f3bc1f7

…ate test

refact: remeve outdated imports

2681d4a

refact: check and set expiration date reimplemented

0d67dd5

refact: update key generation

ba7d3eb

renatav force-pushed the feature/tuf-repositoty branch from ebd033a to 3000095 Compare November 29, 2024 20:55

renatav added 3 commits November 29, 2024 18:01

refact: update yubikey cli functions

65c128b

test: refact yubikey tests

4805128

refact: refactor creation of repositories using yubikeys

789b591

renatav force-pushed the feature/tuf-repositoty branch from 61919f0 to 385a0be Compare November 30, 2024 04:05

chore: formatting and mypy fixes

ff146df

renatav force-pushed the feature/tuf-repositoty branch from 385a0be to ff146df Compare November 30, 2024 04:07

test: add pytest-mock to test requirements

e402196

renatav changed the base branch from dev to master November 30, 2024 04:19

Merge branch 'master' into feature/tuf-repositoty

de1bd82

renatav changed the title ~~Feature/tuf repositoty~~ Transition to the newest version of TUF Nov 30, 2024

renatav self-assigned this Nov 30, 2024

renatav added 2 commits November 29, 2024 23:38

chore: update changelog

da5344d

chore: merge master

2f16959

renatav marked this pull request as ready for review November 30, 2024 04:50

renatav requested review from n-dusan and sale3 November 30, 2024 04:50

renatav added 2 commits December 4, 2024 00:49

test: tun tests without having ykman installed

f22da80

chore: ignore unused import

7db3588

renatav force-pushed the feature/tuf-repositoty branch 2 times, most recently from 6ee2279 to ada9ead Compare December 4, 2024 20:52

docs: GitStorageBackend docstring

6410f52

renatav force-pushed the feature/tuf-repositoty branch from ada9ead to 6410f52 Compare December 4, 2024 21:12

renatav added 4 commits December 9, 2024 17:42

feat: add a command for rotating keys

4ab8c48

test: add test revoke signing key test

dc431c5

docs: update docs related to repository classes

5ea21e7

chore: merge master and resolve conflicts

bef7f8e

n-dusan requested changes Dec 17, 2024

View reviewed changes

renatav added 2 commits December 19, 2024 02:05

docs: add/update a number of metadata repository docstrings

4490e9b

docs: add docstrings to metadata repository

81e0799

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Transition to the newest version of TUF #561

Transition to the newest version of TUF #561

renatav commented Oct 29, 2024 •

edited

Loading

n-dusan left a comment

n-dusan Dec 16, 2024

renatav Dec 18, 2024

n-dusan Dec 16, 2024

renatav Dec 18, 2024

n-dusan Dec 16, 2024

n-dusan Dec 16, 2024

n-dusan Dec 16, 2024

n-dusan Dec 17, 2024

n-dusan Dec 17, 2024

n-dusan Dec 17, 2024

renatav Dec 19, 2024

n-dusan Dec 17, 2024

n-dusan Dec 17, 2024

renatav Dec 19, 2024

		def create_signer(priv, pub):
		return CryptoSigner(priv, _from_crypto(pub))

		# Convert to string if necessary, or use it as a Path object
		return full_path

		def __new__(cls, args, *kwargs):
		return super(FilesystemBackend, cls).__new__(cls, args, *kwargs) # Bypass singleton

Transition to the newest version of TUF #561

Are you sure you want to change the base?

Transition to the newest version of TUF #561

Conversation

renatav commented Oct 29, 2024 • edited Loading

Description (e.g. "Related to ...", etc.)

Code review checklist (for code reviewer to complete)

n-dusan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

renatav commented Oct 29, 2024 •

edited

Loading