Skip to content

VerificationResult should include keys for keyids #2544

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

Closed
lukpueh opened this issue Jan 19, 2024 · 4 comments · Fixed by #2551
Closed

VerificationResult should include keys for keyids #2544

lukpueh opened this issue Jan 19, 2024 · 4 comments · Fixed by #2551

Comments

@lukpueh
Copy link
Member

lukpueh commented Jan 19, 2024

The get_verification_result method returns a VerificationResult, which contains two sets of TUF keyids.

Typical usage of a VerificationResult entails retrieving the Key objects for those keyids from the corresponding root metadata keystore. It would be useful, if VerificationResult included Key objects right away.

Options are:

  1. Use sets of Key objects - This requires making the Key class hashable and immutable, which is possible but maybe overly complicated (see discussion))
  2. Use lists of Key objects and promise that they only contain unique keys, and optionally provide tooling for set-like operations (see wrapper workaround)
  3. Use "keyid: Key"- dictionaries (see wrapper workaround)
@lukpueh
Copy link
Member Author

lukpueh commented Jan 19, 2024

IMO we should dismiss option 1. And I slightly lean towards option 3, because it doesn't require a promises about but actually provides a set-like behavior for the relevant fields.

If we use a dicts we could also remove the union method, which makes sense for two reasons:

  • @jku says, providing such a method is a bit fishy, because keyids aren't globally unique. (imo the problem exists regardless of the method, but if we remove the method, the responsibility shifts towards the user).
  • If we don't provide a union method, we could also include the threshold value with the VerificationResult, which is similarly helpful as including the keys in the result.

@lukpueh lukpueh added the good first issue Bite-sized items for first time contributors label Jan 19, 2024
@lukpueh
Copy link
Member Author

lukpueh commented Jan 31, 2024

There's another subtle issue with union:

On a single result, which isn't verified any valid signature from a key in unsigned will count towards the threshold. This is not the case, if I make a union of two results, and one of the results is verified but als has unsigned keys.

That said, it would be very nice to have an abstraction over the two verification results potentially needed for root, in order to reduce case handling in calling code.

@jku
Copy link
Member

jku commented Feb 1, 2024

So the new requirements/wishlist based on these implementation experiences and a short chat with Lukas:

  • include actual keys in verification result, not just keyids
  • include threshold in verification result
  • provide a simpler way to get root verification result
    • takes into account two roles
    • returns two verification results (but provides easy-to-use wrapper)

So possibly something like

class _DelegatorMixin():
    # this method signature is unchanged
    def get_verification_result(
        self,
        delegated_role: str,
        payload: bytes,
        signatures: Dict[str, Signature],
    ) -> VerificationResult

class Root():
    # new method to get a combined verification result from two roles (self and other) 
    def get_root_verification_result(
        self,
        other: Root,
        payload: bytes,
        signatures: Dict[str, Signature],
    ) -> RootVerificationResult
        # build a RootVerificationResult using self.get_verification_result()
        # and other.get_verification_result()

@dataclass
class VerificationResult:
    # New field threshold, modified type of signed and unsigned
    verified: bool
    threshold: int
    signed: Dict[str, Key]
    unsigned: Dict[str, Key]
 
    def __bool__(self) -> bool:
        return self.verified

@dataclass
class RootVerificationResult:
    # This dataclass operates like a VerificationResult except:
    # * No threshold
    # * you can access underlying VerificationResults if you need to

    results: tuple[VerificationResult, VerificationResult]

    def __bool__(self) -> bool:
        return self.verified

    @property
    def verified(self) -> bool:
        return self.results[0].verified and self.results[1].verified 
    
    @property
    def signed(self) -> dict[str, Key]:
        # return a "union" of both results[0].signed and results[1].signed

    @property
    def unsigned(self) -> dict[str, Key]:
        # return a "union" of both results[0].unsigned and results[1].unsigned

@lukpueh
Copy link
Member Author

lukpueh commented Feb 1, 2024

This look great. But we also need to take into account special case root v1, where RootVerificationResult has only one result.

@jku jku removed the good first issue Bite-sized items for first time contributors label Feb 1, 2024
@jku jku mentioned this issue Feb 1, 2024
3 tasks
@jku jku closed this as completed in #2551 Feb 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants