connlib: fix wildcard checking in gateways (#3148)

the way we were checking for subdomains in the gateways completely broke, didn't detect it before because the deployed staging version for gateways is too old. ~~Added a few CI tests so this doesn't' happen again.~~ seems like github runners [doesn't support pinging the outside world](https://github.com/actions/runner-images/issues/1519) so I'm putting that off for now.
2026-01-27 10:18:54 +00:00 · 2024-01-09 22:55:23 -03:00
parent 8fddde371e
commit 9844a4b7b7
8 changed files with 209 additions and 65 deletions
--- a/elixir/apps/domain/priv/repo/seeds.exs
+++ b/elixir/apps/domain/priv/repo/seeds.exs
@@ -488,24 +488,36 @@ IO.puts("")
    admin_subject
  )

-{:ok, t_firez_one} =
+{:ok, firez_one} =
  Resources.create_resource(
    %{
      type: :dns,
-      name: "t.firez.one",
-      address: "t.firez.one",
+      name: "*.firez.one",
+      address: "*.firez.one",
      connections: [%{gateway_group_id: gateway_group.id}],
      filters: [%{protocol: :all}]
    },
    admin_subject
  )

-{:ok, ping_firez_one} =
+{:ok, firezone_dev} =
  Resources.create_resource(
    %{
      type: :dns,
-      name: "ping.firez.one",
-      address: "ping.firez.one",
+      name: "?.firezone.dev",
+      address: "?.firezone.dev",
+      connections: [%{gateway_group_id: gateway_group.id}],
+      filters: [%{protocol: :all}]
+    },
+    admin_subject
+  )
+
+{:ok, example_dns} =
+  Resources.create_resource(
+    %{
+      type: :dns,
+      name: "example.com",
+      address: "example.com",
      connections: [%{gateway_group_id: gateway_group.id}],
      filters: [%{protocol: :all}]
    },
@@ -555,6 +567,9 @@ IO.puts("")
 IO.puts("Created resources:")
 IO.puts("  #{dns_google_resource.address} - DNS - gateways: #{gateway_name}")
 IO.puts("  #{dns_gitlab_resource.address} - DNS - gateways: #{gateway_name}")
+IO.puts("  #{firez_one.address} - DNS - gateways: #{gateway_name}")
+IO.puts("  #{firezone_dev.address} - DNS - gateways: #{gateway_name}")
+IO.puts("  #{example_dns.address} - DNS - gateways: #{gateway_name}")
 IO.puts("  #{cidr_resource.address} - CIDR - gateways: #{gateway_name}")
 IO.puts("")

@@ -571,9 +586,9 @@ IO.puts("")
 {:ok, _} =
  Policies.create_policy(
    %{
-      name: "All Access To t.firez.one",
+      name: "All Access To firez.one",
      actor_group_id: all_group.id,
-      resource_id: t_firez_one.id
+      resource_id: firez_one.id
    },
    admin_subject
  )
@@ -581,9 +596,19 @@ IO.puts("")
 {:ok, _} =
  Policies.create_policy(
    %{
-      name: "All Access To ping.firez.one",
+      name: "All Access To firez.one",
      actor_group_id: all_group.id,
-      resource_id: ping_firez_one.id
+      resource_id: example_dns.id
+    },
+    admin_subject
+  )
+
+{:ok, _} =
+  Policies.create_policy(
+    %{
+      name: "All Access To firezone.dev",
+      actor_group_id: all_group.id,
+      resource_id: firezone_dev.id
    },
    admin_subject
  )
--- a/elixir/apps/domain/test/domain/auth/adapters/google_workspace/jobs_test.exs
+++ b/elixir/apps/domain/test/domain/auth/adapters/google_workspace/jobs_test.exs
@@ -476,43 +476,42 @@ defmodule Domain.Auth.Adapters.GoogleWorkspace.JobsTest do
          "Enable it by visiting https://console.developers.google.com/apis/api/admin.googleapis.com/overview?project=XXXX " <>
          "then retry. If you enabled this API recently, wait a few minutes for the action to propagate to our systems and retry."

-      response =
-        %{
-          "error" => %{
-            "code" => 403,
-            "message" => error_message,
-            "errors" => [
-              %{
-                "message" => error_message,
-                "domain" => "usageLimits",
-                "reason" => "accessNotConfigured",
-                "extendedHelp" => "https://console.developers.google.com"
-              }
-            ],
-            "status" => "PERMISSION_DENIED",
-            "details" => [
-              %{
-                "@type" => "type.googleapis.com/google.rpc.Help",
-                "links" => [
-                  %{
-                    "description" => "Google developers console API activation",
-                    "url" =>
-                      "https://console.developers.google.com/apis/api/admin.googleapis.com/overview?project=100421656358"
-                  }
-                ]
-              },
-              %{
-                "@type" => "type.googleapis.com/google.rpc.ErrorInfo",
-                "reason" => "SERVICE_DISABLED",
-                "domain" => "googleapis.com",
-                "metadata" => %{
-                  "service" => "admin.googleapis.com",
-                  "consumer" => "projects/100421656358"
+      response = %{
+        "error" => %{
+          "code" => 403,
+          "message" => error_message,
+          "errors" => [
+            %{
+              "message" => error_message,
+              "domain" => "usageLimits",
+              "reason" => "accessNotConfigured",
+              "extendedHelp" => "https://console.developers.google.com"
+            }
+          ],
+          "status" => "PERMISSION_DENIED",
+          "details" => [
+            %{
+              "@type" => "type.googleapis.com/google.rpc.Help",
+              "links" => [
+                %{
+                  "description" => "Google developers console API activation",
+                  "url" =>
+                    "https://console.developers.google.com/apis/api/admin.googleapis.com/overview?project=100421656358"
                }
+              ]
+            },
+            %{
+              "@type" => "type.googleapis.com/google.rpc.ErrorInfo",
+              "reason" => "SERVICE_DISABLED",
+              "domain" => "googleapis.com",
+              "metadata" => %{
+                "service" => "admin.googleapis.com",
+                "consumer" => "projects/100421656358"
              }
-            ]
-          }
+            }
+          ]
        }
+      }

      Bypass.expect_once(bypass, "GET", "/admin/directory/v1/users", fn conn ->
        Plug.Conn.send_resp(conn, 403, Jason.encode!(response))
--- a/elixir/apps/web/test/support/conn_case.ex
+++ b/elixir/apps/web/test/support/conn_case.ex
@@ -94,8 +94,7 @@ defmodule Web.ConnCase do
    cookie_key = "fz_auth_state_#{provider.id}"
    %{value: signed_state} = redirected_conn.resp_cookies[cookie_key]

-    conn_with_cookie =
-      put_req_cookie(conn, "fz_auth_state_#{provider.id}", signed_state)
+    conn_with_cookie = put_req_cookie(conn, "fz_auth_state_#{provider.id}", signed_state)

    {conn_with_cookie, secret}
  end
@@ -113,8 +112,7 @@ defmodule Web.ConnCase do

    %{value: signed_state} = redirected_conn.resp_cookies[cookie_key]

-    conn_with_cookie =
-      put_req_cookie(conn, "fz_auth_state_#{provider.id}", signed_state)
+    conn_with_cookie = put_req_cookie(conn, "fz_auth_state_#{provider.id}", signed_state)

    {conn_with_cookie, state, verifier}
  end
--- a/elixir/apps/web/test/web/controllers/auth_controller_test.exs
+++ b/elixir/apps/web/test/web/controllers/auth_controller_test.exs
@@ -429,8 +429,7 @@ defmodule Web.AuthControllerTest do
      actor = Fixtures.Actors.create_actor(type: :account_admin_user, account: account)
      identity = Fixtures.Auth.create_identity(account: account, provider: provider, actor: actor)

-      {conn_with_cookie, secret} =
-        put_magic_link_auth_state(conn, account, provider, identity)
+      {conn_with_cookie, secret} = put_magic_link_auth_state(conn, account, provider, identity)

      %{
        account: account,
@@ -877,8 +876,7 @@ defmodule Web.AuthControllerTest do
        nonce: "NONCE"
      }

-      {conn, state, _verifier} =
-        put_idp_auth_state(conn, account, provider, redirect_params)
+      {conn, state, _verifier} = put_idp_auth_state(conn, account, provider, redirect_params)

      Mocks.OpenIDConnect.expect_refresh_token(bypass, %{"id_token" => "foo"})

@@ -1146,8 +1144,7 @@ defmodule Web.AuthControllerTest do
            provider: provider
          )

-        {conn, secret} =
-          put_magic_link_auth_state(conn, account, provider, identity)
+        {conn, secret} = put_magic_link_auth_state(conn, account, provider, identity)

        authorized_conn =
          conn
--- a/elixir/apps/web/test/web/live/sign_in/email_test.exs
+++ b/elixir/apps/web/test/web/live/sign_in/email_test.exs
@@ -26,8 +26,7 @@ defmodule Web.SignIn.EmailTest do
    identity: identity,
    conn: conn
  } do
-    {conn, _secret} =
-      put_magic_link_auth_state(conn, account, provider, identity)
+    {conn, _secret} = put_magic_link_auth_state(conn, account, provider, identity)

    {:ok, lv, html} =
      live(conn, ~p"/#{account}/sign_in/providers/email/#{provider}?provider_identifier=foo")
@@ -43,8 +42,7 @@ defmodule Web.SignIn.EmailTest do
    identity: identity,
    conn: conn
  } do
-    {conn, secret} =
-      put_magic_link_auth_state(conn, account, provider, identity)
+    {conn, secret} = put_magic_link_auth_state(conn, account, provider, identity)

    {:ok, lv, _html} =
      live(
@@ -83,8 +81,7 @@ defmodule Web.SignIn.EmailTest do
      "redirect_to" => "/foo"
    }

-    {conn, secret} =
-      put_magic_link_auth_state(conn, account, provider, identity, redirect_params)
+    {conn, secret} = put_magic_link_auth_state(conn, account, provider, identity, redirect_params)

    {:ok, lv, _html} =
      live(
@@ -116,8 +113,7 @@ defmodule Web.SignIn.EmailTest do
    identity: identity,
    conn: conn
  } do
-    {conn, _secret} =
-      put_magic_link_auth_state(conn, account, provider, identity)
+    {conn, _secret} = put_magic_link_auth_state(conn, account, provider, identity)

    {:ok, lv, _html} =
      live(
--- a/rust/connlib/tunnel/src/control_protocol/client.rs
+++ b/rust/connlib/tunnel/src/control_protocol/client.rs
@@ -84,7 +84,7 @@ where
        relays: Vec<Relay>,
        reference: Option<Reference>,
    ) -> Result<Request> {
-        tracing::trace!("request_connection");
+        tracing::trace!(%gateway_id, %resource_id, "request_connection");

        let reference: usize = reference
            .ok_or(Error::InvalidReference)?
--- a/rust/connlib/tunnel/src/control_protocol/gateway.rs
+++ b/rust/connlib/tunnel/src/control_protocol/gateway.rs
@@ -1,5 +1,6 @@
 use crate::{
    control_protocol::{insert_peers, start_handlers},
+    dns::is_subdomain,
    peer::{PacketTransformGateway, Peer},
    ConnectedPeer, GatewayState, PeerConfig, Tunnel, PEER_QUEUE_SIZE,
 };
@@ -95,7 +96,7 @@ where
                    return Err(Error::ControlProtocolError);
                };

-                if !domain.iter_suffixes().any(|d| d.to_string() == r.address) {
+                if !is_subdomain(&domain, &r.address) {
                    let _ = ice.stop().await;
                    return Err(Error::InvalidResource);
                }
@@ -181,7 +182,7 @@ where
                        return None;
                    };

-                    if !domain.iter_suffixes().any(|d| d.to_string() == r.address) {
+                    if !is_subdomain(domain, &r.address) {
                        return None;
                    }

--- a/rust/connlib/tunnel/src/dns.rs
+++ b/rust/connlib/tunnel/src/dns.rs
@@ -244,6 +244,28 @@ enum RecordData<T> {
    Ptr(domain::rdata::Ptr<T>),
 }

+pub fn is_subdomain(name: &Dname, resource: &str) -> bool {
+    let question_mark = RelativeDname::<Vec<_>>::from_octets(b"\x01?".as_ref().into()).unwrap();
+    let Ok(resource) = Dname::vec_from_str(resource) else {
+        return false;
+    };
+
+    if resource.starts_with(&question_mark) {
+        return resource
+            .parent()
+            .is_some_and(|r| r == name || name.parent().is_some_and(|n| r == n));
+    }
+
+    if resource.starts_with(&RelativeDname::wildcard_vec()) {
+        let Some(resource) = resource.parent() else {
+            return false;
+        };
+        return name.iter_suffixes().any(|n| n == resource);
+    }
+
+    name == &resource
+}
+
 fn get_description(
    name: &Dname,
    dns_resources: &HashMap<String, ResourceDescriptionDns>,
@@ -401,6 +423,8 @@ fn reverse_dns_addr_v6<'a>(dns_parts: &mut impl Iterator<Item = &'a str>) -> Opt
 mod test {
    use connlib_shared::{messages::ResourceDescriptionDns, Dname};

+    use crate::dns::is_subdomain;
+
    use super::{get_description, reverse_dns_addr};
    use std::{collections::HashMap, net::Ipv4Addr};

@@ -594,4 +618,108 @@ mod test {
        )
        .is_none(),);
    }
+
+    #[test]
+    fn exact_subdomain_match() {
+        assert!(is_subdomain(
+            &Dname::vec_from_str("foo.com").unwrap(),
+            "foo.com"
+        ));
+
+        assert!(!is_subdomain(
+            &Dname::vec_from_str("a.foo.com").unwrap(),
+            "foo.com"
+        ));
+
+        assert!(!is_subdomain(
+            &Dname::vec_from_str("a.b.foo.com").unwrap(),
+            "foo.com"
+        ));
+
+        assert!(!is_subdomain(
+            &Dname::vec_from_str("foo.com").unwrap(),
+            "a.foo.com"
+        ));
+    }
+
+    #[test]
+    fn wildcard_subdomain_match() {
+        assert!(is_subdomain(
+            &Dname::vec_from_str("foo.com").unwrap(),
+            "*.foo.com"
+        ));
+
+        assert!(is_subdomain(
+            &Dname::vec_from_str("a.foo.com").unwrap(),
+            "*.foo.com"
+        ));
+
+        assert!(is_subdomain(
+            &Dname::vec_from_str("a.foo.com").unwrap(),
+            "*.a.foo.com"
+        ));
+
+        assert!(is_subdomain(
+            &Dname::vec_from_str("b.a.foo.com").unwrap(),
+            "*.a.foo.com"
+        ));
+
+        assert!(is_subdomain(
+            &Dname::vec_from_str("a.b.foo.com").unwrap(),
+            "*.foo.com"
+        ));
+
+        assert!(!is_subdomain(
+            &Dname::vec_from_str("afoo.com").unwrap(),
+            "*.foo.com"
+        ));
+
+        assert!(!is_subdomain(
+            &Dname::vec_from_str("b.afoo.com").unwrap(),
+            "*.foo.com"
+        ));
+
+        assert!(!is_subdomain(
+            &Dname::vec_from_str("bar.com").unwrap(),
+            "*.foo.com"
+        ));
+
+        assert!(!is_subdomain(
+            &Dname::vec_from_str("foo.com").unwrap(),
+            "*.a.foo.com"
+        ));
+    }
+
+    #[test]
+    fn question_mark_subdomain_match() {
+        assert!(is_subdomain(
+            &Dname::vec_from_str("foo.com").unwrap(),
+            "?.foo.com"
+        ));
+
+        assert!(is_subdomain(
+            &Dname::vec_from_str("a.foo.com").unwrap(),
+            "?.foo.com"
+        ));
+
+        assert!(!is_subdomain(
+            &Dname::vec_from_str("a.b.foo.com").unwrap(),
+            "?.foo.com"
+        ));
+
+        assert!(!is_subdomain(
+            &Dname::vec_from_str("bar.com").unwrap(),
+            "?.foo.com"
+        ));
+
+        assert!(!is_subdomain(
+            &Dname::vec_from_str("foo.com").unwrap(),
+            "?.a.foo.com"
+        ));
+
+        assert!(!is_subdomain(
+            &Dname::vec_from_str("afoo.com").unwrap(),
+            "?.foo.com"
+        ));
+    }
 }