rpm icon indicating copy to clipboard operation
rpm copied to clipboard

Add Key Fingerprints to rpmsinfoMsg()

Open ffesti opened this issue 1 year ago • 14 comments

This does not yet adjust the test cases. So 198 209 216 217 220 221 222 223 226 227 are failing due to unexpected key finger prints.

Can probably be used in combination with #3292 as this does not touch the keyid output. Although we might want to skip the key id for verified signatures.

Related: #2403

ffesti avatar Sep 20 '24 12:09 ffesti

Fishing out an example output from the CI run to make it easier to discuss:

 /tmp/hello-2.0-1.x86_64.rpm:
-    Header V4 ECDSA/SHA256 Signature, key ID 5f65bbe8: OK
+    Header V4 ECDSA/SHA256 Signature, key ID 5f65bbe8: OK, Key Fingerprint: e8a62c0512b06b5d2183ba207f1c21f95f65bbe8

The :OK (or whatever the result) should continue to be the very last thing on this line to make it trivial to see/parse what the TLDR outcome of each part is.

The keyid becomes redundant if the fingerprint is there, so I guess we should just go with what @nwalfield suggested in https://github.com/rpm-software-management/rpm/pull/3292#issuecomment-2340504380 and show either the fingerprint or if not available, the long keyid. Some programs split up the fingerprint a bit to make it easier for humans to consume (eg split by space so the keyid is easy to see), but I don't know if there's a "best practise" on that.

Just FWIW, we're in no way married to using pgpIdentItem() for the rest of our days. That dumb thing never should've been a public API but the rpmio/lib split and lack of other APIs forced the issue at the time - it was supposedly the lesser evil compared leaving all the structs wide open. If we want to dynamically decide on fingerprint/keyid we'll need to ditch it anyhow it seems.

pmatilai avatar Sep 23 '24 07:09 pmatilai

Maybe it's a good idea to also return the pubkey version with the fingerprint if you're adding a new API function.

mlschroe avatar Sep 23 '24 09:09 mlschroe

OK, put https://github.com/rpm-software-management/rpm/pull/3292 underneath, adjusted the message and the name of rpmPubkeyFingerprint and make the test cases pass.

For now I have not remove the key IDs from the messages. I wonder if keeping them is more backward compatible. While they are technical redundant figuring out the key IDs from the fingerprints is kinda annoying for anyone parsing the output. But I agree that the messages are getting overly long. Any other opinions? @nwalfield ?

ffesti avatar Sep 23 '24 09:09 ffesti

For now I have not remove the key IDs from the messages. I wonder if keeping them is more backward compatible. While they are technical redundant figuring out the key IDs from the fingerprints is kinda annoying for anyone parsing the output.

See my point about splitting the fingerprint up a bit, that way you could make the keyid part stand out.

pmatilai avatar Sep 23 '24 10:09 pmatilai

OK, put #3292 underneath, adjusted the message and the name of rpmPubkeyFingerprint and make the test cases pass.

For now I have not remove the key IDs from the messages.

I think when the fingerprint is available, we should strictly prefer it.

I wonder if keeping them is more backward compatible.

What are you thinking of here? Are you imaging a script breaking because it matches on the key ID? Will those scripts still work when we add the fingerprint to the output?

figuring out the key IDs from the fingerprints is kinda annoying

In what cases does one need a key ID, but a fingerprint is rejected? In other words, why would anyone ever need to derive the key ID?

nwalfield avatar Sep 24 '24 07:09 nwalfield

Maybe it's a good idea to also return the pubkey version with the fingerprint if you're adding a new API function.

How do you imagine rpm or an application that uses the library using this information?

nwalfield avatar Sep 24 '24 07:09 nwalfield

How do you imagine rpm or an application that uses the library using this information?

Lets say you want to put the key in a database so that you can find it if you want to check a signature. In that case you need to use pubkey version plus fingerprint as a key.

mlschroe avatar Sep 24 '24 08:09 mlschroe

How do you imagine rpm or an application that uses the library using this information?

Lets say you want to put the key in a database so that you can find it if you want to check a signature. In that case you need to use pubkey version plus fingerprint as a key.

Why do you need to use the version as part of the key? Are you worried about collisions? Fingerprints don't require collision resistance, because the attacker doesn't control the key material. As such, both SHA-1 and MD5 are safe. See, for instance, this wikipedia article.

nwalfield avatar Sep 24 '24 09:09 nwalfield

Are you really arguing that the keyid would have been enough in the signature and switching to the fingerprint was a mistake? Note that the fingerprint subpackage does include the version of the key.

mlschroe avatar Sep 24 '24 09:09 mlschroe

Are you really arguing that the keyid would have been enough in the signature and switching to the fingerprint was a mistake? Note that the fingerprint subpackage does include the version of the key.

I really don't understand you. Sure, the issuer fingerprint subpacket includes the version, in which case you have the fingerprint and don't need the key ID. But in the case of the issuer subpacket, you don't have a version. Also, the issuer is self-authenticating. So if multiple key IDs match, then you (should) try them all.

nwalfield avatar Sep 24 '24 09:09 nwalfield

I wonder what's the deal with upper verses lower case hex strings. I split the fingerprints into groups of four and in lowercase this looks very wrong to me for some reason. Any opinions on that topic? Looks like RPM has always used lowercase in one continuous string.

ffesti avatar Sep 25 '24 16:09 ffesti

I wonder what's the deal with upper verses lower case hex strings. I split the fingerprints into groups of four and in lowercase this looks very wrong to me for some reason. Any opinions on that topic? Looks like RPM has always used lowercase in one continuous string.

In my experience most software uses upper case letters. One issue with splitting the fingerprint into "readable" groups is that it makes the string harder to copy and paste. We default to not splitting in Sequoia for that reason.

nwalfield avatar Sep 26 '24 07:09 nwalfield

Rpm indeed uses lowercase hex everywhere, and changing that would be inconsistent / a bit jarring . But then at least Sequoia and GPG do use uppercase so for the fingerprints (and keyids) it would make it easier to compare to those :thinking:

I remember seeing split up fingerprints someplace but looking now, indeed Sequoia and GPG use whole strings. I guess we should follow suit on that. Nothing will make a long hex string particularly suitable to human consumption anyhow :laughing:

pmatilai avatar Sep 26 '24 09:09 pmatilai

Nothing will make a long hex string particularly suitable to human consumption anyhow 😆

Exactly.

nwalfield avatar Sep 26 '24 09:09 nwalfield

OK, back to fingerprints in one piece.

ffesti avatar Sep 26 '24 12:09 ffesti

From my POV this is now complete.

ffesti avatar Sep 26 '24 12:09 ffesti

As for the upper/lowercase thing: lets leave it at lowercase here - like it is in the PR now. We'll want to revisit that when we deal with #3313 but there's no need to mess with that here just now.

pmatilai avatar Sep 27 '24 07:09 pmatilai