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

Use long PGP Key IDs for all outputs #3292

Closed
wants to merge 2 commits into from

Conversation

ffesti
Copy link
Contributor

@ffesti ffesti commented Sep 10, 2024

Here how dropping the Short PGP KEy IDs in favor of long ones would look like. We still need to discuss if this really is a change we want in this magnitue or if we need to keep the old behavior at some places.

@ffesti ffesti added the DONT DO NOT merge, for whatever reason label Sep 10, 2024
@nwalfield
Copy link
Contributor

If you are considering breaking the output format, then I would strongly encourage you to just use the full fingerprint.

A key ID is only 64-bits. Happily, the security of key IDs does not depend on collision resistance, which has already been demonstrated to be broken, but collisions can trigger implementation bugs since they are hard to test, and attacks are only going to get stronger.

Short (4 bytes) PGP Key IDs or insecure and prone to collisions.
Long Key IDs (8 bytes) are still not as good as full Fingerprints with
20 to 32 bytes but a huge step up.

Related: rpm-software-management#2403
@pmatilai
Copy link
Member

We want to move to full fingerprint sure, but since it appears we can't do that without changes to rpm-sequoia (and the legacy parser) we might as well do this as an interim step to find out what exactly (if anything) breaks. It'll be a good while before the change (in either shape) hits the end-user systems.

Another rather user-visible instance of the short keyid use is the gpg-pubkey version, that'll need to change too and it's probably the bigger headache compatibility-wise.

@nwalfield
Copy link
Contributor

There's another option: we use fingerprints when available and long key IDs otherwise. I think this would be a less invasive change. What do you think?

@pmatilai
Copy link
Member

pmatilai commented Sep 10, 2024

It's a different kind of invasive if the output keeps changing.

One important thing here is that there needs to be a clear and easy mapping from the message(s) to what we have in the database, and vice versa. Currently that's the short keyid and that just has to go.

@pmatilai
Copy link
Member

So really, I'd merrily merge this change if you change the "other side" of the equation too, ie the imported keys.

It may not be our end goal but it's step up from the disaster where we are currently, and having that step up cemented in the code is a hundred times better than finding ourselves in the same situation in March because unexpected stuff happened. Having the short -> long keyid change in now will also help shake out anything that may be relying on that format in the meanwhile (by the way of dogfooding this stuff on real systems).

@ffesti
Copy link
Contributor Author

ffesti commented Sep 21, 2024

Turns out we actually can output the fingerprints without changing the backends.

@ffesti
Copy link
Contributor Author

ffesti commented Sep 23, 2024

I have added these changes to #3321. So it is redundant now. Closing.

@ffesti ffesti closed this Sep 23, 2024
@ffesti ffesti deleted the 2403 branch October 25, 2024 09:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DONT DO NOT merge, for whatever reason
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants