From 676f0a27976a07a6a3893d4835195bb68a55a9c5 Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Thu, 10 Nov 2016 11:42:35 -0500 Subject: [PATCH 01/24] travis: Drop debian unstable since we can't fetch packages reliably I don't know what's going on, I suspect mirror churn. Anyways, it seems to be consistently failing now, so let's drop it. Closes: #571 Approved by: jlebon --- .travis.yml | 1 - 1 file changed, 1 deletion(-) diff --git a/.travis.yml b/.travis.yml index 8594ff78..44b78b1e 100644 --- a/.travis.yml +++ b/.travis.yml @@ -5,7 +5,6 @@ sudo: required env: - ci_distro=ubuntu ci_suite=trusty - ci_docker=debian:jessie ci_distro=debian ci_suite=jessie - - ci_docker=debian:unstable ci_distro=debian ci_suite=unstable - ci_docker=ubuntu:xenial ci_distro=ubuntu ci_suite=xenial script: From 4b7ab5167cbb14b7a925f361bb464affecd2fd5b Mon Sep 17 00:00:00 2001 From: Jonathan Lebon Date: Thu, 10 Nov 2016 12:56:37 -0500 Subject: [PATCH 02/24] .redhat-ci.yml: no longer install libubsan & clang Since they're now part of the auto-built image. Closes: #572 Approved by: cgwalters --- .redhat-ci.yml | 9 --------- 1 file changed, 9 deletions(-) diff --git a/.redhat-ci.yml b/.redhat-ci.yml index 9994aa10..6c170be1 100644 --- a/.redhat-ci.yml +++ b/.redhat-ci.yml @@ -6,11 +6,6 @@ branches: container: image: projectatomic/ostree-tester -# XXX: we can wipe this off once a newer image is built with -# it already included -packages: - - libubsan - env: CFLAGS: '-fsanitize=undefined' @@ -36,10 +31,6 @@ artifacts: inherit: true -# XXX: ditto -packages: - - clang - context: Clang env: From 37c07d2f1c90b12bcfba85a7d900f81a7c362eb4 Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Tue, 15 Nov 2016 21:03:26 -0500 Subject: [PATCH 03/24] pull: Add support for `http-headers` option Some deployments may want to gate access to content based on things like OAuth. In this model, the client system would normally compute a token and pass it to the server via an API. We could theoretically support this in the remote config too, but that'd be a bit weird for OAuth as the information is dynamic. Therefore this cleans up the code a little bit to more clearly handle the case that the fetcher is initialized from both remote config data plus pull options. Closes: #574 Approved by: giuseppe --- Makefile-tests.am | 1 + src/libostree/ostree-fetcher.c | 34 ++++++++++++++++++ src/libostree/ostree-fetcher.h | 3 ++ src/libostree/ostree-repo-pull.c | 29 ++++++++++++--- src/ostree/ot-builtin-pull.c | 25 +++++++++++++ src/ostree/ot-builtin-trivial-httpd.c | 33 ++++++++++++++++- tests/test-remote-headers.sh | 52 +++++++++++++++++++++++++++ 7 files changed, 171 insertions(+), 6 deletions(-) create mode 100755 tests/test-remote-headers.sh diff --git a/Makefile-tests.am b/Makefile-tests.am index 6c2301ce..2160cd55 100644 --- a/Makefile-tests.am +++ b/Makefile-tests.am @@ -46,6 +46,7 @@ dist_test_scripts = \ tests/test-archivez.sh \ tests/test-remote-add.sh \ tests/test-remote-cookies.sh \ + tests/test-remote-headers.sh \ tests/test-remote-gpg-import.sh \ tests/test-commit-sign.sh \ tests/test-export.sh \ diff --git a/src/libostree/ostree-fetcher.c b/src/libostree/ostree-fetcher.c index 18794ce1..87e08441 100644 --- a/src/libostree/ostree-fetcher.c +++ b/src/libostree/ostree-fetcher.c @@ -53,6 +53,7 @@ typedef struct { GLnxLockFile tmpdir_lock; int base_tmpdir_dfd; + GVariant *extra_headers; int max_outstanding; /* Queue for libsoup, see bgo#708591 */ @@ -148,6 +149,8 @@ thread_closure_unref (ThreadClosure *thread_closure) g_clear_pointer (&thread_closure->main_context, g_main_context_unref); + g_clear_pointer (&thread_closure->extra_headers, (GDestroyNotify)g_variant_unref); + if (thread_closure->tmpdir_dfd != -1) close (thread_closure->tmpdir_dfd); @@ -336,6 +339,16 @@ session_thread_set_cookie_jar_cb (ThreadClosure *thread_closure, SOUP_SESSION_FEATURE (jar)); } +static void +session_thread_set_headers_cb (ThreadClosure *thread_closure, + gpointer data) +{ + GVariant *headers = data; + + g_clear_pointer (&thread_closure->extra_headers, (GDestroyNotify)g_variant_unref); + thread_closure->extra_headers = g_variant_ref (headers); +} + #ifdef HAVE_LIBSOUP_CLIENT_CERTS static void session_thread_set_tls_interaction_cb (ThreadClosure *thread_closure, @@ -448,6 +461,17 @@ session_thread_request_uri (ThreadClosure *thread_closure, return; } + if (SOUP_IS_REQUEST_HTTP (pending->request) && thread_closure->extra_headers) + { + glnx_unref_object SoupMessage *msg = soup_request_http_get_message ((SoupRequestHTTP*) pending->request); + g_autoptr(GVariantIter) viter = g_variant_iter_new (thread_closure->extra_headers); + const char *key; + const char *value; + + while (g_variant_iter_next (viter, "(&s&s)", &key, &value)) + soup_message_headers_append (msg->request_headers, key, value); + } + if (pending->is_stream) { soup_request_send_async (pending->request, @@ -812,6 +836,16 @@ _ostree_fetcher_set_tls_database (OstreeFetcher *self, } } +void +_ostree_fetcher_set_extra_headers (OstreeFetcher *self, + GVariant *extra_headers) +{ + session_thread_idle_add (self->thread_closure, + session_thread_set_headers_cb, + g_variant_ref (extra_headers), + (GDestroyNotify) g_variant_unref); +} + static gboolean finish_stream (OstreeFetcherPendingURI *pending, GCancellable *cancellable, diff --git a/src/libostree/ostree-fetcher.h b/src/libostree/ostree-fetcher.h index ae20edaa..0bfba5b2 100644 --- a/src/libostree/ostree-fetcher.h +++ b/src/libostree/ostree-fetcher.h @@ -71,6 +71,9 @@ void _ostree_fetcher_set_client_cert (OstreeFetcher *fetcher, void _ostree_fetcher_set_tls_database (OstreeFetcher *self, GTlsDatabase *db); +void _ostree_fetcher_set_extra_headers (OstreeFetcher *self, + GVariant *extra_headers); + guint64 _ostree_fetcher_bytes_transferred (OstreeFetcher *self); void _ostree_fetcher_mirrored_request_with_partial_async (OstreeFetcher *self, diff --git a/src/libostree/ostree-repo-pull.c b/src/libostree/ostree-repo-pull.c index 8facf8cb..183812cc 100644 --- a/src/libostree/ostree-repo-pull.c +++ b/src/libostree/ostree-repo-pull.c @@ -54,6 +54,8 @@ typedef struct { GCancellable *cancellable; OstreeAsyncProgress *progress; + GVariant *extra_headers; + gboolean dry_run; gboolean dry_run_emitted_progress; gboolean legacy_transaction_resuming; @@ -2276,6 +2278,23 @@ repo_remote_fetch_summary (OstreeRepo *self, return ret; } +/* Create the fetcher by unioning options from the remote config, plus + * any options specific to this pull (such as extra headers). + */ +static gboolean +reinitialize_fetcher (OtPullData *pull_data, const char *remote_name, GError **error) +{ + g_clear_object (&pull_data->fetcher); + pull_data->fetcher = _ostree_repo_remote_new_fetcher (pull_data->repo, remote_name, error); + if (pull_data->fetcher == NULL) + return FALSE; + + if (pull_data->extra_headers) + _ostree_fetcher_set_extra_headers (pull_data->fetcher, pull_data->extra_headers); + + return TRUE; +} + /* ------------------------------------------------------------------------------------------ * Below is the libsoup-invariant API; these should match * the stub functions in the #else clause @@ -2308,6 +2327,7 @@ repo_remote_fetch_summary (OstreeRepo *self, * * dry-run (b): Only print information on what will be downloaded (requires static deltas) * * override-url (s): Fetch objects from this URL if remote specifies no metalink in options * * inherit-transaction (b): Don't initiate, finish or abort a transaction, usefult to do mutliple pulls in one transaction. + * * http-headers (a(ss)): Additional headers to add to all HTTP requests */ gboolean ostree_repo_pull_with_options (OstreeRepo *self, @@ -2368,6 +2388,7 @@ ostree_repo_pull_with_options (OstreeRepo *self, (void) g_variant_lookup (options, "dry-run", "b", &pull_data->dry_run); (void) g_variant_lookup (options, "override-url", "&s", &url_override); (void) g_variant_lookup (options, "inherit-transaction", "b", &inherit_transaction); + (void) g_variant_lookup (options, "http-headers", "@a(ss)", &pull_data->extra_headers); } g_return_val_if_fail (pull_data->maxdepth >= -1, FALSE); @@ -2467,8 +2488,7 @@ ostree_repo_pull_with_options (OstreeRepo *self, pull_data->phase = OSTREE_PULL_PHASE_FETCHING_REFS; - pull_data->fetcher = _ostree_repo_remote_new_fetcher (self, remote_name_or_baseurl, error); - if (pull_data->fetcher == NULL) + if (!reinitialize_fetcher (pull_data, remote_name_or_baseurl, error)) goto out; pull_data->tmpdir_dfd = pull_data->repo->tmp_dir_fd; @@ -2906,9 +2926,7 @@ ostree_repo_pull_with_options (OstreeRepo *self, /* Now discard the previous fetcher, as it was bound to a temporary main context * for synchronous requests. */ - g_clear_object (&pull_data->fetcher); - pull_data->fetcher = _ostree_repo_remote_new_fetcher (self, remote_name_or_baseurl, error); - if (pull_data->fetcher == NULL) + if (!reinitialize_fetcher (pull_data, remote_name_or_baseurl, error)) goto out; pull_data->legacy_transaction_resuming = FALSE; @@ -3120,6 +3138,7 @@ ostree_repo_pull_with_options (OstreeRepo *self, g_source_destroy (update_timeout); g_strfreev (configured_branches); g_clear_object (&pull_data->fetcher); + g_clear_pointer (&pull_data->extra_headers, (GDestroyNotify)g_variant_unref); g_clear_object (&pull_data->cancellable); g_clear_object (&pull_data->remote_repo_local); g_free (pull_data->remote_name); diff --git a/src/ostree/ot-builtin-pull.c b/src/ostree/ot-builtin-pull.c index 7981f18a..8fa51002 100644 --- a/src/ostree/ot-builtin-pull.c +++ b/src/ostree/ot-builtin-pull.c @@ -35,6 +35,7 @@ static gboolean opt_disable_static_deltas; static gboolean opt_require_static_deltas; static gboolean opt_untrusted; static char** opt_subpaths; +static char** opt_http_headers; static char* opt_cache_dir; static int opt_depth = 0; static char* opt_url; @@ -51,6 +52,7 @@ static GOptionEntry options[] = { { "dry-run", 0, 0, G_OPTION_ARG_NONE, &opt_dry_run, "Only print information on what will be downloaded (requires static deltas)", NULL }, { "depth", 0, 0, G_OPTION_ARG_INT, &opt_depth, "Traverse DEPTH parents (-1=infinite) (default: 0)", "DEPTH" }, { "url", 0, 0, G_OPTION_ARG_STRING, &opt_url, "Pull objects from this URL instead of the one from the remote config", NULL }, + { "http-header", 0, 0, G_OPTION_ARG_STRING_ARRAY, &opt_http_headers, "Add NAME=VALUE as HTTP header to all requests", "NAME=VALUE" }, { NULL } }; @@ -249,6 +251,29 @@ ostree_builtin_pull (int argc, char **argv, GCancellable *cancellable, GError ** g_variant_builder_add (&builder, "{s@v}", "override-commit-ids", g_variant_new_variant (g_variant_new_strv ((const char*const*)override_commit_ids->pdata, override_commit_ids->len))); + if (opt_http_headers) + { + GVariantBuilder hdr_builder; + g_variant_builder_init (&hdr_builder, G_VARIANT_TYPE ("a(ss)")); + + for (char **iter = opt_http_headers; iter && *iter; iter++) + { + const char *kv = *iter; + const char *eq = strchr (kv, '='); + g_autofree char *key = NULL; + if (!eq) + { + g_set_error (error, G_IO_ERROR, G_IO_ERROR_FAILED, + "Missing '=' in --http-header"); + goto out; + } + key = g_strndup (kv, eq - kv); + g_variant_builder_add (&hdr_builder, "(ss)", key, eq + 1); + } + g_variant_builder_add (&builder, "{s@v}", "http-headers", + g_variant_new_variant (g_variant_builder_end (&hdr_builder))); + } + if (!opt_dry_run) { if (console.is_tty) diff --git a/src/ostree/ot-builtin-trivial-httpd.c b/src/ostree/ot-builtin-trivial-httpd.c index 6e6415dd..0a553858 100644 --- a/src/ostree/ot-builtin-trivial-httpd.c +++ b/src/ostree/ot-builtin-trivial-httpd.c @@ -43,8 +43,8 @@ static int opt_random_500s_percentage; * cases involving repeated random 500s. */ static int opt_random_500s_max = 100; static gint opt_port = 0; - static gchar **opt_expected_cookies; +static gchar **opt_expected_headers; static guint emitted_random_500s_count = 0; @@ -64,6 +64,7 @@ static GOptionEntry options[] = { { "random-500s-max", 0, 0, G_OPTION_ARG_INT, &opt_random_500s_max, "Limit HTTP 500 errors to MAX (default 100)", "MAX" }, { "log-file", 0, 0, G_OPTION_ARG_FILENAME, &opt_log, "Put logs here", "PATH" }, { "expected-cookies", 0, 0, G_OPTION_ARG_STRING_ARRAY, &opt_expected_cookies, "Expect given cookies in the http request", "KEY=VALUE" }, + { "expected-header", 0, 0, G_OPTION_ARG_STRING_ARRAY, &opt_expected_headers, "Expect given headers in the http request", "KEY=VALUE" }, { NULL } }; @@ -238,6 +239,36 @@ do_get (OtTrivialHttpd *self, soup_cookies_free (cookies); } + if (opt_expected_headers) + { + for (int i = 0 ; opt_expected_headers[i] != NULL; i++) + { + const gchar *kv = opt_expected_headers[i]; + const gchar *eq = strchr (kv, '='); + + g_assert (eq); + + { + g_autofree char *k = g_strndup (kv, eq - kv); + const gchar *expected_v = eq + 1; + const gchar *found_v = soup_message_headers_get_one (msg->request_headers, k); + + if (!found_v) + { + httpd_log (self, "Expected header not found %s\n", k); + soup_message_set_status (msg, SOUP_STATUS_FORBIDDEN); + goto out; + } + if (strcmp (found_v, expected_v) != 0) + { + httpd_log (self, "Expected header %s: %s but found %s\n", k, expected_v, found_v); + soup_message_set_status (msg, SOUP_STATUS_FORBIDDEN); + goto out; + } + } + } + } + if (strstr (path, "../") != NULL) { soup_message_set_status (msg, SOUP_STATUS_FORBIDDEN); diff --git a/tests/test-remote-headers.sh b/tests/test-remote-headers.sh new file mode 100755 index 00000000..bca46204 --- /dev/null +++ b/tests/test-remote-headers.sh @@ -0,0 +1,52 @@ +#!/bin/bash +# +# Copyright (C) 2016 Red Hat, Inc. +# +# This library is free software; you can redistribute it and/or +# modify it under the terms of the GNU Lesser General Public +# License as published by the Free Software Foundation; either +# version 2 of the License, or (at your option) any later version. +# +# This library is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU +# Lesser General Public License for more details. +# +# You should have received a copy of the GNU Lesser General Public +# License along with this library; if not, write to the +# Free Software Foundation, Inc., 59 Temple Place - Suite 330, +# Boston, MA 02111-1307, USA. + +set -euo pipefail + +echo '1..2' + +. $(dirname $0)/libtest.sh + +setup_fake_remote_repo1 "archive" "" \ + "--expected-header foo=bar --expected-header baz=badger" + +assert_fail (){ + set +e + $@ + if [ $? = 0 ] ; then + echo 1>&2 "$@ did not fail"; exit 1 + fi + set -euo pipefail +} + +cd ${test_tmpdir} +rm repo -rf +mkdir repo +${CMD_PREFIX} ostree --repo=repo init +${CMD_PREFIX} ostree --repo=repo remote add --set=gpg-verify=false origin $(cat httpd-address)/ostree/gnomerepo + +# Sanity check the setup, without headers the pull should fail +assert_fail ${CMD_PREFIX} ostree --repo=repo pull origin main + +echo "ok, setup done" + +# Now pull should succeed now +${CMD_PREFIX} ostree --repo=repo pull --http-header foo=bar --http-header baz=badger origin main + +echo "ok, pull succeeded" From 814aa9682514f2bc342f05032021774d3f128f17 Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Tue, 1 Nov 2016 13:51:55 -0400 Subject: [PATCH 04/24] pull: Redo logic for "scanning" What in the code is called "scanning" is ensuring (potentially recursively) have an object, and if not, fetching it. And then if it's metadata, parsing it and finding new objects to fetch. This logic has grown fairly complex. What I'm trying to fix right now is that if we're doing a pull-local to a remote repository via `sshfs` (FUSE) we still end up scanning, which is inefficient. We can take advantage of the "commitpartial" logic here - if a commit isn't partial, it's complete, hence we don't need to scan it. At the same time, I'm changing the logic here to *always* do scans for dirtree objects. This will fix cases where multiple commits share dirtree objects. We have "commitpartial" metadata, but no such concept of partial/complete for dirtrees. But, we'll only ever scan dirtrees if we scan commits, which is what the section above fixes. Closes: https://github.com/ostreedev/ostree/issues/543 Closes: #564 Approved by: alexlarsson --- src/libostree/ostree-repo-pull.c | 80 +++++++++++--------------------- 1 file changed, 26 insertions(+), 54 deletions(-) diff --git a/src/libostree/ostree-repo-pull.c b/src/libostree/ostree-repo-pull.c index 183812cc..fd35e87a 100644 --- a/src/libostree/ostree-repo-pull.c +++ b/src/libostree/ostree-repo-pull.c @@ -102,7 +102,6 @@ typedef struct { gboolean is_untrusted; GPtrArray *dirs; - gboolean commitpartial_exists; gboolean have_previous_bytes; guint64 previous_bytes_sec; @@ -1036,11 +1035,17 @@ scan_commit_object (OtPullData *pull_data, GError **error) { gboolean ret = FALSE; + /* If we found a legacy transaction flag, assume we have to scan. + * We always do a scan of dirtree objects; see + * https://github.com/ostreedev/ostree/issues/543 + */ + OstreeRepoCommitState commitstate; g_autoptr(GVariant) commit = NULL; g_autoptr(GVariant) parent_csum = NULL; const guchar *parent_csum_bytes = NULL; gpointer depthp; gint depth; + gboolean is_partial; if (recursion_depth > OSTREE_MAX_RECURSION) { @@ -1089,6 +1094,13 @@ scan_commit_object (OtPullData *pull_data, } } + if (!ostree_repo_load_commit (pull_data->repo, checksum, &commit, &commitstate, error)) + goto out; + + /* If we found a legacy transaction flag, assume all commits are partial */ + is_partial = pull_data->legacy_transaction_resuming + || (commitstate & OSTREE_REPO_COMMIT_STATE_PARTIAL) > 0; + if (!ostree_repo_load_variant (pull_data->repo, OSTREE_OBJECT_TYPE_COMMIT, checksum, &commit, error)) goto out; @@ -1137,7 +1149,11 @@ scan_commit_object (OtPullData *pull_data, } } - if (!pull_data->is_commit_only) + /* We only recurse to looking whether we need dirtree/dirmeta + * objects if the commit is partial, and we're not doing a + * commit-only fetch. + */ + if (is_partial && !pull_data->is_commit_only) { g_autoptr(GVariant) tree_contents_csum = NULL; g_autoptr(GVariant) tree_meta_csum = NULL; @@ -1251,8 +1267,11 @@ scan_one_metadata_object_c (OtPullData *pull_data, do_fetch_detached = (objtype == OSTREE_OBJECT_TYPE_COMMIT); enqueue_one_object_request (pull_data, tmp_checksum, objtype, path, do_fetch_detached, FALSE); } - else if (objtype == OSTREE_OBJECT_TYPE_COMMIT && pull_data->is_commit_only) + else if (is_stored && objtype == OSTREE_OBJECT_TYPE_COMMIT) { + /* For commits, always refetch detached metadata. */ + enqueue_one_object_request (pull_data, tmp_checksum, objtype, path, TRUE, TRUE); + if (!scan_commit_object (pull_data, tmp_checksum, recursion_depth, pull_data->cancellable, error)) goto out; @@ -1260,59 +1279,12 @@ scan_one_metadata_object_c (OtPullData *pull_data, g_hash_table_insert (pull_data->scanned_metadata, g_variant_ref (object), object); pull_data->n_scanned_metadata++; } - else if (is_stored) + else if (is_stored && objtype == OSTREE_OBJECT_TYPE_DIR_TREE) { - gboolean do_scan = pull_data->legacy_transaction_resuming || is_requested || pull_data->commitpartial_exists; + if (!scan_dirtree_object (pull_data, tmp_checksum, path, recursion_depth, + pull_data->cancellable, error)) + goto out; - /* For commits, always refetch detached metadata. */ - if (objtype == OSTREE_OBJECT_TYPE_COMMIT) - enqueue_one_object_request (pull_data, tmp_checksum, objtype, path, TRUE, TRUE); - - /* For commits, check whether we only had a partial fetch */ - if (!do_scan && objtype == OSTREE_OBJECT_TYPE_COMMIT) - { - OstreeRepoCommitState commitstate; - - if (!ostree_repo_load_commit (pull_data->repo, tmp_checksum, NULL, &commitstate, error)) - goto out; - - if (commitstate & OSTREE_REPO_COMMIT_STATE_PARTIAL) - { - do_scan = TRUE; - pull_data->commitpartial_exists = TRUE; - } - else if (pull_data->maxdepth != 0) - { - /* Not fully accurate, but the cost here of scanning all - * input commit objects if we're doing a depth fetch is - * pretty low. We'll do more accurate handling of depth - * when parsing the actual commit. - */ - do_scan = TRUE; - } - } - - if (do_scan) - { - switch (objtype) - { - case OSTREE_OBJECT_TYPE_COMMIT: - if (!scan_commit_object (pull_data, tmp_checksum, recursion_depth, - pull_data->cancellable, error)) - goto out; - break; - case OSTREE_OBJECT_TYPE_DIR_META: - break; - case OSTREE_OBJECT_TYPE_DIR_TREE: - if (!scan_dirtree_object (pull_data, tmp_checksum, path, recursion_depth, - pull_data->cancellable, error)) - goto out; - break; - default: - g_assert_not_reached (); - break; - } - } g_hash_table_insert (pull_data->scanned_metadata, g_variant_ref (object), object); pull_data->n_scanned_metadata++; } From bd45e7ac19a4030d6dbd276004aa1678494ec74d Mon Sep 17 00:00:00 2001 From: Alexander Larsson Date: Wed, 16 Nov 2016 22:46:45 +0100 Subject: [PATCH 05/24] commit: Fix reading xattrs from OstreeRepoFile:s When doing commit --tree=ref=XXX while at the same time applying some form of modifier, ostree dies trying to read the xattrs using the raw syscalls. We fix this by falling back to ostree_repo_file_get_xattrs() in this case. Also adds a testcase for this. Closes: #577 Approved by: cgwalters --- src/libostree/ostree-repo-commit.c | 10 +++++++++- tests/basic-test.sh | 5 ++++- 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/src/libostree/ostree-repo-commit.c b/src/libostree/ostree-repo-commit.c index 7d78a19b..6539c26b 100644 --- a/src/libostree/ostree-repo-commit.c +++ b/src/libostree/ostree-repo-commit.c @@ -2281,7 +2281,15 @@ get_modified_xattrs (OstreeRepo *self, } else if (!(modifier && (modifier->flags & OSTREE_REPO_COMMIT_MODIFIER_FLAGS_SKIP_XATTRS) > 0)) { - if (path) + if (path && OSTREE_IS_REPO_FILE (path)) + { + if (!ostree_repo_file_get_xattrs (OSTREE_REPO_FILE (path), + &ret_xattrs, + cancellable, + error)) + goto out; + } + else if (path) { if (!glnx_dfd_name_get_all_xattrs (AT_FDCWD, gs_file_get_path_cached (path), &ret_xattrs, cancellable, error)) diff --git a/tests/basic-test.sh b/tests/basic-test.sh index 9db56e77..ad70522a 100755 --- a/tests/basic-test.sh +++ b/tests/basic-test.sh @@ -19,7 +19,7 @@ set -euo pipefail -echo "1..58" +echo "1..59" $OSTREE checkout test2 checkout-test2 echo "ok checkout" @@ -178,6 +178,9 @@ echo "ok user checkout" $OSTREE commit -b test2 -s "Another commit" --tree=ref=test2 echo "ok commit from ref" +$OSTREE commit -b test2 -s "Another commit with modifier" --tree=ref=test2 --owner-uid=`id -u` +echo "ok commit from ref with modifier" + $OSTREE commit -b trees/test2 -s 'ref with / in it' --tree=ref=test2 echo "ok commit ref with /" From a6cfe62eb8780491edf884fc4ad84c8a4c0e950f Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Wed, 16 Nov 2016 09:19:52 -0500 Subject: [PATCH 06/24] lib: Define and use cleanup functions for gpgme Just a cleanup in preparation for future work. Closes: #575 Approved by: giuseppe --- src/libostree/ostree-gpg-verifier.c | 14 +++----------- src/libostree/ostree-repo.c | 15 +++------------ src/libotutil/ot-gpg-utils.h | 6 ++++++ 3 files changed, 12 insertions(+), 23 deletions(-) diff --git a/src/libostree/ostree-gpg-verifier.c b/src/libostree/ostree-gpg-verifier.c index d487507f..9eae7ceb 100644 --- a/src/libostree/ostree-gpg-verifier.c +++ b/src/libostree/ostree-gpg-verifier.c @@ -25,6 +25,7 @@ #include "libglnx.h" #include "ostree-gpg-verifier.h" +#include "ot-gpg-utils.h" #include "ostree-gpg-verify-result-private.h" #include "otutil.h" @@ -89,10 +90,9 @@ _ostree_gpg_verifier_check_signature (OstreeGpgVerifier *self, GCancellable *cancellable, GError **error) { - gpgme_ctx_t gpg_ctx = NULL; gpgme_error_t gpg_error = 0; - gpgme_data_t data_buffer = NULL; - gpgme_data_t signature_buffer = NULL; + ot_auto_gpgme_data gpgme_data_t data_buffer = NULL; + ot_auto_gpgme_data gpgme_data_t signature_buffer = NULL; g_autofree char *tmp_dir = NULL; g_autoptr(GOutputStream) target_stream = NULL; OstreeGpgVerifyResult *result = NULL; @@ -191,14 +191,6 @@ _ostree_gpg_verifier_check_signature (OstreeGpgVerifier *self, success = TRUE; out: - - if (gpg_ctx != NULL) - gpgme_release (gpg_ctx); - if (data_buffer != NULL) - gpgme_data_release (data_buffer); - if (signature_buffer != NULL) - gpgme_data_release (signature_buffer); - if (success) { /* Keep the temporary directory around for the life of the result diff --git a/src/libostree/ostree-repo.c b/src/libostree/ostree-repo.c index ad629421..183feba5 100644 --- a/src/libostree/ostree-repo.c +++ b/src/libostree/ostree-repo.c @@ -1440,9 +1440,9 @@ ostree_repo_remote_gpg_import (OstreeRepo *self, GError **error) { OstreeRemote *remote; - gpgme_ctx_t source_context = NULL; - gpgme_ctx_t target_context = NULL; - gpgme_data_t data_buffer = NULL; + ot_auto_gpgme_ctx gpgme_ctx_t source_context = NULL; + ot_auto_gpgme_ctx gpgme_ctx_t target_context = NULL; + ot_auto_gpgme_data gpgme_data_t data_buffer = NULL; gpgme_import_result_t import_result; gpgme_import_status_t import_status; const char *tmp_dir = NULL; @@ -1700,15 +1700,6 @@ out: if (target_tmp_dir != NULL) (void) glnx_shutil_rm_rf_at (AT_FDCWD, target_tmp_dir, NULL, NULL); - if (source_context != NULL) - gpgme_release (source_context); - - if (target_context != NULL) - gpgme_release (target_context); - - if (data_buffer != NULL) - gpgme_data_release (data_buffer); - g_prefix_error (error, "GPG: "); return ret; diff --git a/src/libotutil/ot-gpg-utils.h b/src/libotutil/ot-gpg-utils.h index edb249f4..b479a3a9 100644 --- a/src/libotutil/ot-gpg-utils.h +++ b/src/libotutil/ot-gpg-utils.h @@ -22,9 +22,15 @@ #include #include +#include "libglnx.h" G_BEGIN_DECLS +GLNX_DEFINE_CLEANUP_FUNCTION0(gpgme_data_t, ot_cleanup_gpgme_data, gpgme_data_release) +#define ot_auto_gpgme_data __attribute__((cleanup(ot_cleanup_gpgme_data))) +GLNX_DEFINE_CLEANUP_FUNCTION0(gpgme_ctx_t, ot_cleanup_gpgme_ctx, gpgme_release) +#define ot_auto_gpgme_ctx __attribute__((cleanup(ot_cleanup_gpgme_ctx))) + void ot_gpgme_error_to_gio_error (gpgme_error_t gpg_error, GError **error); gboolean ot_gpgme_ctx_tmp_home_dir (gpgme_ctx_t gpgme_ctx, From 3cd5e6b41a6730ff2624ab125b51e12601913712 Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Wed, 16 Nov 2016 09:10:39 -0500 Subject: [PATCH 07/24] lib: Split out helper function to create GPG context In prep for future work. Closes: #575 Approved by: giuseppe --- src/libostree/ostree-repo.c | 54 +++++++----------------------------- src/libotutil/ot-gpg-utils.c | 33 ++++++++++++++++++++++ src/libotutil/ot-gpg-utils.h | 3 ++ 3 files changed, 46 insertions(+), 44 deletions(-) diff --git a/src/libostree/ostree-repo.c b/src/libostree/ostree-repo.c index 183feba5..584e570c 100644 --- a/src/libostree/ostree-repo.c +++ b/src/libostree/ostree-repo.c @@ -1479,13 +1479,9 @@ ostree_repo_remote_gpg_import (OstreeRepo *self, * the keys to a new pubring.gpg file. If the key data format is ASCII * armored, this step will convert them to binary. */ - gpg_error = gpgme_new (&source_context); - if (gpg_error != GPG_ERR_NO_ERROR) - { - ot_gpgme_error_to_gio_error (gpg_error, error); - g_prefix_error (error, "Unable to create context: "); - goto out; - } + source_context = ot_gpgme_new_ctx (NULL, error); + if (!source_context) + goto out; if (source_stream != NULL) { @@ -1569,13 +1565,9 @@ ostree_repo_remote_gpg_import (OstreeRepo *self, * of the remote's keyring file. We'll let the import operation alter * the pubring.gpg file, then rename it back to its permanent home. */ - gpg_error = gpgme_new (&target_context); - if (gpg_error != GPG_ERR_NO_ERROR) - { - ot_gpgme_error_to_gio_error (gpg_error, error); - g_prefix_error (error, "Unable to create context: "); - goto out; - } + target_context = ot_gpgme_new_ctx (NULL, error); + if (!target_context) + goto out; /* No need for an output stream since we copy in a pubring.gpg. */ if (!ot_gpgme_ctx_tmp_home_dir (target_context, tmp_dir, &target_tmp_dir, @@ -3980,7 +3972,6 @@ sign_data (OstreeRepo *self, g_autoptr(GOutputStream) tmp_signature_output = NULL; gpgme_ctx_t context = NULL; g_autoptr(GBytes) ret_signature = NULL; - gpgme_engine_info_t info; gpgme_error_t err; gpgme_key_t key = NULL; gpgme_data_t commit_buffer = NULL; @@ -3992,34 +3983,9 @@ sign_data (OstreeRepo *self, goto out; tmp_signature_output = g_unix_output_stream_new (tmp_fd, FALSE); - if ((err = gpgme_new (&context)) != GPG_ERR_NO_ERROR) - { - ot_gpgme_error_to_gio_error (err, error); - g_prefix_error (error, "Unable to create gpg context: "); - goto out; - } - - info = gpgme_ctx_get_engine_info (context); - - if ((err = gpgme_set_protocol (context, GPGME_PROTOCOL_OpenPGP)) != - GPG_ERR_NO_ERROR) - { - ot_gpgme_error_to_gio_error (err, error); - g_prefix_error (error, "Unable to set gpg protocol: "); - goto out; - } - - if (homedir != NULL) - { - if ((err = gpgme_ctx_set_engine_info (context, info->protocol, NULL, homedir)) - != GPG_ERR_NO_ERROR) - { - ot_gpgme_error_to_gio_error (err, error); - g_prefix_error (error, "Unable to set gpg homedir to '%s': ", - homedir); - goto out; - } - } + context = ot_gpgme_new_ctx (homedir, error); + if (!context) + goto out; /* Get the secret keys with the given key id */ err = gpgme_get_key (context, key_id, &key, 1); @@ -4036,7 +4002,7 @@ sign_data (OstreeRepo *self, g_prefix_error (error, "Unable to lookup key ID %s: ", key_id); goto out; } - + /* Add the key to the context as a signer */ if ((err = gpgme_signers_add (context, key)) != GPG_ERR_NO_ERROR) { diff --git a/src/libotutil/ot-gpg-utils.c b/src/libotutil/ot-gpg-utils.c index 9042d88b..b71f4845 100644 --- a/src/libotutil/ot-gpg-utils.c +++ b/src/libotutil/ot-gpg-utils.c @@ -411,3 +411,36 @@ ot_gpgme_data_output (GOutputStream *output_stream) return data; } + +gpgme_ctx_t +ot_gpgme_new_ctx (const char *homedir, + GError **error) +{ + gpgme_error_t err; + ot_auto_gpgme_ctx gpgme_ctx_t context = NULL; + + if ((err = gpgme_new (&context)) != GPG_ERR_NO_ERROR) + { + ot_gpgme_error_to_gio_error (err, error); + g_prefix_error (error, "Unable to create gpg context: "); + return NULL; + } + + if (homedir != NULL) + { + gpgme_engine_info_t info; + + info = gpgme_ctx_get_engine_info (context); + + if ((err = gpgme_ctx_set_engine_info (context, info->protocol, NULL, homedir)) + != GPG_ERR_NO_ERROR) + { + ot_gpgme_error_to_gio_error (err, error); + g_prefix_error (error, "Unable to set gpg homedir to '%s': ", + homedir); + return NULL; + } + } + + return g_steal_pointer (&context); +} diff --git a/src/libotutil/ot-gpg-utils.h b/src/libotutil/ot-gpg-utils.h index b479a3a9..c2337f7b 100644 --- a/src/libotutil/ot-gpg-utils.h +++ b/src/libotutil/ot-gpg-utils.h @@ -43,4 +43,7 @@ gboolean ot_gpgme_ctx_tmp_home_dir (gpgme_ctx_t gpgme_ctx, gpgme_data_t ot_gpgme_data_input (GInputStream *input_stream); gpgme_data_t ot_gpgme_data_output (GOutputStream *output_stream); +gpgme_ctx_t ot_gpgme_new_ctx (const char *homedir, + GError **error); + G_END_DECLS From f244c702772c69378099685316033d4a6f7b862c Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Wed, 16 Nov 2016 09:13:54 -0500 Subject: [PATCH 08/24] Add "gpgkeypath" option to remotes For Project Atomic, we already have RPM signatures which use files in `/etc/pki/rpm-gpg`. It's convenient to simply bind the OSTree remote configuration to those file paths, rather than having duplicate key data. This does mean that we need to parse the files for verification, so we end up importing them into the verifier's temporary keyring, which is a bit ugly, but it's what other projects do. Closes: https://github.com/ostreedev/ostree/issues/573 Closes: #575 Approved by: giuseppe --- man/ostree.repo-config.xml | 3 +- man/ostree.xml | 8 +++-- src/libostree/ostree-gpg-verifier.c | 51 +++++++++++++++++++++++++++++ src/libostree/ostree-gpg-verifier.h | 3 ++ src/libostree/ostree-repo.c | 8 +++++ tests/test-remote-gpg-import.sh | 31 ++++++++++++++++-- 6 files changed, 99 insertions(+), 5 deletions(-) diff --git a/man/ostree.repo-config.xml b/man/ostree.repo-config.xml index 1182aa93..876c9dfc 100644 --- a/man/ostree.repo-config.xml +++ b/man/ostree.repo-config.xml @@ -199,7 +199,8 @@ Boston, MA 02111-1307, USA. Per-remote GPG keyrings and verification - OSTree supports a per-remote GPG keyring. For more information see + OSTree supports a per-remote GPG keyring, as well as a + gpgkeypath option. For more information see ostree1. in the section GPG verification. diff --git a/man/ostree.xml b/man/ostree.xml index 80b0b0c1..e31d58b2 100644 --- a/man/ostree.xml +++ b/man/ostree.xml @@ -433,8 +433,12 @@ Boston, MA 02111-1307, USA. in this directory. - In addition to the system repository, OSTree supports a - per-remote + In addition to the system repository, OSTree supports two + other paths. First, there is a + gpgkeypath option for remotes, which must + point to the filename of an ASCII-armored key. + + Second, there is support for a per-remote remotename.trustedkeys.gpg file stored in the toplevel of the repository (alongside objects/ and such). This is diff --git a/src/libostree/ostree-gpg-verifier.c b/src/libostree/ostree-gpg-verifier.c index 9eae7ceb..eda05e9a 100644 --- a/src/libostree/ostree-gpg-verifier.c +++ b/src/libostree/ostree-gpg-verifier.c @@ -40,6 +40,7 @@ struct OstreeGpgVerifier { GObject parent; GList *keyrings; + GPtrArray *key_ascii_files; }; G_DEFINE_TYPE (OstreeGpgVerifier, _ostree_gpg_verifier, G_TYPE_OBJECT) @@ -50,6 +51,8 @@ ostree_gpg_verifier_finalize (GObject *object) OstreeGpgVerifier *self = OSTREE_GPG_VERIFIER (object); g_list_free_full (self->keyrings, g_object_unref); + if (self->key_ascii_files) + g_ptr_array_unref (self->key_ascii_files); G_OBJECT_CLASS (_ostree_gpg_verifier_parent_class)->finalize (object); } @@ -98,6 +101,7 @@ _ostree_gpg_verifier_check_signature (OstreeGpgVerifier *self, OstreeGpgVerifyResult *result = NULL; gboolean success = FALSE; GList *link; + int armor; /* GPGME has no API for using multiple keyrings (aka, gpg --keyring), * so we concatenate all the keyring files into one pubring.gpg in a @@ -149,6 +153,44 @@ _ostree_gpg_verifier_check_signature (OstreeGpgVerifier *self, if (!g_output_stream_close (target_stream, cancellable, error)) goto out; + /* Save the previous armor value - we need it on for importing ASCII keys */ + armor = gpgme_get_armor (result->context); + gpgme_set_armor (result->context, 1); + + /* Now, use the API to import ASCII-armored keys */ + if (self->key_ascii_files) + { + for (guint i = 0; i < self->key_ascii_files->len; i++) + { + const char *path = self->key_ascii_files->pdata[i]; + glnx_fd_close int fd = -1; + ot_auto_gpgme_data gpgme_data_t kdata = NULL; + + fd = openat (AT_FDCWD, path, O_RDONLY | O_CLOEXEC) ; + if (fd < 0) + { + glnx_set_prefix_error_from_errno (error, "Opening %s", path); + goto out; + } + + gpg_error = gpgme_data_new_from_fd (&kdata, fd); + if (gpg_error != GPG_ERR_NO_ERROR) + { + ot_gpgme_error_to_gio_error (gpg_error, error); + goto out; + } + + gpg_error = gpgme_op_import (result->context, kdata); + if (gpg_error != GPG_ERR_NO_ERROR) + { + ot_gpgme_error_to_gio_error (gpg_error, error); + goto out; + } + } + } + + gpgme_set_armor (result->context, armor); + /* Both the signed data and signature GBytes instances will outlive the * gpgme_data_t structs, so we can safely reuse the GBytes memory buffer * directly and avoid a copy. */ @@ -225,6 +267,15 @@ _ostree_gpg_verifier_add_keyring (OstreeGpgVerifier *self, self->keyrings = g_list_append (self->keyrings, g_object_ref (path)); } +void +_ostree_gpg_verifier_add_key_ascii_file (OstreeGpgVerifier *self, + const char *path) +{ + if (!self->key_ascii_files) + self->key_ascii_files = g_ptr_array_new_with_free_func (g_free); + g_ptr_array_add (self->key_ascii_files, g_strdup (path)); +} + gboolean _ostree_gpg_verifier_add_keyring_dir (OstreeGpgVerifier *self, GFile *path, diff --git a/src/libostree/ostree-gpg-verifier.h b/src/libostree/ostree-gpg-verifier.h index 2db39f3b..4156d1bd 100644 --- a/src/libostree/ostree-gpg-verifier.h +++ b/src/libostree/ostree-gpg-verifier.h @@ -62,4 +62,7 @@ gboolean _ostree_gpg_verifier_add_global_keyring_dir (OstreeGpgVerifier *s void _ostree_gpg_verifier_add_keyring (OstreeGpgVerifier *self, GFile *path); +void _ostree_gpg_verifier_add_key_ascii_file (OstreeGpgVerifier *self, + const char *path); + G_END_DECLS diff --git a/src/libostree/ostree-repo.c b/src/libostree/ostree-repo.c index 584e570c..c0cbede6 100644 --- a/src/libostree/ostree-repo.c +++ b/src/libostree/ostree-repo.c @@ -4282,6 +4282,7 @@ _ostree_repo_gpg_verify_data_internal (OstreeRepo *self, } else if (remote_name != NULL) { + g_autofree char *gpgkeypath = NULL; /* Add the remote's keyring file if it exists. */ OstreeRemote *remote; @@ -4299,6 +4300,13 @@ _ostree_repo_gpg_verify_data_internal (OstreeRepo *self, add_global_keyring_dir = FALSE; } + if (!ot_keyfile_get_value_with_default (remote->options, remote->group, "gpgkeypath", NULL, + &gpgkeypath, error)) + return NULL; + + if (gpgkeypath) + _ostree_gpg_verifier_add_key_ascii_file (verifier, gpgkeypath); + ost_remote_unref (remote); } diff --git a/tests/test-remote-gpg-import.sh b/tests/test-remote-gpg-import.sh index 8d155f79..4dbe7fd9 100755 --- a/tests/test-remote-gpg-import.sh +++ b/tests/test-remote-gpg-import.sh @@ -26,7 +26,7 @@ unset OSTREE_GPG_HOME setup_fake_remote_repo1 "archive-z2" -echo "1..1" +echo "1..2" cd ${test_tmpdir} mkdir repo @@ -143,5 +143,32 @@ if ${OSTREE} pull R2:main >/dev/null 2>&1; then fi ${OSTREE} pull R3:main >/dev/null -libtest_cleanup_gpg echo "ok" + +rm repo/refs/remotes/* -rf +${OSTREE} prune --refs-only + +# Test the successful gpgkeypath option +${OSTREE} remote add --set=gpgkeypath=${test_tmpdir}/gpghome/key3.asc R4 $(cat httpd-address)/ostree/gnomerepo +${OSTREE} pull R4:main >/dev/null + +rm repo/refs/remotes/* -rf +${OSTREE} prune --refs-only + +${OSTREE} remote add --set=gpgkeypath=${test_tmpdir}/gpghome/INVALIDKEYPATH.asc R5 $(cat httpd-address)/ostree/gnomerepo +if ${OSTREE} pull R5:main 2>err.txt; then + assert_not_reached "Unexpectedly succeeded at pulling with nonexistent key" +fi +assert_file_has_content err.txt "INVALIDKEYPATH.*No such file or directory" + +rm repo/refs/remotes/* -rf +${OSTREE} prune --refs-only + +${OSTREE} remote add --set=gpgkeypath=${test_tmpdir}/gpghome/key2.asc R6 $(cat httpd-address)/ostree/gnomerepo +if ${OSTREE} pull R6:main 2>err.txt; then + assert_not_reached "Unexpectedly succeeded at pulling with different key" +fi +assert_file_has_content err.txt "GPG signatures found, but none are in trusted keyring" + +echo "ok" +libtest_cleanup_gpg From 24bf257ee9f16959a6f480c7c1c16cdbe4aabc91 Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Wed, 16 Nov 2016 11:50:43 -0500 Subject: [PATCH 09/24] lib: Add an API to GPG verify a commit given a remote Conceptually we've been moving towards having our GPG verification paths be per-remote. The code internally supports this, but we didn't expose an API to use it conveniently. This came up when trying to add a new `gpgkeypath` option, since right now rpm-ostree manually finds keyrings for the remote, and hence it wasn't looking at the keypath, and said "Unknown key" in status. Adding an API fixes this nicely. Closes: #576 Approved by: giuseppe --- apidoc/ostree-sections.txt | 1 + src/libostree/libostree.sym | 12 ++++++++++-- src/libostree/ostree-repo.c | 30 ++++++++++++++++++++++++++++++ src/libostree/ostree-repo.h | 8 ++++++++ src/ostree/ot-builtin-show.c | 16 +++++++++++++--- tests/test-commit-sign.sh | 2 ++ 6 files changed, 64 insertions(+), 5 deletions(-) diff --git a/apidoc/ostree-sections.txt b/apidoc/ostree-sections.txt index 37c7e11e..3cda9150 100644 --- a/apidoc/ostree-sections.txt +++ b/apidoc/ostree-sections.txt @@ -379,6 +379,7 @@ ostree_repo_add_gpg_signature_summary ostree_repo_gpg_verify_data ostree_repo_verify_commit ostree_repo_verify_commit_ext +ostree_repo_verify_commit_for_remote ostree_repo_verify_summary ostree_repo_regenerate_summary diff --git a/src/libostree/libostree.sym b/src/libostree/libostree.sym index fb7e5848..ed382fe5 100644 --- a/src/libostree/libostree.sym +++ b/src/libostree/libostree.sym @@ -364,9 +364,17 @@ global: * NOTE NOTE NOTE */ +LIBOSTREE_2016.14 { +global: + ostree_repo_verify_commit_for_remote; +} LIBOSTREE_2016.8; + +/* Section for the stable release *after* this development one; don't + * edit this other than to update the last number. */ + /* Remove comment when first new symbol is added, replace XX with new stable version. -LIBOSTREE_2016.XX +LIBOSTREE_2016.XX { global: someostree_symbol_deleteme; -} LIBOSTREE_2016.8; +} LIBOSTREE_2016.14; * Remove comment when first new symbol is added */ diff --git a/src/libostree/ostree-repo.c b/src/libostree/ostree-repo.c index c0cbede6..d3762521 100644 --- a/src/libostree/ostree-repo.c +++ b/src/libostree/ostree-repo.c @@ -4509,6 +4509,36 @@ ostree_repo_verify_commit_ext (OstreeRepo *self, error); } +/** + * ostree_repo_verify_commit_for_remote: + * @self: Repository + * @commit_checksum: ASCII SHA256 checksum + * @remote: OSTree remote to use for configuration + * @cancellable: Cancellable + * @error: Error + * + * Read GPG signature(s) on the commit named by the ASCII checksum + * @commit_checksum and return detailed results, based on the keyring + * configured for @remote. + * + * Returns: (transfer full): an #OstreeGpgVerifyResult, or %NULL on error + */ +OstreeGpgVerifyResult * +ostree_repo_verify_commit_for_remote (OstreeRepo *self, + const gchar *commit_checksum, + const gchar *remote_name, + GCancellable *cancellable, + GError **error) +{ + return _ostree_repo_verify_commit_internal (self, + commit_checksum, + remote_name, + NULL, + NULL, + cancellable, + error); +} + /** * ostree_repo_gpg_verify_data: * @self: Repository diff --git a/src/libostree/ostree-repo.h b/src/libostree/ostree-repo.h index f1f9da41..d5303e41 100644 --- a/src/libostree/ostree-repo.h +++ b/src/libostree/ostree-repo.h @@ -1064,6 +1064,14 @@ OstreeGpgVerifyResult * ostree_repo_verify_commit_ext (OstreeRepo *self, GCancellable *cancellable, GError **error); +_OSTREE_PUBLIC +OstreeGpgVerifyResult * +ostree_repo_verify_commit_for_remote (OstreeRepo *self, + const gchar *commit_checksum, + const gchar *remote_name, + GCancellable *cancellable, + GError **error); + _OSTREE_PUBLIC OstreeGpgVerifyResult * ostree_repo_gpg_verify_data (OstreeRepo *self, const gchar *remote_name, diff --git a/src/ostree/ot-builtin-show.c b/src/ostree/ot-builtin-show.c index ef541c2a..a9c1fbbc 100644 --- a/src/ostree/ot-builtin-show.c +++ b/src/ostree/ot-builtin-show.c @@ -34,6 +34,7 @@ static char* opt_print_metadata_key; static char* opt_print_detached_metadata_key; static gboolean opt_raw; static char *opt_gpg_homedir; +static char *opt_gpg_verify_remote; static GOptionEntry options[] = { { "print-related", 0, 0, G_OPTION_ARG_NONE, &opt_print_related, "Show the \"related\" commits", NULL }, @@ -42,6 +43,7 @@ static GOptionEntry options[] = { { "print-detached-metadata-key", 0, 0, G_OPTION_ARG_STRING, &opt_print_detached_metadata_key, "Print string value of detached metadata key", "KEY" }, { "raw", 0, 0, G_OPTION_ARG_NONE, &opt_raw, "Show raw variant data" }, { "gpg-homedir", 0, 0, G_OPTION_ARG_STRING, &opt_gpg_homedir, "GPG Homedir to use when looking for keyrings", "HOMEDIR"}, + { "gpg-verify-remote", 0, 0, G_OPTION_ARG_STRING, &opt_gpg_verify_remote, "Use REMOTE name for GPG configuration", "REMOTE"}, { NULL } }; @@ -170,9 +172,17 @@ print_object (OstreeRepo *repo, GError *local_error = NULL; g_autoptr(GFile) gpg_homedir = opt_gpg_homedir ? g_file_new_for_path (opt_gpg_homedir) : NULL; - result = ostree_repo_verify_commit_ext (repo, checksum, - gpg_homedir, NULL, NULL, - &local_error); + if (opt_gpg_verify_remote) + { + result = ostree_repo_verify_commit_for_remote (repo, checksum, opt_gpg_verify_remote, + NULL, &local_error); + } + else + { + result = ostree_repo_verify_commit_ext (repo, checksum, + gpg_homedir, NULL, NULL, + &local_error); + } if (g_error_matches (local_error, G_IO_ERROR, G_IO_ERROR_NOT_FOUND)) { diff --git a/tests/test-commit-sign.sh b/tests/test-commit-sign.sh index 01eb45f8..60265c1a 100755 --- a/tests/test-commit-sign.sh +++ b/tests/test-commit-sign.sh @@ -80,6 +80,8 @@ mkdir repo ${CMD_PREFIX} ostree --repo=repo init ${CMD_PREFIX} ostree --repo=repo remote add origin $(cat httpd-address)/ostree/gnomerepo ${CMD_PREFIX} ostree --repo=repo pull origin main +${CMD_PREFIX} ostree --repo=repo show --gpg-verify-remote=origin main | grep -o 'Found [[:digit:]] signature' > show-verify-remote +assert_file_has_content show-verify-remote 'Found 1 signature' rm repo -rf # A test with corrupted detached signature From 0ee9e221beecd2261f75da46afa02d54b1230886 Mon Sep 17 00:00:00 2001 From: William Manley Date: Tue, 19 Jul 2016 03:14:26 +0100 Subject: [PATCH 10/24] ostree commit: Fix combining trees with multiple --tree=ref arguments You'd expect ostree commit --tree=ref=A --tree=ref=B to produce a commit with the union of the trees given. Instead you'd get a commit with the contents of just the latter commit. This was due to an optimisation where we'd skip filling out the `files` and `subdirs` members of the mtree, just filling in the metadata instead. This backfires becuase this same code relies on checking the `files` and `subdirs` members itself to work out whether the mtree is empty. This commit removes the optimisation, fixing the bug. Maybe there's a way to keep the optimisation and still fix the bug but it's not obvious to me. Closes: #581 Approved by: cgwalters --- src/libostree/ostree-repo-commit.c | 10 ---------- tests/basic-test.sh | 16 +++++++++++++++- 2 files changed, 15 insertions(+), 11 deletions(-) diff --git a/src/libostree/ostree-repo-commit.c b/src/libostree/ostree-repo-commit.c index 6539c26b..84d15374 100644 --- a/src/libostree/ostree-repo-commit.c +++ b/src/libostree/ostree-repo-commit.c @@ -2550,16 +2550,6 @@ write_directory_to_mtree_internal (OstreeRepo *self, ostree_mutable_tree_set_metadata_checksum (mtree, ostree_repo_file_tree_get_metadata_checksum (repo_dir)); - /* If the mtree was empty beforehand, the checksums on the mtree can simply - * become the checksums on the tree in the repo. Super simple. */ - if (g_hash_table_size (ostree_mutable_tree_get_files (mtree)) == 0 && - g_hash_table_size (ostree_mutable_tree_get_subdirs (mtree)) == 0) - { - ostree_mutable_tree_set_contents_checksum (mtree, ostree_repo_file_tree_get_contents_checksum (repo_dir)); - ret = TRUE; - goto out; - } - filter_result = OSTREE_REPO_COMMIT_FILTER_ALLOW; } else diff --git a/tests/basic-test.sh b/tests/basic-test.sh index ad70522a..bb387501 100755 --- a/tests/basic-test.sh +++ b/tests/basic-test.sh @@ -19,7 +19,7 @@ set -euo pipefail -echo "1..59" +echo "1..60" $OSTREE checkout test2 checkout-test2 echo "ok checkout" @@ -194,6 +194,20 @@ cd ${test_tmpdir}/checkout-test2-4 $OSTREE commit -b test2 -s "no xattrs" --no-xattrs echo "ok commit with no xattrs" +mkdir tree-A tree-B +touch tree-A/file-a tree-B/file-b + +$OSTREE commit -b test3-1 -s "Initial tree" --tree=dir=tree-A +$OSTREE commit -b test3-2 -s "Replacement tree" --tree=dir=tree-B +$OSTREE commit -b test3-combined -s "combined tree" --tree=ref=test3-1 --tree=ref=test3-2 + +$OSTREE checkout test3-combined checkout-test3-combined + +assert_has_file checkout-test3-combined/file-a +assert_has_file checkout-test3-combined/file-b + +echo "ok commit combined ref trees" + # NB: The + is optional, but we need to make sure we support it cd ${test_tmpdir} cat > test-statoverride.txt < Date: Thu, 17 Nov 2016 13:48:58 -0500 Subject: [PATCH 11/24] [UBSAN] deltas: Don't call memset(NULL, NULL, 0) with no xattrs This is actually fine in practice, but it triggers this `-fsanitize=undefined` warning I saw in the test suite log: ``` src/libostree/ostree-repo-static-delta-compilation.c:160:10: runtime error: null pointer passed as argument 1, which is declared to never be null ``` Closes: #584 Approved by: jlebon --- src/libostree/ostree-repo-static-delta-compilation.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/libostree/ostree-repo-static-delta-compilation.c b/src/libostree/ostree-repo-static-delta-compilation.c index 286c74e1..4b0bc507 100644 --- a/src/libostree/ostree-repo-static-delta-compilation.c +++ b/src/libostree/ostree-repo-static-delta-compilation.c @@ -157,6 +157,9 @@ xattr_chunk_equals (const void *one, const void *two) if (l1 != l2) return FALSE; + if (l1 == 0) + return l2 == 0; + return memcmp (g_variant_get_data (v1), g_variant_get_data (v2), l1) == 0; } From f0519e541f296fc277db636567bb0f0e4d9fc62d Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Thu, 17 Nov 2016 11:39:27 -0500 Subject: [PATCH 12/24] [TSAN] main: Stop calling g_set_prgname() It turns out this is basically racy with the presence of other threads. It was really cosmetic so let's stop doing it and make `-fsanitize=thread` happy. Closes: #582 Approved by: jlebon --- src/ostree/ot-main.c | 5 ----- 1 file changed, 5 deletions(-) diff --git a/src/ostree/ot-main.c b/src/ostree/ot-main.c index 5080e8c6..0d0587f2 100644 --- a/src/ostree/ot-main.c +++ b/src/ostree/ot-main.c @@ -125,7 +125,6 @@ ostree_run (int argc, GError *error = NULL; GCancellable *cancellable = NULL; const char *command_name = NULL; - g_autofree char *prgname = NULL; gboolean success = FALSE; int in, out; @@ -202,10 +201,6 @@ ostree_run (int argc, goto out; } - prgname = g_strdup_printf ("%s %s", g_get_prgname (), command_name); - g_set_prgname (prgname); - - if (!command->fn (argc, argv, cancellable, &error)) goto out; From c1c70bceb7a77031cba2a4433e595b3e280f0bf8 Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Thu, 17 Nov 2016 11:40:59 -0500 Subject: [PATCH 13/24] [TSAN] Rework assertions to always access refcount atomically `-fsanitize=address` complained that the `refcount > 0` assertions were reading without atomics. We can fix this by reworking them to read the previous value. Closes: #582 Approved by: jlebon --- src/libostree/ostree-fetcher.c | 15 ++++++--------- src/libostree/ostree-repo-commit.c | 3 ++- src/libostree/ostree-repo.c | 7 +++---- 3 files changed, 11 insertions(+), 14 deletions(-) diff --git a/src/libostree/ostree-fetcher.c b/src/libostree/ostree-fetcher.c index 87e08441..c9161d40 100644 --- a/src/libostree/ostree-fetcher.c +++ b/src/libostree/ostree-fetcher.c @@ -128,11 +128,10 @@ G_DEFINE_TYPE (OstreeFetcher, _ostree_fetcher, G_TYPE_OBJECT) static ThreadClosure * thread_closure_ref (ThreadClosure *thread_closure) { + int refcount; g_return_val_if_fail (thread_closure != NULL, NULL); - g_return_val_if_fail (thread_closure->ref_count > 0, NULL); - - g_atomic_int_inc (&thread_closure->ref_count); - + refcount = g_atomic_int_add (&thread_closure->ref_count, 1); + g_assert (refcount > 0); return thread_closure; } @@ -140,7 +139,6 @@ static void thread_closure_unref (ThreadClosure *thread_closure) { g_return_if_fail (thread_closure != NULL); - g_return_if_fail (thread_closure->ref_count > 0); if (g_atomic_int_dec_and_test (&thread_closure->ref_count)) { @@ -197,11 +195,10 @@ pending_task_compare (gconstpointer a, static OstreeFetcherPendingURI * pending_uri_ref (OstreeFetcherPendingURI *pending) { + gint refcount; g_return_val_if_fail (pending != NULL, NULL); - g_return_val_if_fail (pending->ref_count > 0, NULL); - - g_atomic_int_inc (&pending->ref_count); - + refcount = g_atomic_int_add (&pending->ref_count, 1); + g_assert (refcount > 0); return pending; } diff --git a/src/libostree/ostree-repo-commit.c b/src/libostree/ostree-repo-commit.c index 84d15374..eb9733e8 100644 --- a/src/libostree/ostree-repo-commit.c +++ b/src/libostree/ostree-repo-commit.c @@ -2959,7 +2959,8 @@ ostree_repo_commit_modifier_new (OstreeRepoCommitModifierFlags flags, OstreeRepoCommitModifier * ostree_repo_commit_modifier_ref (OstreeRepoCommitModifier *modifier) { - g_atomic_int_inc (&modifier->refcount); + gint refcount = g_atomic_int_add (&modifier->refcount, 1); + g_assert (refcount > 0); return modifier; } diff --git a/src/libostree/ostree-repo.c b/src/libostree/ostree-repo.c index d3762521..b4cf4e38 100644 --- a/src/libostree/ostree-repo.c +++ b/src/libostree/ostree-repo.c @@ -161,11 +161,10 @@ ost_remote_new_from_keyfile (GKeyFile *keyfile, static OstreeRemote * ost_remote_ref (OstreeRemote *remote) { + gint refcount; g_return_val_if_fail (remote != NULL, NULL); - g_return_val_if_fail (remote->ref_count > 0, NULL); - - g_atomic_int_inc (&remote->ref_count); - + refcount = g_atomic_int_add (&remote->ref_count, 1); + g_assert (refcount > 0); return remote; } From abbd7acaf3767659804f557563021eee0fe7d09a Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Fri, 18 Nov 2016 20:22:10 -0500 Subject: [PATCH 14/24] pull: Dedup code for checking for > 0 valid results We have a public API for this, let's use it internally. Closes: #589 Approved by: jlebon --- src/libostree/ostree-repo-pull.c | 17 +++-------------- 1 file changed, 3 insertions(+), 14 deletions(-) diff --git a/src/libostree/ostree-repo-pull.c b/src/libostree/ostree-repo-pull.c index fd35e87a..7a4ccfdb 100644 --- a/src/libostree/ostree-repo-pull.c +++ b/src/libostree/ostree-repo-pull.c @@ -1086,12 +1086,8 @@ scan_commit_object (OtPullData *pull_data, "gpg-verify-result", checksum, result); - if (ostree_gpg_verify_result_count_valid (result) == 0) - { - g_set_error (error, G_IO_ERROR, G_IO_ERROR_FAILED, - "GPG signatures found, but none are in trusted keyring"); - goto out; - } + if (!ostree_gpg_verify_result_require_valid_signature (result, error)) + goto out; } if (!ostree_repo_load_commit (pull_data->repo, checksum, &commit, &commitstate, error)) @@ -2737,15 +2733,8 @@ ostree_repo_pull_with_options (OstreeRepo *self, NULL, cancellable, error); - if (result == NULL) + if (!ostree_gpg_verify_result_require_valid_signature (result, error)) goto out; - - if (ostree_gpg_verify_result_count_valid (result) == 0) - { - g_set_error (error, G_IO_ERROR, G_IO_ERROR_FAILED, - "GPG signatures found, but none are in trusted keyring"); - goto out; - } } if (pull_data->summary) From cb57338a12819b0b1221cda62616a0aeb5d5f489 Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Fri, 18 Nov 2016 20:23:13 -0500 Subject: [PATCH 15/24] pull: Use new per-remote API for GPG verification Trivial change, but makes things more obvious. And we get test coverage of the new API for free. Closes: #589 Approved by: jlebon --- src/libostree/ostree-repo-pull.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/src/libostree/ostree-repo-pull.c b/src/libostree/ostree-repo-pull.c index 7a4ccfdb..943e1706 100644 --- a/src/libostree/ostree-repo-pull.c +++ b/src/libostree/ostree-repo-pull.c @@ -1070,13 +1070,11 @@ scan_commit_object (OtPullData *pull_data, { glnx_unref_object OstreeGpgVerifyResult *result = NULL; - result = _ostree_repo_verify_commit_internal (pull_data->repo, - checksum, - pull_data->remote_name, - NULL, - NULL, - cancellable, - error); + result = ostree_repo_verify_commit_for_remote (pull_data->repo, + checksum, + pull_data->remote_name, + cancellable, + error); if (result == NULL) goto out; From 41ef2aeb3824d52e33cb0d9eafa62393e5065fb0 Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Sun, 20 Nov 2016 16:17:22 -0500 Subject: [PATCH 16/24] pull: Do GPG verify commit objects when using deltas The fact that we weren't doing this is at best an oversight, and for some deployment models a security vulnerability. Having both `gpg-verify` and `gpg-verify-summary` shows that we were intending them to be orthogonal/independent. Lately I've been advocating moving towards pinned TLS instead of gpg-signed summaries, and if we follow that path, performing GPG verification of commit objects even if using deltas is more important, as it provides an at-rest verifiable authenticity and integrity mechanism. Content providers which are signing their summary files and/or using TLS (particularly pinned TLS) for transport should treat this as a nice-to-have. However, for providers which are serving content over plain HTTP and relying on GPG, this is a critical update. Closes: https://github.com/ostreedev/ostree/issues/517 Closes: #589 Approved by: jlebon --- src/libostree/ostree-repo-pull.c | 71 ++++++++++++++++++++++++++------ tests/test-remote-gpg-import.sh | 37 ++++++++++++++++- 2 files changed, 94 insertions(+), 14 deletions(-) diff --git a/src/libostree/ostree-repo-pull.c b/src/libostree/ostree-repo-pull.c index 943e1706..8c18052f 100644 --- a/src/libostree/ostree-repo-pull.c +++ b/src/libostree/ostree-repo-pull.c @@ -1027,6 +1027,57 @@ static_deltapart_fetch_on_complete (GObject *object, fetch_static_delta_data_free (fetch_data); } +static gboolean +process_verify_result (OtPullData *pull_data, + const char *checksum, + OstreeGpgVerifyResult *result, + GError **error) +{ + if (result == NULL) + return FALSE; + + /* Allow callers to output the results immediately. */ + g_signal_emit_by_name (pull_data->repo, + "gpg-verify-result", + checksum, result); + + return ostree_gpg_verify_result_require_valid_signature (result, error); +} + +static gboolean +gpg_verify_unwritten_commit (OtPullData *pull_data, + const char *checksum, + GVariant *commit, + GVariant *detached_metadata, + GCancellable *cancellable, + GError **error) +{ + if (pull_data->gpg_verify) + { + glnx_unref_object OstreeGpgVerifyResult *result = NULL; + g_autoptr(GBytes) signed_data = g_variant_get_data_as_bytes (commit); + + if (!detached_metadata) + { + g_set_error_literal (error, G_IO_ERROR, G_IO_ERROR_FAILED, + "No detached metadata found for GPG verification"); + return FALSE; + } + + result = _ostree_repo_gpg_verify_with_metadata (pull_data->repo, + signed_data, + detached_metadata, + pull_data->remote_name, + NULL, NULL, + cancellable, + error); + if (!process_verify_result (pull_data, checksum, result, error)) + return FALSE; + } + + return TRUE; +} + static gboolean scan_commit_object (OtPullData *pull_data, const char *checksum, @@ -1075,16 +1126,7 @@ scan_commit_object (OtPullData *pull_data, pull_data->remote_name, cancellable, error); - - if (result == NULL) - goto out; - - /* Allow callers to output the results immediately. */ - g_signal_emit_by_name (pull_data->repo, - "gpg-verify-result", - checksum, result); - - if (!ostree_gpg_verify_result_require_valid_signature (result, error)) + if (!process_verify_result (pull_data, checksum, result, error)) goto out; } @@ -1563,7 +1605,6 @@ process_one_static_delta (OtPullData *pull_data, { g_autoptr(GVariant) to_csum_v = NULL; g_autofree char *to_checksum = NULL; - g_autoptr(GVariant) to_commit = NULL; gboolean have_to_commit; to_csum_v = g_variant_get_child_value (delta_superblock, 3); @@ -1578,10 +1619,16 @@ process_one_static_delta (OtPullData *pull_data, if (!have_to_commit) { FetchObjectData *fetch_data; + g_autoptr(GVariant) to_commit = g_variant_get_child_value (delta_superblock, 4); g_autofree char *detached_path = _ostree_get_relative_static_delta_path (from_revision, to_revision, "commitmeta"); g_autoptr(GVariant) detached_data = NULL; detached_data = g_variant_lookup_value (metadata, detached_path, G_VARIANT_TYPE("a{sv}")); + + if (!gpg_verify_unwritten_commit (pull_data, to_revision, to_commit, detached_data, + cancellable, error)) + goto out; + if (detached_data && !ostree_repo_write_commit_detached_metadata (pull_data->repo, to_revision, detached_data, @@ -1595,8 +1642,6 @@ process_one_static_delta (OtPullData *pull_data, fetch_data->is_detached_meta = FALSE; fetch_data->object_is_stored = FALSE; - to_commit = g_variant_get_child_value (delta_superblock, 4); - ostree_repo_write_metadata_async (pull_data->repo, OSTREE_OBJECT_TYPE_COMMIT, to_checksum, to_commit, pull_data->cancellable, diff --git a/tests/test-remote-gpg-import.sh b/tests/test-remote-gpg-import.sh index 4dbe7fd9..4429d8bc 100755 --- a/tests/test-remote-gpg-import.sh +++ b/tests/test-remote-gpg-import.sh @@ -26,7 +26,7 @@ unset OSTREE_GPG_HOME setup_fake_remote_repo1 "archive-z2" -echo "1..2" +echo "1..4" cd ${test_tmpdir} mkdir repo @@ -171,4 +171,39 @@ fi assert_file_has_content err.txt "GPG signatures found, but none are in trusted keyring" echo "ok" + +# Test deltas with signed commits; this test is a bit +# weird here but this file has separate per-remote keys. +cd ${test_tmpdir} +rm repo/refs/remotes/* -rf +${OSTREE} prune --refs-only +echo $(date) > workdir/testfile-for-deltas-1 +# Sign with keyid 1 for first commit +${CMD_PREFIX} ostree --repo=${test_tmpdir}/ostree-srv/gnomerepo commit -b main --gpg-sign ${TEST_GPG_KEYID_1} --gpg-homedir ${test_tmpdir}/gpghome workdir +prevrev=$(${CMD_PREFIX} ostree --repo=${test_tmpdir}/ostree-srv/gnomerepo rev-parse main) +# Pull the previous revision +${OSTREE} pull R1:main +assert_streq $(${OSTREE} rev-parse R1:main) ${prevrev} +# Sign with keyid 2, but use remote r1 +echo $(date) > workdir/testfile-for-deltas-2 +${CMD_PREFIX} ostree --repo=${test_tmpdir}/ostree-srv/gnomerepo commit -b main --gpg-sign ${TEST_GPG_KEYID_2} --gpg-homedir ${test_tmpdir}/gpghome workdir +${CMD_PREFIX} ostree --repo=${test_tmpdir}/ostree-srv/gnomerepo static-delta generate main +# Summary is signed with key1 +${CMD_PREFIX} ostree --repo=${test_tmpdir}/ostree-srv/gnomerepo summary -u --gpg-sign ${TEST_GPG_KEYID_1} --gpg-homedir ${test_tmpdir}/gpghome +newrev=$(${CMD_PREFIX} ostree --repo=${test_tmpdir}/ostree-srv/gnomerepo rev-parse main) +if ${OSTREE} pull --require-static-deltas R1:main 2>err.txt; then + assert_not_reached "Unexpectedly succeeded at pulling commit signed with untrusted key" +fi +assert_file_has_content err.txt "GPG signatures found, but none are in trusted keyring" + +echo "ok gpg untrusted signed commit for delta upgrades" + +${CMD_PREFIX} ostree --repo=${test_tmpdir}/ostree-srv/gnomerepo reset main{,^} +${CMD_PREFIX} ostree --repo=${test_tmpdir}/ostree-srv/gnomerepo commit -b main --gpg-sign ${TEST_GPG_KEYID_1} --gpg-homedir ${test_tmpdir}/gpghome workdir +${CMD_PREFIX} ostree --repo=${test_tmpdir}/ostree-srv/gnomerepo static-delta generate main +${CMD_PREFIX} ostree --repo=${test_tmpdir}/ostree-srv/gnomerepo summary -u --gpg-sign ${TEST_GPG_KEYID_1} --gpg-homedir ${test_tmpdir}/gpghome +${OSTREE} pull --require-static-deltas R1:main + +echo "ok gpg trusted signed commit for delta upgrades" + libtest_cleanup_gpg From a6eb8bbcf6d0e0bbc5163dea1d41fff1706299d5 Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Fri, 18 Nov 2016 15:07:52 -0500 Subject: [PATCH 17/24] tests: Support TEST_SKIP_CLEANUP=err I find myself often wanting to debug interactively failing tests. This makes it more convenient to keep around the temporary directories just for those tests, rather than accumulating tons of tempdirs from the successful tests as well. Closes: #588 Approved by: jlebon --- buildutil/tap-test | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/buildutil/tap-test b/buildutil/tap-test index 6b2eb5c1..c8da31e7 100755 --- a/buildutil/tap-test +++ b/buildutil/tap-test @@ -11,13 +11,24 @@ bn=$(basename $1) tempdir=$(mktemp -d /var/tmp/tap-test.XXXXXX) touch ${tempdir}/.testtmp function cleanup () { - if test -n "${TEST_SKIP_CLEANUP:-}"; then - echo "Skipping cleanup of ${tempdir}" - else if test -f ${tempdir}/.testtmp; then + if test -f ${tempdir}/.testtmp; then rm "${tempdir}" -rf fi - fi } -trap cleanup EXIT +function skip_cleanup() { + echo "Skipping cleanup of ${tempdir}" +} cd ${tempdir} ${srcd}/${bn} -k --tap +rc=$? +case "${TEST_SKIP_CLEANUP:-}" in + no|"") cleanup ;; + err) + if test $rc != 0; then + skip_cleanup + else + cleanup + fi ;; + *) skip_cleanup ;; +esac +exit $rc From eb7ba645af8bc8332dcd5a73f682d46e8642a2ce Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Fri, 18 Nov 2016 10:32:59 -0500 Subject: [PATCH 18/24] [ASAN] tests: Fix some memleaks in libarchive importer Caught by `-fsanitize=address`. Closes: #587 Approved by: jlebon --- tests/test-libarchive-import.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tests/test-libarchive-import.c b/tests/test-libarchive-import.c index ad2e4c94..05c5568d 100644 --- a/tests/test-libarchive-import.c +++ b/tests/test-libarchive-import.c @@ -468,6 +468,8 @@ entry_pathname_test_helper (gconstpointer data, gboolean on) goto out; } + archive_read_free (a); + ostree_repo_commit_modifier_unref (modifier); out: g_assert_no_error (error); } @@ -534,6 +536,7 @@ test_libarchive_selinux (gconstpointer data) g_assert_cmpstr (buf, ==, "system_u:object_r:etc_t:s0"); out: + archive_read_free (a); if (modifier) ostree_repo_commit_modifier_unref (modifier); g_assert_no_error (error); @@ -562,5 +565,6 @@ int main (int argc, char **argv) if (td.tmpd && g_getenv ("TEST_SKIP_CLEANUP") == NULL) (void) glnx_shutil_rm_rf_at (AT_FDCWD, td.tmpd, NULL, NULL); + g_free (td.tmpd); return r; } From 49b8a726221de7b9abf6c70bddadc0c21ff7b697 Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Fri, 18 Nov 2016 10:33:50 -0500 Subject: [PATCH 19/24] [ASAN] lib: Squash various leaks in library and commandline The pull one is the most likely to affect users. Otherwise mostly just cleaning up `-fsanitize=address`. Closes: #587 Approved by: jlebon --- src/libostree/ostree-metalink.c | 3 +++ src/libostree/ostree-repo-libarchive.c | 5 ++++- src/libostree/ostree-repo-pull.c | 6 +++--- .../ostree-repo-static-delta-compilation-analysis.c | 2 +- src/libostree/ostree-repo-static-delta-core.c | 4 +++- src/libostree/ostree-sepolicy.c | 4 ++-- src/libostree/ostree-sysroot-cleanup.c | 2 +- src/libostree/ostree-sysroot-upgrader.c | 1 + src/ostree/ot-builtin-export.c | 6 +++++- src/ostree/ot-builtin-fsck.c | 2 +- src/ostree/ot-remote-builtin-add-cookie.c | 4 +++- src/ostree/ot-remote-builtin-delete-cookie.c | 4 +++- 12 files changed, 30 insertions(+), 13 deletions(-) diff --git a/src/libostree/ostree-metalink.c b/src/libostree/ostree-metalink.c index b850818d..ad3d5bf9 100644 --- a/src/libostree/ostree-metalink.c +++ b/src/libostree/ostree-metalink.c @@ -632,6 +632,9 @@ _ostree_metalink_request_sync (OstreeMetalink *self, if (mainctx) g_main_context_pop_thread_default (mainctx); g_clear_object (&request.metalink); + g_clear_pointer (&request.verification_sha256, g_free); + g_clear_pointer (&request.verification_sha512, g_free); + g_clear_pointer (&request.last_metalink_error, g_free); g_clear_pointer (&request.urls, g_ptr_array_unref); g_clear_pointer (&request.parser, g_markup_parse_context_free); return ret; diff --git a/src/libostree/ostree-repo-libarchive.c b/src/libostree/ostree-repo-libarchive.c index 3a58d106..02a1180a 100644 --- a/src/libostree/ostree-repo-libarchive.c +++ b/src/libostree/ostree-repo-libarchive.c @@ -943,7 +943,10 @@ ostree_repo_write_archive_to_mtree (OstreeRepo *self, ret = TRUE; out: if (a) - (void)archive_read_close (a); + { + (void)archive_read_close (a); + (void)archive_read_free (a); + } return ret; #else g_set_error (error, G_IO_ERROR, G_IO_ERROR_NOT_SUPPORTED, diff --git a/src/libostree/ostree-repo-pull.c b/src/libostree/ostree-repo-pull.c index 8c18052f..9e960792 100644 --- a/src/libostree/ostree-repo-pull.c +++ b/src/libostree/ostree-repo-pull.c @@ -1982,7 +1982,7 @@ _ostree_repo_remote_new_fetcher (OstreeRepo *self, g_autofree char *cookie_file = g_strdup_printf ("%s.cookies.txt", remote_name); - jar_path = g_build_filename (g_file_get_path (self->repodir), cookie_file, + jar_path = g_build_filename (gs_file_get_path_cached (self->repodir), cookie_file, NULL); if (g_file_test(jar_path, G_FILE_TEST_IS_REGULAR)) @@ -2061,7 +2061,7 @@ fetch_mirrorlist (OstreeFetcher *fetcher, GError **error) { gboolean ret = FALSE; - char **lines = NULL; + g_auto(GStrv) lines = NULL; g_autofree char *contents = NULL; SoupURI *mirrorlist = NULL; g_autoptr(GPtrArray) ret_mirrorlist = @@ -2367,7 +2367,7 @@ ostree_repo_pull_with_options (OstreeRepo *self, const char *dir_to_pull = NULL; g_autofree char **dirs_to_pull = NULL; g_autofree char **refs_to_fetch = NULL; - char **override_commit_ids = NULL; + g_autofree char **override_commit_ids = NULL; GSource *update_timeout = NULL; gboolean disable_static_deltas = FALSE; gboolean require_static_deltas = FALSE; diff --git a/src/libostree/ostree-repo-static-delta-compilation-analysis.c b/src/libostree/ostree-repo-static-delta-compilation-analysis.c index c1990591..2b9b006f 100644 --- a/src/libostree/ostree-repo-static-delta-compilation-analysis.c +++ b/src/libostree/ostree-repo-static-delta-compilation-analysis.c @@ -243,7 +243,7 @@ _ostree_delta_compute_similar_objects (OstreeRepo *repo, { gboolean ret = FALSE; g_autoptr(GHashTable) ret_modified_regfile_content = - g_hash_table_new_full (g_str_hash, g_str_equal, g_free, (GDestroyNotify)g_ptr_array_unref); + g_hash_table_new_full (g_str_hash, g_str_equal, g_free, g_free); g_autoptr(GPtrArray) from_sizes = NULL; g_autoptr(GPtrArray) to_sizes = NULL; guint i, j; diff --git a/src/libostree/ostree-repo-static-delta-core.c b/src/libostree/ostree-repo-static-delta-core.c index 0b0d7067..133ab016 100644 --- a/src/libostree/ostree-repo-static-delta-core.c +++ b/src/libostree/ostree-repo-static-delta-core.c @@ -880,7 +880,9 @@ _ostree_repo_static_delta_dump (OstreeRepo *self, OT_VARIANT_MAP_TRUSTED, &delta_superblock, error)) goto out; - g_print ("%s\n", g_variant_print (delta_superblock, 1)); + { g_autofree char *variant_string = g_variant_print (delta_superblock, 1); + g_print ("%s\n", variant_string); + } g_print ("Delta: %s\n", delta_id); { const char *endianness_description; diff --git a/src/libostree/ostree-sepolicy.c b/src/libostree/ostree-sepolicy.c index 8e49428f..fa264704 100644 --- a/src/libostree/ostree-sepolicy.c +++ b/src/libostree/ostree-sepolicy.c @@ -258,7 +258,7 @@ initable_init (GInitable *initable, g_autoptr(GFileInputStream) filein = NULL; g_autoptr(GDataInputStream) datain = NULL; gboolean enabled = FALSE; - char *policytype = NULL; + g_autofree char *policytype = NULL; const char *selinux_prefix = "SELINUX="; const char *selinuxtype_prefix = "SELINUXTYPE="; @@ -352,7 +352,7 @@ initable_init (GInitable *initable, goto out; } - self->selinux_policy_name = g_strdup (policytype); + self->selinux_policy_name = g_steal_pointer (&policytype); self->selinux_policy_root = g_object_ref (etc_selinux_dir); } diff --git a/src/libostree/ostree-sysroot-cleanup.c b/src/libostree/ostree-sysroot-cleanup.c index 862d8154..2a78d2ce 100644 --- a/src/libostree/ostree-sysroot-cleanup.c +++ b/src/libostree/ostree-sysroot-cleanup.c @@ -491,7 +491,7 @@ prune_repo (OstreeRepo *repo, if (freed_space > 0) { - char *freed_space_str = g_format_size_full (freed_space, 0); + g_autofree char *freed_space_str = g_format_size_full (freed_space, 0); g_print ("Freed objects: %s\n", freed_space_str); } diff --git a/src/libostree/ostree-sysroot-upgrader.c b/src/libostree/ostree-sysroot-upgrader.c index b0061c87..447bd82b 100644 --- a/src/libostree/ostree-sysroot-upgrader.c +++ b/src/libostree/ostree-sysroot-upgrader.c @@ -105,6 +105,7 @@ parse_refspec (OstreeSysrootUpgrader *self, csum = g_key_file_get_string (self->origin, "origin", "override-commit", NULL); if (csum != NULL && !ostree_validate_checksum_string (csum, error)) goto out; + g_clear_pointer (&self->override_csum, g_free); self->override_csum = g_steal_pointer (&csum); ret = TRUE; diff --git a/src/ostree/ot-builtin-export.c b/src/ostree/ot-builtin-export.c index 5b84d1ab..db4dbc31 100644 --- a/src/ostree/ot-builtin-export.c +++ b/src/ostree/ot-builtin-export.c @@ -67,7 +67,7 @@ ostree_builtin_export (int argc, char **argv, GCancellable *cancellable, GError g_autoptr(GFile) subtree = NULL; g_autofree char *commit = NULL; g_autoptr(GVariant) commit_data = NULL; - struct archive *a; + struct archive *a = NULL; OstreeRepoExportArchiveOptions opts = { 0, }; context = g_option_context_new ("COMMIT - Stream COMMIT to stdout in tar format"); @@ -154,6 +154,10 @@ ostree_builtin_export (int argc, char **argv, GCancellable *cancellable, GError ret = TRUE; out: +#ifdef HAVE_LIBARCHIVE + if (a) + archive_write_free (a); +#endif if (context) g_option_context_free (context); return ret; diff --git a/src/ostree/ot-builtin-fsck.c b/src/ostree/ot-builtin-fsck.c index 5afa435f..9809ad29 100644 --- a/src/ostree/ot-builtin-fsck.c +++ b/src/ostree/ot-builtin-fsck.c @@ -287,7 +287,7 @@ ostree_builtin_fsck (int argc, char **argv, GCancellable *cancellable, GError ** if (opt_add_tombstones) { GError *local_error = NULL; - const char *parent = ostree_commit_get_parent (commit); + g_autofree char *parent = ostree_commit_get_parent (commit); if (parent) { g_autoptr(GVariant) parent_commit = NULL; diff --git a/src/ostree/ot-remote-builtin-add-cookie.c b/src/ostree/ot-remote-builtin-add-cookie.c index 439e7503..f1657bb8 100644 --- a/src/ostree/ot-remote-builtin-add-cookie.c +++ b/src/ostree/ot-remote-builtin-add-cookie.c @@ -68,7 +68,7 @@ ot_remote_builtin_add_cookie (int argc, char **argv, GCancellable *cancellable, value = argv[5]; cookie_file = g_strdup_printf ("%s.cookies.txt", remote_name); - jar_path = g_build_filename (g_file_get_path (repo->repodir), cookie_file, NULL); + jar_path = g_build_filename (gs_file_get_path_cached (repo->repodir), cookie_file, NULL); jar = soup_cookie_jar_text_new (jar_path, FALSE); @@ -80,5 +80,7 @@ ot_remote_builtin_add_cookie (int argc, char **argv, GCancellable *cancellable, /* jar takes ownership of cookie */ soup_cookie_jar_add_cookie (jar, cookie); + if (context) + g_option_context_free (context); return TRUE; } diff --git a/src/ostree/ot-remote-builtin-delete-cookie.c b/src/ostree/ot-remote-builtin-delete-cookie.c index 9f05a564..df65b277 100644 --- a/src/ostree/ot-remote-builtin-delete-cookie.c +++ b/src/ostree/ot-remote-builtin-delete-cookie.c @@ -67,7 +67,7 @@ ot_remote_builtin_delete_cookie (int argc, char **argv, GCancellable *cancellabl cookie_name = argv[4]; cookie_file = g_strdup_printf ("%s.cookies.txt", remote_name); - jar_path = g_build_filename (g_file_get_path (repo->repodir), cookie_file, NULL); + jar_path = g_build_filename (gs_file_get_path_cached (repo->repodir), cookie_file, NULL); jar = soup_cookie_jar_text_new (jar_path, FALSE); cookies = soup_cookie_jar_all_cookies (jar); @@ -92,5 +92,7 @@ ot_remote_builtin_delete_cookie (int argc, char **argv, GCancellable *cancellabl if (!found) g_set_error_literal (error, G_IO_ERROR, G_IO_ERROR_FAILED, "Cookie not found in jar"); + if (context) + g_option_context_free (context); return found; } From a8f5c2020958d700bdfad5b39a9d1ceab8ecc2a1 Mon Sep 17 00:00:00 2001 From: "Jasper St. Pierre" Date: Mon, 21 Nov 2016 15:10:19 -0800 Subject: [PATCH 20/24] ostree-repo: Fix parameter name Closes: #591 Approved by: cgwalters --- src/libostree/ostree-repo.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libostree/ostree-repo.c b/src/libostree/ostree-repo.c index b4cf4e38..25893fca 100644 --- a/src/libostree/ostree-repo.c +++ b/src/libostree/ostree-repo.c @@ -4512,7 +4512,7 @@ ostree_repo_verify_commit_ext (OstreeRepo *self, * ostree_repo_verify_commit_for_remote: * @self: Repository * @commit_checksum: ASCII SHA256 checksum - * @remote: OSTree remote to use for configuration + * @remote_name: OSTree remote to use for configuration * @cancellable: Cancellable * @error: Error * From fbce60817727a3d48ee41e19a3427d902d15a2bc Mon Sep 17 00:00:00 2001 From: "Jasper St. Pierre" Date: Mon, 21 Nov 2016 15:10:24 -0800 Subject: [PATCH 21/24] ostree-repo-static-delta-processing: Don't close(-1) Ultimately harmless, but causes somewhat scary strace messages. Closes: #591 Approved by: cgwalters --- src/libostree/ostree-repo-static-delta-processing.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libostree/ostree-repo-static-delta-processing.c b/src/libostree/ostree-repo-static-delta-processing.c index 95f9ddba..eabe3925 100644 --- a/src/libostree/ostree-repo-static-delta-processing.c +++ b/src/libostree/ostree-repo-static-delta-processing.c @@ -842,7 +842,7 @@ dispatch_set_read_source (OstreeRepo *repo, gboolean ret = FALSE; guint64 source_offset; - if (state->read_source_fd) + if (state->read_source_fd != -1) { (void) close (state->read_source_fd); state->read_source_fd = -1; @@ -887,7 +887,7 @@ dispatch_unset_read_source (OstreeRepo *repo, goto out; } - if (state->read_source_fd) + if (state->read_source_fd != -1) { (void) close (state->read_source_fd); state->read_source_fd = -1; From fd6ba80d0734663f0b239c145d3b099ae12566ad Mon Sep 17 00:00:00 2001 From: "Jasper St. Pierre" Date: Mon, 21 Nov 2016 16:05:55 -0800 Subject: [PATCH 22/24] ostree-repo: Make the lock with a long-lasting FD glnx_make_lock_file requires that the dfd passed in survives the lifetime of the lock. Since dfd_iter.fd gets cleaned up after the function returns, this isn't the case. dfd_iter.fd should be equivalent to tmpdir_dfd, since we iter on ".", and that survives past the function, so just use that instead. Closes: #591 Approved by: cgwalters --- src/libostree/ostree-repo.c | 2 +- tests/test-admin-deploy-bootid-gc.sh | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libostree/ostree-repo.c b/src/libostree/ostree-repo.c index 25893fca..8aa76dac 100644 --- a/src/libostree/ostree-repo.c +++ b/src/libostree/ostree-repo.c @@ -4869,7 +4869,7 @@ _ostree_repo_allocate_tmpdir (int tmpdir_dfd, /* We put the lock outside the dir, so we can hold the lock * until the directory is fully removed */ - if (!_ostree_repo_try_lock_tmpdir (dfd_iter.fd, dent->d_name, + if (!_ostree_repo_try_lock_tmpdir (tmpdir_dfd, dent->d_name, file_lock_out, &did_lock, error)) goto out; diff --git a/tests/test-admin-deploy-bootid-gc.sh b/tests/test-admin-deploy-bootid-gc.sh index ba16f336..29360e38 100755 --- a/tests/test-admin-deploy-bootid-gc.sh +++ b/tests/test-admin-deploy-bootid-gc.sh @@ -51,7 +51,7 @@ fi newstagepath=$(ls -d sysroot/ostree/repo/tmp/staging-${NEW_TEST_BOOTID}-*) assert_has_dir "${newstagepath}" env OSTREE_BOOTID=${NEW_TEST_BOOTID} ${CMD_PREFIX} ostree admin deploy --karg=root=LABEL=MOO --karg=quiet --os=testos testos:testos/buildmaster/x86_64-runtime -newstagepath=$(ls -d sysroot/ostree/repo/tmp/staging-${NEW_TEST_BOOTID}-*) +newstagepath=$(echo sysroot/ostree/repo/tmp/staging-${NEW_TEST_BOOTID}-*) assert_not_has_dir "${stagepath}" assert_not_has_dir "${newstagepath}" From 13360cb1c4e4d2e9464b94fc5e2b57bdccb3d248 Mon Sep 17 00:00:00 2001 From: Abhay Kadam Date: Tue, 22 Nov 2016 18:34:52 +0530 Subject: [PATCH 23/24] Fix broken link in docs/CONTRIBUTING.md The link for git-rebase manual contains stray character (]) at the end. Closes: #592 Approved by: jlebon --- docs/CONTRIBUTING.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/CONTRIBUTING.md b/docs/CONTRIBUTING.md index a26f3975..36588113 100644 --- a/docs/CONTRIBUTING.md +++ b/docs/CONTRIBUTING.md @@ -15,7 +15,7 @@ for more information. Instead, we use an instance of As a review proceeeds, the preferred method is to push `fixup!` commits via `git commit --fixup`. Homu knows how to use `--autosquash` when performing the final merge. See the -[Git documentation](https://git-scm.com/docs/git-rebase]) for more +[Git documentation](https://git-scm.com/docs/git-rebase) for more information. Alternative methods if you don't like Github (also fully supported): From 7584dc0f25734117db47fef1ba0e24231a819a5e Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Wed, 23 Nov 2016 10:42:39 -0500 Subject: [PATCH 24/24] Release 2016.14 Closes: #593 Approved by: jlebon --- configure.ac | 2 +- src/libostree/libostree.sym | 24 +++++++++++++++--------- 2 files changed, 16 insertions(+), 10 deletions(-) diff --git a/configure.ac b/configure.ac index ffa86295..cb1cc980 100644 --- a/configure.ac +++ b/configure.ac @@ -1,6 +1,6 @@ AC_PREREQ([2.63]) dnl If incrementing the version here, remember to update libostree.sym too -AC_INIT([ostree], [2016.13], [walters@verbum.org]) +AC_INIT([ostree], [2016.14], [walters@verbum.org]) AC_CONFIG_HEADER([config.h]) AC_CONFIG_MACRO_DIR([buildutil]) AC_CONFIG_AUX_DIR([build-aux]) diff --git a/src/libostree/libostree.sym b/src/libostree/libostree.sym index ed382fe5..0540cba6 100644 --- a/src/libostree/libostree.sym +++ b/src/libostree/libostree.sym @@ -359,22 +359,28 @@ global: /* No new symbols in 2016.12 */ /* No new symbols in 2016.13 */ -/* NOTE NOTE NOTE - * Versions above here are released. Only add symbols below this line. - * NOTE NOTE NOTE - */ - LIBOSTREE_2016.14 { global: ostree_repo_verify_commit_for_remote; } LIBOSTREE_2016.8; -/* Section for the stable release *after* this development one; don't - * edit this other than to update the last number. */ +/* NOTE NOTE NOTE + * Versions above here are released. Only add symbols below this line. + * NOTE NOTE NOTE + */ -/* Remove comment when first new symbol is added, replace XX with new stable version. +/* Remove comment when first new symbol is added LIBOSTREE_2016.XX { global: someostree_symbol_deleteme; } LIBOSTREE_2016.14; - * Remove comment when first new symbol is added */ +*/ + +/* Stub section for the stable release *after* this development one; don't + * edit this other than to update the last number. This is just a copy/paste + * source. +LIBOSTREE_2016.XX { +global: + someostree_symbol_deleteme; +} LIBOSTREE_2016.14; +*/