-
Notifications
You must be signed in to change notification settings - Fork 290
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
lib/pull: Default checksum for archive mirror, add TRUSTED_HTTP flag #1212
lib/pull: Default checksum for archive mirror, add TRUSTED_HTTP flag #1212
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Just a minor issue (it wasn't introduced by this PR, though it's related).
src/ostree/ot-builtin-pull.c
Outdated
@@ -57,6 +58,7 @@ static GOptionEntry options[] = { | |||
{ "mirror", 0, 0, G_OPTION_ARG_NONE, &opt_mirror, "Write refs suitable for a mirror and fetches all refs if none provided", NULL }, | |||
{ "subpath", 0, 0, G_OPTION_ARG_FILENAME_ARRAY, &opt_subpaths, "Only pull the provided subpath(s)", NULL }, | |||
{ "untrusted", 0, 0, G_OPTION_ARG_NONE, &opt_untrusted, "Do not verify checksums of local sources (always enabled for HTTP pulls)", NULL }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The symmetry between the two options made me realize the description for this is wrong. It should be "Do verify...".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ooh yes, good catch! Rebased 🏄 and added a prep commit for this ⬇️
It means *do* verify for local.
Make the "local repo" processing conditional the same as the "localcache" bits; this is really just a de-indent. Also add some comments. Prep for further work.
Rather than carrying two booleans, just convert `OstreeRepoPullFlags` into `OstreeRepoImportFlags`. This allows us to drop an internal wrapper function and just directly call `_ostree_repo_import_object()`. This though reveals that our mirroring import path doesn't check the `OSTREE_REPO_PULL_FLAGS_UNTRUSTED` flag...it probably should. Prep for further work.
I now think commit fab1e11 was a mistake; because it breaks the mental model that at least I'd built up that "local repos don't have checksums verified, HTTP does". For example, a problem with this is (with that mental model in place) it's easy for people who set up mirrors like this to then do local pulls, and at that point we've done a deployment with no checksum verification. Further, since then we did PR ostreedev#671 AKA commit 3d38f03 which is really most of the speed hit. So let's switch the default even for this case to doing checksum verification, and add `ostree pull --http-trusted`. People who are in situations where they know they want this can find it and turn it on. Closes: ostreedev#1211
3176289
to
f9b26ef
Compare
Make the "local repo" processing conditional the same as the "localcache" bits; this is really just a de-indent. Also add some comments. Prep for further work. Closes: #1212 Approved by: jlebon
Rather than carrying two booleans, just convert `OstreeRepoPullFlags` into `OstreeRepoImportFlags`. This allows us to drop an internal wrapper function and just directly call `_ostree_repo_import_object()`. This though reveals that our mirroring import path doesn't check the `OSTREE_REPO_PULL_FLAGS_UNTRUSTED` flag...it probably should. Prep for further work. Closes: #1212 Approved by: jlebon
I now think commit fab1e11 was a mistake; because it breaks the mental model that at least I'd built up that "local repos don't have checksums verified, HTTP does". For example, a problem with this is (with that mental model in place) it's easy for people who set up mirrors like this to then do local pulls, and at that point we've done a deployment with no checksum verification. Further, since then we did PR #671 AKA commit 3d38f03 which is really most of the speed hit. So let's switch the default even for this case to doing checksum verification, and add `ostree pull --http-trusted`. People who are in situations where they know they want this can find it and turn it on. Closes: #1211 Closes: #1212 Approved by: jlebon
☀️ Test successful - status-atomicjenkins |
I now think commit fab1e11 was a mistake;
because it breaks the mental model that at least I'd built up that "local repos
don't have checksums verified, HTTP does".
For example, a problem with this is (with that mental model in place) it's easy
for people who set up mirrors like this to then do local pulls, and at that
point we've done a deployment with no checksum verification.
Further, since then we did PR #671 AKA commit 3d38f03 which is really most of
the speed hit.
So let's switch the default even for this case to doing checksum verification,
and add
ostree pull --http-trusted
. People who are in situations where theyknow they want this can find it and turn it on.
Closes: #1211