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

core: Filter locked packages by checksums before depsolving #1927

Merged
merged 1 commit into from
Oct 16, 2019

Conversation

jlebon
Copy link
Member

@jlebon jlebon commented Oct 15, 2019

Don't just filter down packages by NEVRA, but also filter out those that
don't match the checksum too. We were enforcing checksum matches already
before this, but only after depsolving and simply erroring out if they
didn't match.

However, because of how RPM signing is implemented in Fedora, it is
possible to have the same NEVRA in two different repos, each with two
different hashes. E.g. right now for example, efivar-libs wasn't
rebuilt for f31, and so f31 is just shipping the f30 RPM, but signed
with the f31 key. And of course, we also had the f30 version in the
pool.

This patch allows us to transition over to the f31 version with
everything else by not getting thrown off by the f30 version already in
the pool. (Still need to investigate how the pool will deal with this.)

Don't just filter down packages by NEVRA, but also filter out those that
don't match the checksum too. We were enforcing checksum matches already
before this, but only *after* depsolving and simply erroring out if they
didn't match.

However, because of how RPM signing is implemented in Fedora, it is
possible to have the same NEVRA in two different repos, each with two
different hashes. E.g. right now for example, `efivar-libs` wasn't
rebuilt for f31, and so f31 is just shipping the f30 RPM, but signed
with the f31 key. And of course, we also had the f30 version in the
pool.

This patch allows us to transition over to the f31 version with
everything else by not getting thrown off by the f30 version already in
the pool. (Still need to investigate how the pool will deal with this.)
@dustymabe
Copy link
Member

If I remember correctly since coreos-pool tag serves all of our streams coreos-koji-tagger will sign an fXX built rpm with the fXX key (this was the heuristic that made the most sense). What you're saying is that this causes issues because when we promote lockfiles from the bodhi stream where the rpms, even though they were built against f30, are signed with the f31 key and thus will have a different hash in the lockfile.

Is that correct?

@cgwalters
Copy link
Member

/approve

@cgwalters
Copy link
Member

/lgtm

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cgwalters, jlebon

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

@jlebon
Copy link
Member Author

jlebon commented Oct 16, 2019

bot, retest this please

@jlebon
Copy link
Member Author

jlebon commented Oct 16, 2019

What you're saying is that this causes issues because when we promote lockfiles from the bodhi stream where the rpms, even though they were built against f30, are signed with the f31 key and thus will have a different hash in the lockfile.

Is that correct?

Right, that's one of the issues. I think this sums it up well:

$ curl -LO https://kojipkgs.fedoraproject.org//packages/efivar/37/1.fc30/x86_64/efivar-libs-37-1.fc30.x86_64.rpm
$ mv efivar-libs-37-1.fc30.x86_64.rpm{,.koji}
$ dnf download efivar-libs-37-1.fc30.x86_64 --repofrompath=f31,https://dl.fedoraproject.org/pub/fedora/linux/development/31/Everything/x86_64/os/ --disablerepo '*' --enablerepo f31
$ mv efivar-libs-37-1.fc30.x86_64.rpm{,.f31}
$ dnf download efivar-libs-37-1.fc30.x86_64 --repofrompath=f30,https://dl.fedoraproject.org/pub/fedora/linux/releases/30/Everything/x86_64/os/ --disablerepo '*' --enablerepo f30
$ mv efivar-libs-37-1.fc30.x86_64.rpm{,.f30}
$ dnf download efivar-libs-37-1.fc30.x86_64 --repofrompath=pool,https://kojipkgs.fedoraproject.org/repos-dist/coreos-pool/latest/x86_64/ --disablerepo '*' --enablerepo pool
$ mv efivar-libs-37-1.fc30.x86_64.rpm{,.pool}
$ sha256sum *
22fa9d5fecdf138f787ac05b179969d4588f170a3f75fdf5405f057ecc9cefd2  efivar-libs-37-1.fc30.x86_64.rpm.f30
5d07a49039f721a101df068c3fbd9f2695f534dd2513c4ec8a2d4ff427179b45  efivar-libs-37-1.fc30.x86_64.rpm.f31
6797cbe81b0f15aa92da20a03c898cb18c7c2068e06dddbe9d29c8f97e0efcf2  efivar-libs-37-1.fc30.x86_64.rpm.koji
22fa9d5fecdf138f787ac05b179969d4588f170a3f75fdf5405f057ecc9cefd2  efivar-libs-37-1.fc30.x86_64.rpm.pool

The Koji pkg is unsigned, the f31 version is signed with the f31 key, and the f30 and pool versions are signed with the f30 key.

The main concern here is that in contexts where we're not locked, we could choose the package which isn't in the pool. Then we'd get a mismatch when moving the resulting lockfile to a context where we are locked. The main workflow that comes to mind that's like this is in lockfile promotion as you mentioned.

@jlebon
Copy link
Member Author

jlebon commented Oct 16, 2019

The local dev workflow can also hit this when using cosa fetch --update-lockfile (which is what I was doing for coreos/fedora-coreos-config#200).

@dustymabe
Copy link
Member

The main concern here is that in contexts where we're not locked, we could choose the package which isn't in the pool.

One easy way to solve that problem is to only allow the pool repo as the input repo. As noted before this hinders development a bit.

Then we'd get a mismatch when moving the resulting lockfile to a context where we are locked. The main workflow that comes to mind that's like this is in lockfile promotion as you mentioned.

Could we just promote the NVRAs instead of including the checksums? What do we gain or lose by doing that?

@openshift-merge-robot openshift-merge-robot merged commit 9ff9d43 into coreos:master Oct 16, 2019
jlebon added a commit to jlebon/coreos-assembler that referenced this pull request Oct 16, 2019
The digest fields in lockfiles have been more trouble than they're worth
so far. The major problem is signing: adding a signature to an RPM of
course changes its digest.

There are two main issues. The first is that when adding an override,
there's no way to know the digest ahead of time since the package isn't
signed yet. It becomes signed when the override is accepted and tagged
into the pool.

The second is that the same package NEVRA may be present in different
repos and signed with different keys, resulting in different hashes,
even if the payload is the same (for more details on this case, see
coreos/rpm-ostree#1927).

One could add repo origin information as part of the lockfile, but that
does not correspond to what we actually do in FCOS, since packages
initially come from Bodhi, and are then imported into the pool (and even
then, there's no guarantee that the package isn't already there with a
different signature).

A possible solution is to instead use the CPIO payload digest, but that
information is not part of the repo metadata, so it'd require some work
for rpm-ostree to download the packages and select the right one. Not
especially hard, but definitely non-trivial.

In practice, we don't really need digests in the lockfile. For repos
which don't use GPG checking, it could provide some security benefits.
Though in our case, GPG verification is turned on for all the repos
anyway. But more importantly, all our data comes from Koji, which
forbids buildings two packages with the same NEVRA.

We could fold this into rpm-ostree under a new flag, though I think its
current behaviour makes sense overall. It just doesn't work well for us.
@jlebon
Copy link
Member Author

jlebon commented Oct 16, 2019

OK, follow-up to this in coreos/coreos-assembler#854.

jlebon added a commit to coreos/coreos-assembler that referenced this pull request Oct 18, 2019
The digest fields in lockfiles have been more trouble than they're worth
so far. The major problem is signing: adding a signature to an RPM of
course changes its digest.

There are two main issues. The first is that when adding an override,
there's no way to know the digest ahead of time since the package isn't
signed yet. It becomes signed when the override is accepted and tagged
into the pool.

The second is that the same package NEVRA may be present in different
repos and signed with different keys, resulting in different hashes,
even if the payload is the same (for more details on this case, see
coreos/rpm-ostree#1927).

One could add repo origin information as part of the lockfile, but that
does not correspond to what we actually do in FCOS, since packages
initially come from Bodhi, and are then imported into the pool (and even
then, there's no guarantee that the package isn't already there with a
different signature).

A possible solution is to instead use the CPIO payload digest, but that
information is not part of the repo metadata, so it'd require some work
for rpm-ostree to download the packages and select the right one. Not
especially hard, but definitely non-trivial.

In practice, we don't really need digests in the lockfile. For repos
which don't use GPG checking, it could provide some security benefits.
Though in our case, GPG verification is turned on for all the repos
anyway. But more importantly, all our data comes from Koji, which
forbids buildings two packages with the same NEVRA.

We could fold this into rpm-ostree under a new flag, though I think its
current behaviour makes sense overall. It just doesn't work well for us.
@jlebon jlebon deleted the pr/fcos-f31-fixes branch April 23, 2023 23:31
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.

5 participants