Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: Use fsverity library, require keys and add signatures too #2269

Closed
wants to merge 1 commit into from

Conversation

cgwalters
Copy link
Member

At the time we added fsverity code to ostree, fsverity was
just a CLI tool; since then it has
gained a C shared library which wraps all the signature
bits and the enablement ioctl() conveniently.

This makes it much easier for us to support signatures, so
do so. Note that at this time, because ostree doesn't define
a mechanism to transport fsverity signatures across repositories,
this is mostly only useful for locally-generated signatures.

The idea however is this is a starting point from which we can
build more support, including signature transport, remote keys,
etc.

In order to simplify things, drop support for "opportunistic"
use of fsverity. In practice we expect people using this
to set it up fully, or not at all. The "read only files"
aspect is useful, but complicated the code too much relative
to its benefit.

Also drop support for using fsverity for /boot for now; in
practice most things there are read by the bootloader,
which doesn't know about fsverity. Instead those should
be covered by e.g. Secure Boot. This ensures that
we only have one high level API _ostree_tmpf_fsverity()
that is invoked from the core commit path.

xref https://lwn.net/Articles/842002
xref #1959
xref coreos/rpm-ostree#1883

@openshift-ci-robot
Copy link
Collaborator

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cgwalters

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:

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

@cgwalters
Copy link
Member Author

@cgwalters
Copy link
Member Author

cgwalters commented Jan 17, 2021

Initial user story here (see https://lwn.net/ml/fedora-devel/94055225-94c3-4d6b-a21a-08f5d4632437@www.fastmail.com/ ) is basically:

"As a Fedora CoreOS user I can provide my own encryption keys via Ignition, and use it to enable fsverity on the ostree repository on firstboot, and have files from OS updates also signed using that key"

Next steps after this are:

  • Add an API to sign all existing objects (bootstrapping)
  • Documentation
  • Support fetching a key from kernel keyring (libfsverity enhancement?)

Longer term:

  • Also add support for automatically using fsverity on /etc too?
  • Rework ostree fsck so it relies on the kernel check
  • Design a way to ship fsverity signatures (theoretically could pretend they're an xattr on the wire...ugh) - or instead go for a more serious rework of ostree as brainstormed in Initial fs-verity support #1959
  • Add support to xfs for fsverity (important)
  • Teach systemd a flag like ExecFsVerity=true that requires files executed by that unit to be fs-verity signed (with a well-known key in the kernel keyring e.g.)

@cgwalters
Copy link
Member Author

https://lore.kernel.org/linux-fscrypt/2369a655-d7e8-467c-8003-860d35ef1026@www.fastmail.com/T/#u

At the time we added fsverity code to ostree, fsverity was
just a CLI tool; since then it has
gained a C shared library which wraps all the signature
bits and the enablement `ioctl()` conveniently.

This makes it much easier for us to support signatures, so
do so.  Note that at this time, because ostree doesn't define
a mechanism to transport fsverity signatures across repositories,
this is mostly only useful for locally-generated signatures.

The idea however is this is a starting point from which we can
build more support, including signature transport, remote keys,
etc.

In order to simplify things, drop support for "opportunistic"
use of fsverity.  In practice we expect people using this
to set it up fully, or not at all.  The "read only files"
aspect *is* useful, but complicated the code too much relative
to its benefit.

Also drop support for using fsverity for `/boot` for now; in
practice most things there are read by the bootloader,
which doesn't know about fsverity.  Instead those should
be covered by e.g. Secure Boot.  This ensures that
we only have one high level API `_ostree_tmpf_fsverity()`
that is invoked from the core commit path.

xref https://lwn.net/Articles/842002
xref ostreedev#1959
xref coreos/rpm-ostree#1883
@openshift-ci-robot
Copy link
Collaborator

@cgwalters: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-ci
Copy link

openshift-ci bot commented Feb 19, 2022

@cgwalters: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-ci
Copy link

openshift-ci bot commented Apr 6, 2022

@cgwalters: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/sanity b8957bf link true /test sanity

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

intel-lab-lkp pushed a commit to intel-lab-lkp/linux that referenced this pull request Dec 8, 2022
fsverity builtin signatures, at least as currently implemented, are a
mistake and should not be used.  They mix the authentication policy
between the kernel and userspace, which is not a clean design and causes
confusion.  For builtin signatures to actually provide any security
benefit, userspace still has to enforce that specific files have
fsverity enabled.  Since userspace needs to do this, a better design is
to have that same userspace code do the signature check too.

That allows better signature formats and algorithms to be used, avoiding
in-kernel parsing of the notoriously bad PKCS#7 format.  It is also
needed anyway when different keys need to be trusted for different
files, or when it's desired to use fsverity for integrity-only or
auditing on some files and for authenticity on other files.  Basically,
the builtin signatures don't work for any nontrivial use case.

(IMA appraisal is another alternative.  It goes in the opposite
direction -- the full policy is moved into the kernel.)

For these reasons, the master branch of AOSP no longer uses builtin
signatures.  It still uses fsverity for some files, but signatures are
verified in userspace when needed.

None of the public uses of builtin signatures outside Android seem to
have gotten going, either.  Support for builtin signatures was added to
RPM.  However, https://fedoraproject.org/wiki/Changes/FsVerityRPM was
subsequently rejected from Fedora and seems to have been abandoned.
There is also ostreedev/ostree#2269, which was
never merged.  Neither proposal mentioned a plan to set
fs.verity.require_signatures=1 and enforce that files have fs-verity
enabled -- so, they would have had no security benefit on their own.

I'd be glad to hear about any other users of builtin signatures that may
exist, and help with the details of what should be used instead.

Anyway, the feature can't simply be removed, due to the need to maintain
backwards compatibility.  But let's at least make it clear that it's
deprecated.  Update the documentation accordingly, and rename the
kconfig option to CONFIG_FS_VERITY_DEPRECATED_BUILTINSIG.  Also remove
the kconfig option from the s390 defconfigs, as it's unneeded there.

Signed-off-by: Eric Biggers <ebiggers@google.com>
@cgwalters
Copy link
Member Author

I think this is obsoleted by #2640

@cgwalters cgwalters closed this May 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants