From: Nutomic Date: Wed, 21 Apr 2021 13:36:07 +0000 (+0000) Subject: Add option to disable strict allowlist (fixes #1486) (#1581) X-Git-Url: http://these/git/%7B%60%24%7BwebArchiveUrl%7D/%22%7B%7D/readmes/%22https:/hacktivis.me/%7Bs.link%7D?a=commitdiff_plain;h=8bb3ba4a16bda90ceace40fe701af5ce4ce9942a;p=lemmy.git Add option to disable strict allowlist (fixes #1486) (#1581) * Add option to disable strict allowlist (fixes #1486) * adjust docs --- diff --git a/config/config.hjson b/config/config.hjson index 1ab231c2..c532428c 100644 --- a/config/config.hjson +++ b/config/config.hjson @@ -65,11 +65,13 @@ # Allows and blocks are described here: # https://join.lemmy.ml/docs/en/federation/administration.html#instance-allowlist-and-blocklist # - # comma separated list of instances with which federation is allowed - # Only one of these blocks should be uncommented + # list of instances with which federation is allowed # allowed_instances: ["instance1.tld","instance2.tld"] - # comma separated list of instances which are blocked from federating + # instances which we never federate anything with (but previously federated objects are unaffected) # blocked_instances: [] + # If true, only federate with instances on the allowlist and block everything else. If false, + # use allowlist only for remote communities, and posts/comments in local communities. + # strict_allowlist: true } captcha: { enabled: true diff --git a/crates/apub/src/activities/send/community.rs b/crates/apub/src/activities/send/community.rs index f5d32142..c5112f58 100644 --- a/crates/apub/src/activities/send/community.rs +++ b/crates/apub/src/activities/send/community.rs @@ -290,7 +290,7 @@ impl CommunityType for Community { .map(|i| i.into_inner()) .unique() // Don't send to blocked instances - .filter(|inbox| check_is_apub_id_valid(inbox).is_ok()) + .filter(|inbox| check_is_apub_id_valid(inbox, false).is_ok()) .collect(); Ok(inboxes) diff --git a/crates/apub/src/activity_queue.rs b/crates/apub/src/activity_queue.rs index 924b3dd6..7016e0ca 100644 --- a/crates/apub/src/activity_queue.rs +++ b/crates/apub/src/activity_queue.rs @@ -46,7 +46,7 @@ where Kind: Serialize, >::Error: From + Send + Sync + 'static, { - if check_is_apub_id_valid(&inbox).is_ok() { + if check_is_apub_id_valid(&inbox, false).is_ok() { debug!( "Sending activity {:?} to {}", &activity.id_unchecked(), @@ -83,7 +83,7 @@ where .flatten() .unique() .filter(|inbox| inbox.host_str() != Some(&Settings::get().hostname())) - .filter(|inbox| check_is_apub_id_valid(inbox).is_ok()) + .filter(|inbox| check_is_apub_id_valid(inbox, false).is_ok()) .map(|inbox| inbox.to_owned()) .collect(); debug!( @@ -124,7 +124,7 @@ where .await?; } else { let inbox = community.get_shared_inbox_or_inbox_url(); - check_is_apub_id_valid(&inbox)?; + check_is_apub_id_valid(&inbox, false)?; debug!( "Sending activity {:?} to community {}", &activity.id_unchecked(), @@ -160,7 +160,7 @@ where ); let mentions = mentions .iter() - .filter(|inbox| check_is_apub_id_valid(inbox).is_ok()) + .filter(|inbox| check_is_apub_id_valid(inbox, false).is_ok()) .map(|i| i.to_owned()) .collect(); send_activity_internal( diff --git a/crates/apub/src/fetcher/fetch.rs b/crates/apub/src/fetcher/fetch.rs index 304d1cd1..955fc316 100644 --- a/crates/apub/src/fetcher/fetch.rs +++ b/crates/apub/src/fetcher/fetch.rs @@ -59,7 +59,7 @@ where if *recursion_counter > MAX_REQUEST_NUMBER { return Err(LemmyError::from(anyhow!("Maximum recursion depth reached")).into()); } - check_is_apub_id_valid(&url)?; + check_is_apub_id_valid(&url, false)?; let timeout = Duration::from_secs(60); diff --git a/crates/apub/src/lib.rs b/crates/apub/src/lib.rs index 89109318..394de73b 100644 --- a/crates/apub/src/lib.rs +++ b/crates/apub/src/lib.rs @@ -65,8 +65,7 @@ pub static APUB_JSON_CONTENT_TYPE: &str = "application/activity+json"; /// - URL being in the allowlist (if it is active) /// - URL not being in the blocklist (if it is active) /// -/// Note that only one of allowlist and blacklist can be enabled, not both. -pub fn check_is_apub_id_valid(apub_id: &Url) -> Result<(), LemmyError> { +pub fn check_is_apub_id_valid(apub_id: &Url, use_strict_allowlist: bool) -> Result<(), LemmyError> { let settings = Settings::get(); let domain = apub_id.domain().context(location_info!())?.to_string(); let local_instance = settings.get_hostname_without_port()?; @@ -95,30 +94,33 @@ pub fn check_is_apub_id_valid(apub_id: &Url) -> Result<(), LemmyError> { return Err(anyhow!("invalid apub id scheme {}: {}", apub_id.scheme(), apub_id).into()); } - let allowed_instances = Settings::get().get_allowed_instances(); - let blocked_instances = Settings::get().get_blocked_instances(); - - if allowed_instances.is_none() && blocked_instances.is_none() { - Ok(()) - } else if let Some(mut allowed) = allowed_instances { - // need to allow this explicitly because apub receive might contain objects from our local - // instance. split is needed to remove the port in our federation test setup. - allowed.push(local_instance); - - if allowed.contains(&domain) { - Ok(()) - } else { - Err(anyhow!("{} not in federation allowlist", domain).into()) - } - } else if let Some(blocked) = blocked_instances { + // TODO: might be good to put the part above in one method, and below in another + // (which only gets called in apub::objects) + // -> no that doesnt make sense, we still need the code below for blocklist and strict allowlist + if let Some(blocked) = Settings::get().get_blocked_instances() { if blocked.contains(&domain) { - Err(anyhow!("{} is in federation blocklist", domain).into()) - } else { - Ok(()) + return Err(anyhow!("{} is in federation blocklist", domain).into()); } - } else { - panic!("Invalid config, both allowed_instances and blocked_instances are specified"); } + + if let Some(mut allowed) = Settings::get().get_allowed_instances() { + // Only check allowlist if this is a community, or strict allowlist is enabled. + let strict_allowlist = Settings::get() + .federation() + .strict_allowlist + .unwrap_or(true); + if use_strict_allowlist || strict_allowlist { + // need to allow this explicitly because apub receive might contain objects from our local + // instance. + allowed.push(local_instance); + + if !allowed.contains(&domain) { + return Err(anyhow!("{} not in federation allowlist", domain).into()); + } + } + } + + Ok(()) } /// Common functions for ActivityPub objects, which are implemented by most (but not all) objects diff --git a/crates/apub/src/objects/comment.rs b/crates/apub/src/objects/comment.rs index 1480e5e9..ae16b76f 100644 --- a/crates/apub/src/objects/comment.rs +++ b/crates/apub/src/objects/comment.rs @@ -1,6 +1,7 @@ use crate::{ extensions::context::lemmy_context, fetcher::objects::{get_or_fetch_and_insert_comment, get_or_fetch_and_insert_post}, + get_community_from_to_or_cc, objects::{ check_object_domain, check_object_for_community_or_site_ban, @@ -145,6 +146,8 @@ impl FromApubToForm for CommentForm { request_counter: &mut i32, _mod_action_allowed: bool, ) -> Result { + let community = get_community_from_to_or_cc(note, context, request_counter).await?; + let ap_id = Some(check_object_domain(note, expected_domain, community.local)?); let creator_actor_id = ¬e .attributed_to() .context(location_info!())? @@ -202,7 +205,7 @@ impl FromApubToForm for CommentForm { published: note.published().map(|u| u.to_owned().naive_local()), updated: note.updated().map(|u| u.to_owned().naive_local()), deleted: None, - ap_id: Some(check_object_domain(note, expected_domain)?), + ap_id, local: Some(false), }) } diff --git a/crates/apub/src/objects/community.rs b/crates/apub/src/objects/community.rs index f886fd1b..a72575d0 100644 --- a/crates/apub/src/objects/community.rs +++ b/crates/apub/src/objects/community.rs @@ -203,7 +203,7 @@ impl FromApubToForm for CommunityForm { updated: group.inner.updated().map(|u| u.to_owned().naive_local()), deleted: None, nsfw: Some(group.ext_one.sensitive.unwrap_or(false)), - actor_id: Some(check_object_domain(group, expected_domain)?), + actor_id: Some(check_object_domain(group, expected_domain, true)?), local: Some(false), private_key: None, public_key: Some(group.ext_two.to_owned().public_key.public_key_pem), diff --git a/crates/apub/src/objects/mod.rs b/crates/apub/src/objects/mod.rs index 83c78282..351ece1f 100644 --- a/crates/apub/src/objects/mod.rs +++ b/crates/apub/src/objects/mod.rs @@ -98,13 +98,14 @@ where pub(in crate::objects) fn check_object_domain( apub: &T, expected_domain: Url, + use_strict_allowlist: bool, ) -> Result where T: Base + AsBase, { let domain = expected_domain.domain().context(location_info!())?; let object_id = apub.id(domain)?.context(location_info!())?; - check_is_apub_id_valid(object_id)?; + check_is_apub_id_valid(object_id, use_strict_allowlist)?; Ok(object_id.to_owned().into()) } diff --git a/crates/apub/src/objects/person.rs b/crates/apub/src/objects/person.rs index 3e468d5c..34146dc2 100644 --- a/crates/apub/src/objects/person.rs +++ b/crates/apub/src/objects/person.rs @@ -189,7 +189,7 @@ impl FromApubToForm for PersonForm { banner: banner.map(|o| o.map(|i| i.into())), published: person.inner.published().map(|u| u.to_owned().naive_local()), updated: person.updated().map(|u| u.to_owned().naive_local()), - actor_id: Some(check_object_domain(person, expected_domain)?), + actor_id: Some(check_object_domain(person, expected_domain, false)?), bio: Some(bio), local: Some(false), admin: Some(false), diff --git a/crates/apub/src/objects/post.rs b/crates/apub/src/objects/post.rs index 57cb7652..8bbc0760 100644 --- a/crates/apub/src/objects/post.rs +++ b/crates/apub/src/objects/post.rs @@ -143,12 +143,13 @@ impl FromApubToForm for PostForm { request_counter: &mut i32, mod_action_allowed: bool, ) -> Result { + let community = get_community_from_to_or_cc(page, context, request_counter).await?; let ap_id = if mod_action_allowed { let id = page.id_unchecked().context(location_info!())?; - check_is_apub_id_valid(id)?; + check_is_apub_id_valid(id, community.local)?; id.to_owned().into() } else { - check_object_domain(page, expected_domain)? + check_object_domain(page, expected_domain, community.local)? }; let ext = &page.ext_one; let creator_actor_id = page @@ -162,8 +163,6 @@ impl FromApubToForm for PostForm { let creator = get_or_fetch_and_upsert_person(creator_actor_id, context, request_counter).await?; - let community = get_community_from_to_or_cc(page, context, request_counter).await?; - let thumbnail_url: Option = match &page.inner.image() { Some(any_image) => Image::from_any_base( any_image diff --git a/crates/apub/src/objects/private_message.rs b/crates/apub/src/objects/private_message.rs index 5502fc35..6bb4820f 100644 --- a/crates/apub/src/objects/private_message.rs +++ b/crates/apub/src/objects/private_message.rs @@ -1,5 +1,4 @@ use crate::{ - check_is_apub_id_valid, extensions::context::lemmy_context, fetcher::person::get_or_fetch_and_upsert_person, objects::{ @@ -116,8 +115,7 @@ impl FromApubToForm for PrivateMessageForm { .context(location_info!())?; let recipient = get_or_fetch_and_upsert_person(&recipient_actor_id, context, request_counter).await?; - let ap_id = note.id_unchecked().context(location_info!())?.to_string(); - check_is_apub_id_valid(&Url::parse(&ap_id)?)?; + let ap_id = Some(check_object_domain(note, expected_domain, false)?); let content = get_source_markdown_value(note)?.context(location_info!())?; @@ -129,7 +127,7 @@ impl FromApubToForm for PrivateMessageForm { updated: note.updated().map(|u| u.to_owned().naive_local()), deleted: None, read: None, - ap_id: Some(check_object_domain(note, expected_domain)?), + ap_id, local: Some(false), }) } diff --git a/crates/apub_receive/src/activities/receive/private_message.rs b/crates/apub_receive/src/activities/receive/private_message.rs index 6be1a041..a0dcb81e 100644 --- a/crates/apub_receive/src/activities/receive/private_message.rs +++ b/crates/apub_receive/src/activities/receive/private_message.rs @@ -220,7 +220,7 @@ where .to_owned() .single_xsd_any_uri() .context(location_info!())?; - check_is_apub_id_valid(&person_id)?; + check_is_apub_id_valid(&person_id, false)?; // check that the sender is a person, not a community get_or_fetch_and_upsert_person(&person_id, &context, request_counter).await?; diff --git a/crates/apub_receive/src/inbox/mod.rs b/crates/apub_receive/src/inbox/mod.rs index 453781fd..dce2c279 100644 --- a/crates/apub_receive/src/inbox/mod.rs +++ b/crates/apub_receive/src/inbox/mod.rs @@ -85,7 +85,7 @@ where .to_owned() .single_xsd_any_uri() .context(location_info!())?; - check_is_apub_id_valid(&actor_id)?; + check_is_apub_id_valid(&actor_id, false)?; let actor = get_or_fetch_and_upsert_actor(&actor_id, &context, request_counter).await?; verify_signature(&request, actor.as_ref())?; Ok(actor) diff --git a/crates/apub_receive/src/inbox/person_inbox.rs b/crates/apub_receive/src/inbox/person_inbox.rs index 66b6c95a..c9b9df42 100644 --- a/crates/apub_receive/src/inbox/person_inbox.rs +++ b/crates/apub_receive/src/inbox/person_inbox.rs @@ -302,7 +302,7 @@ pub async fn receive_announce( .context(location_info!())?; let inner_id = inner_activity.id().context(location_info!())?.to_owned(); - check_is_apub_id_valid(&inner_id)?; + check_is_apub_id_valid(&inner_id, false)?; if is_activity_already_known(context.pool(), &inner_id).await? { return Ok(()); } diff --git a/crates/utils/src/settings/defaults.rs b/crates/utils/src/settings/defaults.rs index 3e7b344f..2883536a 100644 --- a/crates/utils/src/settings/defaults.rs +++ b/crates/utils/src/settings/defaults.rs @@ -49,6 +49,7 @@ impl Default for FederationConfig { enabled: false, allowed_instances: None, blocked_instances: None, + strict_allowlist: Some(true), } } } diff --git a/crates/utils/src/settings/structs.rs b/crates/utils/src/settings/structs.rs index 3863f6e3..7aad404b 100644 --- a/crates/utils/src/settings/structs.rs +++ b/crates/utils/src/settings/structs.rs @@ -77,6 +77,7 @@ pub struct FederationConfig { pub enabled: bool, pub allowed_instances: Option>, pub blocked_instances: Option>, + pub strict_allowlist: Option, } #[derive(Debug, Deserialize, Clone)]