Skip to content

Rework RPM signing key handling#75

Merged
tonyhutter merged 1 commit intozfsonlinux:masterfrom
Lalufu:Lalufu/feature-versioned-keys-in-zfs-release
Apr 27, 2023
Merged

Rework RPM signing key handling#75
tonyhutter merged 1 commit intozfsonlinux:masterfrom
Lalufu:Lalufu/feature-versioned-keys-in-zfs-release

Conversation

@Lalufu
Copy link
Copy Markdown
Contributor

@Lalufu Lalufu commented Jan 3, 2023

This is an attempt to address
openzfs/zfs#14344. It reworks the zfs-release RPM to

  • Always include all signing keys that are in active use on any release
  • Provide versioned symlinks for the supported releases, pointing to the appropriate key
  • Modifies the .repo files to point to a versioned symlink by utilizing the $releasever variable provided by yum/dnf

This will allow smooth key transitions between different major releases. The same mechanism is used by other major repos, including Fedora itself.

Tested by:

  • Take a F36 system without ZFS
  • Run dnf install -y https://zfsonlinux.org/fedora/zfs-release-2-2$(rpm --eval "%{dist}").noarch.rpm according to the installation instructions on https://openzfs.github.io/openzfs-docs/Getting%20Started/Fedora/index.html#installation
  • Install zfs packages
  • Attempt to upgrade the system to F37 by issuing dnf system-upgrade --releasever=37 download
  • Observe failure to validate F37 ZFS packages
  • Install zfs-release file built from this MR
  • Attempt to upgrade the system to F37 by issuing dnf system-upgrade --releasever=37 download again
  • Observe the upgrade process asking to import new key:
ZFS on Linux for Fedora 37                                                                                                      3.3 MB/s | 3.4 kB     00:00
Importing GPG key 0x9DB84141:
 Userid     : "OpenZFS <release@openzfs.org>"
 Fingerprint: 7DC7 299D CF7C 7FD9 CD87 701B A599 FD5E 9DB8 4141
 From       : /etc/pki/rpm-gpg/RPM-GPG-KEY-openzfs-fedora-37
Is this ok [y/N]: y
Key imported successfully
Running transaction check
Transaction check succeeded.
Running transaction test
Transaction test succeeded.
Complete!

Also tested by installing zfs-release file built from this MR on a F36 system without ZFS, and observing the correct key being imported.

@tonyhutter
Copy link
Copy Markdown
Member

tonyhutter commented Jan 3, 2023

I think this PR is commendable as it's trying to solve a real-world problem. Unfortunately the devil is in the details.

One drawback is that we would would need to update zfs-release.spec on every Fedora release, even though the key is the same. That requires a PR to be opened against the zfsonlinux.github.com repo on every Fedora release, which is an added maintenance burden. Considering the old SHA1 (160bit) key worked for us for 9 years, it's possible the new SHA512 (512bits) key is going to last us way longer. If that's the case, we'd be needlessly updating the spec file every 6 months for each Fedora release. I'm not convinced that it's worth it.

We would also need to make the ZFS RPMs dependent on the zfs-release RPM. Otherwise if you upgrade from F36 -> F37, it's possible dnf would try to install the ZFS RPMs first before the zfs-release RPM (with the updated key), giving you a key-signing error. To get around that we would need to update the zfs spec file to add a dependency on zfs-release, and then re-build the latest ZFS RPMs (which for argument's sake, we'll call zfs-2.1.7-2). Then, a user would would need to do the upgrade from Fedora 3X with zfs-2.1.7-2 to Fedora 37 for everything to work seamlessly. If you had an older installation like Fedora 34 that you wanted to upgrade to F37, or if you try to do an update from F36 -> F37 with the older <= zfs-2.1.7 RPMs, it would not work correctly. There may be additional gotchas with zfs requiring zfs-release that I'm not considering. For example, it would be slightly annoying for people building their own ZFS RPMs locally to have a dependency on zfs-release, which they wouldn't technically need.

@Lalufu
Copy link
Copy Markdown
Contributor Author

Lalufu commented Jan 4, 2023 via email

@tonyhutter
Copy link
Copy Markdown
Member

Ok, I see what your saying. So the user would be required to be fully updated on their current Fedora version before upgrading (which is the normal procedure anyway). That would get the new ZFS keys installed via the updated zfs-release for the next version before doing the upgrade. We'd probably only put the updated zfs-release RPM in the F36/F37 repos since F35 and older are EOL (and upgrading from Fedora two or more versions back becomes more risky anyway).

Maybe I can automate updating the spec file with a script or something, We already need to checkin the rebuilt zfs-releases RPMs with each new Fedora (like 13a7720), so we can roll the spec file changes in with those commits.

I don't think there's anything else you need to do with this PR for now. Let me try building the new RPMs and stick them in a test repo and see how the upgrades work.

@tonyhutter
Copy link
Copy Markdown
Member

@Lalufu just to manage expectations - it's going to be a few months before I'll be able to get to creating/testing/signing these new packages. I also see that Fedora 38 is coming out Apr 18 2023, so let's me use that as a target date to get this out the door. Please leave this PR open in the meantime. I'll assign it to myself so it doesn't get lost in the shuffle.

@tonyhutter tonyhutter self-assigned this Jan 26, 2023
@Lalufu
Copy link
Copy Markdown
Contributor Author

Lalufu commented Jan 27, 2023 via email

Copy link
Copy Markdown
Member

@tonyhutter tonyhutter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can pull this in now. I built RPMs from this patch and installed them on centos[7-9] and fedora[36-38] without issue.

@tonyhutter
Copy link
Copy Markdown
Member

@Lalufu would you mind adding a Signed-off-by line to the commit and I can pull it in?

@Lalufu
Copy link
Copy Markdown
Contributor Author

Lalufu commented Apr 27, 2023 via email

@tonyhutter
Copy link
Copy Markdown
Member

It's ok, it's not merged with master yet.

To add a signed-off by line:

  1. git commit --amend to modify your commit message.
  2. Add your Signed-off-by: line to the bottom of the commit. For example:
Author: Tony Hutter <hutter2@llnl.gov>
Date:   Wed Apr 19 17:24:33 2023 -0700

    Add zfs-2.1.11 release
    
    Signed-off-by: Tony Hutter <hutter2@llnl.gov>
  1. Force push it back (git push --force)

This is an attempt to address
openzfs/zfs#14344. It reworks the zfs-release
RPM to

* Always include all signing keys that are in active use on any
  release
* Provide versioned symlinks for the supported releases, pointing to
  the appropriate key
* Modifies the .repo files to point to a versioned symlink by utilizing
  the $releasever variable provided by yum/dnf

This will allow smooth key transitions between different major releases.
The same mechanism is used by other major repos, including Fedora
itself.

Signed-off-by: Ralf Ertzinger <ralf@skytale.net>
@Lalufu Lalufu force-pushed the Lalufu/feature-versioned-keys-in-zfs-release branch from 665ece4 to f9ed396 Compare April 27, 2023 17:53
@Lalufu
Copy link
Copy Markdown
Contributor Author

Lalufu commented Apr 27, 2023

OK, I was confused, apperently the master branch in my fork already had the change.

Rebased, amended the commit.

@tonyhutter tonyhutter merged commit a58ec60 into zfsonlinux:master Apr 27, 2023
@tonyhutter
Copy link
Copy Markdown
Member

@Lalufu thanks, I just merged it

tonyhutter added a commit to tonyhutter/zfsonlinux.github.com that referenced this pull request Apr 27, 2023
Rebuild the RPMs after specfile changes in zfsonlinux#75 (a58ec60).

Signed-off-by: Tony Hutter <hutter2@llnl.gov>
@tonyhutter tonyhutter mentioned this pull request Apr 27, 2023
behlendorf pushed a commit that referenced this pull request Apr 28, 2023
Rebuild the RPMs after specfile changes in #75 (a58ec60).

Signed-off-by: Tony Hutter <hutter2@llnl.gov>
@ivantomica ivantomica mentioned this pull request Apr 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants