Skip to content

Commit 690ab6d

Browse files
authored
improve component adoption URL parsing (#2674)
* strip incorrect http prefix from auto-adopt urls * also strip scheme in wizard handlers * update log messages * use shared helper * review fixes
1 parent e73c65e commit 690ab6d

3 files changed

Lines changed: 120 additions & 44 deletions

File tree

crates/defguard_common/src/utils.rs

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,15 @@ use std::net::{IpAddr, Ipv4Addr, Ipv6Addr};
33
use ipnetwork::IpNetwork;
44
use serde::Serialize;
55

6+
/// Strip any `http://` or `https://` scheme prefix a user may have accidentally
7+
/// included in a hostname/IP field that expects a bare host, not a URL.
8+
#[must_use]
9+
pub fn strip_scheme(s: &str) -> &str {
10+
s.strip_prefix("https://")
11+
.or_else(|| s.strip_prefix("http://"))
12+
.unwrap_or(s)
13+
}
14+
615
/// Parse a string with comma-separated IP addresses.
716
/// Invalid addresses will be silently ignored.
817
#[must_use]

crates/defguard_core/src/handlers/component_setup.rs

Lines changed: 47 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@ use defguard_common::{
2727
},
2828
},
2929
types::proxy::ProxyControlMessage,
30+
utils::strip_scheme,
3031
};
3132
use defguard_proto::{
3233
common::{CertificateInfo, DerPayload},
@@ -242,31 +243,33 @@ pub async fn setup_proxy_tls_stream(
242243

243244
debug!("License check passed");
244245

246+
let ip_or_domain = strip_scheme(&request.ip_or_domain);
247+
245248
// Step 1: Check configuration
246249
yield Ok(flow.step(SetupStep::CheckingConfiguration));
247-
match Proxy::find_by_address_port(&pool, &request.ip_or_domain, i32::from(request.grpc_port)).await {
250+
match Proxy::find_by_address_port(&pool, ip_or_domain, i32::from(request.grpc_port)).await {
248251
Ok(Some(proxy)) => {
249252
yield Ok(flow.error(&format!(
250-
"Edge with address {}:{} is already registered with name \"{}\".",
251-
request.ip_or_domain, request.grpc_port, proxy.name
253+
"Edge with address {ip_or_domain}:{} is already registered with name \"{}\".",
254+
request.grpc_port, proxy.name
252255
)));
253256
return;
254257
}
255258
Ok(None) => {
256259
debug!(
257-
"Verified no existing proxy registration for {}:{}",
258-
request.ip_or_domain, request.grpc_port
260+
"Verified no existing Edge registration for {ip_or_domain}:{}",
261+
request.grpc_port
259262
);
260263
}
261264
Err(e) => {
262-
yield Ok(flow.error(&format!("Failed to query existing proxy: {e}")));
265+
yield Ok(flow.error(&format!("Failed to query existing Edge: {e}")));
263266
return;
264267
}
265268
}
266269

267270
debug!("Configuration check passed");
268271

269-
let url_str = format!("http://{}:{}", request.ip_or_domain, request.grpc_port);
272+
let url_str = format!("http://{ip_or_domain}:{}", request.grpc_port);
270273
let url = match Url::parse(&url_str) {
271274
Ok(u) => u,
272275
Err(e) => {
@@ -328,8 +331,8 @@ pub async fn setup_proxy_tls_stream(
328331
};
329332

330333
debug!(
331-
"Prepared secure connection endpoint for Edge at {}:{}",
332-
request.ip_or_domain, request.grpc_port
334+
"Prepared secure connection endpoint for Edge at {ip_or_domain}:{}",
335+
request.grpc_port
333336
);
334337

335338
let version = match Version::parse(VERSION) {
@@ -369,8 +372,8 @@ pub async fn setup_proxy_tls_stream(
369372
});
370373

371374
debug!(
372-
"Initiating connection to Edge at {}:{}",
373-
request.ip_or_domain, request.grpc_port
375+
"Initiating connection to Edge at {ip_or_domain}:{}",
376+
request.grpc_port
374377
);
375378

376379
let response_with_metadata = match tokio::time::timeout(CONNECTION_TIMEOUT, client.start(())).await {
@@ -379,24 +382,24 @@ pub async fn setup_proxy_tls_stream(
379382
let error_msg = status.message();
380383
if error_msg.contains("h2 protocol error") || error_msg.contains("http2 error") {
381384
yield Ok(flow.error(&format!(
382-
"Failed to connect to Edge at {}:{}: {error_msg}. This may indicate that \
385+
"Failed to connect to Edge at {ip_or_domain}:{}: {error_msg}. This may indicate that \
383386
the Edge is already configured with TLS. Please check if the Edge has \
384387
already been set up.",
385-
request.ip_or_domain, request.grpc_port
388+
request.grpc_port
386389
)));
387390
} else {
388391
yield Ok(flow.error(&format!(
389-
"Failed to connect to Edge at {}:{}: {error_msg}. Please ensure the \
392+
"Failed to connect to Edge at {ip_or_domain}:{}: {error_msg}. Please ensure the \
390393
address and port are correct and that the Edge component is running.",
391-
request.ip_or_domain, request.grpc_port
394+
request.grpc_port
392395
)));
393396
}
394397
return;
395398
}
396399
Err(_) => {
397400
yield Ok(flow.error(&format!(
398-
"Connection to Edge at {}:{} timed out after {} seconds",
399-
CONNECTION_TIMEOUT.as_secs(), request.ip_or_domain, request.grpc_port
401+
"Connection to Edge at {ip_or_domain}:{} timed out after {} seconds",
402+
request.grpc_port, CONNECTION_TIMEOUT.as_secs()
400403
)));
401404
return;
402405
}
@@ -566,10 +569,10 @@ pub async fn setup_proxy_tls_stream(
566569
debug!("Certificate expiry date determined: {expiry}");
567570

568571
let mut proxy = Proxy::new(
569-
&request.common_name,
570-
&request.ip_or_domain,
572+
request.common_name.as_str(),
573+
ip_or_domain,
571574
i32::from(request.grpc_port),
572-
&session.user.fullname(),
575+
session.user.fullname().as_str(),
573576
);
574577
proxy.certificate = Some(serial);
575578
proxy.certificate_expiry = Some(expiry);
@@ -675,16 +678,16 @@ pub async fn setup_gateway_tls_stream(
675678
flow.step(SetupStep::CheckingConfiguration)
676679
);
677680

681+
let ip_or_domain = strip_scheme(&request.ip_or_domain);
678682

679-
680-
match Gateway::find_by_url(&pool, &request.ip_or_domain, request.grpc_port).await {
683+
match Gateway::find_by_url(&pool, ip_or_domain, request.grpc_port).await {
681684
Ok(Some(gateway)) => {
682-
yield Ok(flow.error(&format!("A Gateway with URL {}:{} is already registered with \
683-
name \"{}\".", request.ip_or_domain, request.grpc_port, gateway.name)));
685+
yield Ok(flow.error(&format!("A Gateway with URL {ip_or_domain}:{} is already registered with \
686+
name \"{}\".", request.grpc_port, gateway.name)));
684687
return;
685688
}
686689
Ok(None) => {
687-
debug!("Verified no existing Gateway registration for {}:{}", request.ip_or_domain,
690+
debug!("Verified no existing Gateway registration for {ip_or_domain}:{}",
688691
request.grpc_port);
689692
},
690693
Err(e) => {
@@ -693,7 +696,7 @@ pub async fn setup_gateway_tls_stream(
693696
}
694697
}
695698

696-
let url_str = format!("http://{}:{}", request.ip_or_domain, request.grpc_port);
699+
let url_str = format!("http://{ip_or_domain}:{}", request.grpc_port);
697700
let url = match Url::parse(&url_str) {
698701
Ok(u) => u,
699702
Err(e) => {
@@ -754,7 +757,7 @@ pub async fn setup_gateway_tls_stream(
754757
}
755758
};
756759

757-
debug!("Prepared secure connection endpoint for Gateway at {}:{}", request.ip_or_domain,
760+
debug!("Prepared secure connection endpoint for Gateway at {ip_or_domain}:{}",
758761
request.grpc_port);
759762

760763
let version = match Version::parse(VERSION) {
@@ -800,7 +803,7 @@ pub async fn setup_gateway_tls_stream(
800803
}
801804
);
802805

803-
debug!("Initiating connection to Gateway at {}:{}", request.ip_or_domain,
806+
debug!("Initiating connection to Gateway at {ip_or_domain}:{}",
804807
request.grpc_port);
805808

806809
let response_with_metadata = match tokio::time::timeout(
@@ -812,24 +815,24 @@ pub async fn setup_gateway_tls_stream(
812815
let error_msg = status.message();
813816
if error_msg.contains("h2 protocol error") || error_msg.contains("http2 error") {
814817
yield Ok(flow.error(&format!(
815-
"Failed to connect to Gateway at {}:{}: {error_msg}. This may indicate \
818+
"Failed to connect to Gateway at {ip_or_domain}:{}: {error_msg}. This may indicate \
816819
that the Gateway is already configured with TLS. Please, check if the \
817820
Gateway has already been set up.",
818-
request.ip_or_domain, request.grpc_port,
821+
request.grpc_port,
819822
)));
820823
} else {
821824
yield Ok(flow.error(&format!(
822-
"Failed to connect to Gateway at {}:{}: {error_msg}. Please ensure the \
825+
"Failed to connect to Gateway at {ip_or_domain}:{}: {error_msg}. Please ensure the \
823826
address and port are correct and that the Gateway is running.",
824-
request.ip_or_domain, request.grpc_port
827+
request.grpc_port
825828
)));
826829
}
827830
return;
828831
}
829832
Err(_) => {
830833
yield Ok(flow.error(&format!(
831-
"Connection to Gateway at {}:{} timed out after 10 seconds.",
832-
request.ip_or_domain, request.grpc_port
834+
"Connection to Gateway at {ip_or_domain}:{} timed out after 10 seconds.",
835+
request.grpc_port
833836
)));
834837
return;
835838
}
@@ -1023,7 +1026,7 @@ pub async fn setup_gateway_tls_stream(
10231026
let mut gateway = Gateway::new(
10241027
network_id,
10251028
request.common_name,
1026-
request.ip_or_domain,
1029+
ip_or_domain.to_owned(),
10271030
request.grpc_port.into(),
10281031
session.user.fullname(),
10291032
);
@@ -1117,7 +1120,7 @@ fn public_proxy_hostname() -> Result<String, String> {
11171120

11181121
if url.is_empty() {
11191122
return Err(
1120-
"Public proxy URL is not configured. Please re-submit the external URL settings \
1123+
"Public Edge URL is not configured. Please re-submit the external URL settings \
11211124
with a Let's Encrypt domain."
11221125
.to_string(),
11231126
);
@@ -1128,7 +1131,7 @@ fn public_proxy_hostname() -> Result<String, String> {
11281131
.and_then(|u| u.host_str().map(ToString::to_string))
11291132
.filter(|host| !host.is_empty())
11301133
.ok_or_else(|| {
1131-
"Public proxy URL is not configured with a valid hostname. Please re-submit the \
1134+
"Public Edge URL is not configured with a valid hostname. Please re-submit the \
11321135
external URL settings with a valid domain."
11331136
.to_string()
11341137
})
@@ -1162,15 +1165,15 @@ async fn call_proxy_trigger_acme(
11621165

11631166
let endpoint_str = format!("https://{proxy_host}:{proxy_port}");
11641167
let endpoint = Endpoint::from_shared(endpoint_str)
1165-
.map_err(|e| (format!("Failed to build proxy endpoint: {e}"), Vec::new()))?
1168+
.map_err(|e| (format!("Failed to build Edge endpoint: {e}"), Vec::new()))?
11661169
.http2_keep_alive_interval(Duration::from_secs(5))
11671170
.tcp_keepalive(Some(Duration::from_secs(5)))
11681171
.keep_alive_while_idle(true);
11691172

11701173
let tls = ClientTlsConfig::new().ca_certificate(Certificate::from_pem(cert_pem));
11711174
let endpoint = endpoint.tls_config(tls).map_err(|e| {
11721175
(
1173-
format!("Failed to configure TLS for proxy endpoint: {e}"),
1176+
format!("Failed to configure TLS for Edge endpoint: {e}"),
11741177
Vec::new(),
11751178
)
11761179
})?;
@@ -1271,7 +1274,7 @@ pub async fn stream_proxy_acme(
12711274
Err(e) => {
12721275
yield Ok(acme_error_event(
12731276
"Connecting",
1274-
format!("Failed to load proxy list from DB: {e}"),
1277+
format!("Failed to load Edge list from DB: {e}"),
12751278
None,
12761279
));
12771280
return;
@@ -1281,7 +1284,7 @@ pub async fn stream_proxy_acme(
12811284
let Some(proxy) = proxies.into_iter().next() else {
12821285
yield Ok(acme_error_event(
12831286
"Connecting",
1284-
"No proxy found in database. Please complete the edge adoption step \
1287+
"No Edge found in database. Please complete the edge adoption step \
12851288
first."
12861289
.to_string(),
12871290
None,
@@ -1292,8 +1295,8 @@ pub async fn stream_proxy_acme(
12921295
let proxy_host = proxy.address.clone();
12931296
let proxy_port = proxy.port as u16;
12941297
info!(
1295-
"Triggering ACME HTTP-01 via Proxy gRPC TriggerAcme for domain: {domain} \
1296-
proxy={proxy_host}:{proxy_port}"
1298+
"Triggering ACME HTTP-01 via Edge gRPC TriggerAcme for domain: {domain} \
1299+
Edge={proxy_host}:{proxy_port}"
12971300
);
12981301

12991302
let (progress_tx, mut progress_rx) =
@@ -1391,7 +1394,7 @@ pub async fn stream_proxy_acme(
13911394
key_pem,
13921395
};
13931396
if let Err(e) = tx.send(msg).await {
1394-
error!("Failed to broadcast HttpsCerts to proxy: {e}");
1397+
error!("Failed to broadcast HttpsCerts to Edge: {e}");
13951398
}
13961399
}
13971400

crates/defguard_setup/src/auto_adoption.rs

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ use defguard_common::{
1919
},
2020
wireguard::{LocationMfaMode, ServiceLocationMode},
2121
},
22+
utils::strip_scheme,
2223
};
2324
use defguard_core::{
2425
setup_logs::scope_setup_logs,
@@ -99,6 +100,9 @@ async fn ensure_ca_for_auto_adoption(pool: &PgPool) -> Result<(), anyhow::Error>
99100
}
100101

101102
fn parse_host_port(input: &str) -> Result<(String, u16), anyhow::Error> {
103+
// Strip any scheme the user may have accidentally included (e.g. "http://host:port")
104+
let input = strip_scheme(input);
105+
102106
if let Some(rest) = input.strip_prefix('[') {
103107
let (host, port_part) = rest
104108
.split_once(']')
@@ -1102,3 +1106,63 @@ pub async fn attempt_auto_adoption(
11021106

11031107
Ok(())
11041108
}
1109+
1110+
#[cfg(test)]
1111+
mod tests {
1112+
use super::parse_host_port;
1113+
1114+
#[test]
1115+
fn test_parse_host_port_plain() {
1116+
let (host, port) = parse_host_port("172.30.0.30:50052").unwrap();
1117+
assert_eq!(host, "172.30.0.30");
1118+
assert_eq!(port, 50052);
1119+
}
1120+
1121+
#[test]
1122+
fn test_parse_host_port_strips_http_scheme() {
1123+
let (host, port) = parse_host_port("http://172.30.0.30:50052").unwrap();
1124+
assert_eq!(host, "172.30.0.30");
1125+
assert_eq!(port, 50052);
1126+
}
1127+
1128+
#[test]
1129+
fn test_parse_host_port_strips_https_scheme() {
1130+
let (host, port) = parse_host_port("https://edge.example.com:50052").unwrap();
1131+
assert_eq!(host, "edge.example.com");
1132+
assert_eq!(port, 50052);
1133+
}
1134+
1135+
#[test]
1136+
fn test_parse_host_port_domain_plain() {
1137+
let (host, port) = parse_host_port("edge.example.com:50052").unwrap();
1138+
assert_eq!(host, "edge.example.com");
1139+
assert_eq!(port, 50052);
1140+
}
1141+
1142+
#[test]
1143+
fn test_parse_host_port_ipv6_plain() {
1144+
let (host, port) = parse_host_port("[::1]:50052").unwrap();
1145+
assert_eq!(host, "::1");
1146+
assert_eq!(port, 50052);
1147+
}
1148+
1149+
#[test]
1150+
fn test_parse_host_port_ipv6_with_http_scheme() {
1151+
// After stripping "http://", input becomes "[::1]:50052" which is valid IPv6 notation
1152+
let (host, port) = parse_host_port("http://[::1]:50052").unwrap();
1153+
assert_eq!(host, "::1");
1154+
assert_eq!(port, 50052);
1155+
}
1156+
1157+
#[test]
1158+
fn test_parse_host_port_missing_port_is_error() {
1159+
assert!(parse_host_port("172.30.0.30").is_err());
1160+
assert!(parse_host_port("http://172.30.0.30").is_err());
1161+
}
1162+
1163+
#[test]
1164+
fn test_parse_host_port_invalid_port_is_error() {
1165+
assert!(parse_host_port("172.30.0.30:99999").is_err());
1166+
assert!(parse_host_port("http://172.30.0.30:notaport").is_err());
1167+
}
1168+
}

0 commit comments

Comments
 (0)