Narrow summary file race #490
Conversation
This is for signing the summary file, not a static delta. Closes: ostreedev#487
Provide internal versions that take a directory fd where the summary and signature are handled. The existing functions continue to operate in the top repository directory, but this allows another variant to manage the summary file in a temporary directory. Closes: ostreedev#487
The ostree_repo_regenerate_summary_ext function allows regenerating and signing the summary in one call. The implementation tries to reduce the race between these operations as much as possible by using a temporary directory and renaming the files. Closes: ostreedev#487
This minimizes the race between generation and signing of the summary file. It also means that if the new summary file can't be signed, the old summary file is retained. Closes: ostreedev#487
| /* Remove comment when first new symbol is added | ||
| LIBOSTREE_2016.10 | ||
| LIBOSTREE_2016.11 { |
There was a problem hiding this comment.
Maybe the patch is just weirdly rendered, but it looks like the new function gets called .10 and the old that .11.
There was a problem hiding this comment.
It's just a little weirdness in the ostree symbols file plus weirdness in symbol version files in general. This part is in a comment with a fake symbol waiting for the next release. My addition is above in the .10 section that chains to the .8 section. There's a little extra noise in this patch because the commented out .10 section prior to this patch (there weren't any new symbols yet) was missing the trailing {.
|
Seems like a good first step to me. If at some point we make it atomic on the client side we need this function anyway. |
|
Debian's bug here is similar: https://bugs.launchpad.net/launchpad/+bug/1430011 A few other ostree bugs related here: https://bugzilla.gnome.org/show_bug.cgi?id=739143 So I see two things here. First, we could introduce Second, we could support inline signatures for the summary file. I suspect most users would be happy enough with that. The problem is though it'd be a hard format change unless we basically doubled the size and had an inline-signed copy? Related to all of this...where I'd like to go is separating metadata fetches from content fetches. The idea here is you fetch metadata atomically from more centralized servers (with e.g. TLS pinning), then you can get content from anywhere. One could pretty easily keep the ostree metadata here (refs/summary/signatures) entirely in memory and update it atomically. The client side of this is #469 |
|
Finally related to all of this...I'd like to consider backing away from storing the static delta data in the summary file. It can get expensive quickly. Flatpak is relying on signed commits in the deltas anyways, and I'd like to push people towards pinned TLS + signed commits as simplest. Metadata is hard, particularly when one involves signing. |
|
@cgwalters |
|
I mean "same summary sig => no need to redownload" |
|
So...I am not opposed to this at all, and sorry for introducing the race in the first place (metadata is hard). But, if we're making changes here I'd like to think a little bit harder about what we can do to fix the race vs narrowing the window. I think what has me hesitating a bit is adding new API without solving the race. |
|
I'd been meaning to follow up here, too. Here's my other idea that removes the race (I think), but would require new client code. Let me see if I can get a reasonable tree representation: When creating a new summary, use a temporary directory under summaries. Optionally sign in the temporary directory. Rename the temporary directory to the checksum of the summary file. Update the current symlink to point to checksum directory. This removes the server side race. On new clients, they'd download Old clients would still race trying to download the toplevel There would need to be some cleanup of old summary directories. Maybe you'd keep 3? BTW, if we're adding the |
|
☔ The latest upstream changes (presumably 36e8ba1) made this pull request unmergeable. Please resolve the merge conflicts. |
|
The proposal from @dbnicholson means we always have to start by downloading the entire summary, instead of starting with the sig, before detecting that there has been no chnges. If we do that, then we have to use an etag or something to avoid downloading the large metadata if nothing changed. Alternatively we can make the subdirectory be named by the hash of the signature. Then you can start by downloading and verifying that, and use its checksum to find the summary file. And for unsigned summaries you can just use the toplevel symlinks, because there is no race. |
|
Yeah, sorry. I'd never thought of that optimization. I thought ostree always downloaded the summary file. Another issue i thought of with that setup is that the symlinks require you to host on a UNIX server. I think the rest of ostree has always allowed it to be hosted on any dumb http server. So, my other thought is that instead of a symlink that summaries/current is a ref file with the checksum of the summary file, which is also the directory name as above. Then you can download the current file and quickly see if the checksum for the summary file has changed. If you want more verification, you can get the signature file from the checksum directory as above. Without the symlinks, the top level compat files wouldn't be able to avoid the server side race. The best you could do is narrow it as in the patches here. |
|
It's going to be really messy to try to fix the race solely on the server side without having more intelligence there (like the split metadata/content server model). If we want to support just rsync for example (which today only works sort of by accident because Let's take a step back - do you need the signed summary file vs what we have with per-commit signatures? And to note again: if we can move the ecosystem to split metadata/content fetches, a lot of things become simpler. The most recent ostree supports this. |
|
Do we need it? I dunno. Without signed summaries we can't protect against certain things. For instance, someone can MITM you and claim the "org.some.app" app actually contains the commit from "org.other.app" from the same remote. Is this bad? I dunno, its somewhat limited as an attack vector. We do require signed summaries today though. What do you mean by split metadata/content, can you describe it in a bit more detail? |
|
Also, we store some extra info in the summary files, such as app permissions/metadata and sizes in the summary file. Those would be fakeable if not signed, although we could re-verify them after download to catch issues. |
|
I'm gonna close this since it doesn't actually address the client side race. I moved a couple pieces into #1946 - a new API that does the generation and signing in one call and use of a temporary directory for the |
|
Do you mean #1490? It seems unrelated. |
|
@dbnicholson: PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Provide a new function,
ostree_repo_regenerate_summary_ext, that creates and signs the summary file in a tmpdir before renaming them into place. This minimizes the race between summary generation and signing without going fully atomic. Hopefully this is good enough for now since being atomic on the client side is quite a bit more complicated.Closes: #487