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

rpmkeys: --checksig should require valid signatures #1630

Conversation

DemiMarie
Copy link
Contributor

rpmkeys --checksig exists specifically to verify the signatures on a
package. Therefore, it should imply --define=_pkgverify_level all and
--define=_pkgverify_flags 0x0. The current behavior is both
counterintuitive and dangerous.

The RPM testsuite relies heavily on controlling the package verification
level via --define=_pkgverify_level $lvl. Therefore, add two new
flags: --no-require-digests and --allow-unsigned. These are
equivalent to --nodigests and --nosignatures, respectively, except
that they only change whether digests (resp. signatures) are required,
not whether they are checked at all. Additionally, update the testsuite
to use the new flags and expect the new NOTFOUND lines. This accounts
for most of the changes.

`rpmkeys --checksig` exists specifically to verify the signatures on a
package.  Therefore, it should imply `--define=_pkgverify_level all` and
`--define=_pkgverify_flags 0x0`.  The current behavior is both
counterintuitive and dangerous.

The RPM testsuite relies heavily on controlling the package verification
level via `--define=_pkgverify_level $lvl`.  Therefore, add two new
flags: `--no-require-digests` and `--allow-unsigned`.  These are
equivalent to `--nodigests` and `--nosignatures`, respectively, except
that they only change whether digests (resp. signatures) are *required*,
not whether they are checked at all.  Additionally, update the testsuite
to use the new flags and expect the new NOTFOUND lines.  This accounts
for most of the changes.
@DemiMarie
Copy link
Contributor Author

DemiMarie commented Apr 11, 2021

Pinging @dmach @Conan-Kudo as per private discussion.

@Conan-Kudo
Copy link
Member

While this generally looks good, we probably want to have tests that verify positive cases with accepted signatures. Admittedly, I'm not sure how that would be wired in...

@DemiMarie
Copy link
Contributor Author

While this generally looks good, we probably want to have tests that verify positive cases with accepted signatures. Admittedly, I'm not sure how that would be wired in...

We already have such tests, and this change does not break them.

@Conan-Kudo
Copy link
Member

Ah, okay.

@pmatilai
Copy link
Member

NAK, this is getting even more confusing.

I totally agree --checksig is far from ideal, but there are LOTS of legacy twists associated with it all. I opened #1631 to track it - the main issue to me is that it's ambiguous - and it needs to be properly though over, taking all the legacy crap, FIPS mode, usability and user expectations into account.

@pmatilai pmatilai closed this Apr 12, 2021
@DemiMarie
Copy link
Contributor Author

NAK, this is getting even more confusing.

I totally agree --checksig is far from ideal, but there are LOTS of legacy twists associated with it all. I opened #1631 to track it - the main issue to me is that it's ambiguous - and it needs to be properly though over, taking all the legacy crap, FIPS mode, usability and user expectations into account.

From my perspective, what is most important is getting --checksig to match what users expect now, and to do so as quickly as possible. The current behavior makes who knows how many downstream programs and scripts vulnerable.

@DemiMarie DemiMarie deleted the checksig-verify-all branch February 7, 2022 15:37
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.

None yet

3 participants