Improve verification results

[jku]

This is an attempt at fixing the verification result handling that has been found to not work well. This API is useful mostly to repository implementations: clients should likely keep using verify_delegate()

Modify VerificationResult

• Remove union(): this is now handled by the new RootVerificationResult
• include threshold: This is useful and can be added now that union() does not exist
• Use dict[str, Key] instead of set[str] for signed and unsigned
• Note that the (broken metadata) case of role keyid that does not have a matching key, is now not counted as "unsigned" as it was before: it's just not counted at all

Add RootVerificationResult

• Externally looks like VerificationResult (without threshold)
• It is actually two VerificationResults in a trenchcoat: Root.get_root_verification_result() is provided as a straightforward way to get one

Note that RootVerificationResult.signed and RootVerificationResult.unsigned assume that there are no keyid collisions between the two verifying roles (same keyid is not used for two different keys): This does not affect verified status in any way (only the signed and unsigned lists) so I think this is totally acceptable.

So far the usage looks quite nice but I think we want to try to use this in an app or two to avoid redoing the mistake of the first VerificationResult...

Fixes #2544

• 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

[jku]

Still wondering if it would be useful for the two result types to have a common base type, instead of being just duck typed to feel similar.

The way I see a signing tool or repository UI using these is this:

• getting verification results happens separately for root and other roles (because it has to)
• but printing the results should be possible with the same code (with maybe a small tweak for root case)

So maybe it is fine for the app to have something like

def describe_result(result: VerificationResult | RootVerificationResult) -> str:
    ...

I would maybe rather not create some complicated inheritance (and inevitably large classes) when the dataclasses look like they might just work like that...

[coveralls]

Pull Request Test Coverage Report for Build 7785217570

• 0 of 37 (100.0%) changed or added relevant lines in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.04%) to 97.847%

---

Totals
Change from base Build 7747121727:	0.04%
Covered Lines:	1382
Relevant Lines:	1405

---

💛 - Coveralls

[lukpueh]

This looks very helpful. Let me try it in RSTUF and see if it really is.

[lukpueh]

I tried this in RSTUF and found it very helpful.

I think adding a common base class would be okay. The inheritance model could look something like this.

class VerificationResult:
  verified: bool
  signed: dict[str, Key]
  unsigned: dict[str, Key]

class Single(VerificationResult):
  threshold: int

class Combined(VerificationResult):
  first: Single
  second: Single

But this does not add much value. The more interesting question is, if we want to provide other common behavior. Two things that I would have found useful:

• a list of missing signature infos:
  unique tuples of threshold - len(signed) and unsigned if verified is False per Single result

• keys that effectively decrease the missing signature count
  union of unsigned if verified is False per Single result

[jku]

The common base class probably looks more complicated: because Combined wants to override verified, signed and unsigned they probably would have to be properties in the parent class already.

[jku]

• a list of missing signature infos:
  unique tuples of threshold - len(signed) and unsigned if verified is False per Single result
• keys that effectively decrease the missing signature count
  union of unsigned if verified is False per Single result

I'm not completely sold on these because :

• this is only relevant for root and only when root signers change
• I think most repositories in practice would want to encourage everyone to sign (to make the signing events Proof-of-Ownership events at the same time)
• Handling multliple tuples does not seem any easier than handling result.first and result.second (or if you think it is we can just make those a tuple instead of two fields) so you could just handle the underlying VerificationResults yourself

We could add this to VerificationResult if it makes things easier:

@property
def missing(self) -> int:
    return max(0, self.threshold - len(self.signed))

[lukpueh]

Interesting. This means that in the new VerificationResult there might be role keyids that are neither in signed nor unsigned. That's probably okay. But should we mention it somewhere. Maybe in the PR description?

[jku]

Yeah it is a change -- I think I mentioned it in the first commit message , can add to PR decription.

I was also considering just raising at this point (because clearly the role is in a bad state)... but then that sounds like punishing the delegated role signing for delegator metadata error -- an error that can still leave the delegated role capable of signing.