diff --git a/backend/ee-repo-ref.txt b/backend/ee-repo-ref.txt index f8d02ba287..975b266446 100644 --- a/backend/ee-repo-ref.txt +++ b/backend/ee-repo-ref.txt @@ -1 +1 @@ -a8ce9bed4b16a01a964d4c35bdc3092b89d10495 +fe2418ff4e5630d6ad3fd85cd2c865bf51c87a2a diff --git a/backend/tests/git_sync_fork_credential.rs b/backend/tests/git_sync_fork_credential.rs index 7e31136727..c594b31af6 100644 --- a/backend/tests/git_sync_fork_credential.rs +++ b/backend/tests/git_sync_fork_credential.rs @@ -12,8 +12,8 @@ use sqlx::{Pool, Postgres}; use windmill_common::git_sync_ee::{ - git_credential_for_url, repo_provider, repo_supports_managed_git_features, set_git_credential, - GitProvider, + create_repo_webhook, git_credential_for_url, repo_provider, repo_supports_managed_git_features, + set_git_credential, GitProvider, }; use windmill_common::workspaces::GitCredentialProvider; @@ -250,3 +250,29 @@ async fn a_credential_is_not_served_over_a_downgraded_transport( ); Ok(()) } + +/// A GitLab the server refuses to reach is the error reported, not the GitHub App +/// lookup that runs after it: for a self-managed GitLab on a private network, +/// "no GitHub App installation" names neither the host nor the setting that +/// allows it. +#[sqlx::test(fixtures("git_sync_fork_credential"))] +async fn a_refused_gitlab_host_is_the_reported_error(db: Pool) -> anyhow::Result<()> { + let err = create_repo_webhook( + &db, + "parent-ws", + "http://glpat-secret@10.0.0.5/grp/proj.git", + "https://windmill.example/api/w/parent-ws/git_sync/webhook/gitlab", + "hook-secret", + ) + .await + .expect_err("a private host is refused"); + assert!( + err.to_string().contains("ALLOW_LOCAL_GIT_REMOTES"), + "unexpected error: {err}" + ); + assert!( + !err.to_string().contains("glpat-secret"), + "the URL credential leaked into the error: {err}" + ); + Ok(()) +} diff --git a/backend/windmill-common/src/error.rs b/backend/windmill-common/src/error.rs index 0d5ebdc2c3..da9f5f3df2 100644 --- a/backend/windmill-common/src/error.rs +++ b/backend/windmill-common/src/error.rs @@ -272,7 +272,10 @@ fn format_db_error(message: &str, detail: Option<&str>, hint: Option<&str>) -> S msg } -fn error_source_chain(e: &dyn std::error::Error) -> String { +/// `e` followed by each of its sources, `: `-separated. The result is meant for +/// users, and a `reqwest::Error` renders its request URL: never pass one built +/// from a URL carrying credentials in its userinfo. +pub fn error_source_chain(e: &dyn std::error::Error) -> String { let mut msg = e.to_string(); let mut source = e.source(); while let Some(cause) = source { diff --git a/backend/windmill-common/src/ssrf.rs b/backend/windmill-common/src/ssrf.rs index 713b2e0289..a16e2d1576 100644 --- a/backend/windmill-common/src/ssrf.rs +++ b/backend/windmill-common/src/ssrf.rs @@ -8,6 +8,12 @@ pub const ALLOW_PRIVATE_SAML_METADATA_URLS_ENV: &str = "ALLOW_PRIVATE_SAML_METAD pub const ALLOW_PRIVATE_GUEST_JWKS_URLS_ENV: &str = "ALLOW_PRIVATE_GUEST_JWKS_URLS"; +/// Lets Windmill's own git calls reach hosts on a private network. Each process +/// reads it for the calls it makes: the server for remote probes, auto-pull and +/// the GitLab API, the worker that ran a git sync job for the merge request it +/// opens afterwards. So a private git server needs it on both. +pub const ALLOW_LOCAL_GIT_REMOTES_ENV: &str = "ALLOW_LOCAL_GIT_REMOTES"; + /// Why a URL failed SSRF validation. /// /// The distinction matters for callers that gate private endpoints behind a @@ -203,6 +209,28 @@ pub fn allow_private_saml_metadata_urls() -> bool { .is_some_and(|v| v == "true" || v == "1") } +pub fn allow_local_git_remotes() -> bool { + std::env::var(ALLOW_LOCAL_GIT_REMOTES_ENV) + .ok() + .is_some_and(|v| v == "true" || v == "1") +} + +/// Appended to a refusal of a private git host, which on a self-hosted +/// instance is usually the organization's own git server rather than an attack. +pub fn local_git_remote_hint() -> String { + format!( + "If your git server is on a private network, set the {ALLOW_LOCAL_GIT_REMOTES_ENV}=true \ + environment variable on the Windmill servers and workers" + ) +} + +pub fn git_remote_ssrf_error_message(e: &SsrfValidationError) -> String { + match e { + SsrfValidationError::Private { .. } => format!("{e}. {}", local_git_remote_hint()), + _ => e.to_string(), + } +} + pub async fn validate_saml_metadata_url(url: &str) -> Result { let parsed = url::Url::parse(url).map_err(|e| SsrfValidationError::InvalidUrl(e.to_string()))?; @@ -647,4 +675,21 @@ mod tests { !saml_ssrf_error_message(&invalid_error).contains(ALLOW_PRIVATE_SAML_METADATA_URLS_ENV) ); } + + #[tokio::test] + async fn git_remote_ssrf_error_message_includes_env_hint_only_for_private_urls() { + let private_error = validate_url_for_ssrf("http://10.0.0.5/api/v4") + .await + .unwrap_err(); + assert!( + git_remote_ssrf_error_message(&private_error).contains("ALLOW_LOCAL_GIT_REMOTES=true") + ); + + let invalid_error = validate_url_for_ssrf("gitlab.example.com") + .await + .unwrap_err(); + assert!( + !git_remote_ssrf_error_message(&invalid_error).contains(ALLOW_LOCAL_GIT_REMOTES_ENV) + ); + } } diff --git a/backend/windmill-store/src/resources.rs b/backend/windmill-store/src/resources.rs index 1ea5545b68..c7fdd05c51 100644 --- a/backend/windmill-store/src/resources.rs +++ b/backend/windmill-store/src/resources.rs @@ -3484,25 +3484,26 @@ async fn validate_git_url(url: &str) -> Result<()> { let host = extract_host_from_git_url(url) .ok_or_else(|| Error::BadRequest("Could not parse hostname from git URL".to_string()))?; - // CI/dev escape hatch: integration tests run their git remote (a Gitea - // container) on localhost, which the network-target checks below reject. - // Scheme and option-injection validation above still applies. - if std::env::var("ALLOW_LOCAL_GIT_REMOTES").is_ok_and(|v| v == "true" || v == "1") { + // The opt-in for a git server on the instance's own network (and for the CI + // Gitea container on localhost). Scheme and option-injection validation above + // still applies. + if windmill_common::ssrf::allow_local_git_remotes() { return Ok(()); } + let hint = windmill_common::ssrf::local_git_remote_hint(); if host == "localhost" || host.ends_with(".local") || host == "[::1]" { - return Err(Error::BadRequest( - "Git URLs targeting localhost or local network are not allowed".to_string(), - )); + return Err(Error::BadRequest(format!( + "Git URLs targeting localhost or local network are not allowed. {hint}" + ))); } // Check literal IP addresses if let Ok(ip) = host.parse::() { if is_private_or_reserved_ip(&ip) { - return Err(Error::BadRequest( - "Git URLs targeting private or reserved IP addresses are not allowed".to_string(), - )); + return Err(Error::BadRequest(format!( + "Git URLs targeting private or reserved IP addresses are not allowed. {hint}" + ))); } } else { // Hostname — resolve via DNS and reject if any address is private. Fail @@ -3523,9 +3524,9 @@ async fn validate_git_url(url: &str) -> Result<()> { } for addr in addrs { if is_private_or_reserved_ip(&addr.ip()) { - return Err(Error::BadRequest( - "Git URL hostname resolves to a private or reserved IP address".to_string(), - )); + return Err(Error::BadRequest(format!( + "Git URL hostname resolves to a private or reserved IP address. {hint}" + ))); } } } @@ -4831,7 +4832,10 @@ mod tests { assert!(validate_git_url("http://169.254.169.254/latest/meta-data/") .await .is_err()); - assert!(validate_git_url("http://10.0.0.1/repo.git").await.is_err()); + let err = validate_git_url("http://10.0.0.1/repo.git") + .await + .unwrap_err(); + assert!(err.to_string().contains("ALLOW_LOCAL_GIT_REMOTES"), "{err}"); assert!(validate_git_url("http://172.16.0.1/repo.git") .await .is_err());