From 0936ef18a80e3a0ff6313dc075cb47c4fc0a9b74 Mon Sep 17 00:00:00 2001 From: kshcherban Date: Fri, 4 Sep 2026 09:54:37 +0200 Subject: [PATCH] Security improvements --- Cargo.lock | 3 + Cargo.toml | 1 + ...60827120000_password_reset_expiry.down.sql | 4 + ...0260827120000_password_reset_expiry.up.sql | 11 + .../database/entities/user/password_reset.rs | 6 +- crates/core/src/egress.rs | 62 ++++++ crates/core/src/egress/tests.rs | 97 +++++++++ crates/core/src/lib.rs | 1 + crates/core/src/storage/storage_path.rs | 23 ++ crates/core/src/storage/storage_path/tests.rs | 14 ++ crates/storage/Cargo.toml | 3 + crates/storage/src/local/error.rs | 3 + crates/storage/src/local/mod.rs | 72 +++++- crates/storage/src/local/tests.rs | 63 ++++++ crates/storage/src/s3/mod.rs | 114 +++++++++- docs/docs/sysAdmin/index.md | 33 +++ examples/config.toml | 8 + pkgly/Cargo.toml | 1 + pkgly/resources/emails/password_reset.html | 4 +- pkgly/resources/emails/password_reset.txt | 2 +- pkgly/src/app/api/mod.rs | 2 - pkgly/src/app/api/repository/management.rs | 40 ++-- .../src/app/api/repository/packages/tests.rs | 43 ++++ pkgly/src/app/api/user.rs | 12 +- pkgly/src/app/api/user/password_reset.rs | 52 +++-- .../src/app/api/user/password_reset/tests.rs | 27 +++ pkgly/src/app/api/user/tests.rs | 6 +- pkgly/src/app/authentication/jwks.rs | 12 +- pkgly/src/app/config.rs | 25 ++- pkgly/src/app/config/security.rs | 12 + pkgly/src/app/config/tests.rs | 26 +++ pkgly/src/app/email.rs | 8 + pkgly/src/app/email/tests.rs | 37 ++++ pkgly/src/app/email_service.rs | 21 +- pkgly/src/app/site.rs | 25 ++- pkgly/src/app/webhooks/mod.rs | 40 +++- pkgly/src/app/webhooks/tests.rs | 12 + pkgly/src/repository/deb/configs.rs | 6 + pkgly/src/repository/deb/hosted/tests.rs | 6 + pkgly/src/repository/deb/proxy.rs | 2 +- pkgly/src/repository/deb/proxy_refresh.rs | 6 +- pkgly/src/repository/docker/configs.rs | 12 +- pkgly/src/repository/docker/mod.rs | 3 + pkgly/src/repository/docker/proxy.rs | 2 +- pkgly/src/repository/go/configs.rs | 9 + pkgly/src/repository/go/proxy.rs | 2 +- pkgly/src/repository/maven/configs.rs | 10 +- pkgly/src/repository/maven/proxy.rs | 6 +- pkgly/src/repository/npm/configs.rs | 8 + pkgly/src/repository/npm/proxy.rs | 2 +- pkgly/src/repository/nuget/configs.rs | 5 + pkgly/src/repository/nuget/mod.rs | 7 + pkgly/src/repository/nuget/proxy.rs | 2 +- pkgly/src/repository/php/configs.rs | 10 +- pkgly/src/repository/php/proxy.rs | 9 +- pkgly/src/repository/python/configs.rs | 8 + pkgly/src/repository/python/proxy.rs | 2 +- pkgly/src/repository/repo_http.rs | 36 ++- pkgly/src/repository/repo_http/tests.rs | 14 ++ pkgly/src/repository/ruby/configs.rs | 7 +- pkgly/src/repository/ruby/proxy.rs | 2 +- pkgly/src/repository/ruby/tests.rs | 6 + pkgly/src/utils/egress.rs | 205 ++++++++++++++++++ pkgly/src/utils/egress/tests.rs | 89 ++++++++ pkgly/src/utils/mod.rs | 1 + pkgly/src/utils/upstream.rs | 69 +++++- pkgly/src/utils/upstream/tests.rs | 40 ++++ tests/README.md | 3 +- tests/docker/config/pkgly.test.toml | 13 ++ tests/docker/docker-compose.test.yml | 10 + tests/docker/seed-data.sql | 43 ++++ tests/integration/test_security.sh | 187 ++++++++++++++++ tests/run_integration_tests.sh | 9 +- 73 files changed, 1666 insertions(+), 110 deletions(-) create mode 100644 crates/core/migrations/20260827120000_password_reset_expiry.down.sql create mode 100644 crates/core/migrations/20260827120000_password_reset_expiry.up.sql create mode 100644 crates/core/src/egress.rs create mode 100644 crates/core/src/egress/tests.rs create mode 100644 pkgly/src/app/api/user/password_reset/tests.rs create mode 100644 pkgly/src/app/config/tests.rs create mode 100644 pkgly/src/app/email/tests.rs create mode 100644 pkgly/src/utils/egress.rs create mode 100644 pkgly/src/utils/egress/tests.rs create mode 100755 tests/integration/test_security.sh diff --git a/Cargo.lock b/Cargo.lock index 8634d1e..d871408 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3232,6 +3232,7 @@ dependencies = [ "aws-config", "aws-credential-types", "aws-sdk-s3", + "aws-smithy-http-client", "aws-smithy-runtime-api", "aws-smithy-types", "aws-types", @@ -3247,6 +3248,7 @@ dependencies = [ "http-body-util", "hyper 1.8.1", "hyper-util", + "ipnet", "lru 0.16.2", "md-5", "mime", @@ -3790,6 +3792,7 @@ dependencies = [ "hyper 1.8.1", "hyper-util", "inquire", + "ipnet", "jsonwebtoken", "lettre", "maven-rs", diff --git a/Cargo.toml b/Cargo.toml index 61c62cc..690a2cd 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -82,6 +82,7 @@ http-body = "1" bytes = "1" pin-project = "1" url = "2" +ipnet = "2" ## Hashing digestible = { git = "https://github.com/kshcherban/digestible.git", features = [ "base64", diff --git a/crates/core/migrations/20260827120000_password_reset_expiry.down.sql b/crates/core/migrations/20260827120000_password_reset_expiry.down.sql new file mode 100644 index 0000000..611eef7 --- /dev/null +++ b/crates/core/migrations/20260827120000_password_reset_expiry.down.sql @@ -0,0 +1,4 @@ +-- ABOUTME: Rolls back password reset token expiry storage. +-- ABOUTME: Restores the schema that predates expiry validation. +ALTER TABLE user_password_reset_tokens + DROP COLUMN expires_at; diff --git a/crates/core/migrations/20260827120000_password_reset_expiry.up.sql b/crates/core/migrations/20260827120000_password_reset_expiry.up.sql new file mode 100644 index 0000000..5b897ff --- /dev/null +++ b/crates/core/migrations/20260827120000_password_reset_expiry.up.sql @@ -0,0 +1,11 @@ +-- ABOUTME: Adds the expiry timestamp required by password reset token validation. +-- ABOUTME: Backfills existing tokens before enforcing the non-null invariant. +ALTER TABLE user_password_reset_tokens + ADD COLUMN expires_at TIMESTAMP WITH TIME ZONE; + +UPDATE user_password_reset_tokens +SET expires_at = created_at + INTERVAL '1 day' +WHERE expires_at IS NULL; + +ALTER TABLE user_password_reset_tokens + ALTER COLUMN expires_at SET NOT NULL; diff --git a/crates/core/src/database/entities/user/password_reset.rs b/crates/core/src/database/entities/user/password_reset.rs index e1e101e..0ad552c 100644 --- a/crates/core/src/database/entities/user/password_reset.rs +++ b/crates/core/src/database/entities/user/password_reset.rs @@ -1,3 +1,5 @@ +// ABOUTME: Persists password reset requests and validates their one-time tokens. +// ABOUTME: Tracks token expiry and the timestamp of terminal state changes. use chrono::Local; use rand::{SeedableRng, rngs::StdRng}; use serde::{Deserialize, Serialize}; @@ -30,7 +32,7 @@ pub struct UserPasswordReset { pub state: PasswordResetState, pub token: String, pub expires_at: DateTime, - pub used_at: DateTime, + pub state_changed_at: Option, pub created_at: DateTime, } impl UserPasswordReset { @@ -103,7 +105,7 @@ impl UserPasswordReset { } pub async fn set_used(&self, database: &PgPool) -> Result<(), sqlx::Error> { sqlx::query( - r#"UPDATE user_password_reset_tokens SET state = 'Used', used_at = NOW() WHERE id = $1"#, + r#"UPDATE user_password_reset_tokens SET state = 'Used', state_changed_at = NOW() WHERE id = $1"#, ) .bind(self.id) .execute(database) diff --git a/crates/core/src/egress.rs b/crates/core/src/egress.rs new file mode 100644 index 0000000..bde8f24 --- /dev/null +++ b/crates/core/src/egress.rs @@ -0,0 +1,62 @@ +// ABOUTME: Classifies IP addresses as globally routable for outbound traffic policy. +// ABOUTME: Shared by the Pkgly HTTP egress policy and the S3 storage egress resolver. +use std::net::IpAddr; + +fn is_in_ipv6_network(address: u128, network: u128, prefix: u32) -> bool { + address >> (128 - prefix) == network >> (128 - prefix) +} + +fn is_ietf_global_ipv6_exception(address: u128) -> bool { + matches!( + address, + 0x2001_0001_0000_0000_0000_0000_0000_0001..=0x2001_0001_0000_0000_0000_0000_0000_0003 + ) || is_in_ipv6_network(address, 0x2001_0003_0000_0000_0000_0000_0000_0000, 32) + || is_in_ipv6_network(address, 0x2001_0004_0112_0000_0000_0000_0000_0000, 48) + || is_in_ipv6_network(address, 0x2001_0020_0000_0000_0000_0000_0000_0000, 28) + || is_in_ipv6_network(address, 0x2001_0030_0000_0000_0000_0000_0000_0000, 28) +} + +/// Returns true when `address` is globally routable. +/// +/// Blocks loopback, private, link-local, multicast, unspecified, documentation, +/// benchmark, carrier-grade NAT, broadcast, all IPv4-mapped IPv6 addresses, +/// ULA, link-local, multicast, and reserved or documentation IPv6 ranges. +pub fn is_global(address: IpAddr) -> bool { + match address { + IpAddr::V4(ip) => { + let [a, b, c, d] = ip.octets(); + !(a == 0 + || a == 10 + || a == 127 + || (a == 100 && (64..=127).contains(&b)) + || (a == 169 && b == 254) + || (a == 172 && (16..=31).contains(&b)) + || (a == 192 && b == 168) + || (a == 192 && b == 0 && (c == 0 || c == 2)) + || (a == 192 && b == 88 && c == 99) + || (a == 198 && (b == 18 || b == 19)) + || (a == 198 && b == 51 && c == 100) + || (a == 203 && b == 0 && c == 113) + || a >= 224 + || (a == 255 && b == 255 && c == 255 && d == 255)) + } + IpAddr::V6(ip) => { + if ip.to_ipv4().is_some() { + return false; + } + let value = u128::from(ip); + let global_unicast = + is_in_ipv6_network(value, 0x2000_0000_0000_0000_0000_0000_0000_0000, 3); + let ietf_assignments = + is_in_ipv6_network(value, 0x2001_0000_0000_0000_0000_0000_0000_0000, 23); + global_unicast + && (!ietf_assignments || is_ietf_global_ipv6_exception(value)) + && !is_in_ipv6_network(value, 0x2001_0db8_0000_0000_0000_0000_0000_0000, 32) + && !is_in_ipv6_network(value, 0x2002_0000_0000_0000_0000_0000_0000_0000, 16) + && !is_in_ipv6_network(value, 0x3fff_0000_0000_0000_0000_0000_0000_0000, 20) + } + } +} + +#[cfg(test)] +mod tests; diff --git a/crates/core/src/egress/tests.rs b/crates/core/src/egress/tests.rs new file mode 100644 index 0000000..cc4fca0 --- /dev/null +++ b/crates/core/src/egress/tests.rs @@ -0,0 +1,97 @@ +// ABOUTME: Tests the global-routability classifier shared by egress policies. +// ABOUTME: Covers IPv4 and IPv6 reserved, private, and mapped ranges. +#![allow(clippy::expect_used)] + +use super::is_global; +use std::net::IpAddr; + +fn ipv4(value: &str) -> IpAddr { + value.parse().expect("valid ipv4 literal") +} + +fn ipv6(value: &str) -> IpAddr { + value.parse().expect("valid ipv6 literal") +} + +#[test] +fn blocks_ipv4_reserved_ranges() { + for value in [ + "0.0.0.0", + "10.0.0.1", + "100.64.0.1", + "100.127.255.254", + "127.0.0.1", + "169.254.169.254", + "172.16.0.1", + "172.31.255.254", + "192.0.0.1", + "192.0.2.1", + "192.168.0.1", + "192.88.99.1", + "198.18.0.1", + "198.19.255.254", + "198.51.100.1", + "203.0.113.1", + "224.0.0.1", + "239.255.255.255", + "240.0.0.1", + "255.255.255.255", + ] { + assert!(!is_global(ipv4(value)), "{value} must not be global"); + } +} + +#[test] +fn allows_ipv4_global_addresses() { + for value in [ + "8.8.8.8", + "1.1.1.1", + "93.184.216.34", + "198.51.101.1", + "203.0.112.1", + ] { + assert!(is_global(ipv4(value)), "{value} must be global"); + } +} + +#[test] +fn blocks_ipv6_reserved_ranges() { + for value in [ + "::", + "::1", + "::ffff:127.0.0.1", + "::ffff:10.0.0.1", + "::ffff:169.254.169.254", + "::ffff:8.8.8.8", + "64:ff9b:1::1", + "100::1", + "100:0:0:1::1", + "fc00::1", + "fd12:3456:789a::1", + "fe80::1", + "ff00::1", + "ff02::1", + "2001:2::1", + "2001:db8::1", + "2001::1", + "2002::1", + "3fff::1", + "5f00::1", + ] { + assert!(!is_global(ipv6(value)), "{value} must not be global"); + } +} + +#[test] +fn allows_ipv6_global_addresses() { + for value in [ + "2001:1::1", + "2001:3::1", + "2001:4:112::1", + "2001:4860:4860::8888", + "2606:4700:4700::1111", + "2a00:1450:4007:810::200e", + ] { + assert!(is_global(ipv6(value)), "{value} must be global"); + } +} diff --git a/crates/core/src/lib.rs b/crates/core/src/lib.rs index a3e954b..39cf9c3 100644 --- a/crates/core/src/lib.rs +++ b/crates/core/src/lib.rs @@ -1,6 +1,7 @@ pub mod user; pub type ConfigTimeStamp = chrono::DateTime; pub mod database; +pub mod egress; pub mod logging; pub mod repository; pub mod storage; diff --git a/crates/core/src/storage/storage_path.rs b/crates/core/src/storage/storage_path.rs index 0aa025f..7659b47 100644 --- a/crates/core/src/storage/storage_path.rs +++ b/crates/core/src/storage/storage_path.rs @@ -97,6 +97,29 @@ impl utoipa::ToSchema for StoragePath { } impl StoragePath { + /// Validates that all components are safe to use below a repository root. + pub fn validate(&self) -> Result<(), InvalidStoragePath> { + if self.components.iter().any(|component| { + matches!(component.as_ref(), "." | "..") || component.as_ref().contains(['\\', '\0']) + }) { + return Err(InvalidStoragePath::InvalidPath); + } + Ok(()) + } + + /// Parses a path received from an external request without normalizing it. + pub fn from_untrusted(value: &str) -> Result { + if value.is_empty() || value == "/" { + return Ok(Self::from(value)); + } + if value.starts_with(['/', '\\']) || value.contains("//") { + return Err(InvalidStoragePath::InvalidPath); + } + let path = Self::from(value); + path.validate()?; + Ok(path) + } + /// The parent of the path is always a directory. pub fn parent(self) -> Self { let mut path = self.components; diff --git a/crates/core/src/storage/storage_path/tests.rs b/crates/core/src/storage/storage_path/tests.rs index ed36bae..c8c9130 100644 --- a/crates/core/src/storage/storage_path/tests.rs +++ b/crates/core/src/storage/storage_path/tests.rs @@ -40,6 +40,20 @@ fn double_slash() { let path = StoragePath::from("test/test2//test3/"); assert_eq!(path.to_string(), "test/test2/test3/"); } + +#[test] +fn rejects_traversal_components() { + for value in ["../secret", "a/../secret", "a/./secret", "a\\..\\secret"] { + assert!(StoragePath::from(value).validate().is_err()); + } + assert!(StoragePath::from("safe/path.txt").validate().is_ok()); +} + +#[test] +fn accepts_repository_root_path() { + assert_eq!(StoragePath::from_untrusted("/").unwrap().to_string(), "/"); + assert_eq!(StoragePath::from_untrusted("").unwrap().to_string(), ""); +} #[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] struct Test { path: StoragePath, diff --git a/crates/storage/Cargo.toml b/crates/storage/Cargo.toml index d359cfb..c64d8d7 100644 --- a/crates/storage/Cargo.toml +++ b/crates/storage/Cargo.toml @@ -51,6 +51,9 @@ aws-smithy-runtime-api = "1" lru = "0.16" hex = "0.4" url = { version = "2.4", features = ["serde"] } +ipnet.workspace = true +aws-smithy-http-client = { version = "1", features = ["rustls-ring"] } +parking_lot.workspace = true [lints] workspace = true diff --git a/crates/storage/src/local/error.rs b/crates/storage/src/local/error.rs index 4c1111a..e56893c 100644 --- a/crates/storage/src/local/error.rs +++ b/crates/storage/src/local/error.rs @@ -1,5 +1,6 @@ use super::{ExtensionError, ParentDirectoryDoesNotExist, PathCollisionError}; use crate::error::WrongFileType; +use nr_core::storage::InvalidStoragePath; #[derive(Debug, thiserror::Error)] pub enum LocalStorageError { @@ -21,6 +22,8 @@ pub enum LocalStorageError { InvalidConfigType(#[from] crate::InvalidConfigType), #[error("Metadata update channel closed")] MetaUpdateChannelClosed, + #[error(transparent)] + InvalidStoragePath(#[from] InvalidStoragePath), #[error("Internal Unknown Error {0}")] Other(Box), } diff --git a/crates/storage/src/local/mod.rs b/crates/storage/src/local/mod.rs index 399cadf..a5c0cbd 100644 --- a/crates/storage/src/local/mod.rs +++ b/crates/storage/src/local/mod.rs @@ -13,7 +13,7 @@ pub use stream::*; pub mod error; mod stream; use error::LocalStorageError; -use nr_core::storage::StoragePath; +use nr_core::storage::{InvalidStoragePath, StoragePath}; use serde::{Deserialize, Serialize}; use tokio::{ sync::Mutex, @@ -182,7 +182,9 @@ impl LocalStorage { location: &StoragePath, hashes: FileHashes, ) { - let path = self.get_path(&repository, location); + let Ok(path) = self.0.checked_path(&repository, location) else { + return; + }; store_precomputed_hash(path, hashes); } @@ -273,6 +275,7 @@ impl LocalStorageInner { repository: Uuid, location: &StoragePath, ) -> Result { + location.validate()?; let mut path = self.config.path.join(repository.to_string()); let mut parent_directory = path.clone(); let mut new_directory_start = None; @@ -318,6 +321,7 @@ impl LocalStorageInner { .into()); } } + self.ensure_path_within_repository(&path, repository)?; Ok(CreatePath { path, parent_directory, @@ -331,6 +335,35 @@ impl LocalStorageInner { path.join(location) } + fn checked_path( + &self, + repository: &Uuid, + location: &StoragePath, + ) -> Result { + location.validate()?; + let root = self.config.path.join(repository.to_string()); + let path = root.join(PathBuf::from(location)); + if !path.starts_with(&root) { + return Err(InvalidStoragePath::InvalidPath.into()); + } + self.ensure_path_within_repository(&path, *repository)?; + Ok(path) + } + + fn ensure_path_within_repository( + &self, + path: &Path, + repository: Uuid, + ) -> Result<(), LocalStorageError> { + let root = self.config.path.join(repository.to_string()); + let canonical_root = canonicalize_existing(&root)?; + let canonical_path = canonicalize_existing(path)?; + if !canonical_path.starts_with(&canonical_root) { + return Err(InvalidStoragePath::InvalidPath.into()); + } + Ok(()) + } + #[instrument] pub fn open_file(&self, path: PathBuf) -> Result { let meta = StorageFileMeta::read_from_file(&path)?; @@ -625,7 +658,7 @@ impl Storage for LocalStorage { repository: Uuid, location: &StoragePath, ) -> Result { - let path = self.get_path(&repository, location); + let path = self.0.checked_path(&repository, location)?; if !path.exists() { debug!(?path, "File does not exist"); return Ok(false); @@ -653,7 +686,7 @@ impl Storage for LocalStorage { from: &StoragePath, to: &StoragePath, ) -> Result { - let from_path = self.get_path(&repository, from); + let from_path = self.0.checked_path(&repository, from)?; if !from_path.exists() { debug!(?from_path, "Source file does not exist"); return Ok(false); @@ -713,7 +746,7 @@ impl Storage for LocalStorage { repository: Uuid, location: &StoragePath, ) -> Result>, LocalStorageError> { - let path = self.get_path(&repository, location); + let path = self.0.checked_path(&repository, location)?; if !path.exists() { debug!(?path, "File does not exist"); @@ -735,7 +768,7 @@ impl Storage for LocalStorage { repository: Uuid, location: &StoragePath, ) -> Result, LocalStorageError> { - let path = self.get_path(&repository, location); + let path = self.0.checked_path(&repository, location)?; if !path.exists() { debug!(?path, "File does not exist"); return Ok(None); @@ -802,7 +835,7 @@ impl Storage for LocalStorage { repository: Uuid, location: &StoragePath, ) -> Result, LocalStorageError> { - let path = self.get_path(&repository, location); + let path = self.0.checked_path(&repository, location)?; if !path.exists() { return Ok(None); } @@ -823,7 +856,7 @@ impl Storage for LocalStorage { location: &StoragePath, value: RepositoryMeta, ) -> Result<(), LocalStorageError> { - let path = self.get_path(&repository, location); + let path = self.0.checked_path(&repository, location)?; if !path.exists() { return Err(LocalStorageError::IOError(io::Error::new( @@ -848,7 +881,7 @@ impl Storage for LocalStorage { repository: Uuid, location: &StoragePath, ) -> Result { - let path = self.get_path(&repository, location); + let path = self.0.checked_path(&repository, location)?; Ok(path.exists()) } @@ -907,7 +940,7 @@ impl Storage for LocalStorage { repository: Uuid, location: &StoragePath, ) -> Result, Self::Error> { - let path = self.get_path(&repository, location); + let path = self.0.checked_path(&repository, location)?; let stream = { let meta = path.metadata(); match meta { @@ -936,6 +969,25 @@ impl Storage for LocalStorage { Ok(Some(stream)) } } + +fn canonicalize_existing(path: &Path) -> Result { + let mut current = path; + loop { + match fs::canonicalize(current) { + Ok(path) => return Ok(path), + Err(error) if error.kind() == ErrorKind::NotFound => { + current = current.parent().ok_or_else(|| { + LocalStorageError::IOError(io::Error::new( + ErrorKind::NotFound, + "storage path has no existing parent", + )) + })?; + } + Err(error) => return Err(LocalStorageError::IOError(error)), + } + } +} + #[derive(Debug, Default)] pub struct LocalStorageFactory; impl StaticStorageFactory for LocalStorageFactory { diff --git a/crates/storage/src/local/tests.rs b/crates/storage/src/local/tests.rs index 207b654..26c474f 100644 --- a/crates/storage/src/local/tests.rs +++ b/crates/storage/src/local/tests.rs @@ -16,6 +16,69 @@ use tokio::time::{Duration, sleep}; use tracing::warn; use uuid::Uuid; +#[tokio::test] +async fn rejects_paths_that_escape_repository_root() -> anyhow::Result<()> { + let temp = tempdir()?; + let storage = + ::create_storage_from_config(StorageConfig { + storage_config: StorageConfigInner::test_config(), + type_config: StorageTypeConfig::Local(LocalConfig { + path: temp.path().to_path_buf(), + }), + }) + .await?; + let repository = Uuid::new_v4(); + let outside = temp.path().join("outside.txt"); + std::fs::write(&outside, b"sentinel")?; + + let result = storage + .save_file( + repository, + FileContent::from(b"overwrite".as_slice()), + &StoragePath::from("../outside.txt"), + ) + .await; + + assert!(result.is_err()); + assert_eq!(std::fs::read(&outside)?, b"sentinel"); + Ok(()) +} + +#[cfg(unix)] +#[tokio::test] +async fn rejects_symlink_paths_that_escape_repository_root() -> anyhow::Result<()> { + use std::os::unix::fs::symlink; + + let temp = tempdir()?; + let storage = + ::create_storage_from_config(StorageConfig { + storage_config: StorageConfigInner::test_config(), + type_config: StorageTypeConfig::Local(LocalConfig { + path: temp.path().to_path_buf(), + }), + }) + .await?; + let repository = Uuid::new_v4(); + let outside = temp.path().join("outside"); + let repository_root = temp.path().join(repository.to_string()); + std::fs::create_dir_all(&outside)?; + std::fs::create_dir_all(&repository_root)?; + std::fs::write(outside.join("sentinel.txt"), b"sentinel")?; + symlink(&outside, repository_root.join("link"))?; + + let result = storage + .save_file( + repository, + FileContent::from(b"overwrite".as_slice()), + &StoragePath::from("link/sentinel.txt"), + ) + .await; + + assert!(result.is_err()); + assert_eq!(std::fs::read(outside.join("sentinel.txt"))?, b"sentinel"); + Ok(()) +} + #[tokio::test] pub async fn generic_test() -> anyhow::Result<()> { let Some(config) = crate::testing::start_storage_test("Local")? else { diff --git a/crates/storage/src/s3/mod.rs b/crates/storage/src/s3/mod.rs index 2e2f115..55e5684 100644 --- a/crates/storage/src/s3/mod.rs +++ b/crates/storage/src/s3/mod.rs @@ -1,7 +1,16 @@ #![allow(dead_code)] use std::{ - borrow::Cow, collections::VecDeque, env, io::ErrorKind, num::NonZeroUsize, ops::Deref, - path::PathBuf, pin::Pin, str::FromStr, sync::Arc, + borrow::Cow, + collections::VecDeque, + env, + io::ErrorKind, + net::IpAddr, + num::NonZeroUsize, + ops::Deref, + path::PathBuf, + pin::Pin, + str::FromStr, + sync::{Arc, OnceLock}, }; use aws_config::BehaviorVersion; @@ -11,6 +20,7 @@ use aws_sdk_s3::{ Client as AwsS3Client, types::{CommonPrefix, Tag}, }; +use aws_smithy_runtime_api::client::dns::{DnsFuture, ResolveDns, ResolveDnsError}; use aws_smithy_runtime_api::client::result::SdkError; use aws_smithy_types::byte_stream::ByteStream; use aws_types::{SdkConfig, region::Region}; @@ -31,13 +41,16 @@ use tokio::{ task, time::{Duration, Instant}, }; -use url::Url; +use url::{Host, Url}; pub mod regions; use serde::{Deserialize, Serialize}; use tracing::{debug, error, info, instrument, warn}; use utoipa::ToSchema; pub mod tags; +use ahash::HashSet; +use ipnet::IpNet; +use parking_lot::RwLock; use uuid::Uuid; #[derive(Debug, thiserror::Error)] pub enum S3StorageError { @@ -59,6 +72,8 @@ pub enum S3StorageError { #[error(transparent)] PathCollision(#[from] PathCollisionError), + #[error("S3 endpoint is blocked by egress policy")] + BlockedEndpoint, } impl S3StorageError { pub fn static_missing_tag(tag: &'static str) -> Self { @@ -68,6 +83,70 @@ impl S3StorageError { S3StorageError::AwsSdkError(err.to_string()) } } + +#[derive(Debug, Clone, Default)] +struct S3EgressPolicy { + allowed_hosts: HashSet, + allowed_cidrs: Vec, +} + +static S3_EGRESS_POLICY: OnceLock> = OnceLock::new(); + +pub fn install_egress_policy( + allowed_hosts: &[String], + allowed_cidrs: &[String], +) -> Result<(), String> { + let policy = S3EgressPolicy { + allowed_hosts: allowed_hosts + .iter() + .map(|host| host.trim_end_matches('.').to_ascii_lowercase()) + .collect(), + allowed_cidrs: allowed_cidrs + .iter() + .map(|cidr| cidr.parse().map_err(|_| cidr.clone())) + .collect::>()?, + }; + let lock = S3_EGRESS_POLICY.get_or_init(|| RwLock::new(policy.clone())); + *lock.write() = policy; + Ok(()) +} + +fn s3_egress_policy() -> S3EgressPolicy { + let lock = S3_EGRESS_POLICY.get_or_init(|| RwLock::new(S3EgressPolicy::default())); + lock.read().clone() +} + +#[derive(Debug, Clone)] +struct S3DnsResolver; + +impl ResolveDns for S3DnsResolver { + fn resolve_dns<'a>(&'a self, name: &'a str) -> DnsFuture<'a> { + let host = name.to_owned(); + let policy = s3_egress_policy(); + DnsFuture::new(async move { + let addresses = tokio::net::lookup_host((host.as_str(), 0)) + .await + .map_err(ResolveDnsError::new)? + .map(|address| address.ip()) + .collect::>(); + if addresses.is_empty() + || addresses.iter().any(|address| { + !nr_core::egress::is_global(*address) + && !policy.allowed_hosts.contains(&host.to_ascii_lowercase()) + && !policy + .allowed_cidrs + .iter() + .any(|cidr| cidr.contains(address)) + }) + { + return Err(ResolveDnsError::new(std::io::Error::other( + "S3 destination blocked by egress policy", + ))); + } + Ok(addresses) + }) + } +} use crate::{ BorrowedStorageConfig, BorrowedStorageTypeConfig, DirectoryFileType, DynStorage, FileContent, FileContentBytes, FileFileType, FileType, InvalidConfigType, PathCollisionError, @@ -652,7 +731,36 @@ impl S3StorageInner { let mut builder = aws_sdk_s3::config::Builder::from(&base_config).force_path_style(config.path_style); + let http_client = aws_smithy_http_client::Builder::new() + .tls_provider(aws_smithy_http_client::tls::Provider::rustls( + aws_smithy_http_client::tls::rustls_provider::CryptoMode::Ring, + )) + .build_with_resolver(S3DnsResolver); + builder = builder.http_client(http_client); + if let Some(endpoint) = config.custom_endpoint() { + let host = endpoint.host_str().ok_or(S3StorageError::BlockedEndpoint)?; + let host_ip = match endpoint.host() { + Some(Host::Ipv4(ip)) => Some(IpAddr::V4(ip)), + Some(Host::Ipv6(ip)) => Some(IpAddr::V6(ip)), + _ => None, + }; + if !matches!(endpoint.scheme(), "http" | "https") + || !endpoint.username().is_empty() + || endpoint.password().is_some() + || host_ip.is_some_and(|ip| { + !nr_core::egress::is_global(ip) + && !s3_egress_policy() + .allowed_hosts + .contains(&host.to_ascii_lowercase()) + && !s3_egress_policy() + .allowed_cidrs + .iter() + .any(|cidr| cidr.contains(&ip)) + }) + { + return Err(S3StorageError::BlockedEndpoint); + } builder = builder.endpoint_url(endpoint.to_string()); } diff --git a/docs/docs/sysAdmin/index.md b/docs/docs/sysAdmin/index.md index 6ebaf64..30fc573 100644 --- a/docs/docs/sysAdmin/index.md +++ b/docs/docs/sysAdmin/index.md @@ -47,6 +47,39 @@ Finally Restart Pkgly - [Package Webhooks](./webhooks.md) — configure outbound publish/delete notifications and delivery retries. - [Package Retention](./retention.md) — configure per-repository cleanup for old package files. +## Security-sensitive configuration + +Set `[site].app_url` to the canonical HTTPS URL of the installation. Password-reset links use this configured value and never use request `Origin` or `Host` headers. Session cookies are `HttpOnly`, `Secure` when HTTPS is enabled, and always `SameSite=Lax`; the API is same-origin only. + +Outbound proxy, webhook, OIDC/JWKS, and custom S3 requests are restricted to globally routable addresses. Private destinations require explicit exact-host or CIDR exceptions: + +```toml +[security.egress] +allowed_hosts = ["s3.internal.example"] +allowed_cidrs = ["10.20.0.0/16"] +``` + +The policy rejects loopback, private, link-local, multicast, and metadata addresses by default. + +Blocked destinations are enforced for the initial request as well as every redirect hop. Webhook deliveries that are blocked at runtime are marked as failed without retries. + +## Email + +Outbound email (e.g. password reset) uses the `[email]` section: + +```toml +[email] +username = "" +password = "" +host = "smtp.example.com" +encryption = "TLS" # NONE, StartTLS, or TLS +from = "admin@pkgly.dev" +``` + +When `port` is omitted, the transport default is selected from `encryption`: `25` for `NONE`, +`587` for `StartTLS`, and `465` for `TLS`. Set `port` only for a non-standard SMTP port. +`NONE` uses plaintext SMTP and should only be used on a trusted network. + ## Enabling SSO Login Pkgly can delegate authentication to an upstream SSO provider (Cloudflare Access, Okta, Auth0, etc.) that issues signed JWT/ID tokens. Configure the security section in `cfg/pkgly.toml` to enable the feature: diff --git a/examples/config.toml b/examples/config.toml index 0562111..1e164f4 100644 --- a/examples/config.toml +++ b/examples/config.toml @@ -60,6 +60,12 @@ is_https = true [security] allow_basic_without_tokens = true +# Outbound requests may use public addresses by default. Add exact hostnames +# or CIDRs only for private proxy, webhook, OIDC, or S3 destinations. +[security.egress] +allowed_hosts = [] +allowed_cidrs = [] + [security.password_rules] min_length = 8 require_uppercase = true @@ -222,6 +228,8 @@ username = "admin@example.com" password = "change-me" # Options: "NONE", "StartTLS", "TLS" encryption = "StartTLS" +# Optional custom port. Defaults: NONE=25, StartTLS=587, TLS=465. +# port = 2525 from = "admin@example.com" reply_to = "support@example.com" diff --git a/pkgly/Cargo.toml b/pkgly/Cargo.toml index f250310..1e8ace3 100644 --- a/pkgly/Cargo.toml +++ b/pkgly/Cargo.toml @@ -136,6 +136,7 @@ lettre = { version = "0.11", features = [ "tokio1-rustls-tls", ], default-features = false } url = "2" +ipnet.workspace = true inquire = "0.7" serde_path_to_error = "0.1" [features] diff --git a/pkgly/resources/emails/password_reset.html b/pkgly/resources/emails/password_reset.html index b71913a..5a49449 100644 --- a/pkgly/resources/emails/password_reset.html +++ b/pkgly/resources/emails/password_reset.html @@ -6,6 +6,6 @@ You are receiving this email because we received a password reset request for your account. - Click Here + Click Here - \ No newline at end of file + diff --git a/pkgly/resources/emails/password_reset.txt b/pkgly/resources/emails/password_reset.txt index 462dc38..8f8a9a1 100644 --- a/pkgly/resources/emails/password_reset.txt +++ b/pkgly/resources/emails/password_reset.txt @@ -1,2 +1,2 @@ You are receiving this email because we received a password reset request for your account. - {{ panel_url }}/reset-password?token={{ token }} \ No newline at end of file + {{{ reset_url }}} diff --git a/pkgly/src/app/api/mod.rs b/pkgly/src/app/api/mod.rs index 12d15e0..c4cb618 100644 --- a/pkgly/src/app/api/mod.rs +++ b/pkgly/src/app/api/mod.rs @@ -15,7 +15,6 @@ use nr_core::{ }; use serde::{Deserialize, Serialize, ser::SerializeStruct}; use strum::IntoEnumIterator; -use tower_http::cors::CorsLayer; use tracing::{error, instrument}; use utoipa::ToSchema; pub mod artipie; @@ -53,7 +52,6 @@ pub fn api_routes() -> axum::Router { .nest("/project", project::project_routes()) .merge(artipie::routes()) .fallback(route_not_found) - .layer(CorsLayer::very_permissive()) } #[utoipa::path( get, diff --git a/pkgly/src/app/api/repository/management.rs b/pkgly/src/app/api/repository/management.rs index 8803867..af77f8f 100644 --- a/pkgly/src/app/api/repository/management.rs +++ b/pkgly/src/app/api/repository/management.rs @@ -274,6 +274,19 @@ pub async fn new_repository( return Ok(response); } + for (config_key, config_value) in configs.iter() { + let Some(config_type) = site.get_repository_config_type(config_key) else { + continue; + }; + if let Err(error) = config_type.validate_config(config_value.clone()) { + return Ok(InvalidRepositoryConfig::InvalidConfig { + config_key: config_key.to_string(), + error, + } + .into_response()); + } + } + let repository = repository_factory .create_new(name, uuid, configs, loaded_storage.clone()) .await; @@ -643,25 +656,18 @@ pub async fn update_config( if !repository_supports_config(&repository, &config_key) { return Ok(unsupported_config_response(&repository, config_key)); } - match GenericDBRepositoryConfig::get_config(repository.id(), &config_key, site.as_ref()).await? + if let Some(old) = + GenericDBRepositoryConfig::get_config(repository.id(), &config_key, site.as_ref()).await? { - Some(old) => { - if let Err(error) = config_type.validate_change(old.value.0, config.clone()) { - error!("Error validating config: {}", error); - return Ok( - InvalidRepositoryConfig::InvalidConfig { config_key, error }.into_response() - ); - } - } - None => { - if let Err(error) = config_type.validate_config(config.clone()) { - error!("Error validating config: {}", error); - return Ok( - InvalidRepositoryConfig::InvalidConfig { config_key, error }.into_response() - ); - } + if let Err(error) = config_type.validate_change(old.value.0, config.clone()) { + error!("Error validating config: {}", error); + return Ok(InvalidRepositoryConfig::InvalidConfig { config_key, error }.into_response()); } - }; + } + if let Err(error) = config_type.validate_config(config.clone()) { + error!("Error validating config: {}", error); + return Ok(InvalidRepositoryConfig::InvalidConfig { config_key, error }.into_response()); + } GenericDBRepositoryConfig::add_or_update(db_repository.id, config_key, config, site.as_ref()) .await?; diff --git a/pkgly/src/app/api/repository/packages/tests.rs b/pkgly/src/app/api/repository/packages/tests.rs index 2d2ebb8..c551435 100644 --- a/pkgly/src/app/api/repository/packages/tests.rs +++ b/pkgly/src/app/api/repository/packages/tests.rs @@ -1376,6 +1376,7 @@ mod catalog_db_tests { use nr_core::{database::DatabaseConfig, repository::config::RepositoryConfigType}; use sqlx::{PgPool, postgres::PgPoolOptions}; use testcontainers::{Container, clients::Cli, images::generic::GenericImage}; + use tower::ServiceExt; use nr_core::{ database::entities::{ @@ -1608,6 +1609,42 @@ mod catalog_db_tests { .expect("create site") } + #[tokio::test] + async fn api_preflight_returns_no_cors_headers() { + let _guard = DB_TEST_LOCK.lock().await; + let db = fresh_pool().await; + reset_database(&db).await; + let root = tempfile::tempdir().expect("tempdir"); + let site = build_site(&db, root.path()).await; + let app = crate::app::api::api_routes().with_state(site); + + let response = app + .oneshot( + axum::http::Request::builder() + .method("OPTIONS") + .uri("/api/user/token/create") + .header("origin", "https://evil.example") + .header("access-control-request-method", "POST") + .body(axum::body::Body::empty()) + .expect("build preflight request"), + ) + .await + .expect("send preflight request"); + + assert!( + !response + .headers() + .contains_key(http::header::ACCESS_CONTROL_ALLOW_ORIGIN), + "foreign-origin preflight must not receive Access-Control-Allow-Origin" + ); + assert!( + !response + .headers() + .contains_key(http::header::ACCESS_CONTROL_ALLOW_CREDENTIALS), + "foreign-origin preflight must not receive Access-Control-Allow-Credentials" + ); + } + fn sample_auth() -> Authentication { let fixed_time = chrono::DateTime::parse_from_rfc3339("2024-01-01T00:00:00+00:00").expect("time"); @@ -1703,6 +1740,12 @@ mod catalog_db_tests { let _guard = DB_TEST_LOCK.lock().await; let db = fresh_pool().await; reset_database(&db).await; + let mut webhook_security = SecuritySettings::default(); + webhook_security + .egress + .allowed_cidrs + .push("127.0.0.0/8".into()); + crate::utils::egress::install(&webhook_security.egress).expect("test egress policy"); let root = tempfile::tempdir().expect("tempdir"); let storage_id = insert_storage_at(db.pool(), root.path()).await; let repository_id = insert_deb_repository(db.pool(), storage_id).await; diff --git a/pkgly/src/app/api/user.rs b/pkgly/src/app/api/user.rs index 66ce033..d2359da 100644 --- a/pkgly/src/app/api/user.rs +++ b/pkgly/src/app/api/user.rs @@ -214,7 +214,7 @@ fn login_success_response(cookie: Cookie<'static>, user_with_session: MeWithSess fn session_cookie(session_id: String, is_https: bool) -> Cookie<'static> { Cookie::build(("session", session_id)) .secure(is_https) - .same_site(session_same_site(is_https)) + .same_site(session_same_site()) .path("/") .http_only(true) .expires(Expiration::Session) @@ -224,19 +224,15 @@ fn session_cookie(session_id: String, is_https: bool) -> Cookie<'static> { fn session_removal_cookie(is_https: bool) -> Cookie<'static> { Cookie::build("session") .secure(is_https) - .same_site(session_same_site(is_https)) + .same_site(session_same_site()) .path("/") .http_only(true) .removal() .build() } -fn session_same_site(is_https: bool) -> SameSite { - if is_https { - SameSite::None - } else { - SameSite::Lax - } +fn session_same_site() -> SameSite { + SameSite::Lax } #[utoipa::path( diff --git a/pkgly/src/app/api/user/password_reset.rs b/pkgly/src/app/api/user/password_reset.rs index df59b64..ef9edab 100644 --- a/pkgly/src/app/api/user/password_reset.rs +++ b/pkgly/src/app/api/user/password_reset.rs @@ -1,4 +1,4 @@ -use std::{net::SocketAddr, str::FromStr}; +use std::{io, net::SocketAddr, str::FromStr}; use axum::{ Json, @@ -6,10 +6,8 @@ use axum::{ response::Response, routing::{get, post}, }; -use axum_extra::{ - TypedHeader, - headers::{Origin, UserAgent}, -}; +use axum_extra::{TypedHeader, headers::UserAgent}; +use http::StatusCode; use lettre::Address; use nr_core::database::entities::user::{ ChangePasswordNoCheck, User, UserSafeData, UserType, @@ -17,18 +15,23 @@ use nr_core::database::entities::user::{ }; use serde::{Deserialize, Serialize}; use tracing::{debug, warn}; +use url::Url; use utoipa::ToSchema; use crate::{ app::{ Pkgly, authentication::password, + config::normalize_app_url, email_service::{Email, EmailDebug, template}, }, - error::InternalError, + error::{InternalError, OtherInternalError}, utils::{ResponseBuilder, request_logging::access_log::AccessLogContext}, }; +#[cfg(test)] +mod tests; + pub fn password_reset_routes() -> axum::Router { axum::Router::new() .route("/request", post(request_password_reset)) @@ -42,11 +45,23 @@ pub struct RequestPasswordReset { #[derive(Debug, Serialize)] pub struct PasswordResetEmail { pub token: UserPasswordReset, - pub panel_url: String, + pub reset_url: String, pub username: String, pub required: bool, } +fn build_reset_url(panel_url: &str, token: &str) -> Result { + let mut reset_url = Url::parse(panel_url).map_err(OtherInternalError::new)?; + { + let mut path = reset_url + .path_segments_mut() + .map_err(|_| OtherInternalError::new(io::Error::other("Invalid password reset URL")))?; + path.pop().push("reset-password"); + } + reset_url.query_pairs_mut().append_pair("token", token); + Ok(reset_url.to_string()) +} + impl Email for PasswordResetEmail { template!("password_reset"); @@ -72,11 +87,22 @@ impl Email for PasswordResetEmail { async fn request_password_reset( State(site): State, Extension(access_log): Extension, - TypedHeader(origin): TypedHeader, TypedHeader(user_agent): TypedHeader, ConnectInfo(addr): ConnectInfo, Json(password_reset): Json, ) -> Result { + let panel_url = { + let instance = site.instance.lock(); + match normalize_app_url(&instance.app_url) { + Ok(url) => url, + Err(error) => { + warn!(%error, "Password reset is unavailable because site.app_url is invalid"); + return Ok(ResponseBuilder::default() + .status(StatusCode::SERVICE_UNAVAILABLE) + .empty()); + } + } + }; let address = match Address::from_str(&password_reset.email) { Ok(ok) => ok, Err(err) => { @@ -88,20 +114,16 @@ async fn request_password_reset( ip_address: addr.ip().to_string(), user_agent: user_agent.to_string(), }; - let origin = if origin.is_null() { - return Ok(ResponseBuilder::bad_request().empty()); - } else { - origin.to_string() - }; - debug!(?request_details, ?origin, "Requesting password reset"); + debug!(?request_details, "Requesting password reset"); let user = User::get_by_email(&password_reset.email, &site.database).await?; if let Some(user) = user { access_log.set_user(user.username.as_ref().to_string()); access_log.set_user_id(user.id); let token = UserPasswordReset::create(user.id, request_details, &site.database).await?; + let reset_url = build_reset_url(&panel_url, &token.token)?; let email: PasswordResetEmail = PasswordResetEmail { token, - panel_url: origin, + reset_url: reset_url.to_string(), username: user.username.into(), required: false, }; diff --git a/pkgly/src/app/api/user/password_reset/tests.rs b/pkgly/src/app/api/user/password_reset/tests.rs new file mode 100644 index 0000000..52f0ff6 --- /dev/null +++ b/pkgly/src/app/api/user/password_reset/tests.rs @@ -0,0 +1,27 @@ +// ABOUTME: Tests password reset link construction and trusted-origin behavior. +// ABOUTME: Ensures request headers cannot control recovery URLs. +use super::*; + +#[test] +fn configured_panel_url_is_normalized() { + assert_eq!( + normalize_app_url("https://panel.example/pkgly").unwrap(), + "https://panel.example/pkgly/" + ); +} + +#[test] +fn reset_url_encodes_token_and_preserves_panel_path() { + let reset_url = build_reset_url("https://panel.example/pkgly/", "a+/=?&").unwrap(); + let parsed = Url::parse(&reset_url).unwrap(); + + assert_eq!(parsed.path(), "/pkgly/reset-password"); + assert_eq!( + parsed + .query_pairs() + .find(|(key, _)| key == "token") + .unwrap() + .1, + "a+/=?&" + ); +} diff --git a/pkgly/src/app/api/user/tests.rs b/pkgly/src/app/api/user/tests.rs index f3080d3..d0cd039 100644 --- a/pkgly/src/app/api/user/tests.rs +++ b/pkgly/src/app/api/user/tests.rs @@ -60,14 +60,14 @@ fn sample_session_for_user(user_id: i32) -> Session { } #[test] -fn session_cookie_uses_secure_none_for_https() { +fn session_cookie_uses_secure_lax_for_https() { let cookie = session_cookie("session-id".to_string(), true); let encoded = cookie.encoded().to_string(); assert!(encoded.contains("HttpOnly")); assert!(encoded.contains("Path=/")); assert!(encoded.contains("Secure")); - assert!(encoded.contains("SameSite=None")); + assert!(encoded.contains("SameSite=Lax")); } #[test] @@ -86,7 +86,7 @@ fn session_removal_cookie_matches_transport_attributes() { let https_cookie = session_removal_cookie(true).encoded().to_string(); assert!(https_cookie.contains("Path=/")); assert!(https_cookie.contains("Secure")); - assert!(https_cookie.contains("SameSite=None")); + assert!(https_cookie.contains("SameSite=Lax")); let http_cookie = session_removal_cookie(false).encoded().to_string(); assert!(http_cookie.contains("Path=/")); diff --git a/pkgly/src/app/authentication/jwks.rs b/pkgly/src/app/authentication/jwks.rs index 7dc5f44..6183596 100644 --- a/pkgly/src/app/authentication/jwks.rs +++ b/pkgly/src/app/authentication/jwks.rs @@ -173,7 +173,7 @@ pub struct ReqwestJwksFetcher { impl ReqwestJwksFetcher { pub fn new() -> Result { - let client = reqwest::Client::builder() + let client = crate::utils::upstream::client_builder() .timeout(Duration::from_secs(5)) .build() .map_err(|err| JwksError::FetchFailed(err.to_string()))?; @@ -184,10 +184,7 @@ impl ReqwestJwksFetcher { #[async_trait] impl JwksFetcher for ReqwestJwksFetcher { async fn fetch(&self, url: &str) -> Result { - let response = self - .client - .get(url) - .send() + let response = crate::utils::upstream::send(&self.client, self.client.get(url)) .await .map_err(|err| JwksError::FetchFailed(err.to_string()))?; let status = response.status(); @@ -208,10 +205,7 @@ impl JwksResolver for ReqwestJwksFetcher { "{}/.well-known/openid-configuration", issuer.trim_end_matches('/') ); - let response = self - .client - .get(&discovery_url) - .send() + let response = crate::utils::upstream::send(&self.client, self.client.get(&discovery_url)) .await .map_err(|err| JwksError::FetchFailed(err.to_string()))?; let status = response.status(); diff --git a/pkgly/src/app/config.rs b/pkgly/src/app/config.rs index 1f332d7..b2fd238 100644 --- a/pkgly/src/app/config.rs +++ b/pkgly/src/app/config.rs @@ -5,6 +5,8 @@ use tuxs_config_types::size_config::InvalidSizeError; use utoipa::ToSchema; mod max_upload; mod security; +#[cfg(test)] +mod tests; pub use max_upload::*; pub use security::*; pub const CONFIG_PREFIX: &str = "PKGLY"; @@ -19,6 +21,27 @@ pub enum ConfigError { error: InvalidSizeError, value: String, }, + #[error("Invalid site app_url: {0}")] + InvalidAppUrl(String), +} + +pub fn normalize_app_url(value: &str) -> Result { + let mut url = + url::Url::parse(value).map_err(|error| ConfigError::InvalidAppUrl(error.to_string()))?; + if !matches!(url.scheme(), "http" | "https") + || url.host_str().is_none() + || !url.username().is_empty() + || url.password().is_some() + || url.query().is_some() + || url.fragment().is_some() + { + return Err(ConfigError::InvalidAppUrl(value.to_owned())); + } + if !url.path().ends_with('/') { + let path = format!("{}/", url.path()); + url.set_path(&path); + } + Ok(url.to_string()) } #[derive(Debug, Deserialize, Serialize, Clone, Copy, PartialEq, Eq, EnumIs, ToSchema)] pub enum Mode { @@ -65,7 +88,7 @@ impl Default for WebServer { #[derive(Debug, Deserialize, Serialize, Clone)] #[serde(default)] pub struct SiteSetting { - /// If not set, the app will load the url from the request. + /// Canonical public URL used for links sent by the application. pub app_url: Option, pub name: String, pub description: String, diff --git a/pkgly/src/app/config/security.rs b/pkgly/src/app/config/security.rs index 2671fb8..adaf3f7 100644 --- a/pkgly/src/app/config/security.rs +++ b/pkgly/src/app/config/security.rs @@ -12,6 +12,8 @@ pub struct SecuritySettings { pub password_rules: Option, pub sso: Option, pub oauth2: Option, + #[serde(default)] + pub egress: EgressSettings, } impl Default for SecuritySettings { fn default() -> Self { @@ -20,10 +22,20 @@ impl Default for SecuritySettings { password_rules: Some(PasswordRules::default()), sso: None, oauth2: None, + egress: EgressSettings::default(), } } } +#[derive(Debug, Deserialize, Serialize, Clone, Default, ToSchema)] +#[serde(default)] +pub struct EgressSettings { + /// Exact hostnames allowed to resolve to private addresses. + pub allowed_hosts: Vec, + /// CIDR ranges allowed for outbound connections. + pub allowed_cidrs: Vec, +} + #[derive(Debug, Deserialize, Serialize, Clone, ToSchema)] #[serde(default)] pub struct SsoSettings { diff --git a/pkgly/src/app/config/tests.rs b/pkgly/src/app/config/tests.rs new file mode 100644 index 0000000..564733e --- /dev/null +++ b/pkgly/src/app/config/tests.rs @@ -0,0 +1,26 @@ +// ABOUTME: Tests validation and normalization of application configuration values. +// ABOUTME: Covers trusted site URL requirements used by security-sensitive links. +use super::{ConfigError, normalize_app_url}; + +#[test] +fn normalizes_site_url_path() { + assert_eq!( + normalize_app_url("https://panel.example/pkgly").unwrap(), + "https://panel.example/pkgly/" + ); +} + +#[test] +fn rejects_untrusted_site_url_shapes() { + for value in [ + "javascript:alert(1)", + "https://user:pass@panel.example/", + "https://panel.example/?redirect=evil", + "https://panel.example/#fragment", + ] { + assert!(matches!( + normalize_app_url(value), + Err(ConfigError::InvalidAppUrl(_)) + )); + } +} diff --git a/pkgly/src/app/email.rs b/pkgly/src/app/email.rs index 1cc057b..f3fac80 100644 --- a/pkgly/src/app/email.rs +++ b/pkgly/src/app/email.rs @@ -1,5 +1,10 @@ +// ABOUTME: Defines serialized SMTP settings and supported transport encryption modes. +// ABOUTME: Keeps custom SMTP ports optional so lettre can select secure defaults. use serde::{Deserialize, Serialize}; +#[cfg(test)] +mod tests; + #[derive(Debug, Deserialize, Serialize, Clone, Default)] pub enum EmailEncryption { #[default] @@ -15,6 +20,8 @@ pub struct EmailSetting { pub username: String, pub password: String, pub host: String, + #[serde(default)] + pub port: Option, pub encryption: EmailEncryption, pub from: String, pub reply_to: Option, @@ -39,6 +46,7 @@ impl Default for EmailSetting { username: "username".to_string(), password: "password".to_string(), host: "smtp.example.com".to_string(), + port: None, encryption: EmailEncryption::NONE, from: "admin@pkgly.dev".to_owned(), reply_to: None, diff --git a/pkgly/src/app/email/tests.rs b/pkgly/src/app/email/tests.rs new file mode 100644 index 0000000..9d9fee4 --- /dev/null +++ b/pkgly/src/app/email/tests.rs @@ -0,0 +1,37 @@ +// ABOUTME: Tests backward-compatible deserialization of SMTP connection settings. +// ABOUTME: Covers optional custom ports and encryption-specific default ports. +use super::{EmailEncryption, EmailSetting}; + +#[test] +fn omitted_port_preserves_transport_default() { + let setting: EmailSetting = toml::from_str( + r#" +username = "sender" +password = "secret" +host = "smtp.example.com" +encryption = "TLS" +from = "sender@example.com" +"#, + ) + .expect("email settings"); + + assert_eq!(setting.port, None); +} + +#[test] +fn explicit_port_is_preserved() { + let setting: EmailSetting = toml::from_str( + r#" +username = "" +password = "" +host = "mailpit" +port = 1025 +encryption = "NONE" +from = "sender@example.com" +"#, + ) + .expect("email settings"); + + assert_eq!(setting.port, Some(1025)); + assert!(matches!(setting.encryption, EmailEncryption::NONE)); +} diff --git a/pkgly/src/app/email_service.rs b/pkgly/src/app/email_service.rs index b5471f4..2837c4c 100644 --- a/pkgly/src/app/email_service.rs +++ b/pkgly/src/app/email_service.rs @@ -1,3 +1,5 @@ +// ABOUTME: Renders and sends queued application email through configured SMTP transports. +// ABOUTME: Supports plaintext, STARTTLS, and implicit TLS with optional authentication. use std::{ fmt::{Debug, Formatter}, io, @@ -304,13 +306,20 @@ impl EmailService { } #[instrument(name = "Connect To Email Server")] async fn build_connection(email: EmailSetting) -> Option { - let credentials = Credentials::new(email.username.clone(), email.password.clone()); - let transport = match email.encryption { - EmailEncryption::StartTLS => Transport::starttls_relay(email.host.as_str()) - .map(|builder| builder.credentials(credentials).build()), - _ => Transport::relay(email.host.as_str()) - .map(|builder| builder.credentials(credentials).build()), + let builder = match email.encryption { + EmailEncryption::NONE => Ok(Transport::builder_dangerous(email.host.as_str())), + EmailEncryption::StartTLS => Transport::starttls_relay(email.host.as_str()), + EmailEncryption::TLS => Transport::relay(email.host.as_str()), }; + let transport = builder.map(|mut builder| { + if let Some(port) = email.port { + builder = builder.port(port); + } + if !email.username.is_empty() || !email.password.is_empty() { + builder = builder.credentials(Credentials::new(email.username, email.password)); + } + builder.build() + }); match transport { Ok(transport) => { let test = match transport.test_connection().await { diff --git a/pkgly/src/app/site.rs b/pkgly/src/app/site.rs index b34dbf0..e68b546 100644 --- a/pkgly/src/app/site.rs +++ b/pkgly/src/app/site.rs @@ -58,7 +58,10 @@ use super::{ oauth::{OAuth2Rbac, OAuth2Service}, session::{SessionManager, SessionManagerConfig}, }, - config::{Mode, OAuth2Settings, PasswordRules, SecuritySettings, SiteSetting, SsoSettings}, + config::{ + Mode, OAuth2Settings, PasswordRules, SecuritySettings, SiteSetting, SsoSettings, + normalize_app_url, + }, email::EmailSetting, email_service::{EmailAccess, EmailService}, state::{Instance, InstanceOAuth2Settings, InstanceSsoSettings, RepositoryStorageName}, @@ -263,6 +266,24 @@ impl Pkgly { database: DatabaseConfig, suggested_local_storage_path: Option, ) -> anyhow::Result { + let app_url = site + .app_url + .as_deref() + .map(normalize_app_url) + .transpose() + .context("Invalid site app_url")?; + if email_settings.is_some() && app_url.is_none() { + return Err(anyhow!( + "site.app_url must be configured when email delivery is enabled" + )); + } + crate::utils::egress::install(&security.egress) + .context("Invalid outbound egress policy configuration")?; + nr_storage::s3::install_egress_policy( + &security.egress.allowed_hosts, + &security.egress.allowed_cidrs, + ) + .map_err(|error| anyhow::anyhow!("Invalid S3 egress policy configuration: {error}"))?; let database = Self::load_database(database).await?; let stored_sso = ApplicationSettings::get::("security.sso", &database) .await @@ -326,7 +347,7 @@ impl Pkgly { mode, version: current_semver!(), commit_id: build_info.commit_id, - app_url: site.app_url.unwrap_or_default(), + app_url: app_url.unwrap_or_default(), is_installed, name: site.name, description: site.description, diff --git a/pkgly/src/app/webhooks/mod.rs b/pkgly/src/app/webhooks/mod.rs index 4cab998..05e50eb 100644 --- a/pkgly/src/app/webhooks/mod.rs +++ b/pkgly/src/app/webhooks/mod.rs @@ -1,3 +1,5 @@ +// ABOUTME: Manages webhook definitions, event delivery queues, retries, and audit logging. +// ABOUTME: Enforces safe outbound headers and non-retryable egress policy failures. use std::{ collections::{BTreeMap, HashMap, HashSet}, sync::Arc, @@ -226,9 +228,10 @@ impl WebhookService { let notify_shutdown = Arc::new(Notify::new()); let worker_notify = notify_new_work.clone(); let shutdown_notify = notify_shutdown.clone(); - let client = Client::builder() + let client = crate::utils::upstream::client_builder() .timeout(DELIVERY_TIMEOUT) .user_agent("Pkgly Webhooks") + .redirect(reqwest::redirect::Policy::none()) .build() .context("Failed to build webhook HTTP client")?; let handle = tokio::spawn(async move { @@ -545,6 +548,8 @@ fn validate_webhook_input( if !matches!(parsed.scheme(), "http" | "https") { return Err(anyhow!("Webhook target URL must use http or https")); } + crate::utils::egress::validate_url(&parsed) + .map_err(|error| anyhow!("Webhook target URL is blocked: {error}"))?; let mut seen_events = HashSet::new(); let mut events = Vec::new(); @@ -590,6 +595,9 @@ fn merge_headers( } HeaderName::from_bytes(trimmed_name.as_bytes()) .map_err(|_| anyhow!("Invalid webhook header name `{trimmed_name}`"))?; + if is_forbidden_webhook_header(trimmed_name) { + return Err(anyhow!("Webhook header `{trimmed_name}` is reserved")); + } let normalized = trimmed_name.to_ascii_lowercase(); if !seen.insert(normalized.clone()) { return Err(anyhow!("Duplicate webhook header `{trimmed_name}`")); @@ -624,6 +632,23 @@ fn merge_headers( Ok(merged) } +fn is_forbidden_webhook_header(name: &str) -> bool { + matches!( + name.to_ascii_lowercase().as_str(), + "host" + | "content-length" + | "transfer-encoding" + | "connection" + | "proxy-authorization" + | "proxy-connection" + | "forwarded" + | "via" + | "x-forwarded-for" + | "x-forwarded-host" + | "x-forwarded-proto" + ) +} + fn webhook_summary_from_row(row: sqlx::postgres::PgRow) -> anyhow::Result { let headers = parse_header_summaries(row.try_get::("headers")?)?; let events = parse_events(row.try_get::("events")?)?; @@ -1022,7 +1047,7 @@ async fn deliver_once(client: &Client, delivery: &ClaimedDelivery) -> DeliveryAt request = request.header(name, value); } - let outcome = match request.send().await { + let outcome = match crate::utils::upstream::send(client, request).await { Ok(response) => classify_http_response(attempt_number, response.status().as_u16()), Err(err) => classify_transport_error(attempt_number, err), }; @@ -1236,7 +1261,16 @@ fn classify_http_response(attempt_number: i32, status: u16) -> DeliveryAttemptOu } } -fn classify_transport_error(attempt_number: i32, error: reqwest::Error) -> DeliveryAttemptOutcome { +fn classify_transport_error( + attempt_number: i32, + error: crate::utils::upstream::UpstreamError, +) -> DeliveryAttemptOutcome { + if crate::utils::egress::is_egress_blocked(&error) { + return DeliveryAttemptOutcome::Failed { + http_status: error.status().map(|value| value.as_u16() as i32), + error: "Webhook target is blocked by egress policy".to_string(), + }; + } if error.is_timeout() || error.is_connect() || error.status().is_none() { if let Some(next_attempt_at) = next_retry_at(Utc::now(), attempt_number) { return DeliveryAttemptOutcome::Retryable { diff --git a/pkgly/src/app/webhooks/tests.rs b/pkgly/src/app/webhooks/tests.rs index b83c321..6ac4576 100644 --- a/pkgly/src/app/webhooks/tests.rs +++ b/pkgly/src/app/webhooks/tests.rs @@ -90,6 +90,18 @@ fn merge_headers_rejects_new_header_without_secret() { assert!(err.to_string().contains("Provide a value")); } +#[test] +fn merge_headers_rejects_reserved_transport_headers() { + let headers = vec![WebhookHeaderInput { + name: "Host".into(), + value: Some("internal.example".into()), + configured: false, + }]; + + let err = merge_headers(None, headers).expect_err("Host must not be user-controlled"); + assert!(err.to_string().contains("reserved")); +} + #[test] fn next_retry_at_uses_exponential_backoff() { let now = DateTime::parse_from_rfc3339("2026-04-22T10:00:00Z") diff --git a/pkgly/src/repository/deb/configs.rs b/pkgly/src/repository/deb/configs.rs index f82eb29..4be4215 100644 --- a/pkgly/src/repository/deb/configs.rs +++ b/pkgly/src/repository/deb/configs.rs @@ -232,6 +232,12 @@ impl RepositoryConfigType for DebRepositoryConfigType { if let Some(refresh) = proxy.refresh.as_ref() { validate_refresh_config(refresh)?; } + + crate::utils::egress::validate_proxy_url(&proxy.upstream_url).map_err(|_| { + RepositoryConfigError::InvalidConfig( + "Proxy route URL is blocked by egress policy", + ) + })?; } } Ok(()) diff --git a/pkgly/src/repository/deb/hosted/tests.rs b/pkgly/src/repository/deb/hosted/tests.rs index fc939fd..10050e2 100644 --- a/pkgly/src/repository/deb/hosted/tests.rs +++ b/pkgly/src/repository/deb/hosted/tests.rs @@ -153,6 +153,12 @@ async fn hosted_upload_enqueues_package_published_webhook() { let db = fresh_db().await; let root = tempfile::tempdir().expect("tempdir"); let site = build_site(&db, root.path()).await; + let mut webhook_security = SecuritySettings::default(); + webhook_security + .egress + .allowed_cidrs + .push("127.0.0.0/8".into()); + crate::utils::egress::install(&webhook_security.egress).expect("test egress policy"); let storage = test_storage().await; let storage_id = Uuid::new_v4(); let repository_id = Uuid::new_v4(); diff --git a/pkgly/src/repository/deb/proxy.rs b/pkgly/src/repository/deb/proxy.rs index 23a4745..0fc87d5 100644 --- a/pkgly/src/repository/deb/proxy.rs +++ b/pkgly/src/repository/deb/proxy.rs @@ -61,7 +61,7 @@ impl DebProxyRepository { repository: DBRepository, config: DebProxyConfig, ) -> Result { - let client = reqwest::Client::builder() + let client = crate::utils::upstream::client_builder() .user_agent("Pkgly Debian Proxy") .build() .map_err(|err| RepositoryFactoryError::InvalidConfig("deb", err.to_string()))?; diff --git a/pkgly/src/repository/deb/proxy_refresh.rs b/pkgly/src/repository/deb/proxy_refresh.rs index f4aa6e1..450e64c 100644 --- a/pkgly/src/repository/deb/proxy_refresh.rs +++ b/pkgly/src/repository/deb/proxy_refresh.rs @@ -1,3 +1,5 @@ +// ABOUTME: Refreshes Debian proxy metadata and packages from configured upstreams. +// ABOUTME: Verifies upstream status, package size, and SHA-256 before persistence. use bytes::Bytes; use http::StatusCode; use nr_core::{repository::proxy_url::ProxyURL, storage::StoragePath}; @@ -22,7 +24,9 @@ pub enum DebProxyRefreshError { #[error(transparent)] Storage(#[from] nr_storage::StorageError), #[error(transparent)] - Upstream(#[from] reqwest::Error), + Upstream(#[from] crate::utils::upstream::UpstreamError), + #[error(transparent)] + UpstreamBody(#[from] reqwest::Error), #[error("invalid upstream url")] InvalidUpstreamUrl, #[error("upstream returned status {0}")] diff --git a/pkgly/src/repository/docker/configs.rs b/pkgly/src/repository/docker/configs.rs index 22ed6dc..2508398 100644 --- a/pkgly/src/repository/docker/configs.rs +++ b/pkgly/src/repository/docker/configs.rs @@ -48,7 +48,17 @@ impl RepositoryConfigType for DockerRegistryConfigType { } fn validate_config(&self, config: Value) -> Result<(), RepositoryConfigError> { - let _config: DockerRegistryConfig = serde_json::from_value(config)?; + let config: DockerRegistryConfig = serde_json::from_value(config)?; + if let DockerRegistryConfig::Proxy(proxy_cfg) = &config { + let url = url::Url::parse(&proxy_cfg.upstream_url).map_err(|_| { + RepositoryConfigError::InvalidConfig("Proxy upstream URL is invalid") + })?; + crate::utils::egress::validate_url(&url).map_err(|_| { + RepositoryConfigError::InvalidConfig( + "Proxy upstream URL is blocked by egress policy", + ) + })?; + } Ok(()) } diff --git a/pkgly/src/repository/docker/mod.rs b/pkgly/src/repository/docker/mod.rs index eaf66a1..abda1ff 100644 --- a/pkgly/src/repository/docker/mod.rs +++ b/pkgly/src/repository/docker/mod.rs @@ -1,3 +1,5 @@ +// ABOUTME: Implements hosted and proxy Docker Registry V2 and OCI repositories. +// ABOUTME: Defines Docker repository construction, dispatch, and typed error handling. //! Docker Registry V2 and OCI Image Format Implementation //! //! This module implements the Docker Registry HTTP API V2 specification @@ -136,6 +138,7 @@ impl_from_error_for_other!(sqlx::Error); impl_from_error_for_other!(serde_json::Error); impl_from_error_for_other!(std::io::Error); impl_from_error_for_other!(reqwest::Error); +impl_from_error_for_other!(crate::utils::upstream::UpstreamError); impl_from_error_for_other!(AuthenticationError); impl_from_error_for_other!(RepositoryHandlerError); impl_from_error_for_other!(nr_storage::StorageError); diff --git a/pkgly/src/repository/docker/proxy.rs b/pkgly/src/repository/docker/proxy.rs index adc85eb..783924d 100644 --- a/pkgly/src/repository/docker/proxy.rs +++ b/pkgly/src/repository/docker/proxy.rs @@ -109,7 +109,7 @@ pub struct ProxyUpstream { impl ProxyUpstream { pub(crate) fn new(config: &DockerProxyConfig) -> Result { let base = Url::parse(&config.upstream_url)?; - let client = Client::builder() + let client = crate::utils::upstream::client_builder() .connect_timeout(Duration::from_secs(10)) .timeout(Duration::from_secs(300)) .build()?; diff --git a/pkgly/src/repository/go/configs.rs b/pkgly/src/repository/go/configs.rs index fb6a15d..8ea291e 100644 --- a/pkgly/src/repository/go/configs.rs +++ b/pkgly/src/repository/go/configs.rs @@ -132,6 +132,15 @@ impl RepositoryConfigType for GoRepositoryConfigType { } } + crate::utils::egress::validate_proxy_urls( + proxy_config.routes.iter().map(|route| &route.url), + ) + .map_err(|_| { + RepositoryConfigError::InvalidConfig( + "Proxy route URL is blocked by egress policy", + ) + })?; + // Check for duplicate priorities let mut priorities = HashSet::new(); for route in proxy_config.routes.iter() { diff --git a/pkgly/src/repository/go/proxy.rs b/pkgly/src/repository/go/proxy.rs index 7cf4157..6f3e096 100644 --- a/pkgly/src/repository/go/proxy.rs +++ b/pkgly/src/repository/go/proxy.rs @@ -95,7 +95,7 @@ impl GoProxy { repository: DBRepository, config: GoProxyConfig, ) -> Result { - let client = reqwest::Client::builder() + let client = crate::utils::upstream::client_builder() .user_agent("Pkgly Go Proxy/1.0") .timeout(Duration::from_secs(30)) .build() diff --git a/pkgly/src/repository/maven/configs.rs b/pkgly/src/repository/maven/configs.rs index 4bf6cdd..472d6f3 100644 --- a/pkgly/src/repository/maven/configs.rs +++ b/pkgly/src/repository/maven/configs.rs @@ -43,7 +43,15 @@ impl RepositoryConfigType for MavenRepositoryConfigType { Some(schema_for!(MavenRepositoryConfig)) } fn validate_config(&self, config: Value) -> Result<(), RepositoryConfigError> { - let _config: MavenRepositoryConfig = serde_json::from_value(config)?; + let config: MavenRepositoryConfig = serde_json::from_value(config)?; + if let MavenRepositoryConfig::Proxy(proxy_cfg) = &config { + crate::utils::egress::validate_proxy_urls( + proxy_cfg.routes.iter().map(|route| &route.url), + ) + .map_err(|_| { + RepositoryConfigError::InvalidConfig("Proxy route URL is blocked by egress policy") + })?; + } Ok(()) } fn validate_change(&self, old: Value, new: Value) -> Result<(), RepositoryConfigError> { diff --git a/pkgly/src/repository/maven/proxy.rs b/pkgly/src/repository/maven/proxy.rs index cf00c4e..c6dfdf1 100644 --- a/pkgly/src/repository/maven/proxy.rs +++ b/pkgly/src/repository/maven/proxy.rs @@ -201,7 +201,7 @@ impl MavenProxy { site: Pkgly, proxy_config: MavenProxyConfig, ) -> Result { - let http_client = reqwest::Client::builder() + let http_client = crate::utils::upstream::client_builder() .user_agent("Pkgly") .build() .map_err(|err| RepositoryFactoryError::InvalidConfig("maven/proxy", err.to_string()))?; @@ -393,7 +393,9 @@ impl MavenProxy { path: StoragePath, ) -> Result, MavenError> { let proxy_config = self.config.read().clone(); - let http_client = reqwest::Client::builder().user_agent("Pkgly").build()?; + let http_client = crate::utils::upstream::client_builder() + .user_agent("Pkgly") + .build()?; for route in proxy_config.routes { let mut path_as_string = path.to_string(); diff --git a/pkgly/src/repository/npm/configs.rs b/pkgly/src/repository/npm/configs.rs index 6ec314c..559c5e6 100644 --- a/pkgly/src/repository/npm/configs.rs +++ b/pkgly/src/repository/npm/configs.rs @@ -52,6 +52,14 @@ impl RepositoryConfigType for NPMRegistryConfigType { validate_virtual_config(virtual_cfg) .map_err(|_| RepositoryConfigError::InvalidConfig("Invalid virtual config"))?; } + if let NPMRegistryConfig::Proxy(proxy_cfg) = &parsed { + crate::utils::egress::validate_proxy_urls( + proxy_cfg.routes.iter().map(|route| &route.url), + ) + .map_err(|_| { + RepositoryConfigError::InvalidConfig("Proxy route URL is blocked by egress policy") + })?; + } Ok(()) } fn validate_change(&self, _old: Value, new: Value) -> Result<(), RepositoryConfigError> { diff --git a/pkgly/src/repository/npm/proxy.rs b/pkgly/src/repository/npm/proxy.rs index e36f439..6877c8c 100644 --- a/pkgly/src/repository/npm/proxy.rs +++ b/pkgly/src/repository/npm/proxy.rs @@ -179,7 +179,7 @@ impl NpmProxyRegistry { repository: DBRepository, config: NpmProxyConfig, ) -> Result { - let client = reqwest::Client::builder() + let client = crate::utils::upstream::client_builder() .user_agent("Pkgly NPM Proxy") .build() .map_err(|err| { diff --git a/pkgly/src/repository/nuget/configs.rs b/pkgly/src/repository/nuget/configs.rs index e17e088..8c81962 100644 --- a/pkgly/src/repository/nuget/configs.rs +++ b/pkgly/src/repository/nuget/configs.rs @@ -47,6 +47,11 @@ impl RepositoryConfigType for NugetRepositoryConfigType { crate::repository::r#virtual::config::validate_virtual_repository_config(virtual_cfg) .map_err(|_| RepositoryConfigError::InvalidConfig("Invalid virtual config"))?; } + if let NugetRepositoryConfig::Proxy(proxy_cfg) = &parsed { + crate::utils::egress::validate_proxy_url(&proxy_cfg.upstream_url).map_err(|_| { + RepositoryConfigError::InvalidConfig("Proxy route URL is blocked by egress policy") + })?; + } Ok(()) } diff --git a/pkgly/src/repository/nuget/mod.rs b/pkgly/src/repository/nuget/mod.rs index 3003dd2..9ed51b5 100644 --- a/pkgly/src/repository/nuget/mod.rs +++ b/pkgly/src/repository/nuget/mod.rs @@ -1,3 +1,5 @@ +// ABOUTME: Defines hosted, proxy, and virtual NuGet repository implementations. +// ABOUTME: Provides NuGet repository construction, dispatch, and typed error conversion. use ahash::HashMap; use futures::future::BoxFuture; use nr_core::{ @@ -113,6 +115,11 @@ impl From for NugetError { NugetError::Other(Box::new(OtherInternalError::new(value))) } } +impl From for NugetError { + fn from(value: crate::utils::upstream::UpstreamError) -> Self { + NugetError::Other(Box::new(value)) + } +} impl From for NugetError { fn from(value: std::string::FromUtf8Error) -> Self { NugetError::Other(Box::new(OtherInternalError::new(value))) diff --git a/pkgly/src/repository/nuget/proxy.rs b/pkgly/src/repository/nuget/proxy.rs index 1c2ad0f..3e6b557 100644 --- a/pkgly/src/repository/nuget/proxy.rs +++ b/pkgly/src/repository/nuget/proxy.rs @@ -62,7 +62,7 @@ impl NugetProxy { repository: DBRepository, config: NugetProxyConfig, ) -> Result { - let client = reqwest::Client::builder() + let client = crate::utils::upstream::client_builder() .user_agent("Pkgly NuGet Proxy") .build() .map_err(|err| { diff --git a/pkgly/src/repository/php/configs.rs b/pkgly/src/repository/php/configs.rs index e1d0c24..fa15313 100644 --- a/pkgly/src/repository/php/configs.rs +++ b/pkgly/src/repository/php/configs.rs @@ -47,7 +47,15 @@ impl RepositoryConfigType for PhpRepositoryConfigType { } fn validate_config(&self, config: Value) -> Result<(), RepositoryConfigError> { - serde_json::from_value::(config)?; + let parsed = serde_json::from_value::(config)?; + if let PhpRepositoryConfig::Proxy(proxy_cfg) = &parsed { + crate::utils::egress::validate_proxy_urls( + proxy_cfg.routes.iter().map(|route| &route.url), + ) + .map_err(|_| { + RepositoryConfigError::InvalidConfig("Proxy route URL is blocked by egress policy") + })?; + } Ok(()) } diff --git a/pkgly/src/repository/php/proxy.rs b/pkgly/src/repository/php/proxy.rs index 71802e7..9c12836 100644 --- a/pkgly/src/repository/php/proxy.rs +++ b/pkgly/src/repository/php/proxy.rs @@ -1,3 +1,5 @@ +// ABOUTME: Implements Composer proxy routing, metadata aggregation, and artifact caching. +// ABOUTME: Fetches upstream package metadata and distributions through guarded HTTP clients. use std::sync::{Arc, LazyLock}; use chrono::Utc; @@ -90,7 +92,7 @@ impl PhpProxy { repository: DBRepository, config: PhpProxyConfig, ) -> Result { - let client = reqwest::Client::builder() + let client = crate::utils::upstream::client_builder() .user_agent("Pkgly PHP Proxy") .build() .map_err(|err| { @@ -568,7 +570,10 @@ impl PhpProxy { RepoResponse::Other(builder.body(body)) } - async fn fetch_upstream_dist(&self, url: &str) -> Result { + async fn fetch_upstream_dist( + &self, + url: &str, + ) -> Result { crate::utils::upstream::send( self.client(), self.client() diff --git a/pkgly/src/repository/python/configs.rs b/pkgly/src/repository/python/configs.rs index 7a9dfab..4a77e79 100644 --- a/pkgly/src/repository/python/configs.rs +++ b/pkgly/src/repository/python/configs.rs @@ -55,6 +55,14 @@ impl RepositoryConfigType for PythonRepositoryConfigType { crate::repository::r#virtual::config::validate_virtual_repository_config(virtual_cfg) .map_err(|_| RepositoryConfigError::InvalidConfig("Invalid virtual config"))?; } + if let PythonRepositoryConfig::Proxy(proxy_cfg) = &parsed { + crate::utils::egress::validate_proxy_urls( + proxy_cfg.routes.iter().map(|route| &route.url), + ) + .map_err(|_| { + RepositoryConfigError::InvalidConfig("Proxy route URL is blocked by egress policy") + })?; + } Ok(()) } diff --git a/pkgly/src/repository/python/proxy.rs b/pkgly/src/repository/python/proxy.rs index 8a0cb9f..5a72145 100644 --- a/pkgly/src/repository/python/proxy.rs +++ b/pkgly/src/repository/python/proxy.rs @@ -86,7 +86,7 @@ impl PythonProxy { repository: DBRepository, config: PythonProxyConfig, ) -> Result { - let client = reqwest::Client::builder() + let client = crate::utils::upstream::client_builder() .user_agent("Pkgly Python Proxy") .build() .map_err(|err| { diff --git a/pkgly/src/repository/repo_http.rs b/pkgly/src/repository/repo_http.rs index 9a94904..0ea2295 100644 --- a/pkgly/src/repository/repo_http.rs +++ b/pkgly/src/repository/repo_http.rs @@ -599,16 +599,48 @@ impl From>> for RepoResponse { } } #[allow(dead_code)] -#[derive(Debug, Clone, Deserialize)] +#[derive(Debug, Clone)] pub struct RepoRequestPath { storage: String, repository: String, - #[serde(default)] path: Option, + docker_scope: Option, +} + +#[derive(Debug, Deserialize)] +struct RawRepoRequestPath { + storage: String, + repository: String, + #[serde(default)] + path: Option, #[serde(default)] docker_scope: Option, } +impl<'de> Deserialize<'de> for RepoRequestPath { + fn deserialize(deserializer: D) -> Result + where + D: serde::Deserializer<'de>, + { + let raw = RawRepoRequestPath::deserialize(deserializer)?; + let path = raw + .path + .map(|path| { + // Axum may retain the separator between the repository and wildcard. + let path = path.strip_prefix('/').unwrap_or(&path); + StoragePath::from_untrusted(path) + }) + .transpose() + .map_err(serde::de::Error::custom)?; + Ok(Self { + storage: raw.storage, + repository: raw.repository, + path, + docker_scope: raw.docker_scope, + }) + } +} + /// Core repository request handler logic (extracted for reuse) async fn handle_repo_request_core( site: Pkgly, diff --git a/pkgly/src/repository/repo_http/tests.rs b/pkgly/src/repository/repo_http/tests.rs index a117c2b..654af32 100644 --- a/pkgly/src/repository/repo_http/tests.rs +++ b/pkgly/src/repository/repo_http/tests.rs @@ -25,6 +25,20 @@ use std::sync::LazyLock; use testcontainers::{Container, clients::Cli, images::generic::GenericImage}; use uuid::Uuid; +#[test] +fn repository_request_rejects_traversal_path() { + let result: Result = + serde_json::from_str(r#"{"storage":"local","repository":"repo","path":"../secret"}"#); + assert!(result.is_err()); +} + +#[test] +fn repository_request_accepts_router_separator_before_path() { + let result: Result = + serde_json::from_str(r#"{"storage":"local","repository":"repo","path":"/api/v1/gems"}"#); + assert!(result.is_ok()); +} + static DB_LOCK: LazyLock> = LazyLock::new(|| tokio::sync::Mutex::new(())); struct TestDb { diff --git a/pkgly/src/repository/ruby/configs.rs b/pkgly/src/repository/ruby/configs.rs index d936058..93dd8fe 100644 --- a/pkgly/src/repository/ruby/configs.rs +++ b/pkgly/src/repository/ruby/configs.rs @@ -44,7 +44,12 @@ impl RepositoryConfigType for RubyRepositoryConfigType { } fn validate_config(&self, config: Value) -> Result<(), RepositoryConfigError> { - serde_json::from_value::(config)?; + let parsed = serde_json::from_value::(config)?; + if let RubyRepositoryConfig::Proxy(proxy_cfg) = &parsed { + crate::utils::egress::validate_proxy_url(&proxy_cfg.upstream_url).map_err(|_| { + RepositoryConfigError::InvalidConfig("Proxy route URL is blocked by egress policy") + })?; + } Ok(()) } diff --git a/pkgly/src/repository/ruby/proxy.rs b/pkgly/src/repository/ruby/proxy.rs index 40034b8..0f1f1e6 100644 --- a/pkgly/src/repository/ruby/proxy.rs +++ b/pkgly/src/repository/ruby/proxy.rs @@ -68,7 +68,7 @@ impl RubyProxy { repository: DBRepository, config: RubyProxyConfig, ) -> Result { - let client = reqwest::Client::builder() + let client = crate::utils::upstream::client_builder() .user_agent("Pkgly Ruby Proxy") .build() .map_err(|err| { diff --git a/pkgly/src/repository/ruby/tests.rs b/pkgly/src/repository/ruby/tests.rs index 3e645c0..6925608 100644 --- a/pkgly/src/repository/ruby/tests.rs +++ b/pkgly/src/repository/ruby/tests.rs @@ -198,6 +198,12 @@ async fn ruby_yank_enqueues_delete_webhook_before_catalog_row_is_removed() { let db = fresh_db().await; let root = tempfile::tempdir().expect("tempdir"); let site = build_site(&db, root.path()).await; + let mut webhook_security = SecuritySettings::default(); + webhook_security + .egress + .allowed_cidrs + .push("127.0.0.0/8".into()); + crate::utils::egress::install(&webhook_security.egress).expect("test egress policy"); let storage = test_storage().await; let storage_id = Uuid::new_v4(); let repository_id = Uuid::new_v4(); diff --git a/pkgly/src/utils/egress.rs b/pkgly/src/utils/egress.rs new file mode 100644 index 0000000..9650d46 --- /dev/null +++ b/pkgly/src/utils/egress.rs @@ -0,0 +1,205 @@ +// ABOUTME: Defines the outbound network policy and DNS resolver used by Pkgly. +// ABOUTME: Blocks non-global destinations unless an explicit host or CIDR exception exists. +use std::{ + net::IpAddr, + sync::{Arc, OnceLock}, +}; + +use ahash::{HashSet, HashSetExt}; +use ipnet::IpNet; +use parking_lot::RwLock; +use reqwest::dns::{Addrs, Name, Resolve, Resolving}; + +use crate::app::config::EgressSettings; + +#[derive(Debug, thiserror::Error, PartialEq, Eq)] +pub enum EgressPolicyError { + #[error("unsupported outbound URL scheme")] + UnsupportedScheme, + #[error("outbound URL host is missing")] + MissingHost, + #[error("outbound URL credentials are not allowed")] + Credentials, + #[error("outbound destination is blocked by egress policy")] + Blocked, + #[error("invalid egress CIDR: {0}")] + InvalidCidr(String), +} + +#[derive(Debug, Clone)] +pub struct EgressPolicy { + allowed_hosts: HashSet, + allowed_cidrs: Vec, +} + +impl EgressPolicy { + pub fn from_settings(settings: &EgressSettings) -> Result { + let allowed_cidrs = settings + .allowed_cidrs + .iter() + .map(|value| { + value + .parse() + .map_err(|_| EgressPolicyError::InvalidCidr(value.clone())) + }) + .collect::, _>>()?; + let allowed_hosts = settings + .allowed_hosts + .iter() + .map(|host| host.trim_end_matches('.').to_ascii_lowercase()) + .filter(|host| !host.is_empty()) + .collect(); + Ok(Self { + allowed_hosts, + allowed_cidrs, + }) + } + + pub fn validate_url(&self, url: &url::Url) -> Result<(), EgressPolicyError> { + if !matches!(url.scheme(), "http" | "https") { + return Err(EgressPolicyError::UnsupportedScheme); + } + let host = url.host_str().ok_or(EgressPolicyError::MissingHost)?; + if !url.username().is_empty() || url.password().is_some() { + return Err(EgressPolicyError::Credentials); + } + match url.host() { + Some(url::Host::Ipv4(ip)) => self.validate_address(host, IpAddr::V4(ip)), + Some(url::Host::Ipv6(ip)) => self.validate_address(host, IpAddr::V6(ip)), + Some(url::Host::Domain(_)) => Ok(()), + None => Err(EgressPolicyError::MissingHost), + } + } + + fn validate_address(&self, host: &str, address: IpAddr) -> Result<(), EgressPolicyError> { + let host_allowed = self + .allowed_hosts + .contains(&host.trim_end_matches('.').to_ascii_lowercase()); + let range_allowed = self + .allowed_cidrs + .iter() + .any(|network| network.contains(&address)); + if nr_core::egress::is_global(address) || host_allowed || range_allowed { + Ok(()) + } else { + Err(EgressPolicyError::Blocked) + } + } +} + +static GLOBAL_POLICY: OnceLock>> = OnceLock::new(); + +fn runtime_policy(settings: &EgressSettings) -> Result { + let policy = EgressPolicy::from_settings(settings)?; + #[cfg(test)] + let policy = { + let mut policy = policy; + if let Ok(test_server) = "127.0.0.1/32".parse() { + policy.allowed_cidrs.push(test_server); + } + policy + }; + Ok(policy) +} + +pub fn install(settings: &EgressSettings) -> Result<(), EgressPolicyError> { + let policy = runtime_policy(settings)?; + let lock = GLOBAL_POLICY.get_or_init(|| Arc::new(RwLock::new(policy.clone()))); + *lock.write() = policy; + Ok(()) +} + +fn global() -> EgressPolicy { + let lock = GLOBAL_POLICY.get_or_init(|| { + Arc::new(RwLock::new( + runtime_policy(&EgressSettings::default()).unwrap_or_else(|_| EgressPolicy { + allowed_hosts: HashSet::new(), + allowed_cidrs: Vec::new(), + }), + )) + }); + lock.read().clone() +} + +pub fn validate_url(url: &url::Url) -> Result<(), EgressPolicyError> { + global().validate_url(url) +} + +/// Validates a repository proxy route URL against the egress policy. +pub fn validate_proxy_url( + value: &nr_core::repository::proxy_url::ProxyURL, +) -> Result<(), EgressPolicyError> { + let parsed = url::Url::parse(value.as_str()).map_err(|_| EgressPolicyError::MissingHost)?; + validate_url(&parsed) +} + +/// Validates every proxy route URL, rejecting any that violates the policy. +pub fn validate_proxy_urls<'a>( + urls: impl IntoIterator, +) -> Result<(), EgressPolicyError> { + for url in urls { + validate_proxy_url(url)?; + } + Ok(()) +} + +/// Marker error surfacing through DNS resolution and transport layers when a +/// destination is blocked by the egress policy. Detected via the source chain. +#[derive(Debug)] +pub struct EgressBlockedError; + +impl std::fmt::Display for EgressBlockedError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "outbound destination is blocked by egress policy") + } +} + +impl std::error::Error for EgressBlockedError {} + +/// Returns true when the error chain contains the egress policy marker. +pub fn is_egress_blocked(error: &E) -> bool { + fn walk(source: &(dyn std::error::Error + 'static)) -> bool { + if source.downcast_ref::().is_some() { + return true; + } + source.source().is_some_and(walk) + } + walk(error) +} + +#[derive(Debug, Clone)] +pub struct SafeResolver; + +impl Resolve for SafeResolver { + fn resolve(&self, name: Name) -> Resolving { + let host = name.as_str().to_owned(); + let policy = global(); + Box::pin(async move { + let addresses = tokio::net::lookup_host((host.as_str(), 0)) + .await + .map_err(|error| -> Box { Box::new(error) })? + .collect::>(); + if addresses.is_empty() { + return Err( + Box::new(EgressBlockedError) as Box + ); + } + for address in &addresses { + policy.validate_address(&host, address.ip()).map_err( + |_| -> Box { + Box::new(EgressBlockedError) + }, + )?; + } + let addrs: Addrs = Box::new(addresses.into_iter()); + Ok(addrs) + }) + } +} + +pub fn resolver() -> Arc { + Arc::new(SafeResolver) +} + +#[cfg(test)] +mod tests; diff --git a/pkgly/src/utils/egress/tests.rs b/pkgly/src/utils/egress/tests.rs new file mode 100644 index 0000000..f4ee277 --- /dev/null +++ b/pkgly/src/utils/egress/tests.rs @@ -0,0 +1,89 @@ +// ABOUTME: Tests outbound destination classification and explicit exceptions. +// ABOUTME: Covers private, global, hostname, and CIDR policy behavior. +use super::*; + +fn policy() -> EgressPolicy { + EgressPolicy::from_settings(&EgressSettings { + allowed_hosts: vec!["internal.example".into()], + allowed_cidrs: vec!["127.0.0.0/8".into()], + }) + .unwrap() +} + +#[test] +fn blocks_private_literal_without_exception() { + let policy = EgressPolicy::from_settings(&EgressSettings::default()).unwrap(); + assert_eq!( + policy.validate_url(&url::Url::parse("http://127.0.0.1/").unwrap()), + Err(EgressPolicyError::Blocked) + ); +} + +#[test] +fn blocks_noncanonical_loopback_ipv4_literals() { + let policy = EgressPolicy::from_settings(&EgressSettings::default()).unwrap(); + for value in ["http://127.1/", "http://2130706433/", "http://0x7f000001/"] { + let url = url::Url::parse(value).unwrap(); + assert_eq!(policy.validate_url(&url), Err(EgressPolicyError::Blocked)); + } +} + +#[test] +fn blocks_private_ipv4_mapped_ipv6_without_exception() { + let policy = EgressPolicy::from_settings(&EgressSettings::default()).unwrap(); + let url = url::Url::parse("http://[::ffff:127.0.0.1]/").unwrap(); + assert_eq!(policy.validate_url(&url), Err(EgressPolicyError::Blocked)); +} + +#[test] +fn allows_global_literal_and_explicit_cidr() { + let policy = policy(); + assert!( + policy + .validate_url(&url::Url::parse("https://8.8.8.8/").unwrap()) + .is_ok() + ); + assert!( + policy + .validate_url(&url::Url::parse("http://127.0.0.1/").unwrap()) + .is_ok() + ); +} + +#[test] +fn rejects_credentials_and_non_http_schemes() { + let policy = policy(); + assert_eq!( + policy.validate_url(&url::Url::parse("ftp://8.8.8.8/").unwrap()), + Err(EgressPolicyError::UnsupportedScheme) + ); + assert_eq!( + policy.validate_url(&url::Url::parse("http://user@8.8.8.8/").unwrap()), + Err(EgressPolicyError::Credentials) + ); +} + +#[derive(Debug)] +struct Wrapper(Box); + +impl std::fmt::Display for Wrapper { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "wrapped") + } +} + +impl std::error::Error for Wrapper { + fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { + Some(self.0.as_ref()) + } +} + +#[test] +fn egress_blocked_detection_walks_source_chain() { + let blocked = Wrapper(Box::new(EgressBlockedError)); + let doubly_wrapped = Wrapper(Box::new(blocked)); + assert!(is_egress_blocked(&doubly_wrapped)); + assert!(!is_egress_blocked(&Wrapper(Box::new( + std::io::Error::other("network down") + )))); +} diff --git a/pkgly/src/utils/mod.rs b/pkgly/src/utils/mod.rs index 1142752..c7e606c 100644 --- a/pkgly/src/utils/mod.rs +++ b/pkgly/src/utils/mod.rs @@ -4,6 +4,7 @@ use sha2_0_11::Digest; pub mod requests; pub mod response; pub use response::*; +pub mod egress; pub mod header; pub mod other; pub mod request_logging; diff --git a/pkgly/src/utils/upstream.rs b/pkgly/src/utils/upstream.rs index 3a659f1..99fbb57 100644 --- a/pkgly/src/utils/upstream.rs +++ b/pkgly/src/utils/upstream.rs @@ -1,3 +1,5 @@ +// ABOUTME: Builds policy-enforced outbound HTTP clients and records upstream traces. +// ABOUTME: Rejects blocked initial URLs and redirects before any network connection. use std::time::Instant; use http::HeaderValue; @@ -9,6 +11,66 @@ use tracing::{Instrument as _, Span, info_span}; use tracing_opentelemetry::OpenTelemetrySpanExt as _; use url::Url; +#[derive(Debug, thiserror::Error)] +pub enum UpstreamError { + #[error(transparent)] + Request(#[from] reqwest::Error), + #[error("outbound destination is blocked by egress policy")] + Blocked(#[source] crate::utils::egress::EgressBlockedError), +} + +impl UpstreamError { + fn blocked() -> Self { + Self::Blocked(crate::utils::egress::EgressBlockedError) + } + + pub fn status(&self) -> Option { + match self { + Self::Request(error) => error.status(), + Self::Blocked(_) => None, + } + } + + pub fn is_timeout(&self) -> bool { + matches!(self, Self::Request(error) if error.is_timeout()) + } + + pub fn is_connect(&self) -> bool { + matches!(self, Self::Request(error) if error.is_connect()) + } + + pub fn is_body(&self) -> bool { + matches!(self, Self::Request(error) if error.is_body()) + } + + pub fn is_decode(&self) -> bool { + matches!(self, Self::Request(error) if error.is_decode()) + } +} + +impl crate::utils::IntoErrorResponse for UpstreamError { + fn into_response_boxed(self: Box) -> axum::response::Response { + crate::utils::ResponseBuilder::default() + .status(http::StatusCode::BAD_GATEWAY) + .body("Upstream request failed") + } +} + +pub fn client_builder() -> reqwest::ClientBuilder { + reqwest::Client::builder() + .no_proxy() + .dns_resolver(crate::utils::egress::resolver()) + .redirect(reqwest::redirect::Policy::custom(|attempt| { + if attempt.previous().len() >= 5 { + attempt.stop() + } else if crate::utils::egress::validate_url(attempt.url()).is_err() { + attempt.error(crate::utils::egress::EgressBlockedError) + } else { + attempt.follow() + } + })) +} + struct HeaderMapInjector<'a>(&'a mut HeaderMap); impl Injector for HeaderMapInjector<'_> { @@ -92,7 +154,7 @@ fn is_sensitive_query_key(key_lower: &str) -> bool { pub async fn send( client: &reqwest::Client, builder: reqwest::RequestBuilder, -) -> Result { +) -> Result { let request = builder.build()?; execute(client, request).await } @@ -100,9 +162,10 @@ pub async fn send( pub async fn execute( client: &reqwest::Client, mut request: reqwest::Request, -) -> Result { +) -> Result { let method = request.method().as_str().to_string(); let url = request.url().clone(); + crate::utils::egress::validate_url(&url).map_err(|_| UpstreamError::blocked())?; let sanitized_url = sanitize_url_for_logging(&url); let host = url.host_str().unwrap_or(""); @@ -155,7 +218,7 @@ pub async fn execute( } } - response + response.map_err(UpstreamError::from) } #[cfg(test)] diff --git a/pkgly/src/utils/upstream/tests.rs b/pkgly/src/utils/upstream/tests.rs index b2c60eb..473ca8b 100644 --- a/pkgly/src/utils/upstream/tests.rs +++ b/pkgly/src/utils/upstream/tests.rs @@ -1,3 +1,10 @@ +// ABOUTME: Tests outbound HTTP tracing, log sanitization, and egress enforcement. +// ABOUTME: Uses a real loopback listener to prove blocked literals are never contacted. +use std::sync::{ + Arc, + atomic::{AtomicUsize, Ordering}, +}; + use opentelemetry::trace::TraceContextExt as _; use opentelemetry::{ Context as OtelContext, global, @@ -36,3 +43,36 @@ fn inject_trace_headers_adds_traceparent() { inject_trace_headers(&cx, &mut headers); assert!(headers.contains_key("traceparent")); } + +#[tokio::test] +async fn send_blocks_initial_non_global_literal_before_connecting() { + crate::utils::egress::install(&crate::app::config::EgressSettings::default()) + .expect("default egress policy"); + let listener = tokio::net::TcpListener::bind("0.0.0.0:0") + .await + .expect("bind loopback listener"); + let address = listener.local_addr().expect("listener address"); + let accepted = Arc::new(AtomicUsize::new(0)); + let accepted_for_server = Arc::clone(&accepted); + let server = tokio::spawn(async move { + if let Ok((mut stream, _)) = listener.accept().await { + accepted_for_server.fetch_add(1, Ordering::SeqCst); + use tokio::io::AsyncWriteExt as _; + let _ = stream + .write_all(b"HTTP/1.1 200 OK\r\nContent-Length: 0\r\n\r\n") + .await; + } + }); + + let client = super::client_builder().build().expect("egress HTTP client"); + let error = super::send(&client, client.get(format!("http://{address}/"))) + .await + .expect_err("non-global literal must be blocked"); + + server.abort(); + assert!( + crate::utils::egress::is_egress_blocked(&error), + "unexpected error chain: {error:?}" + ); + assert_eq!(accepted.load(Ordering::SeqCst), 0); +} diff --git a/tests/README.md b/tests/README.md index 3f06c2f..3cb501e 100644 --- a/tests/README.md +++ b/tests/README.md @@ -27,7 +27,8 @@ tests/ │ ├── test_go.sh # Go module integration tests │ ├── test_debian.sh # Debian repository integration tests │ ├── test_cargo.sh # Cargo registry integration tests -│ └── test_helm.sh # Helm chart integration tests +│ ├── test_helm.sh # Helm chart integration tests +│ ├── test_security.sh # Security hardening integration tests (CORS, traversal, egress, reset poisoning) ├── fixtures/ # Test packages │ ├── maven/simple-lib/ # Maven test library │ ├── npm/hello-pkg/ # NPM test package diff --git a/tests/docker/config/pkgly.test.toml b/tests/docker/config/pkgly.test.toml index 437f60a..2202386 100644 --- a/tests/docker/config/pkgly.test.toml +++ b/tests/docker/config/pkgly.test.toml @@ -19,9 +19,22 @@ app_url = "http://pkgly:8888" [web_server] bind_address = "0.0.0.0:8888" +[email] +username = "" +password = "" +host = "mailpit" +port = 1025 +encryption = "NONE" +from = "pkgly@test.local" + [security] allow_basic_without_tokens = false +[security.egress] +# Proxy fixtures intentionally point back to the in-stack Pkgly service. +allowed_hosts = ["pkgly"] +allowed_cidrs = [] + [log.levels] default = "Debug" diff --git a/tests/docker/docker-compose.test.yml b/tests/docker/docker-compose.test.yml index 15a2340..14ae4b0 100644 --- a/tests/docker/docker-compose.test.yml +++ b/tests/docker/docker-compose.test.yml @@ -23,6 +23,8 @@ services: depends_on: postgres: condition: service_healthy + mailpit: + condition: service_healthy environment: - RUST_LOG=debug - DATABASE_URL=postgresql://pkgly:pkgly@postgres:5432/pkgly_test @@ -34,6 +36,14 @@ services: networks: - integration + mailpit: + image: axllent/mailpit:latest + environment: + - MP_SMTP_BIND_ADDR=0.0.0.0:1025 + - MP_HTTP_BIND_ADDR=0.0.0.0:8025 + networks: + - integration + db-seeder: image: postgres:17 depends_on: diff --git a/tests/docker/seed-data.sql b/tests/docker/seed-data.sql index c5f2f1d..93dedff 100644 --- a/tests/docker/seed-data.sql +++ b/tests/docker/seed-data.sql @@ -46,6 +46,49 @@ INSERT INTO repository_configs (repository_id, key, value) VALUES ('11111111-0000-0000-0000-000000000001'::uuid, 'auth', '{"enabled": false}'::jsonb) ON CONFLICT (repository_id, key) DO NOTHING; +-- Persisted pre-hardening webhook used to verify runtime egress enforcement. +INSERT INTO webhooks (id, name, enabled, target_url, events, headers) +VALUES ( + 'eeeeeeee-0000-0000-0000-000000000001'::uuid, + 'runtime-loopback-security-test', + false, + 'http://127.0.0.1:8888/api/health', + '["package.published"]'::jsonb, + '{}'::jsonb +) +ON CONFLICT (id) DO NOTHING; + +INSERT INTO webhook_deliveries ( + webhook_id, + webhook_name, + event_type, + subscription_key, + target_url, + headers, + payload, + status, + attempts, + max_attempts, + next_attempt_at +) +SELECT + 'eeeeeeee-0000-0000-0000-000000000001'::uuid, + 'runtime-loopback-security-test', + 'package.published', + 'runtime-loopback-security-test', + 'http://127.0.0.1:8888/api/health', + '{}'::jsonb, + '{"event_type":"package.published"}'::jsonb, + 'pending', + 0, + 5, + NOW() +WHERE NOT EXISTS ( + SELECT 1 + FROM webhook_deliveries + WHERE subscription_key = 'runtime-loopback-security-test' +); + -- Maven Proxy Repository INSERT INTO repositories (id, storage_id, name, repository_type, visibility, active) VALUES ( diff --git a/tests/integration/test_security.sh b/tests/integration/test_security.sh new file mode 100755 index 0000000..2bce432 --- /dev/null +++ b/tests/integration/test_security.sh @@ -0,0 +1,187 @@ +#!/bin/bash +# ABOUTME: Docker E2E coverage for security hardening: CORS, traversal, egress. +# ABOUTME: Verifies reset-link poisoning, traversal block, and egress allowlist behavior. +set -euo pipefail + +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +# shellcheck source=common.sh +source "${SCRIPT_DIR}/common.sh" + +MAILPIT_URL="${MAILPIT_URL:-http://mailpit:8025}" + +print_section "Security hardening E2E" + +# 1. CORS: foreign-origin preflight must receive no CORS headers +print_test "foreign-origin preflight gets no CORS headers" +preflight_headers=$(curl -s -D - -o /dev/null \ + -X OPTIONS "${PKGLY_URL}/api/user/token/create" \ + -H "Origin: https://evil.example" \ + -H "Access-Control-Request-Method: POST" || true) +if echo "${preflight_headers}" | grep -qi "access-control-allow-origin" \ + || echo "${preflight_headers}" | grep -qi "access-control-allow-credentials"; then + fail "preflight response contains CORS headers: ${preflight_headers}" +else + pass +fi + +# 2. Traversal: raw and encoded path traversal must be rejected with 400 +print_test "traversal read returns 400" +status=$(curl -s -o /dev/null -w "%{http_code}" --path-as-is \ + "${PKGLY_URL}/local/security-test/../outside.txt" -H "Accept: */*") +if [ "$status" = "400" ]; then + pass +else + fail "traversal read expected 400, got ${status}" +fi + +print_test "encoded traversal write returns 400" +status=$(curl -s -o /dev/null -w "%{http_code}" --path-as-is \ + -X PUT --data-binary "evil" \ + "${PKGLY_URL}/local/security-test/%2e%2e/outside.txt" -H "Accept: */*") +if [ "$status" = "400" ]; then + pass +else + fail "encoded traversal write expected 400, got ${status}" +fi + +print_test "traversal delete returns 400" +status=$(curl -s -o /dev/null -w "%{http_code}" --path-as-is \ + -X DELETE "${PKGLY_URL}/local/security-test/a/../outside.txt" -H "Accept: */*") +if [ "$status" = "400" ]; then + pass +else + fail "traversal delete expected 400, got ${status}" +fi + +# 3. Egress: webhook create rejects private loopback literals +print_test "webhook to loopback literal blocked on create" +status=$(curl -s -o /dev/null -w "%{http_code}" \ + -X POST "${PKGLY_URL}/api/system/webhooks" \ + -H "Authorization: Bearer ${TEST_TOKEN}" \ + -H "Content-Type: application/json" \ + -d '{"name":"blocked","enabled":true,"target_url":"http://127.0.0.1:8080/hook","events":["package.published"],"headers":[]}') +if [ "$status" = "400" ]; then + pass +else + fail "loopback webhook create expected 400, got ${status}" +fi + +print_test "reserved IPv6 webhook literals are blocked on create" +ipv6_blocked=true +for target in 'http://[::ffff:8.8.8.8]/hook' 'http://[64:ff9b:1::1]/hook' 'http://[3fff::1]/hook'; do + status=$(curl -s -o /dev/null -w "%{http_code}" \ + -X POST "${PKGLY_URL}/api/system/webhooks" \ + -H "Authorization: Bearer ${TEST_TOKEN}" \ + -H "Content-Type: application/json" \ + -d "{\"name\":\"blocked-ipv6\",\"enabled\":true,\"target_url\":\"${target}\",\"events\":[\"package.published\"],\"headers\":[]}") + if [ "$status" != "400" ]; then + ipv6_blocked=false + fail "reserved IPv6 webhook ${target} expected 400, got ${status}" + break + fi +done +if [ "$ipv6_blocked" = true ]; then + pass +fi + +print_test "existing proxy config cannot be updated to loopback" +status=$(curl -s -o /dev/null -w "%{http_code}" \ + -X PUT "${PKGLY_URL}/api/repository/55555555-0000-0000-0000-000000000002/config/php" \ + -H "Authorization: Bearer ${TEST_TOKEN}" \ + -H "Content-Type: application/json" \ + -d '{"type":"Proxy","config":{"routes":[{"url":"http://127.0.0.1:8888","name":"blocked"}]}}') +if [ "$status" = "400" ]; then + pass +else + fail "loopback proxy update expected 400, got ${status}" +fi + +print_test "persisted loopback webhook is blocked at delivery time without retry" +runtime_webhook="" +for attempt in $(seq 1 45); do + runtime_webhook=$(curl -sf \ + "${PKGLY_URL}/api/system/webhooks/eeeeeeee-0000-0000-0000-000000000001" \ + -H "Authorization: Bearer ${TEST_TOKEN}" || echo '{}') + if [ "$(echo "${runtime_webhook}" | jq -r '.last_delivery_status // empty')" = "failed" ]; then + break + fi + sleep 1 +done +runtime_status=$(echo "${runtime_webhook}" | jq -r '.last_delivery_status // empty') +runtime_error=$(echo "${runtime_webhook}" | jq -r '.last_error // empty') +if [ "$runtime_status" != "failed" ]; then + fail "persisted loopback delivery did not fail: ${runtime_webhook}" +elif [ "$runtime_error" != "Webhook target is blocked by egress policy" ]; then + fail "persisted loopback delivery was not rejected by egress policy: ${runtime_webhook}" +else + pass +fi + +# 4. Egress: allowlisted hostname exception is accepted +print_test "webhook to allowlisted host accepted" +created=$(curl -s -w "\n%{http_code}" \ + -X POST "${PKGLY_URL}/api/system/webhooks" \ + -H "Authorization: Bearer ${TEST_TOKEN}" \ + -H "Content-Type: application/json" \ + -d '{"name":"allowed","enabled":true,"target_url":"http://pkgly:8888/hook","events":["package.published"],"headers":[]}') +status=$(echo "${created}" | tail -1) +if [ "$status" != "201" ]; then + fail "allowlisted webhook create expected 201, got ${status}: ${created}" + exit 1 +fi +webhook_id=$(echo "${created}" | head -1 | jq -r .id) +curl -s -o /dev/null \ + -X DELETE "${PKGLY_URL}/api/system/webhooks/${webhook_id}" \ + -H "Authorization: Bearer ${TEST_TOKEN}" || true +pass + +# 5. Password reset poisoning: hostile Origin must not appear in the link +print_test "password reset link ignores hostile Origin header" +previous_message_id=$(curl -sf "${MAILPIT_URL}/api/v1/messages" \ + | jq -r '[.messages[] | select(any(.To[]; .Address == "admin@pkgly.test"))] | sort_by(.Created) | last | .ID // empty' \ + || true) +reset_response=$(curl -s -w "\n%{http_code}" \ + -X POST "${PKGLY_URL}/api/user/password-reset/request" \ + -H "Origin: https://evil.example" \ + -H "Content-Type: application/json" \ + -d '{"email":"admin@pkgly.test"}' || true) +reset_status=$(echo "${reset_response}" | tail -1) +if [ "${reset_status}" != "200" ]; then + fail "password reset request expected 200, got ${reset_status}: ${reset_response}" + exit 1 +fi +message_id="" +for attempt in $(seq 1 20); do + messages=$(curl -sf "${MAILPIT_URL}/api/v1/messages" || echo '{"messages":[]}') + candidate_message_id=$(echo "${messages}" | jq -r \ + '[.messages[] | select(any(.To[]; .Address == "admin@pkgly.test"))] | sort_by(.Created) | last | .ID // empty') + if [ -n "${candidate_message_id}" ] && [ "${candidate_message_id}" != "${previous_message_id}" ]; then + message_id="${candidate_message_id}" + break + fi + sleep 1 +done +if [ -z "${message_id}" ]; then + fail "no password reset email arrived at mailpit" + exit 1 +fi +message_body=$(curl -sf "${MAILPIT_URL}/view/${message_id}.txt" || true) +if [ -z "${message_body}" ]; then + fail "password reset email ${message_id} has no text body" + exit 1 +fi +reset_url=$(echo "${message_body}" | grep -oE 'https?://[^[:space:]]+' | head -1 || true) +if echo "${reset_url}" | grep -q "evil.example"; then + fail "reset link contains hostile Origin: ${reset_url}" +elif ! echo "${reset_url}" | grep -q "^http://pkgly:8888/reset-password?token="; then + fail "reset link does not point to site.app_url: ${reset_url}" +else + pass +fi + +print_section "Security hardening E2E complete" +echo "" +echo "Security tests: ${TESTS_RUN} run, ${TESTS_PASSED} passed, ${TESTS_FAILED} failed" +if [ "${TESTS_FAILED}" -gt 0 ]; then + exit 1 +fi diff --git a/tests/run_integration_tests.sh b/tests/run_integration_tests.sh index 36d0138..94f6702 100755 --- a/tests/run_integration_tests.sh +++ b/tests/run_integration_tests.sh @@ -67,6 +67,7 @@ TEST_SUITES: nuget Run NuGet integration tests web_refresh Run web route refresh integration tests access_logs Run HTTP access log enrichment integration tests + security Run security hardening integration tests all Run all test suites (default) EXAMPLES: @@ -124,12 +125,12 @@ while [[ $# -gt 0 ]]; do STOP=0 shift ;; - maven|npm|docker|docker_proxy|python|python_virtual|php|ruby|go|debian|cargo|helm|nuget|web_refresh|access_logs) + maven|npm|docker|docker_proxy|python|python_virtual|php|ruby|go|debian|cargo|helm|nuget|web_refresh|access_logs|security) TEST_SUITES+=("$1") shift ;; all) - TEST_SUITES=(maven npm docker docker_proxy python python_virtual php ruby go debian cargo helm nuget web_refresh access_logs) + TEST_SUITES=(maven npm docker docker_proxy python python_virtual php ruby go debian cargo helm nuget web_refresh access_logs security) shift ;; *) @@ -142,7 +143,7 @@ done # Default to all tests if none specified if [ ${#TEST_SUITES[@]} -eq 0 ]; then - TEST_SUITES=(maven npm docker docker_proxy python python_virtual php ruby go debian cargo helm nuget web_refresh access_logs) + TEST_SUITES=(maven npm docker docker_proxy python python_virtual php ruby go debian cargo helm nuget web_refresh access_logs security) fi # Enable verbose mode @@ -168,7 +169,7 @@ if [ $BUILD -eq 1 ]; then echo "" fi -REQUIRED_SERVICES=(postgres pkgly test-runner docker) +REQUIRED_SERVICES=(postgres pkgly test-runner docker mailpit) RUNNING_SERVICES=$("${COMPOSE_CMD[@]}" ps --status running --services 2>/dev/null || true) ALL_REQUIRED_RUNNING=1 for svc in "${REQUIRED_SERVICES[@]}"; do