Client::with_root_pinned should take public keys, not key ids

[erickt]

As @ComputerDruid pointed out, Client::with_root_pinned should be taking the public keys used to sign the metadata, rather than trusting the initial root metadata has the correct mapping from key id to key. Otherwise, if we removed recomputing the key ids (see theupdateframework/python-tuf#848 (comment)), then a malicious server could swap out the public keys but preserve the key ids and we wouldn't notice.

[trishankatdatadog]

💯

Or, since keyid=sha2-256(pubkey), you might check that way that a pubkey given by the server matches the expected keyid

However, this assumes that the keyid is always exactly that, which might change in the future

So best to just pass the pubkeys directly

Cc @lukpueh who might be interested

[heartsucker]

This is a chicken and egg problem. The chain of trust has to start somewhere, and it's simple enough to share a key ID over chat or by reading them from one laptop to another. If you already have the full key and know it's trusted, then Client::new does the job. Also, in this case if "a remote server could swap out the public keys," then someone has already made a mistake with Client::with_root_pinned which explicitly states that it assumes the key is trusted. This function was designed to be used when trust is definitely known.

For example, I am some dev making a CLI tool for verifying downloads of $whatever. I as the person managing the infra for downloads, I definitely know what keys are trusted. I hardcode their IDs into the binary, and ship that. So, if someone gets an authentic binary, then they will have effectively established the link that allows them to fetch a remote root without worrying about MITM or tampering. I shouldn't have to encode the full key into the binary to do this, and at that point I might as well hard code the root metadata itself.

The comment above on TUF-848 brings up another thing from the TUF spec that has been a long running issue: Is the spec rigid enough to mandate interop or is it flexible? In that comment it specifically says that impls can choose how they calculate key ID.

If rust-tuf is supposed to interop with Notary, then it should just copy exactly what they do.

[erickt]

In our situation, we are currently storing the public keys (and eventually the root version) in a read-only config file that's part of our verified boot chain (see this doc for more about this subject). On go-tuf, we use use Client.Init(keys []*data.Key, threshold int) for this. I had intended on using rust-tuf's Client::with_root_pinned, but I missed that it worked with keyids, not keys.

Perhaps we need a few separate Client constructors for these different use cases:

• Client::new, which trusts that Local has been initialized, or blindly trusts the first root metadata Remote returns. Least safe
• Client::with_root_keyids_pinned, which requires KeyIds to be cryptographically derived from the public keys, otherwise an attacker could substitute in alternative root metadata without the client observing. (I'm personally pushing TUF-848, which breaks this requirement, but as of right now using the TUF-1.0 keyid algorithm should be secure (but what do I know, I'm not a cryptographer :)).
• Client::with_root_keys_pinned, which would take an initial root version and &[tuf::crypto::PublicKey] to verify the initial downloaded metadata. This should always be secure.
• Client::with_initial_root, which takes a SignedMetadata<tuf::metadata::RootMetadata>, which is used to bootstrap trusting the local repositories, then updating to the most recent remote repositories.

We don't want to establish our chain of trust by just copying our metadata to the Local repository, because we want to protect ourselves from an attacker compromising our mutable filesystem service. We could of course create an alternative Repository that initially only returns our trusted root, but that seems a little complicated.

If rust-tuf is supposed to interop with Notary, then it should just copy exactly what they do.

I think you mean go-tuf. I think Notary has since diverged from the TUF spec.

[heartsucker]

Ok, I think multiple constructors makes sense then. I just think that the current pinning impl needs to stay because it's rather ergonomic for a number of real world use cases.