fix: Mark the gossip keys from the message as verified, not the ones from the db #5247
Conversation
| /// The address which introduces the given contact. | ||
| /// If we are verifying the contact, use that contacts address. | ||
| pub fn set_verified( | ||
| &mut self, | ||
| which_key: PeerstateKeyType, | ||
| key: SignedPublicKey, | ||
| fingerprint: Fingerprint, |
There was a problem hiding this comment.
I'm not completely sure whether we still need the fingerprint parameter. We could also remove it and unconditionally set the verified_key - I can't come up with a scenario where this would be a problem, but also I'm not sure I thought of every possible scenario.
There was a problem hiding this comment.
It's still needed -- see the verify_sender_by_fingerprint() call for Alice in the Steps 5+6 in "Setup verified contact" protocol section, in that case the fingerprint must be taken from the Secure-Join-Fingerprint header
There was a problem hiding this comment.
Since verify_sender_by_fingerprint() first loads the key with this fingerprint and only then calls set_verified(), I don't think that this particular usage of set_verified() requires the fingerprint parameter?
There was a problem hiding this comment.
Yes, i didn't noticed -- verify_sender_by_fingerprint() already checks the fingerprint on its own:
.filter(|&fp| fp == fingerprint)
Looks like this parameter indeed can be removed, but mark_peer_as_verified() must check then that the fingerprint is a public key fingerprint of the peer.
Hocuri
force-pushed
the
hoc/fix-verifying-gossip-keys
branch
from
4017193 to
bb2d421
Compare
| /// The address which introduces the given contact. | ||
| /// If we are verifying the contact, use that contacts address. | ||
| pub fn set_verified( | ||
| &mut self, | ||
| which_key: PeerstateKeyType, | ||
| key: SignedPublicKey, | ||
| fingerprint: Fingerprint, |
There was a problem hiding this comment.
It's still needed -- see the verify_sender_by_fingerprint() call for Alice in the Steps 5+6 in "Setup verified contact" protocol section, in that case the fingerprint must be taken from the Secure-Join-Fingerprint header
Hocuri
force-pushed
the
hoc/fix-verifying-gossip-keys
branch
from
bb2d421 to
d33de5d
Compare
…from the db
Previously, `set_verified()` loaded the gossip key from the database and
then set it as verified. Now, if, for some reason, the database doesn't
contain the key that was just gossiped with the incoming message,
then we'll mark the wrong key as verified ("gossip key injection").
Instead, remember the `SignedPublicKey`s in the `MimeMessage` and
directly mark those as verified.
Hocuri
force-pushed
the
hoc/fix-verifying-gossip-keys
branch
from
d33de5d to
4bf08b6
Compare
Hocuri
changed the title
I don't think it's feasible to write a test for this.