Improved url verification, explicit debug mode

This commit is contained in:
Felix Ableitner 2022-06-02 14:30:08 +02:00
parent 318995b456
commit 73d34d8011
7 changed files with 59 additions and 15 deletions

View file

@ -14,20 +14,27 @@ steps:
- /root/.cargo/bin/cargo fmt -- --check - /root/.cargo/bin/cargo fmt -- --check
- name: cargo check - name: cargo check
image: clux/muslrust:1.59.0 image: rust:1.61-bullseye
commands: commands:
- cargo check - cargo check --all --all-targets
- name: cargo clippy - name: cargo clippy
image: clux/muslrust:1.59.0 image: rust:1.61-bullseye
commands: commands:
- rustup component add clippy - rustup component add clippy
- cargo clippy --workspace --tests --all-targets --all-features -- -D warnings -D deprecated -D clippy::perf -D clippy::complexity -D clippy::dbg_macro - cargo clippy --workspace --tests --all-targets --all-features -- -D warnings -D deprecated -D clippy::perf -D clippy::complexity -D clippy::dbg_macro
- cargo clippy --workspace -- -D clippy::unwrap_used - cargo clippy --workspace -- -D clippy::unwrap_used
- name: cargo test - name: cargo test
image: clux/muslrust:1.59.0 image: rust:1.61-bullseye
environment: environment:
RUST_BACKTRACE: 1 RUST_BACKTRACE: 1
commands: commands:
- cargo test --workspace --no-fail-fast - cargo test --workspace --no-fail-fast
- name: cargo run
image: rust:1.61-bullseye
environment:
RUST_BACKTRACE: 1
commands:
- cargo run -p activitypub_federation --example federation

View file

@ -40,7 +40,7 @@ pub struct Instance {
impl Instance { impl Instance {
pub fn new(hostname: String) -> Result<InstanceHandle, Error> { pub fn new(hostname: String) -> Result<InstanceHandle, Error> {
let settings = InstanceSettingsBuilder::default() let settings = InstanceSettingsBuilder::default()
.testing_send_sync(true) .debug(true)
.worker_count(1) .worker_count(1)
.build()?; .build()?;
let local_instance = let local_instance =

View file

@ -1,5 +1,6 @@
use crate::{ use crate::{
core::signatures::{sign_request, PublicKey}, core::signatures::{sign_request, PublicKey},
utils::verify_url_valid,
Error, Error,
LocalInstance, LocalInstance,
APUB_JSON_CONTENT_TYPE, APUB_JSON_CONTENT_TYPE,
@ -44,6 +45,9 @@ impl SendActivity {
pub async fn send(self, instance: &LocalInstance) -> Result<(), Error> { pub async fn send(self, instance: &LocalInstance) -> Result<(), Error> {
let activity_queue = &instance.activity_queue; let activity_queue = &instance.activity_queue;
for inbox in self.inboxes { for inbox in self.inboxes {
if verify_url_valid(&inbox, &instance.settings).is_err() {
continue;
}
let message = SendActivityTask { let message = SendActivityTask {
activity_id: self.activity_id.clone(), activity_id: self.activity_id.clone(),
inbox, inbox,
@ -51,7 +55,7 @@ impl SendActivity {
public_key: self.actor_public_key.clone(), public_key: self.actor_public_key.clone(),
private_key: self.actor_private_key.clone(), private_key: self.actor_private_key.clone(),
}; };
if instance.settings.testing_send_sync { if instance.settings.debug {
let res = let res =
do_send(message, &instance.client, instance.settings.request_timeout).await; do_send(message, &instance.client, instance.settings.request_timeout).await;
// Don't fail on error, as we intentionally do some invalid actions in tests, to verify that // Don't fail on error, as we intentionally do some invalid actions in tests, to verify that

View file

@ -2,7 +2,7 @@ use crate::{
core::{object_id::ObjectId, signatures::verify_signature}, core::{object_id::ObjectId, signatures::verify_signature},
data::Data, data::Data,
traits::{ActivityHandler, ApubObject}, traits::{ActivityHandler, ApubObject},
utils::verify_domains_match, utils::{verify_domains_match, verify_url_valid},
Error, Error,
LocalInstance, LocalInstance,
}; };
@ -29,13 +29,11 @@ where
E: From<anyhow::Error> + From<Error>, E: From<anyhow::Error> + From<Error>,
{ {
verify_domains_match(activity.id(), activity.actor())?; verify_domains_match(activity.id(), activity.actor())?;
verify_url_valid(activity.id(), &local_instance.settings)?;
if local_instance.is_local_url(activity.id()) { if local_instance.is_local_url(activity.id()) {
return Err(Error::UrlVerificationError("Activity was sent from local instance").into()); return Err(Error::UrlVerificationError("Activity was sent from local instance").into());
} }
(local_instance.settings.verify_url_function)(activity.id())
.map_err(Error::UrlVerificationError)?;
let request_counter = &mut 0; let request_counter = &mut 0;
let actor = ObjectId::<Actor>::new(activity.actor().clone()) let actor = ObjectId::<Actor>::new(activity.actor().clone())
.dereference::<E>(data, local_instance, request_counter) .dereference::<E>(data, local_instance, request_counter)

View file

@ -159,7 +159,7 @@ where
Kind: ApubObject + Send + 'static, Kind: ApubObject + Send + 'static,
for<'de2> <Kind as ApubObject>::ApubType: serde::Deserialize<'de2>, for<'de2> <Kind as ApubObject>::ApubType: serde::Deserialize<'de2>,
{ {
#[allow(clippy::to_string_in_display)] #[allow(clippy::recursive_format_impl)]
fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
// Use to_string here because Url.display is not useful for us // Use to_string here because Url.display is not useful for us
write!(f, "{}", self.0) write!(f, "{}", self.0)

View file

@ -32,10 +32,12 @@ pub struct InstanceSettings {
/// Number of worker threads for sending outgoing activities /// Number of worker threads for sending outgoing activities
#[builder(default = "64")] #[builder(default = "64")]
worker_count: u64, worker_count: u64,
/// Send outgoing activities synchronously, not in background thread. Helps to make tests /// Run library in debug mode. This allows usage of http and localhost urls. It also sends
/// more consistent, but not recommended for production. /// outgoing activities synchronously, not in background thread. This helps to make tests
/// more consistent.
/// Do not use for production.
#[builder(default = "false")] #[builder(default = "false")]
testing_send_sync: bool, debug: bool,
/// Timeout for all HTTP requests. HTTP signatures are valid for 10s, so it makes sense to /// Timeout for all HTTP requests. HTTP signatures are valid for 10s, so it makes sense to
/// use the same as timeout when sending /// use the same as timeout when sending
#[builder(default = "Duration::from_secs(10)")] #[builder(default = "Duration::from_secs(10)")]

View file

@ -1,4 +1,4 @@
use crate::{Error, LocalInstance, APUB_JSON_CONTENT_TYPE}; use crate::{Error, InstanceSettings, LocalInstance, APUB_JSON_CONTENT_TYPE};
use http::StatusCode; use http::StatusCode;
use serde::de::DeserializeOwned; use serde::de::DeserializeOwned;
use tracing::log::info; use tracing::log::info;
@ -11,6 +11,7 @@ pub async fn fetch_object_http<Kind: DeserializeOwned>(
) -> Result<Kind, Error> { ) -> Result<Kind, Error> {
// dont fetch local objects this way // dont fetch local objects this way
debug_assert!(url.domain() != Some(&instance.hostname)); debug_assert!(url.domain() != Some(&instance.hostname));
verify_url_valid(url, &instance.settings)?;
info!("Fetching remote object {}", url.to_string()); info!("Fetching remote object {}", url.to_string());
*request_counter += 1; *request_counter += 1;
@ -49,3 +50,35 @@ pub fn verify_urls_match(a: &Url, b: &Url) -> Result<(), Error> {
} }
Ok(()) Ok(())
} }
/// Perform some security checks on URLs as mentioned in activitypub spec, and call user-supplied
/// [`InstanceSettings.verify_url_function`].
///
/// https://www.w3.org/TR/activitypub/#security-considerations
pub fn verify_url_valid(url: &Url, settings: &InstanceSettings) -> Result<(), Error> {
match url.scheme() {
"https" => {}
"http" => {
if !settings.debug {
return Err(Error::UrlVerificationError(
"Http urls are only allowed in debug mode",
));
}
}
_ => return Err(Error::UrlVerificationError("Invalid url scheme")),
};
if url.domain().is_none() {
return Err(Error::UrlVerificationError("Url must have a domain"));
}
if url.domain() == Some("localhost") && !settings.debug {
return Err(Error::UrlVerificationError(
"Localhost is only allowed in debug mode",
));
}
(settings.verify_url_function)(url).map_err(Error::UrlVerificationError)?;
Ok(())
}