Move verify_delegate() to Root/Targets #2378

jku · 2023-05-05T07:39:41Z

Description of the changes being introduced by the pull request:

Add an internal _Delegator class that implements verify_delegate()
Inherit that class in Root, Targets: the internal class is just an implementation detail for sharing the implementation, not something users of the library should care about
Document Metadata.verify_delegate() as deprecated (and use the new implementation internally)
Use the new method in tests, examples and ngclient

This will make both ngclient and external repository code slightly easier to read and write (but note that the ngclient cleanups are not included here as they touch quite a lot of lines even if they are simple).

There are several ways to add verify_delegate() to Root and Targets, I think this helper class is a decent compromise between modularity and simplicity:

the _Delegator class isn't intended to be part of a longer "inheritance path" (and does not have a matching element in the metadata format): it just provides an extra method for Root and Targets. If there's a naming style for this style of inheritance I'm happy to use it
_Delegator is internal: users of the library don't need to care about it

Please verify and check that the pull request fulfills 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 · 2023-05-05T07:43:07Z

Pull Request Test Coverage Report for Build 4892122297

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

42 of 46 (91.3%) changed or added relevant lines in 2 files are covered.
6 unchanged lines in 1 file lost coverage.
Overall coverage decreased (-0.09%) to 97.604%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
tuf/api/metadata.py	31	35	88.57%

Files with Coverage Reduction	New Missed Lines	%
tuf/api/metadata.py	6	97.53%

Totals
Change from base Build 4891622435:	-0.09%
Covered Lines:	1328
Relevant Lines:	1353

💛 - Coveralls

jku · 2023-05-05T09:56:36Z

If there's a naming style for this style of inheritance I'm happy to use it

A "Mixin" is likely what I'm trying to remember here?

jku · 2023-05-10T07:17:22Z

I think this is ready for review. I have a branch cleanup-after-verify-delegate-move with changes based on this PR... but they are not required so I'll do those separately.

jku · 2023-05-10T07:21:21Z

tuf/api/metadata.py

@@ -1926,7 +1962,7 @@ def get_delegated_role(self, delegated_role: str) -> Role:
    def get_key(self, keyid: str) -> Key:
        """Return the key object for the given keyid.

-        Raises ValueError if keyid is not found.
+        Raises ValueError if key is not found.


this change just makes the two implementations have the same docstring.

You could actually remove docstrings for get_key and get_delegated_role in Targets and Root, since they can be reused from their definitions in the parent class. Pylint would need to be told to not complain. So maybe not worth it, the docstrings are short enough.

I was sure that wouldn't work with sphinx but I tried and you are right.

jku · 2023-05-10T07:57:10Z

tuf/api/metadata.py

+    def verify_delegate(
+        self,
+        delegated_role: str,
+        delegated_metadata: "Metadata",
+        signed_serializer: Optional[SignedSerializer] = None,
+    ) -> None:


Maybe a meaningful note here is that this makes Targets and Root "aware" of Metadata now.

The alternative would be for the arguments to include payload bytes and signatures so something like

def verify_delegate( self, delegated_role: str, payload: bytes, signatures: Dict[str, Signature] )

Unfortunately I'm not sure how one would make that usable in calling code

lukpueh

This change makes sense. I pointed out some nits that you may ignore. The only thing that should be changed is test_api.TestMetadata.test_metadata_verify_delegate, which still uses Metadata.verify_delegate. Otherwise, the tests pass even without that method.

Also, I think the version number in the deprecation notice is wrong (see inline comment).

tuf/api/metadata.py

lukpueh · 2023-05-12T08:44:07Z

tuf/api/metadata.py

@@ -1926,7 +1962,7 @@ def get_delegated_role(self, delegated_role: str) -> Role:
    def get_key(self, keyid: str) -> Key:
        """Return the key object for the given keyid.

-        Raises ValueError if keyid is not found.
+        Raises ValueError if key is not found.


You could actually remove docstrings for get_key and get_delegated_role in Targets and Root, since they can be reused from their definitions in the parent class. Pylint would need to be told to not complain. So maybe not worth it, the docstrings are short enough.

tests/test_api.py

jku · 2023-05-15T08:12:44Z

test_api.TestMetadata.test_metadata_verify_delegate, which still uses Metadata.verify_delegate

I left a small test in place to make sure I didn't break that one -- just enough testing to make sure it's connected to the actual implementation in Signed.

test_signed_verify_delegate() contains all of the tests that existed before.

jku · 2023-05-15T08:28:56Z

lint: commands[1]> isort --check --diff tuf examples tests verify_release
ERROR: /home/runner/work/python-tuf/python-tuf/verify_release Imports are incorrectly sorted and/or formatted.
--- /home/runner/work/python-tuf/python-tuf/verify_release:before	2023-05-15 08:14:43.101068
+++ /home/runner/work/python-tuf/python-tuf/verify_release:after	2023-05-15 08:15:05.396047
@@ -19,9 +19,8 @@
 from typing import Optional
 
 try:
+    import build as _
     import requests
-
-    import build as _
 except ImportError:
     print("Error: verify_release requires modules 'requests' and 'build':")
     print("    pip install requests build")
lint: exit 1 (0.27 seconds) /home/runner/work/python-tuf/python-tuf> isort --check --diff tuf examples tests verify_release pid=1912

The code that is failing on CI is exactly the code that my isort is suggesting I use.

Same exact isort version

(╯°□°)╯︵ ┻━┻

jku · 2023-05-15T08:36:52Z

I've dropped the commit that would make lint work on my machine... CI doesn't seems to mind 🤷

Let's maybe not merge this yet though: I still feel like we might be designing ourselves into a corner wrt DSSE.

Marking draft while thinking about it...

This makes logical sense and makes a lot of code using verify_delegate() a little easier since there is no need to keep a reference to the containing metadata anymore. The implementation is in practice in a new class but that's an implementation detail that allows sharing between Targets and Root. Signed-off-by: Jussi Kukkonen <jkukkonen@google.com>

Avoid Metadata.verify_delegate() now that it's deprecated. Note that this commit does not try to make any code cleanups that are now possible: this is the minimal change to use the new API. Future improvements can make code in TrustedMetadataSet and Updater slightly easier to read: as an example there's no need for TrustedMetadataSet to actually store or expose actual Metadata in its cache -- Signed is all that's needed. Signed-off-by: Jussi Kukkonen <jkukkonen@google.com>

Make it clearer that this is not part of the main inheritance path. Signed-off-by: Jussi Kukkonen <jkukkonen@google.com>

Signed-off-by: Jussi Kukkonen <jkukkonen@google.com>

Remove duplicate docstrings: these are already documented in _DelegatorMixin and sphinx will find them there. Tweak a few other strings to remove duplication in the sentence. Signed-off-by: Jussi Kukkonen <jkukkonen@google.com> Metadata API: Improve dosctrings Signed-off-by: Jussi Kukkonen <jkukkonen@google.com>

Signed-off-by: Jussi Kukkonen <jkukkonen@google.com>

- Add shortcut to canonical json representation of self.signed - Use in tests and Metadata.sign - Do not use in _Delegator.verify_delegate (will be updated in subsequent commit). Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>

Change new _Delegator.verify_delegate to take payload bytes and signatures instead of a Metadata object and a payload serializer. This allows using verify_delegate for payloads that do not come in a Metadata container, but e.g. in a DSSE envelope (see theupdateframework#2385). Usage becomes a bit more cumbersome, but still feels reasonable with the recently added shortcut for default canonical bytes representation of Metadata.signed. Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>

lukpueh · 2023-08-01T14:16:35Z

The alternative would be for the arguments to include payload bytes and signatures so something like
def verify_delegate(
    self,
    delegated_role: str,
    payload: bytes,
    signatures: Dict[str, Signature]
)
Unfortunately I'm not sure how one would make that usable in calling code

I have a patch for this on top of your (rebased) branch. It also adds an alias for default canonical signed bytes to Metadata. This makes the calling code only slightly more complicated: 635a287...lukpueh:move-verify-delegate-v2

I think this is a reasonable path towards #2385. @jku, feel free to take over my branch for this PR, or let me know if you want me to force push.

lukpueh · 2023-08-16T09:52:25Z

@jku, you may want to approve this pr too, now that it's a collaboration.

jku · 2023-08-21T11:08:53Z

It looks like original author cannot click "approve"... but I think this looks correct and reasonable.

jku force-pushed the move-verify-delegate-v2 branch from ec09778 to 4c7a08d Compare May 5, 2023 09:51

jku mentioned this pull request May 5, 2023

New release #2382

Closed

lukpueh mentioned this pull request May 8, 2023

Support DSSE in client and metadata API. #2385

Closed

jku marked this pull request as ready for review May 10, 2023 07:07

jku commented May 10, 2023

View reviewed changes

lukpueh requested changes May 12, 2023

View reviewed changes

jku force-pushed the move-verify-delegate-v2 branch from ba14ec0 to 8b7c2df Compare May 15, 2023 08:33

jku marked this pull request as draft May 15, 2023 08:35

jku and others added 9 commits August 1, 2023 13:08

metadata: Rename _Delegator to _DelegatorMixin

2ace345

Make it clearer that this is not part of the main inheritance path. Signed-off-by: Jussi Kukkonen <jkukkonen@google.com>

Metadata API: Annotation syntax tweak

0184edc

Signed-off-by: Jussi Kukkonen <jkukkonen@google.com>

tests: Move lint disable to inside block as intended

e51c0be

Signed-off-by: Jussi Kukkonen <jkukkonen@google.com>

Metadata API: Bump deprecation version to next likely candidate

635a287

Signed-off-by: Jussi Kukkonen <jkukkonen@google.com>

Metadata API: add Metadata.signed_bytes property

fc6c91a

- Add shortcut to canonical json representation of self.signed - Use in tests and Metadata.sign - Do not use in _Delegator.verify_delegate (will be updated in subsequent commit). Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>

lukpueh mentioned this pull request Aug 9, 2023

Add basic DSSE equivalent for Metadata API and configurable DSSE support in ngclient #2436

Merged

lukpueh force-pushed the move-verify-delegate-v2 branch from 8b7c2df to 15dd931 Compare August 16, 2023 09:50

lukpueh approved these changes Aug 16, 2023

View reviewed changes

jku marked this pull request as ready for review August 21, 2023 11:10

lukpueh merged commit a871f64 into theupdateframework:develop Aug 21, 2023

jku deleted the move-verify-delegate-v2 branch December 30, 2024 09:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move verify_delegate() to Root/Targets #2378

Move verify_delegate() to Root/Targets #2378

jku commented May 5, 2023 •

edited

Loading

coveralls commented May 5, 2023 •

edited

Loading

jku commented May 5, 2023

jku commented May 10, 2023

jku May 10, 2023

lukpueh May 12, 2023

jku May 12, 2023

jku May 10, 2023

lukpueh left a comment

lukpueh May 12, 2023

jku commented May 15, 2023

jku commented May 15, 2023 •

edited

Loading

jku commented May 15, 2023

lukpueh commented Aug 1, 2023

lukpueh commented Aug 16, 2023

jku commented Aug 21, 2023

Move verify_delegate() to Root/Targets #2378

Move verify_delegate() to Root/Targets #2378

Conversation

jku commented May 5, 2023 • edited Loading

coveralls commented May 5, 2023 • edited Loading

Pull Request Test Coverage Report for Build 4892122297

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

jku commented May 5, 2023

jku commented May 10, 2023

jku May 10, 2023

Choose a reason for hiding this comment

lukpueh May 12, 2023

Choose a reason for hiding this comment

jku May 12, 2023

Choose a reason for hiding this comment

jku May 10, 2023

Choose a reason for hiding this comment

lukpueh left a comment

Choose a reason for hiding this comment

lukpueh May 12, 2023

Choose a reason for hiding this comment

jku commented May 15, 2023

jku commented May 15, 2023 • edited Loading

jku commented May 15, 2023

lukpueh commented Aug 1, 2023

lukpueh commented Aug 16, 2023

jku commented Aug 21, 2023

jku commented May 5, 2023 •

edited

Loading

coveralls commented May 5, 2023 •

edited

Loading

jku commented May 15, 2023 •

edited

Loading