Use cases:

• RSTUF: Task: Improve _validate_threshold, so it tells how many more signatures are required repository-service-tuf/repository-service-tuf-worker#367 (cc @MVrachev, @kairoaraujo)
• tuf-on-ci: ?? (cc @jku)

For tuf-on-ci use case I want:

• list of keys that have signed correctly
• list of keys that have not signed (I don't care if the sig is missing or incorrect)
• has threshold been reached

Things that I don't care that much about:

• the actual signatures (easy to lookup with keys if needed)
• threshold (easy to lookup)
• number of good sigs (easy to calculate)

so from my POV something like this:

@dataclass
class SigningStatus:
    verified: bool
    signed_keys: list[Key]
    unsigned_keys: list[Key]

def delegate_signing_status(
    delegated_role: str,
    delegated_metadata: "Metadata",
    signed_serializer: Optional[SignedSerializer] = None
) -> SigningStatus:

One additional related use case I saw in both tuf-on-ci and RSTUF:

• is a given signature valid for delegate according to delegator?

It's easy enough to implement in the application. I'm bringing it up here, because, if delegate_signing_status returns the valid signatures, it could be used for that purpose. OTOH, the validity of a given signature might be interesting by itself, regardless of other signatures. So maybe we should treat this as separate feature. Any thoughts?

Re name: I'm a bit unhappy with the verb/noun ambiguity of delegate. delegate_signing_status, to me, sounds like the activity of delegating the signing status, for whatever that means. But I'm okay, if we clarify this in the docstring.

is a given signature valid for delegate according to delegator?

It could be I just don't remember the specific location but I think tuf-on-ci question is (or should be) "has this signing key signed this delegated role?"

I think in 99% of cases applications should not touch or think about the actual signatures at all.

It could be I just don't remember the specific location but I think tuf-on-ci question is (or should be) "has this signing key signed this delegated role?"

You are right, I mis-skimmed the code I was referring to. nvm

[edit: fixed math after talking to Jussi]

I think Jussi's proposal is complete.

Returning "invalid signatures" as well does not make a lot of sense, because the signatures could be invalid according to one delegator, but valid according to another, which is not unlikely in the case of a root update. So, to get the desired information -- "does the metadata have invalid signatures?" -- we'd need to do set operations on multiple results anyway, and this can be done with the existing fields too. E.g. root update

stat_old = old_root.delegate_signing_status("root", new_root)
stat_new = new_root.delegate_signing_status("root", new_root)

has_invalid_sigs = len(metadata.signatures) > len(stat_old.signed_keys | stat_new.signed_keys)  # assume signed_keys is a set of keyids
invalid_sigs = metadata.signatures.keys() - (stat_old.signed_keys | stat_new.signed_keys)

Yes, I agree with what you said.

I think I like the unstated assumption in your examples that the key lists are actually sets -- then you can do unions more easily, and I can see that would be useful.

The "math" in your code has some issues but I don't think they're very important:

has_invalid_sigs = len(metadata.signatures) > (len(stat_old.signed_keys) + len(stat_new.signed_keys)) # assume signed_keys is a set of keyids

That's not right (keys might be duplicate and anyway thresholds may be different). Like you said, for root you'd just have to duplicate whatever you do normally to find invalid sigs (whatever "invalid" means in this context): you'd then have two separate lists of "invalid sigs"... unsure what you would want to do with them.

I think there are more useful questions and they seem fairly easy to answer:

• "which root keys have not yet signed correctly, but could still do so":

  unsigned_keys = stat_old.unsigned_keys | stat_new.unsigned_keys
  

• "Which root keys may need to act for the metadata to become verified":

  missing_keys = set()
  if not stat_old.verified:
      missing_keys |= stat_old.unsigned_keys
  if not stat_new.verified:
      missing_keys |= stat_new.unsigned_keys
  

On the idea of using set instead of list for Keys: this does not currently work as Key is not hashable. We could easily make them hashable though

Cross-posting from slack, by "invalid signature" I meant:

• there is no authorized keyid for a signature's keyid in any eligible delegator, or
• there is an authorized keyid but no public key in the corresponding keystore, or
• there is an authorized keyid and a public key but verification fails cryptographically

I edited my "math" in the comment above to correctly support these checks

On the idea of using set instead of list for Keys: this does not currently work as Key is not hashable. We could easily make them hashable though

Or maybe not: hashable objects are not supposed to be mutable (the hash should be a constant for the lifetime of the object). Strictly speaking that's not true for Keys even if our keys almost never mutate. We could return keyids instead of keys to get around this. Then my use case is still reasonable if not anymore trivial:

status = root.delegate_signing_status("timestamp", timestamp_metadata)
signed = [root.get_key(id) for id in status.signed_keys]

Or we could just say the Keys are immutable enough so that we can put them in this short lived set 🤷

Or we could just say the Keys are immutable enough so that we can put them in this short lived set 🤷

Hm, I suppose it's okay, if we document the caveat. Alternatively, we could implement a custom key container, which provides all the desired set operations for contained Key objects. And which clearly documents that mutating the items breaks the container.

I think returning set of keyids is ok and fits the rest of the API.

If someone wants to take the WIP commit above and polish, I'm totally fine with that.
As a note on the implementation, I originally included threshold in the result but decided against that: A useful operation is to "union" two VerificationResults (in the root case) and I think providing that in the library might be good... Two thresholds however cannot be "unioned".