Add Key Fingerprints to rpmsinfoMsg() #3321

ffesti · 2024-09-20T12:51:08Z

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

pmatilai · 2024-09-23T07:10:12Z

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 #3292 (comment) 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.

include/rpm/rpmkeyring.h

mlschroe · 2024-09-23T09:26:26Z

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

ffesti · 2024-09-23T09:39:54Z

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 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 ?

pmatilai · 2024-09-23T10:09:35Z

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.

lib/rpmvs.c

pmatilai · 2024-09-23T10:44:12Z

rpmio/rpmkeyring.c

+    if (key == NULL)
+	return -1;
+    key = key->mainkey;
+    pthread_rwlock_rdlock(&key->lock);


The lock should be taken one line earlier, above the key assignment.

Not quite sure about that. To be fair may be I should just use an new variable for that.
But right now we are locking the main key. The one we are actually processing. For sub keys we might need to lock that, too, if the assignment needs a lock around it. Ofc we'd need to be more careful for main keys to not lock them twice.

Oh, I see it's a bit different than the average key member, and it's indeed trickier than that. But mainkey can go away between the assignment and the locking, so that's not safe as it is.

Subkeys can become separated from the main key (call rpmGetSubkeys() and then free the main key), so the mainkey needs to be reference counted instead. And then you here take a reference to the mainkey.

Also, it's indeed better to use a new variable than reuse an argument, such reuse doesn't save anything at all and makes the code less obvious.

BTW now that #3302 got merged, you'll need to rebase this and switch to the C++ locking instead of pthread_*.

OK, now that the locking is completely different I have re-done this.

rpmio/rpmkeyring.c

tests/rpmsigdig.at

rpmio/rpmkeyring.c

nwalfield · 2024-09-24T07:36:56Z

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 · 2024-09-24T07:46:01Z

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?

mlschroe · 2024-09-24T08:31:01Z

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.

nwalfield · 2024-09-24T09:07:18Z

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.

mlschroe · 2024-09-24T09:41:02Z

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.

nwalfield · 2024-09-24T09:46:09Z

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.

ffesti · 2024-09-25T16:16:03Z

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 · 2024-09-26T12:04:56Z

From my POV this is now complete.

include/rpm/rpmkeyring.h

pmatilai · 2024-09-27T07:15:18Z

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

There are some unresolved review remarks still. Marking as request changes to make it stand out, the main conversation is getting crowded to the point where its hard to see what's still relevant.

pmatilai · 2024-09-23T10:59:40Z

tests/rpmvfylevel.at

@@ -319,15 +319,15 @@ done
 [0],
 [nopls
 /data/RPMS/hello-2.0-1.x86_64-signed.rpm:
-    Header V4 RSA/SHA256 Signature, key ID 1964c5fc: OK
+    Header V4 RSA/SHA256 Signature, key ID 4344591e1964c5fc, key fingerprint: 771b18d3d7baa28734333c424344591e1964c5fc: OK


Hmm, I realize now why you originally put the fingerprint after the OK: the "key ID" part belongs to the signature. "key fingerprint" after that seems just repetitive, until you realize it comes from a different source entirely.

The message should probably make this clear somehow 🤔

On a related note, this is also why you should really decide what to output first, and once that is agreed upon, go and implement. You don't need a single line of code to mock up various versions of such messages, and concrete examples are always easier to discuss than highlevel this or that.

If you really want to keep both, then perhaps:

... signing subkey 4344591e1964c5fc, certificate: 771b18d3d7baa28734333c424344591e1964c5fc

But I'd rather just show the certificate as users don't need to know about this detail.

tests/rpmi.at

pmatilai · 2024-09-24T07:20:48Z

rpmio/rpmkeyring.c

+    if (key == NULL)
+	return -1;
+    key = key->mainkey;
+    pthread_rwlock_rdlock(&key->lock);


Oh, I see it's a bit different than the average key member, and it's indeed trickier than that. But mainkey can go away between the assignment and the locking, so that's not safe as it is.

Subkeys can become separated from the main key (call rpmGetSubkeys() and then free the main key), so the mainkey needs to be reference counted instead. And then you here take a reference to the mainkey.

Also, it's indeed better to use a new variable than reuse an argument, such reuse doesn't save anything at all and makes the code less obvious.

pmatilai · 2024-09-24T08:15:37Z

rpmio/rpmkeyring.c

+    if (key == NULL)
+	return -1;
+    key = key->mainkey;
+    pthread_rwlock_rdlock(&key->lock);


BTW now that #3302 got merged, you'll need to rebase this and switch to the C++ locking instead of pthread_*.

lib/rpmvs.c

rpmio/rpmkeyring.c

include/rpm/rpmkeyring.h

rpmio/rpmkeyring.c

include/rpm/rpmkeyring.h

rpmio/rpmkeyring.c

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

Related: rpm-software-management#2403

Store pointer to primarykey and use it to get the Fingerprint of the primary key for sub keys.

Add the key to rpmsinfo_s so we have the key available with the verified signature and can print the key's finger print when desired

The Key IDs may only be confusing when subkeys are used and the key ID won't match up with the finger print of the main key.

Even if a signature fails giving the fingerprint of the public key that is involved has some value. The key can no longer be trusted for various reasons or the package was tempered with. In both cases it might be of interest which key is outdated or attacked.

pmatilai · 2024-10-03T05:52:35Z

rpmio/rpmkeyring.c

 {
    rpmPubkey key = new rpmPubkey_s {};
-    /* Packets with all subkeys already stored in main key */
+    /* Packets with all subkeys already stored in primary key */
+    key->primarykey = primarykey;


I suppose it should actually take the reference here already, might simplify the rest of stuff as well.
But l agree, lets take another look at the whole in #3350/#3351, it should be easier to see the big picture when this is already in place. We're both getting battle-weary on this PR.

pmatilai

Okay, I think this is pretty nice now.

The issues with the subkey refcounts and locking were there to begin with, and its quite impossible to reason about those things if one of the places doesn't honor the rules at all. Would've probably been easier if we'd spotted the subkey handling doesn't make much sense to begin and address that first, but it is what it is.

ffesti added RFE crypto Signatures, keys, hashes and their verification labels Sep 20, 2024

ffesti marked this pull request as draft September 20, 2024 12:52

pmatilai reviewed Sep 23, 2024

View reviewed changes

include/rpm/rpmkeyring.h Outdated Show resolved Hide resolved

ffesti force-pushed the fingerprint branch from 3f239a8 to 7f8d5c1 Compare September 23, 2024 09:36

ffesti marked this pull request as ready for review September 23, 2024 09:43

ffesti requested a review from a team as a code owner September 23, 2024 09:43

ffesti requested review from pmatilai and removed request for a team September 23, 2024 09:43

pmatilai reviewed Sep 23, 2024

View reviewed changes

lib/rpmvs.c Outdated Show resolved Hide resolved

pmatilai reviewed Sep 23, 2024

View reviewed changes

lib/rpmvs.c Outdated Show resolved Hide resolved

pmatilai reviewed Sep 23, 2024

View reviewed changes

rpmio/rpmkeyring.c Outdated Show resolved Hide resolved

ffesti force-pushed the fingerprint branch from 7f8d5c1 to 0352def Compare September 23, 2024 10:50

ffesti mentioned this pull request Sep 23, 2024

Use long PGP Key IDs for all outputs #3292

Closed

nwalfield reviewed Sep 24, 2024

View reviewed changes

tests/rpmsigdig.at Outdated Show resolved Hide resolved

rpmio/rpmkeyring.c Outdated Show resolved Hide resolved

ffesti force-pushed the fingerprint branch from 0352def to 6721c18 Compare September 24, 2024 09:41

ffesti force-pushed the fingerprint branch 2 times, most recently from 0a63303 to a927b9d Compare September 25, 2024 15:22

nwalfield reviewed Sep 26, 2024

View reviewed changes

include/rpm/rpmkeyring.h Outdated Show resolved Hide resolved

ffesti force-pushed the fingerprint branch from a927b9d to 2bde1f4 Compare September 26, 2024 13:11

pmatilai requested changes Sep 30, 2024

View reviewed changes

ffesti force-pushed the fingerprint branch from 2bde1f4 to a42c3ae Compare September 30, 2024 13:01

pmatilai reviewed Sep 30, 2024

View reviewed changes

rpmio/rpmkeyring.c Outdated Show resolved Hide resolved

pmatilai reviewed Oct 1, 2024

View reviewed changes

include/rpm/rpmkeyring.h Show resolved Hide resolved

pmatilai reviewed Oct 1, 2024

View reviewed changes

rpmio/rpmkeyring.c Outdated Show resolved Hide resolved

ffesti force-pushed the fingerprint branch from a42c3ae to a97658f Compare October 1, 2024 10:34

ffesti requested a review from pmatilai October 1, 2024 10:39

pmatilai reviewed Oct 1, 2024

View reviewed changes

rpmio/rpmkeyring.c Show resolved Hide resolved

ffesti force-pushed the fingerprint branch from a97658f to 1db35db Compare October 1, 2024 14:18

ffesti requested a review from pmatilai October 1, 2024 14:22

pmatilai reviewed Oct 2, 2024

View reviewed changes

rpmio/rpmkeyring.c Outdated Show resolved Hide resolved

ffesti added 2 commits October 2, 2024 10:50

Use long PGP Key IDs for all outputs

4fce6ec

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

Use long PGP Key Ids in our test cases

536679e

Related: rpm-software-management#2403

ffesti force-pushed the fingerprint branch from 1db35db to 76b945f Compare October 2, 2024 09:01

ffesti added 5 commits October 2, 2024 13:51

Add rpmPubkeyFingerprint and rpmPubkeyFingerprintAsHex

8181626

Store pointer to primarykey and use it to get the Fingerprint of the primary key for sub keys.

Add rpmKeyringVerifySig2 that return the matching primary key

8df5270

Add the key to rpmsinfo_s so we have the key available with the verified signature and can print the key's finger print when desired

Add Key Fingerprints to rpmsinfoMsg()

5056380

Don't print key ID if we have the key fingerprint

3b9af6c

The Key IDs may only be confusing when subkeys are used and the key ID won't match up with the finger print of the main key.

ffesti force-pushed the fingerprint branch from 76b945f to aeb9b30 Compare October 2, 2024 11:57

ffesti requested a review from pmatilai October 2, 2024 11:58

pmatilai reviewed Oct 3, 2024

View reviewed changes

pmatilai approved these changes Oct 3, 2024

View reviewed changes

pmatilai merged commit 332edb7 into rpm-software-management:master Oct 3, 2024
1 check passed

ffesti deleted the fingerprint branch October 7, 2024 09:13

pmatilai mentioned this pull request Oct 22, 2024

RFE: use fingerprints instead of key IDs internally #1549

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Key Fingerprints to rpmsinfoMsg() #3321

Add Key Fingerprints to rpmsinfoMsg() #3321

ffesti commented Sep 20, 2024

pmatilai commented Sep 23, 2024 •

edited

Loading

mlschroe commented Sep 23, 2024

ffesti commented Sep 23, 2024

pmatilai commented Sep 23, 2024

pmatilai Sep 23, 2024 •

edited

Loading

ffesti Sep 23, 2024

pmatilai Sep 24, 2024

pmatilai Sep 24, 2024

ffesti Sep 25, 2024

nwalfield commented Sep 24, 2024

nwalfield commented Sep 24, 2024

mlschroe commented Sep 24, 2024

nwalfield commented Sep 24, 2024

mlschroe commented Sep 24, 2024

nwalfield commented Sep 24, 2024

ffesti commented Sep 25, 2024

ffesti commented Sep 26, 2024

pmatilai commented Sep 27, 2024

pmatilai left a comment

pmatilai Sep 23, 2024

pmatilai Sep 23, 2024

nwalfield Sep 30, 2024

pmatilai Sep 24, 2024

pmatilai Sep 24, 2024

pmatilai Oct 3, 2024

pmatilai left a comment

Add Key Fingerprints to rpmsinfoMsg() #3321

Add Key Fingerprints to rpmsinfoMsg() #3321

Conversation

ffesti commented Sep 20, 2024

pmatilai commented Sep 23, 2024 • edited Loading

mlschroe commented Sep 23, 2024

ffesti commented Sep 23, 2024

pmatilai commented Sep 23, 2024

pmatilai Sep 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nwalfield commented Sep 24, 2024

nwalfield commented Sep 24, 2024

mlschroe commented Sep 24, 2024

nwalfield commented Sep 24, 2024

mlschroe commented Sep 24, 2024

nwalfield commented Sep 24, 2024

ffesti commented Sep 25, 2024

ffesti commented Sep 26, 2024

pmatilai commented Sep 27, 2024

pmatilai left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmatilai left a comment

Choose a reason for hiding this comment

pmatilai commented Sep 23, 2024 •

edited

Loading

pmatilai Sep 23, 2024 •

edited

Loading