-
Notifications
You must be signed in to change notification settings - Fork 340
REG-1639: Allow unencrypted calls to fetch graph artifacts #8919
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
Changes from 5 commits
7d8847b
e5c7635
7d59ccf
fa63915
44f0467
993a691
4ae8073
3d8941e
a9d23f3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| ### Allow Router to pull graph artifacts from unsecure (non-SSL) registries. | ||
|
|
||
| Allow users to configure a list of safe registry hostnames, so Router can pull graph artifacts over HTTP instead of HTTPS. Unsecure registries are commonly run within a private network such as a Kubernetes cluster or as a pull-through cache, where users want to avoid the overhead of setting up and distributing SSL certificates. | ||
|
|
||
| By [@sirddoger](https://github.com/sirdodger) in https://github.com/apollographql/router/pull/8919 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,6 +19,7 @@ use thiserror::Error; | |
| use tokio::sync::mpsc::channel; | ||
| use tokio_stream::wrappers::ReceiverStream; | ||
| use tracing::instrument::WithSubscriber; | ||
| use url::Url; | ||
|
|
||
| use crate::uplink::schema::SchemaState; | ||
|
|
||
|
|
@@ -296,14 +297,59 @@ async fn fetch_oci_blob( | |
| Ok(blob_data) | ||
| } | ||
|
|
||
| /// The oci reference may not contain the protocol, only hostname[:port]. As a result, | ||
| /// in order to test locally without SSL, either (1) protocol needs to be exposed as an | ||
| /// env var or (2) protocol needs to be inferred from hostname. Rather than introduce a | ||
| /// largely unused configuration option, this function checks the hostname for local | ||
| /// development/testing and disables SSL accordingly. | ||
| const UNSECURE_HOSTS_ENV_VAR: &str = "APOLLO_GRAPH_ARTIFACT_UNSECURE_HOSTS"; | ||
| const DEFAULT_UNSECURE_HOSTS: &[&str] = &["localhost", "127.0.0.1", "dockerhost"]; | ||
|
|
||
| /// Parse a comma-separated string of unsecure hosts. Empty entries are ignored. | ||
| fn parse_unsecure_hosts(value: &str) -> Vec<String> { | ||
| value | ||
| .split(',') | ||
| .map(|s| s.trim().to_string()) | ||
| .filter(|s| !s.is_empty()) | ||
| .collect() | ||
| } | ||
|
|
||
| fn unsecure_hosts() -> Vec<String> { | ||
| match std::env::var(UNSECURE_HOSTS_ENV_VAR) { | ||
| Ok(val) => parse_unsecure_hosts(&val), | ||
| Err(_) => DEFAULT_UNSECURE_HOSTS | ||
| .iter() | ||
| .map(|s| s.to_string()) | ||
| .collect(), | ||
| } | ||
| } | ||
|
|
||
| /// Extract the hostname from a registry string like "host", "host:port", or | ||
| /// an IPv6 address like "[::1]:port", using `url::Url` for robust parsing. | ||
| /// IPv6 addresses are returned without brackets (e.g. "::1" not "[::1]"). | ||
| fn extract_host(registry: &str) -> Option<String> { | ||
| Url::parse(&format!("dummy://{registry}")) | ||
| .ok() | ||
| .and_then(|url| { | ||
| url.host().map(|h| match h { | ||
| url::Host::Ipv6(addr) => addr.to_string(), | ||
| other => other.to_string(), | ||
| }) | ||
| }) | ||
| } | ||
|
|
||
| /// Check whether `registry` matches any entry in `hosts`, comparing only the | ||
| /// hostname portion (stripping any port). | ||
| fn is_unsecure_host(registry: &str, hosts: &[String]) -> bool { | ||
| extract_host(registry) | ||
| .map(|host| hosts.iter().any(|h| h == &host)) | ||
| .unwrap_or(false) | ||
|
sirdodger marked this conversation as resolved.
Outdated
|
||
| } | ||
|
|
||
| /// Determine whether to use HTTP or HTTPS for the OCI registry. | ||
| /// | ||
| /// Uses the `APOLLO_GRAPH_ARTIFACT_UNSECURE_HOSTS` environment variable, which | ||
| /// contains a comma-separated list of hostnames that should use HTTP instead of | ||
| /// HTTPS. When the variable is unset, the defaults are "localhost", "127.0.0.1", | ||
| /// and "dockerhost". Setting it to an empty string disables all HTTP overrides. | ||
| async fn infer_oci_protocol(registry: &str) -> ClientProtocol { | ||
| let host = registry.split(":").next().expect("host must be provided"); | ||
| if host == "localhost" || host == "127.0.0.1" || host == "dockerhost" { | ||
| let hosts = unsecure_hosts(); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't know this area of the code well - is this something that will be called repeatedly, or just on router initiation? If it's called repeatedly, it's probably worth putting the env value into a OnceLock or LazyLock (or similar) so that it only gets instantiated once
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It gets called repeatedly, but it doesn't need to since the reference cannot change. I'll move the whole inference to startup instead of on the network call. |
||
| if is_unsecure_host(registry, &hosts) { | ||
| ClientProtocol::Http | ||
| } else { | ||
| ClientProtocol::Https | ||
|
|
@@ -925,6 +971,130 @@ mod tests { | |
| assert_eq!(result, ClientProtocol::Https); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_parse_unsecure_hosts_comma_separated() { | ||
| let hosts = parse_unsecure_hosts("host1,host2,host3"); | ||
| assert_eq!(hosts, vec!["host1", "host2", "host3"]); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_parse_unsecure_hosts_with_whitespace() { | ||
| let hosts = parse_unsecure_hosts(" host1 , host2 , host3 "); | ||
| assert_eq!(hosts, vec!["host1", "host2", "host3"]); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_parse_unsecure_hosts_empty_string() { | ||
| let hosts = parse_unsecure_hosts(""); | ||
| assert!(hosts.is_empty()); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_parse_unsecure_hosts_trailing_commas() { | ||
| let hosts = parse_unsecure_hosts("host1,,host2,"); | ||
| assert_eq!(hosts, vec!["host1", "host2"]); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_parse_unsecure_hosts_single_host() { | ||
| let hosts = parse_unsecure_hosts("myregistry.local"); | ||
| assert_eq!(hosts, vec!["myregistry.local"]); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_is_unsecure_host_exact_match() { | ||
| let hosts = vec!["myregistry.local".to_string()]; | ||
| assert!(is_unsecure_host("myregistry.local", &hosts)); | ||
| assert!(is_unsecure_host("myregistry.local:5000", &hosts)); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_is_unsecure_host_no_match() { | ||
| let hosts = vec!["myregistry.local".to_string()]; | ||
| assert!(!is_unsecure_host("other.registry.com", &hosts)); | ||
| assert!(!is_unsecure_host("docker.io", &hosts)); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_is_unsecure_host_empty_list() { | ||
| let hosts: Vec<String> = vec![]; | ||
| assert!(!is_unsecure_host("localhost", &hosts)); | ||
| assert!(!is_unsecure_host("127.0.0.1", &hosts)); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_is_unsecure_host_defaults() { | ||
| let hosts: Vec<String> = DEFAULT_UNSECURE_HOSTS | ||
| .iter() | ||
| .map(|s| s.to_string()) | ||
| .collect(); | ||
| assert!(is_unsecure_host("localhost", &hosts)); | ||
| assert!(is_unsecure_host("localhost:5000", &hosts)); | ||
| assert!(is_unsecure_host("127.0.0.1", &hosts)); | ||
| assert!(is_unsecure_host("127.0.0.1:5000", &hosts)); | ||
| assert!(is_unsecure_host("dockerhost", &hosts)); | ||
| assert!(is_unsecure_host("dockerhost:5000", &hosts)); | ||
| assert!(!is_unsecure_host("docker.io", &hosts)); | ||
| assert!(!is_unsecure_host("registry.apollographql.com", &hosts)); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_is_unsecure_host_custom_list_replaces_defaults() { | ||
| let hosts = parse_unsecure_hosts("internal.registry.corp"); | ||
| assert!(is_unsecure_host("internal.registry.corp", &hosts)); | ||
| assert!(is_unsecure_host("internal.registry.corp:8080", &hosts)); | ||
| assert!(!is_unsecure_host("localhost", &hosts)); | ||
| assert!(!is_unsecure_host("127.0.0.1", &hosts)); | ||
| assert!(!is_unsecure_host("dockerhost", &hosts)); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_is_unsecure_host_no_substring_match() { | ||
| let hosts = vec!["localhost".to_string()]; | ||
| assert!(!is_unsecure_host("localhost.example.com", &hosts)); | ||
| assert!(!is_unsecure_host("notlocalhost", &hosts)); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_extract_host_simple() { | ||
| assert_eq!(extract_host("localhost"), Some("localhost".to_string())); | ||
| assert_eq!( | ||
| extract_host("localhost:5000"), | ||
| Some("localhost".to_string()) | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_extract_host_ipv4() { | ||
| assert_eq!(extract_host("127.0.0.1"), Some("127.0.0.1".to_string())); | ||
| assert_eq!( | ||
| extract_host("127.0.0.1:5000"), | ||
| Some("127.0.0.1".to_string()) | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_extract_host_ipv6() { | ||
| assert_eq!(extract_host("[::1]"), Some("::1".to_string())); | ||
| assert_eq!(extract_host("[::1]:5000"), Some("::1".to_string())); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_extract_host_domain_with_port() { | ||
| assert_eq!( | ||
| extract_host("registry.example.com:443"), | ||
| Some("registry.example.com".to_string()) | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_is_unsecure_host_ipv6() { | ||
| let hosts = vec!["::1".to_string()]; | ||
| assert!(is_unsecure_host("[::1]", &hosts)); | ||
| assert!(is_unsecure_host("[::1]:5000", &hosts)); | ||
| assert!(!is_unsecure_host("localhost", &hosts)); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_validate_oci_reference_valid_cases() { | ||
| // Test valid digest references with different algorithms | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there any cases where the URL will come in with a scheme, so this will 'double up' on schemes and cause parsing to fail?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The user could type whatever they want, but the graph artifacts URLs they use elsewhere will not include scheme, and the documentation does not show scheme, so I think it will feel natural to the user this way. Making it a little more robust is simple though, so I'll go ahead and touch it up.