Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 17 additions & 4 deletions src/contact.rs
Original file line number Diff line number Diff line change
Expand Up @@ -372,7 +372,11 @@ pub(crate) async fn import_public_key(
.await?
{
let old_public_key = SignedPublicKey::from_slice(&public_key_bytes)?;
merged_public_key = merge_openpgp_certificates(public_key.clone(), old_public_key)

// Order of merge_openpgp_certificates() arguments is important.
// Second argument is the new key, it is preferred in case of ties,
// i.e. same signature timestamps.
merged_public_key = merge_openpgp_certificates(old_public_key, public_key.clone())
.context("Failed to merge public keys")?;
&merged_public_key
} else {
Expand Down Expand Up @@ -1472,9 +1476,7 @@ WHERE addr=?
);
}

if let Some(public_key) = contact.public_key(context).await?
&& let Some(relay_addrs) = addresses_from_public_key(&public_key)
{
if let Some(relay_addrs) = contact.relay_addresses(context).await? {
ret += "\n\nRelays:";
for relay in &relay_addrs {
ret += "\n";
Expand Down Expand Up @@ -1604,6 +1606,17 @@ WHERE addr=?
}
}

/// Returns a list of relay addresses for the contact if it is distributed in the public key.
pub(crate) async fn relay_addresses(&self, context: &Context) -> Result<Option<Vec<String>>> {
if let Some(public_key) = self.public_key(context).await?
&& let Some(relay_addrs) = addresses_from_public_key(&public_key)
{
Ok(Some(relay_addrs))
} else {
Ok(None)
}
}

/// Get name authorized by the contact.
pub fn get_authname(&self) -> &str {
&self.authname
Expand Down
42 changes: 42 additions & 0 deletions src/contact/contact_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ use deltachat_contact_tools::{addr_cmp, may_be_valid_addr};
use super::*;
use crate::chat::{Chat, get_chat_contacts, send_text_msg};
use crate::chatlist::Chatlist;
use crate::key::secret_key_to_public_key;
use crate::receive_imf::receive_imf;
use crate::securejoin::get_securejoin_qr;
use crate::test_utils::{self, TestContext, TestContextManager, TimeShiftFalsePositiveNote, sync};
Expand Down Expand Up @@ -1560,3 +1561,44 @@ async fn test_same_avatar_no_changed_event() -> Result<()> {

Ok(())
}

/// Tests that relay list from imported key is preferred if the signature
/// has the same timestamp as already imported key.
/// If they key is signed twice with different relay list at the same second,
/// we want to use the relay list from the message that arrives last
/// so the newest relay list is used unless the messages are reordered.
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
async fn test_import_public_key_tiebreak() -> Result<()> {
let mut tcm = TestContextManager::new();
let alice = &tcm.alice().await;
let bob = &tcm.bob().await;

let contact = bob.add_or_lookup_contact(alice).await;

let secret_key = test_utils::alice_keypair();
let timestamp = pgp::types::Timestamp::now();

let addr = "alice@example.net";
let relays1 = "alice@example.net,alice@example.org";
let relays2 = "alice@example.net,alice@example.com";
let relays1_vec: Vec<String> = relays1.split(',').map(String::from).collect();
let relays2_vec: Vec<String> = relays2.split(',').map(String::from).collect();

let public_key1 =
secret_key_to_public_key(secret_key.clone(), timestamp.as_secs(), addr, relays1)?;
let public_key2 = secret_key_to_public_key(secret_key, timestamp.as_secs(), addr, relays2)?;

import_public_key(bob, &public_key1).await?;
assert_eq!(contact.relay_addresses(bob).await?.unwrap(), relays1_vec);

import_public_key(bob, &public_key2).await?;
assert_eq!(contact.relay_addresses(bob).await?.unwrap(), relays2_vec);

import_public_key(bob, &public_key1).await?;
assert_eq!(contact.relay_addresses(bob).await?.unwrap(), relays1_vec);

import_public_key(bob, &public_key2).await?;
assert_eq!(contact.relay_addresses(bob).await?.unwrap(), relays2_vec);

Ok(())
}
3 changes: 3 additions & 0 deletions src/key.rs
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,9 @@ pub trait DcKey: Serialize + Deserializable + Clone {
}

/// Converts secret key to public key.
///
/// `addr` is the address used in the User ID which is generated for compatibility.
/// `relay_addrs` is the comma-separated list of relay addresses.
pub(crate) fn secret_key_to_public_key(
mut signed_secret_key: SignedSecretKey,
timestamp: u32,
Expand Down
11 changes: 8 additions & 3 deletions src/pgp.rs
Original file line number Diff line number Diff line change
Expand Up @@ -162,12 +162,13 @@ pub enum SeipdVersion {
/// encrypted to `public_keys_for_encryption`.
#[expect(clippy::arithmetic_side_effects)]
fn signature_subpackets(
now: pgp::types::Timestamp,
private_key_for_signing: &SignedSecretKey,
public_keys_for_encryption: &[SignedPublicKey],
) -> Result<SubpacketConfig> {
let mut hashed = Vec::with_capacity(1 + public_keys_for_encryption.len() + 1);
hashed.push(Subpacket::critical(SubpacketData::SignatureCreationTime(
pgp::types::Timestamp::now(),
now,
))?);
for key in public_keys_for_encryption {
let data = SubpacketData::IntendedRecipientFingerprint(key.fingerprint());
Expand Down Expand Up @@ -219,7 +220,8 @@ pub fn pk_encrypt(
}

if let Some(secret_key) = private_key_for_signing {
let subpkts = signature_subpackets(secret_key, &public_keys_for_encryption)?;
let subpkts =
signature_subpackets(now, secret_key, &public_keys_for_encryption)?;
let hash_algorithm = secret_key.hash_alg();
msg.sign_with_subpackets(
&**secret_key,
Expand Down Expand Up @@ -247,7 +249,8 @@ pub fn pk_encrypt(
}

if let Some(secret_key) = private_key_for_signing {
let subpkts = signature_subpackets(secret_key, &public_keys_for_encryption)?;
let subpkts =
signature_subpackets(now, secret_key, &public_keys_for_encryption)?;
let hash_algorithm = secret_key.hash_alg();
msg.sign_with_subpackets(
&**secret_key,
Expand Down Expand Up @@ -379,6 +382,8 @@ pub fn symm_encrypt_message(
/// or even have some packets maliciously dropped
/// (for example, all encryption subkeys dropped)
/// or restored from some older version of the certificate.
///
/// In case of ties, i.e. same timestamps, data from the new certificate is preferred.
pub fn merge_openpgp_certificates(
old_certificate: SignedPublicKey,
new_certificate: SignedPublicKey,
Expand Down
26 changes: 26 additions & 0 deletions src/pgp/pgp_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ use std::sync::LazyLock;
use tokio::sync::OnceCell;

use super::*;
use crate::key::secret_key_to_public_key;
use crate::transport::add_pseudo_transport;
use crate::{
config::Config,
Expand Down Expand Up @@ -388,6 +389,31 @@ fn test_merge_openpgp_certificates() {
assert!(merge_openpgp_certificates(bob, alice).is_err());
}

/// Tests that Direct Key Signature from the new certificate is preferred
/// when the timestamp is the same.
#[test]
fn test_merge_openpgp_certificates_tiebreak() -> Result<()> {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this test also passes on main, so it's not about what this PR changes which is what i expected it would do. Wouldn't it be a more interesting test to import two same-second signed certs via import_public_key and see that the second one wins?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Added similar test_import_public_key_tiebreak, it is failing without the fix.

let secret_key = alice_keypair();
let timestamp = pgp::types::Timestamp::now();

let addr = "alice@example.net";
let relays1 = "alice@example.net,alice@example.org";
let relays2 = "alice@example.net,alice@example.com";

let public_key1 =
secret_key_to_public_key(secret_key.clone(), timestamp.as_secs(), addr, relays1)?;
let public_key2 = secret_key_to_public_key(secret_key, timestamp.as_secs(), addr, relays2)?;

let merge12 = merge_openpgp_certificates(public_key1.clone(), public_key2.clone())?;
let merge21 = merge_openpgp_certificates(public_key2.clone(), public_key1.clone())?;

assert_eq!(merge12, public_key2);
assert_eq!(merge21, public_key1);
assert_ne!(merge12, merge21);

Ok(())
}

/// Test PQC support.
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
async fn test_pqc() -> Result<()> {
Expand Down
Loading