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

Don't allow installing packages without ARCH or OS #3478

Merged
merged 5 commits into from
Dec 3, 2024

Conversation

ffesti
Copy link
Contributor

@ffesti ffesti commented Nov 29, 2024

We had an exception for public key packages in rpmte. With those now being handled in the keystore without going through the transaction machinery this is no longer needed. This also prevents people from just contrsucting their own pubkey packages and install them.

Resolves: #3344

@ffesti ffesti requested a review from a team as a code owner November 29, 2024 08:25
@ffesti ffesti requested review from pmatilai and removed request for a team November 29, 2024 08:25
@pmatilai
Copy link
Member

pmatilai commented Nov 29, 2024

This wants tests too to make sure the loophole gets closed and stays closed. Adding elements can be easily tested by creating a partial header with the python bindings and tossing it at ts.addInstall(). Erase can be tested on the database in tests/data/misc, there's a gpg-pubkey in there, but what the exact outcome of that should be, lets decide in the ticket first.

@ffesti
Copy link
Contributor Author

ffesti commented Nov 29, 2024

OK, added all kind of error handling and check - even if they don't quite fit into the #3344 umbrella.

Test are still missing as it is not quite as easy to get a "properly" broken v4 header constructed in the Python API - now that v3 headers without RPMTAG_HEADERIMMUTABLE are no longer allowed.

@ffesti
Copy link
Contributor Author

ffesti commented Nov 29, 2024

I wonder if I can just take a proper header from on of our test packages and remove tags to test the error handling.

@ffesti
Copy link
Contributor Author

ffesti commented Nov 30, 2024

Turns out the answer is "yes".

lib/rpmte.cc Outdated Show resolved Hide resolved
lib/rpmte.cc Outdated Show resolved Hide resolved
lib/rpmte.cc Outdated Show resolved Hide resolved
@pmatilai
Copy link
Member

pmatilai commented Dec 2, 2024

I wonder if I can just take a proper header from on of our test packages and remove tags to test the error handling.

It's an interesting thing. Deleting the name tag like in the test doesn't actually delete it in the header because it's in the immutable region, it just replaces what you see from the "outside", this is basically how file relocation works. But it's a bit weird that you can replace a name, version etc and the header will still pass signature checking inside rpmdb because the immutable region is untouched. You can only do such a change from the API, but it's another strange loophole we probably should close one of these days (obviously way out of scope here)

@ffesti
Copy link
Contributor Author

ffesti commented Dec 2, 2024

OK, adjusted the messages and reverted to the old check for missing tags.

Last patch still doesn't have a test case. But we just made it impossible to even create such a scenario...

lib/rpmte.cc Outdated Show resolved Hide resolved
lib/rpmte.cc Outdated Show resolved Hide resolved
lib/rpmte.cc Outdated Show resolved Hide resolved
lib/rpmte.cc Outdated Show resolved Hide resolved
lib/rpmte.cc Outdated
}

if (p->type != TR_REMOVED && rstreq(p->name, "gpg-pubkey")) {
rpmlog(RPMLOG_ERR, "public keys can not be installed as gpg-pubkey packages; use rpmkeys --import <keyfile> for that\n");
Copy link
Member

Choose a reason for hiding this comment

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

This and the erase warning above should be marked for translation - the _() thing around the string.

Since there's going to be at least one more round, minor nits / thoughts:

  • just noticed there's a typo in one of the commit messages: "contrsucting"
  • "for that" seems redundant, especially when the other message doesn't have (or need) it either
  • the --delete command is something that can be copy-pasted directly, --import message isn't - maybe there should be a presentation difference (quotes around the --delete command or something)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, there should not. If you are seeing both messages and you get confused by them being formatted the same way, there is really nothing I can do for you.

We had an exception for public key packages in rpmte. With those now
being handled in the keystore without going through the transaction
machinery this is no longer needed. This also prevents people from just
contructing their own pubkey packages and install them.

We still allow removing gpg-pubkey packages but give a warning pointing
people to rpmkeys.

Resolves: rpm-software-management#3344
People could install gpg-pubkey if they had the ARCH and OS tag set. Do
not allow that.
Reject normal packages named gpg-pubkey which do have OS and ARCH.
Only packages from properly imported keys don't.
@pmatilai pmatilai merged commit aa72243 into rpm-software-management:master Dec 3, 2024
1 check passed
@ffesti ffesti deleted the 3344 branch December 5, 2024 13:33
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.

Refuse to create transaction elements from headers with no RPMTAG_ARCH
2 participants