Static-delta's superblock signature support #1985

fdanis-oss · 2019-12-13T13:45:30Z

While the commits contained in the single static-delta file are signed so we can check them and operate on trusted data, the superblock isn't signed in any way, so it end up operating on untrusted data to:

actually find where the trusted data is, and
check whether the update is fit for the current device by looking at the collection id stored in the metadata

These commits generate a signature of all superblock data (serialized), except inline commits and the signature, then add the signature to the superblock itself as an additional metadata field.

New APIs are added to check if a superblock is signed, and to verify its signature.

This is part of the Add support for alternative signature systems #1233 and is based on top of Alternative signing system #1878

Fixes: #1977

openshift-ci-robot · 2019-12-13T13:45:45Z

Hi @fdanis-oss. Thanks for your PR.

I'm waiting for a ostreedev member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

rh-atomic-bot · 2019-12-13T13:48:22Z

Can one of the admins verify this patch?
I understand the following commands:

bot, add author to whitelist
bot, test pull request
bot, test pull request once

fdanis-oss · 2020-01-28T17:04:36Z

Hi @cgwalters
These commits are based on #1878, so only the last 6 commits are relevant to add static-delta's superblock signature
/assign @cgwalters

rh-atomic-bot · 2020-02-20T16:41:59Z

☔ The latest upstream changes (presumably 7db7cfc) made this pull request unmergeable. Please resolve the merge conflicts.

rh-atomic-bot · 2020-03-24T19:08:19Z

☔ The latest upstream changes (presumably #2041) made this pull request unmergeable. Please resolve the merge conflicts.

src/libostree/ostree-repo-static-delta-compilation.c

src/libostree/ostree-repo-static-delta-core.c

fdanis-oss · 2020-06-26T07:28:50Z

Hi @cgwalters , can I kindly ask you a review of those commits, please?

cgwalters · 2020-07-01T20:56:37Z

Right, this came up before...I'd have to really dig to find it, but I think where we ended up was to say to use signed summary files. The summary file contains a checksum of the deltas (though I'd want to double check all this).

There is also ensuring transport integrity - signing is a great "backstop", but I've been pushing "pinned TLS", see e.g. #707 and ... https://pagure.io/fedora-infrastructure/issue/5372 (which links to this thread https://mail.gnome.org/archives/ostree-list/2016-September/msg00015.html )

Is that not sufficient for you? Anything involving "inline" signatures quickly gets into exactly the problem you're talking about and we'd need to carefully review the code.

d4s · 2020-07-01T21:32:30Z

@cgwalters this is for other use case.
We can prepare the self-containg upgrade bundle as a single file, for instance:
ostree static-delta generate --from=... --to=... --inline --min-fallback-size=1024 --filename update.bundle

In this case the user need to have only a single file update.bundle for offline updates -- just need to copy it to any mass-storage device with any supported filesystem.
The commit inside of that bundle is signed and is verified with signapi, but here we want to verify the meta-information of delta file itself which is used during extracting data. And need to do that before extracting the contents of the file and checking the commit to prevent some attack vectors caused by header modification and/or re-creation of the update bundle.

fdanis-oss · 2020-09-01T13:15:49Z

Finally found a correct way to load keys from well-known directories, similar to the signed commit verification.

cgwalters · 2020-09-11T15:22:09Z

/approve

cgwalters · 2020-09-11T15:22:14Z

/ok-to-test

src/ostree/ot-builtin-static-delta.c

cgwalters · 2020-09-11T15:24:46Z

src/ostree/ot-builtin-static-delta.c

@@ -438,13 +446,63 @@ ot_static_delta_builtin_apply_offline (int argc, char **argv, OstreeCommandInvoc
      return FALSE;
    }

+#if defined(HAVE_LIBSODIUM)
+  /* Initialize crypto system */
+  opt_sign_name = opt_sign_name ?: OSTREE_SIGN_NAME_ED25519;


Hmm...we don't have this default elsewhere I don't think. Is it really necessary? In other code the default comes from the repository configuration or the CLI and that's it.

This is initialized this way in ostree_builtin_commit(), ostree_builtin_sign() and ostree_builtin_summary().

Iiuc, the default crypto system is only defined in configuration file for remote repositories.
When we try to apply a static delta offline, there's no reference to a remote repository.

src/ostree/ot-builtin-static-delta.c

cgwalters · 2020-09-11T15:27:40Z

src/ostree/ot-builtin-static-delta.c

+
+          if (!ostree_sign_load_pk (sign, options, error))
+            {
+              /* If it fails to load system default public keys, consider there no signature engine */


I really don't like silently masking errors. This is one of the reasons for the refactoring done after the initial signapi landed, see e.g. #2105

Basically if the system is explicitly configured to use e.g. ed25519, then if we somehow fail to load that key, that should be a fatal error.

While the commits contained in the single static-delta file are signed so we can check them and operate on trusted data, the superblock isn't signed in any way, so it end up operating on untrusted data to: 1. actually find where the trusted data is, and 2. check whether the update is fit for the current device by looking at the collection id stored in the metadata This commit generates signatures of all static data, and concatenate them to the existing static delta format, i.e. as a GVariant layout `a{sv}ay` where - a{sv}: signatures - ay: existing delta variant Signed-off-by: Frédéric Danis <frederic.danis@collabora.com>

Add signing ability to "static-delta generate" builtin. Signed-off-by: Frédéric Danis <frederic.danis@collabora.com>

This retrieves the signatures and pass the static delta block as an array of bytes to ostree_sign_data_verify(). Signed-off-by: Frédéric Danis <frederic.danis@collabora.com>

Add new "static-delta verify" sub-command. This supports multiple keys to verify the static-delta file. Signed-off-by: Frédéric Danis <frederic.danis@collabora.com>

This checks if the static delta file is signed or not to be able to correctly get the superblock to apply. Signed-off-by: Frédéric Danis <frederic.danis@collabora.com>

This checks if the static delta file is signed or not to be able to correctly get the superblock to dump. Signed-off-by: Frédéric Danis <frederic.danis@collabora.com>

Add tests to generate signed deltas and verify them using 'dummy' signature engine. Signed-off-by: Frédéric Danis <frederic.danis@collabora.com>

Signed-off-by: Frédéric Danis <frederic.danis@collabora.com>

Add tests to generate signed deltas and verify them using 'ed25519' signature engine. Signed-off-by: Frédéric Danis <frederic.danis@collabora.com>

Add a new function `ostree_repo_static_delta_execute_offline_with_signature` which takes a signature engine to verify the delta before applying it. The `ostree_repo_static_delta_execute_offline` is just a wrapper to this new function, passing a NULL signature engine. When this function is called without signature engine, but with a sign delta, it will only fails if `sign-verify-deltas` is set to true in repo core options. This commits move signature existence check and delta signature verification to share common parts between existing APIs and the new function. Signed-off-by: Frédéric Danis <frederic.danis@collabora.com>

This allows to check the delta signature before applying it. Signed-off-by: Frédéric Danis <frederic.danis@collabora.com>

Add new test to apply offline signed deltas. Signed-off-by: Frédéric Danis <frederic.danis@collabora.com>

cgwalters · 2020-09-24T14:07:33Z

OK I think we can get this in an address some things as followups. Thanks for your patience on this!
/approve
/lgtm

openshift-ci-robot · 2020-09-24T14:08:13Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cgwalters, d4s, fdanis-oss

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [cgwalters]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

cgwalters · 2020-09-24T22:13:57Z

Followup in #2203

openshift-ci-robot requested review from mwleeds and rfairley December 13, 2019 13:45

openshift-ci-robot added size/XXL needs-ok-to-test labels Dec 13, 2019

openshift-ci-robot assigned cgwalters Jan 28, 2020

openshift-ci-robot added the needs-rebase label Feb 20, 2020

fdanis-oss force-pushed the wip/fda/sign_delta_metadata branch from 2eafb69 to d22a84c Compare February 27, 2020 10:57

openshift-ci-robot removed the needs-rebase label Feb 27, 2020

fdanis-oss force-pushed the wip/fda/sign_delta_metadata branch from d22a84c to 545cc8d Compare April 6, 2020 07:13

d4s requested changes Apr 12, 2020

View reviewed changes

fdanis-oss force-pushed the wip/fda/sign_delta_metadata branch 4 times, most recently from fcb814a to 1ce9ee7 Compare April 21, 2020 16:26

fdanis-oss requested a review from d4s April 21, 2020 16:29

fdanis-oss force-pushed the wip/fda/sign_delta_metadata branch 2 times, most recently from 97aaf38 to 2ab7ed5 Compare April 23, 2020 14:05

d4s mentioned this pull request Apr 24, 2020

signing: explicit expected signature types #2080

Open

d4s approved these changes Apr 24, 2020

View reviewed changes

d4s requested a review from cgwalters April 24, 2020 22:58

fdanis-oss force-pushed the wip/fda/sign_delta_metadata branch from 2ab7ed5 to 6ee753e Compare May 11, 2020 13:26

fdanis-oss force-pushed the wip/fda/sign_delta_metadata branch from 6ee753e to abd2960 Compare June 4, 2020 13:35

fdanis-oss force-pushed the wip/fda/sign_delta_metadata branch 2 times, most recently from 22a4a73 to 9265baf Compare September 1, 2020 14:16

openshift-ci-robot added approved ok-to-test and removed needs-ok-to-test labels Sep 11, 2020

cgwalters requested changes Sep 11, 2020

View reviewed changes

openshift-ci-robot removed the approved label Sep 11, 2020

fdanis-oss added 12 commits September 14, 2020 09:27

bin/static-delta: Add support to sign superblock

92efbc0

Add signing ability to "static-delta generate" builtin. Signed-off-by: Frédéric Danis <frederic.danis@collabora.com>

lib/deltas: Add signature check API for static-delta superblock

02a19b2

This retrieves the signatures and pass the static delta block as an array of bytes to ostree_sign_data_verify(). Signed-off-by: Frédéric Danis <frederic.danis@collabora.com>

bin/static-delta: Add command to verify delta signature

512db04

Add new "static-delta verify" sub-command. This supports multiple keys to verify the static-delta file. Signed-off-by: Frédéric Danis <frederic.danis@collabora.com>

lib/deltas: Support signed delta in execute_offline

0c48423

This checks if the static delta file is signed or not to be able to correctly get the superblock to apply. Signed-off-by: Frédéric Danis <frederic.danis@collabora.com>

lib/deltas: Support signed delta in dump

bf0c09f

This checks if the static delta file is signed or not to be able to correctly get the superblock to dump. Signed-off-by: Frédéric Danis <frederic.danis@collabora.com>

tests/delta: new tests for signed deltas

c98a993

Add tests to generate signed deltas and verify them using 'dummy' signature engine. Signed-off-by: Frédéric Danis <frederic.danis@collabora.com>

tests/libtest.sh: Add skip_without_sign_ed25519() function

96bcc25

Signed-off-by: Frédéric Danis <frederic.danis@collabora.com>

tests/delta: new tests for 'ed25519' signed deltas

869dbc0

Add tests to generate signed deltas and verify them using 'ed25519' signature engine. Signed-off-by: Frédéric Danis <frederic.danis@collabora.com>

bin/static-delta: Add signature parameters to apply-offline

2e97f56

This allows to check the delta signature before applying it. Signed-off-by: Frédéric Danis <frederic.danis@collabora.com>

tests/delta: Add new tests for applying signed deltas

ecbfe08

Add new test to apply offline signed deltas. Signed-off-by: Frédéric Danis <frederic.danis@collabora.com>

fdanis-oss force-pushed the wip/fda/sign_delta_metadata branch from 9265baf to ecbfe08 Compare September 14, 2020 07:28

openshift-ci-robot added lgtm approved labels Sep 24, 2020

cgwalters approved these changes Sep 24, 2020

View reviewed changes

openshift-merge-robot merged commit 00df896 into ostreedev:master Sep 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Static-delta's superblock signature support #1985

Static-delta's superblock signature support #1985

fdanis-oss commented Dec 13, 2019

openshift-ci-robot commented Dec 13, 2019

rh-atomic-bot commented Dec 13, 2019

fdanis-oss commented Jan 28, 2020

rh-atomic-bot commented Feb 20, 2020

rh-atomic-bot commented Mar 24, 2020

fdanis-oss commented Jun 26, 2020

cgwalters commented Jul 1, 2020

d4s commented Jul 1, 2020

fdanis-oss commented Sep 1, 2020 •

edited

Loading

cgwalters commented Sep 11, 2020

cgwalters commented Sep 11, 2020

cgwalters Sep 11, 2020

fdanis-oss Sep 11, 2020

cgwalters Sep 11, 2020

cgwalters commented Sep 24, 2020

openshift-ci-robot commented Sep 24, 2020

cgwalters commented Sep 24, 2020

Static-delta's superblock signature support #1985

Static-delta's superblock signature support #1985

Conversation

fdanis-oss commented Dec 13, 2019

openshift-ci-robot commented Dec 13, 2019

rh-atomic-bot commented Dec 13, 2019

fdanis-oss commented Jan 28, 2020

rh-atomic-bot commented Feb 20, 2020

rh-atomic-bot commented Mar 24, 2020

fdanis-oss commented Jun 26, 2020

cgwalters commented Jul 1, 2020

d4s commented Jul 1, 2020

fdanis-oss commented Sep 1, 2020 • edited Loading

cgwalters commented Sep 11, 2020

cgwalters commented Sep 11, 2020

cgwalters Sep 11, 2020

Choose a reason for hiding this comment

fdanis-oss Sep 11, 2020

Choose a reason for hiding this comment

cgwalters Sep 11, 2020

Choose a reason for hiding this comment

cgwalters commented Sep 24, 2020

openshift-ci-robot commented Sep 24, 2020

cgwalters commented Sep 24, 2020

fdanis-oss commented Sep 1, 2020 •

edited

Loading