Repository navigation
fix(git-remote): refuse a plaintext node off this machine unless opted in #414
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
beardthelion
wants to merge
3
commits into
main
Choose a base branch
from
fix/helper-refuses-plaintext-remote-node
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -66,6 +66,10 @@ fn main() -> Result<()> { | |
| // v0.1: default to localhost. Override with GITLAWB_NODE env var. | ||
| let node_base = | ||
| std::env::var("GITLAWB_NODE").unwrap_or_else(|_| "http://127.0.0.1:7545".to_string()); | ||
| check_transport_security( | ||
| &node_base, | ||
| std::env::var_os("GITLAWB_ALLOW_INSECURE_HTTP").is_some(), | ||
| )?; | ||
| let repo_base = format!("{}/{}/{}", node_base, short_owner, repo_name); | ||
| tracing::debug!("repo_base: {repo_base}"); | ||
|
|
||
|
|
@@ -78,6 +82,76 @@ fn main() -> Result<()> { | |
| run_helper(&repo_base, keypair.as_ref()) | ||
| } | ||
|
|
||
| /// Whether `url` is plaintext http, regardless of where it points. | ||
| fn is_http(url: &str) -> bool { | ||
| reqwest::Url::parse(url.trim()) | ||
| .map(|u| u.scheme() == "http") | ||
| .unwrap_or(false) | ||
| } | ||
|
|
||
| /// Whether `url` would send git data off this machine in cleartext. | ||
| /// | ||
| /// True only for `http://` to a non-loopback host. RFC 9421 signs the request | ||
| /// but does not encrypt it, so on a plaintext hop the pack contents and the | ||
| /// `Signature` header are both readable, and a captured signature is replayable | ||
| /// for its freshness window against any host. | ||
| /// | ||
| /// Loopback is decided from the parsed address rather than a string match, so | ||
| /// `127.0.0.2`, `[::1]` and an IPv4-mapped `[::ffff:127.0.0.1]` are all | ||
| /// recognised as this machine. A value that does not parse, or that is not | ||
| /// http(s), is not this guard's business and returns false: it fails later with | ||
| /// its own error, and naming it a TLS problem would misdirect the reader. | ||
| fn is_insecure_remote(url: &str) -> bool { | ||
| let Ok(parsed) = reqwest::Url::parse(url.trim()) else { | ||
| return false; | ||
| }; | ||
| if parsed.scheme() != "http" { | ||
| return false; | ||
| } | ||
| let Some(host) = parsed.host_str() else { | ||
| return false; | ||
| }; | ||
| let host = host.trim_end_matches('.').to_ascii_lowercase(); | ||
| if host == "localhost" { | ||
| return false; | ||
| } | ||
| // host_str() keeps the brackets on an IPv6 literal. | ||
| let bare = host.trim_start_matches('[').trim_end_matches(']'); | ||
| if let Ok(ip) = bare.parse::<std::net::IpAddr>() { | ||
| if ip.is_loopback() { | ||
| return false; | ||
| } | ||
| // An IPv4-mapped IPv6 literal hides a v4 loopback from is_loopback(). | ||
| if let std::net::IpAddr::V6(v6) = ip { | ||
| if let Some(v4) = v6.to_ipv4_mapped() { | ||
| if v4.is_loopback() { | ||
| return false; | ||
| } | ||
| } | ||
| } | ||
| } | ||
| true | ||
| } | ||
|
|
||
| /// Refuse a cleartext hop off this machine unless the operator has opted in. | ||
| /// | ||
| /// Fail closed: the alternative is signing a request and then handing it, and | ||
| /// the pack it carries, to anyone on the path. `GITLAWB_ALLOW_INSECURE_HTTP` | ||
| /// exists for a private LAN where the operator has decided that is acceptable. | ||
| fn check_transport_security(node_base: &str, allow_insecure: bool) -> Result<()> { | ||
| if allow_insecure || !is_insecure_remote(node_base) { | ||
| return Ok(()); | ||
| } | ||
| bail!( | ||
| "refusing to send git data to {node_base} over plaintext http.\n\ | ||
| Requests are signed but not encrypted, so the pack contents and the \ | ||
| Signature header are readable by anyone on the path, and a captured \ | ||
| signature can be replayed.\n\ | ||
| Use https://, or set GITLAWB_ALLOW_INSECURE_HTTP=1 to accept the risk \ | ||
| (for a trusted private network only)." | ||
| ) | ||
| } | ||
|
|
||
| // ── CLI argument handling ────────────────────────────────────────────────────── | ||
|
|
||
| /// How the binary was invoked, derived from its CLI arguments. | ||
|
|
@@ -125,6 +199,7 @@ fn help_text() -> String { | |
| ENVIRONMENT:\n\ | ||
| \x20 GITLAWB_NODE Node base URL (default: http://127.0.0.1:7545)\n\ | ||
| \x20 GITLAWB_KEY Identity PEM path for signed fetch/push (default: ~/.gitlawb/identity.pem)\n\ | ||
| \x20 GITLAWB_ALLOW_INSECURE_HTTP Permit plaintext http:// to a non-loopback node (unset by default)\n\ | ||
| \x20 GITLAWB_LOG Log filter (default: warn)\n\ | ||
| \n\ | ||
| FLAGS:\n\ | ||
|
|
@@ -199,7 +274,8 @@ fn handle_connect<R: Read>( | |
| other => bail!("unsupported git service: {other}"), | ||
| } | ||
|
|
||
| let client = build_http_client()?; | ||
| // A loopback node must never be proxied; see build_http_client. | ||
| let client = build_http_client(!is_insecure_remote(repo_base) && is_http(repo_base))?; | ||
|
|
||
| // ── Phase 1: ref advertisement (GET /info/refs?service=<service>) ───────── | ||
| // | ||
|
|
@@ -335,8 +411,22 @@ const HTTP_TIMEOUT: std::time::Duration = std::time::Duration::from_secs(300); | |
| /// node named, and on a 307/308 the pack body went with them. Scope the follow to the | ||
| /// origin that issued the redirect AND to an identical request-target, which is the | ||
| /// same predicate `gl` uses. | ||
| fn build_http_client() -> Result<reqwest::blocking::Client> { | ||
| Ok(reqwest::blocking::Client::builder() | ||
| /// `bypass_proxy` must be true whenever the node is on this machine. reqwest's | ||
| /// default client honours `HTTP_PROXY` / `ALL_PROXY` for a loopback URL too, so a | ||
| /// proxy variable pointing off-machine turns an allowed local request into a | ||
| /// cleartext hop carrying the `Signature` header, straight past | ||
| /// `check_transport_security`. Verified: with `HTTP_PROXY` set, reqwest logged | ||
| /// `proxy(...) intercepts 'http://127.0.0.1:7545/'` and dialled the proxy. A | ||
| /// local node is never legitimately reached through a proxy, so this refuses one | ||
| /// rather than trying to reimplement `NO_PROXY` parsing. | ||
| fn build_http_client(bypass_proxy: bool) -> Result<reqwest::blocking::Client> { | ||
| let builder = reqwest::blocking::Client::builder(); | ||
| let builder = if bypass_proxy { | ||
| builder.no_proxy() | ||
| } else { | ||
| builder | ||
| }; | ||
| Ok(builder | ||
| .timeout(HTTP_TIMEOUT) | ||
| .redirect(reqwest::redirect::Policy::custom(same_origin_redirect)) | ||
| .build()?) | ||
|
|
@@ -829,6 +919,148 @@ fn resolve_key_path() -> std::path::PathBuf { | |
|
|
||
| // ── Tests ───────────────────────────────────────────────────────────────────── | ||
|
|
||
| #[cfg(test)] | ||
| mod insecure_transport_tests { | ||
| use super::*; | ||
|
|
||
| /// Loopback in every form the transport can legitimately be pointed at. | ||
| /// Plaintext to this machine is the documented local-alpha default and must | ||
| /// keep working, so a false positive here breaks every stock install. | ||
| #[test] | ||
| fn loopback_http_is_allowed() { | ||
| for url in [ | ||
| "http://127.0.0.1:7545", | ||
| "http://localhost:7545", | ||
| "http://[::1]:7545", | ||
| "http://127.0.0.2:7545", | ||
| "http://[::ffff:127.0.0.1]:7545", | ||
| "http://LocalHost:7545", | ||
| ] { | ||
| assert!( | ||
| !is_insecure_remote(url), | ||
| "{url} is this machine; plaintext to it must stay allowed" | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| /// The case the guard exists for: cleartext git data leaving the machine. | ||
| #[test] | ||
| fn remote_http_is_refused() { | ||
| for url in [ | ||
| "http://node.example.com:7545", | ||
| "http://10.0.0.36:7777", | ||
| "http://192.168.1.10", | ||
| "http://[2001:db8::1]:7545", | ||
| "http://8.8.8.8", | ||
| ] { | ||
| assert!( | ||
| is_insecure_remote(url), | ||
| "{url} sends git data off-machine in cleartext and must be refused" | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| /// TLS is always fine, loopback or not, so the guard keys on the scheme and | ||
| /// not merely on the host being remote. | ||
| #[test] | ||
| fn https_is_always_allowed() { | ||
| for url in [ | ||
| "https://node.gitlawb.com", | ||
| "https://10.0.0.36:7777", | ||
| "https://127.0.0.1:7545", | ||
| ] { | ||
| assert!(!is_insecure_remote(url), "{url} is TLS and must be allowed"); | ||
| } | ||
| } | ||
|
|
||
| /// Spellings that defeat a naive loopback check. `Url` normalizes the scheme | ||
| /// and the host at parse time (lowercasing, and the WHATWG numeric forms), so | ||
| /// these must all still read as this machine. | ||
| #[test] | ||
| fn loopback_spellings_are_normalized_not_string_matched() { | ||
| for url in [ | ||
| "HTTP://127.0.0.1:7545", | ||
| "Http://LOCALHOST:7545", | ||
| "http://localhost.:7545", | ||
| "http://user:[email protected]:7545", | ||
| "http://2130706433:7545", | ||
| "http://0x7f000001:7545", | ||
| "http://[::ffff:7f00:1]:7545", | ||
| ] { | ||
| assert!( | ||
| !is_insecure_remote(url), | ||
| "{url} resolves to this machine; plaintext to it must stay allowed" | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| /// The mirror of the above: an uppercase scheme must not become an escape | ||
| /// from the guard, and a remote host in any spelling is still remote. | ||
| #[test] | ||
| fn uppercase_scheme_does_not_escape_the_guard() { | ||
| for url in [ | ||
| "HTTP://node.example.com:7545", | ||
| "Http://8.8.8.8", | ||
| "http://NODE.EXAMPLE.COM", | ||
| "http://user:[email protected]", | ||
| ] { | ||
| assert!( | ||
| is_insecure_remote(url), | ||
| "{url} is a remote cleartext hop and must be refused" | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| /// The gate itself, both directions, including the opt-in escape hatch. | ||
| #[test] | ||
| fn gate_refuses_remote_plaintext_unless_opted_in() { | ||
| let err = check_transport_security("http://node.example.com:7545", false) | ||
| .expect_err("remote plaintext must be refused by default"); | ||
| let msg = err.to_string(); | ||
| assert!( | ||
| msg.contains("plaintext http"), | ||
| "the refusal must say why: {msg}" | ||
| ); | ||
| assert!( | ||
| msg.contains("GITLAWB_ALLOW_INSECURE_HTTP"), | ||
| "the refusal must name the escape hatch: {msg}" | ||
| ); | ||
|
|
||
| check_transport_security("http://node.example.com:7545", true) | ||
| .expect("the opt-in must permit the same URL"); | ||
| check_transport_security("http://127.0.0.1:7545", false) | ||
| .expect("the local default must keep working with no opt-in"); | ||
| check_transport_security("https://node.gitlawb.com", false) | ||
| .expect("TLS must need no opt-in"); | ||
| } | ||
|
|
||
| /// The documented knob must appear in --help. A gate the operator cannot | ||
| /// discover reads as a broken transport rather than a deliberate refusal. | ||
| #[test] | ||
| fn help_documents_the_opt_in() { | ||
| assert!(help_text().contains("GITLAWB_ALLOW_INSECURE_HTTP")); | ||
| } | ||
|
|
||
| /// An unparseable or non-http value is not classified as insecure here: it | ||
| /// fails later with its own error, and reporting it as a TLS problem would | ||
| /// send the reader after the wrong thing. | ||
| #[test] | ||
| fn unparseable_or_other_scheme_is_not_this_guards_problem() { | ||
| for url in [ | ||
| "", | ||
| " ", | ||
| "not a url", | ||
| "ftp://node.example.com", | ||
| "file:///tmp/x", | ||
| ] { | ||
| assert!( | ||
| !is_insecure_remote(url), | ||
| "{url:?} is not a cleartext-http-to-remote case" | ||
| ); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { | ||
| use super::*; | ||
|
|
@@ -929,7 +1161,7 @@ mod tests { | |
| #[test] | ||
| fn signed_requests_do_not_follow_a_redirect_off_the_node_origin() { | ||
| let kp = Keypair::generate(); | ||
| let client = build_http_client().unwrap(); | ||
| let client = build_http_client(false).unwrap(); | ||
|
|
||
| let mut elsewhere = mockito::Server::new(); | ||
| let never = elsewhere | ||
|
|
@@ -1031,7 +1263,7 @@ mod tests { | |
| #[test] | ||
| fn a_same_origin_path_changing_redirect_is_refused() { | ||
| let kp = Keypair::generate(); | ||
| let client = build_http_client().unwrap(); | ||
| let client = build_http_client(false).unwrap(); | ||
|
|
||
| let mut node = mockito::Server::new(); | ||
| let bounce = node | ||
|
|
@@ -1082,7 +1314,7 @@ mod tests { | |
| #[test] | ||
| fn an_identical_target_redirect_is_still_followed_up_to_the_chain_bound() { | ||
| let kp = Keypair::generate(); | ||
| let client = build_http_client().unwrap(); | ||
| let client = build_http_client(false).unwrap(); | ||
|
|
||
| let mut node = mockito::Server::new(); | ||
| let loop_route = node | ||
|
|
@@ -1243,7 +1475,7 @@ mod tests { | |
| fn a_rewritten_target_never_receives_the_signature() { | ||
| let kp = Keypair::generate(); | ||
| let expected_did = kp.did().to_string(); | ||
| let client = build_http_client().unwrap(); | ||
| let client = build_http_client(false).unwrap(); | ||
| let slot = std::sync::Arc::new(std::sync::Mutex::new(None::<Verdict>)); | ||
|
|
||
| let mut node = mockito::Server::new(); | ||
|
|
@@ -1305,7 +1537,7 @@ mod tests { | |
| fn a_direct_signed_advertisement_verifies_under_the_node_verifier() { | ||
| let kp = Keypair::generate(); | ||
| let expected_did = kp.did().to_string(); | ||
| let client = build_http_client().unwrap(); | ||
| let client = build_http_client(false).unwrap(); | ||
| let slot = std::sync::Arc::new(std::sync::Mutex::new(None::<Verdict>)); | ||
|
|
||
| let mut node = mockito::Server::new(); | ||
|
|
||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.