Skip to content
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

Root.json size/hashes in snapshot.json are superfluous #451

Closed
heartsucker opened this issue May 31, 2017 · 13 comments
Closed

Root.json size/hashes in snapshot.json are superfluous #451

heartsucker opened this issue May 31, 2017 · 13 comments

Comments

@heartsucker
Copy link

From the spec

For the root role, the hash(es), size, and version number are listed.

Since the downloading and verification of the root.json is never done with a dependency on snapshot.json, there is no reason to have these in the metadata.

@vladimir-v-diaz
Copy link
Contributor

I think the Timestamp and Snapshot roles can ensure that the Root delivered to the client is indeed the latest version. Listing Root in Snapshot metadata can help with consistency.

@JustinCappos @trishankkarthik @awwad What do you guys think?

@awwad
Copy link
Contributor

awwad commented May 31, 2017

In TAP 5, we were planning to remove the expectation that Root is validated by Snapshot (and so the listing of Root in Snapshot) anyway. Pinning does involve having custom Root.json files, and these shouldn't be validated by a separate party's Snapshot.json.

The current security analysis for the removal of root metadata from snapshot is here. I haven't thought through this version properly, but in short this creates a window for Root freezes, though replay attacks are not possible.

@heartsucker
Copy link
Author

It appears I'm making a redundant observation. :)

@vladimir-v-diaz
Copy link
Contributor

Oh, that's right. TAP 5 states that a Root isn't listed in Snapshot, but we do lose the ability to ensure that the Root delivered to the client is the latest version.

@heartsucker
Copy link
Author

we do lose the ability to ensure that the Root delivered to the client is the latest version

I'm not sure what the use case here is. If the first step is to download the root metadata, we could only get to this situation if:

  1. The snapshot was compromised and is lying
  2. There are some N root.json's with version greater than what was received (MITM plays old root as newest, but also doesn't role it back to a version before the one the client knows about)

@awwad
Copy link
Contributor

awwad commented May 31, 2017

Yes, that's the scenario. It's pretty narrow. It might be of interest in vehicles, where you can certainly imagine many vehicles having very old metadata (including root files).

@heartsucker
Copy link
Author

In that case, could the spec say "it MAY include the size/hash to ensure the correct root was received. Should the root not match these values, the client MAY report the presence of an attack."

@vladimir-v-diaz
Copy link
Contributor

vladimir-v-diaz commented May 31, 2017

In TAP 5, we were planning to remove the expectation that Root is validated by Snapshot (and so the listing of Root in Snapshot) anyway.

According to TAP 5, Snapshot metadata should continue to list Root for backwards compatibility. (although the mix of shall and should is confusing, at least to me):
https://github.com/theupdateframework/taps/blob/master/tap5.md#changes-to-the-snapshot-metadata-file

@trishankkarthik
Copy link
Contributor

Hello, usual suspects! I was just going to say what @vladimir-v-diaz did :)

I agree that we should clean up the SHALLs and SHOULDs. Vlad, are you up for it?

@awwad
Copy link
Contributor

awwad commented May 31, 2017

Requirements:
What @heartsucker suggests is basically what it should be, both currently and post-TAP5: listing Root in Snapshot or checking Root per Snapshot should be optional.

(I guess we don't really need to worry about changing the current spec on its own as long as TAP 5 is adopted, but as far as I'm concerned, an implementation is not going to be non-conformant just because it doesn't list Root in Snapshot.)

Backwards compatibility:
So, there's a semantic issue on backwards compatibility here. I would have said that the short version is that TAP 5 is backwards incompatible.
The particulars -- that a new client can process old metadata without security issues, but an old client will be unable to process new metadata that includes URLs or excludes root -- are worth mentioning in the Backwards Compatibility section.

Edit: @ecordell puts it well here, arguing that this should be considered backwards compatible, and that the caveats should be included clearly in the TAPs.

Editing TAP 5:
It looks like comments have kinda glommed onto the end of sections over time in a way that makes things contradictory or confusing, yeah.

As a final note, I'm curious about what @JustinCappos thinks and if any thinking on this has changed per the last conversations with CoreOS or Docker.

@vladimir-v-diaz
Copy link
Contributor

Issue #13 and TAP 6: Include specification version identifier in metadata are relevant to this discussion.

@heartsucker
Copy link
Author

I think this was addressed by some recent merges. Can we close this?

@vladimir-v-diaz
Copy link
Contributor

Yup!
Root's hash and length no longer listed in Snapshot. Implemented in #469

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

No branches or pull requests

4 participants