refactor(router): http/stream subsystem does not share schema

changelog/unreleased/kong/subsystems_do_not_share_router_schemas.yml

@@ -0,0 +1,6 @@
+message: |
+  Expressions route in `http` and `stream` subsystem now have stricter validation.
+  Previously they share the same validation schema which means admin can configure expressions
+  route using fields like `http.path` even for stream routes. This is no longer allowed.
+type: bugfix
+scope: Core

kong/db/schema/entities/routes.lua

@@ -3,30 +3,22 @@ local router = require("resty.router.router")
 local deprecation = require("kong.deprecation")
 
 local validate_route
-local has_paths
 do
-  local isempty        = require("table.isempty")
-  local CACHED_SCHEMA  = require("kong.router.atc").schema
+  local get_schema     = require("kong.router.atc").schema
   local get_expression = require("kong.router.compat").get_expression
 
-  local type = type
-
   -- works with both `traditional_compatiable` and `expressions` routes`
   validate_route = function(entity)
+    local schema = get_schema(entity.protocols)
     local exp = entity.expression or get_expression(entity)
 
-    local ok, err = router.validate(CACHED_SCHEMA, exp)
+    local ok, err = router.validate(schema, exp)
     if not ok then
       return nil, "Router Expression failed validation: " .. err
     end
 
     return true
   end
-
-  has_paths = function(entity)
-    local paths = entity.paths
-    return type(paths) == "table" and not isempty(paths)
-  end
 end
 
 local kong_router_flavor = kong and kong.configuration and kong.configuration.router_flavor
@@ -73,15 +65,8 @@ if kong_router_flavor == "expressions" then
 
     entity_checks = {
       { custom_entity_check = {
-        field_sources = { "expression", "id", },
-        fn = function(entity)
-          local ok, err = validate_route(entity)
-          if not ok then
-            return nil, err
-          end
-
-          return true
-        end,
+        field_sources = { "expression", "id", "protocols", },
+        fn = validate_route,
       } },
     },
   }
@@ -126,17 +111,7 @@ else
     table.insert(entity_checks,
       { custom_entity_check = {
         run_with_missing_fields = true,
-        field_sources = { "id", "paths", },
-        fn = function(entity)
-          if has_paths(entity) then
-            local ok, err = validate_route(entity)
-            if not ok then
-              return nil, err
-            end
-          end
-
-          return true
-        end,
+        fn = validate_route,
       }}
     )
   end

kong/router/atc.lua

@@ -55,8 +55,10 @@ local values_buf = buffer.new(64)
 
 
 local CACHED_SCHEMA
+local HTTP_SCHEMA
+local STREAM_SCHEMA
 do
-  local FIELDS = {
+  local HTTP_FIELDS = {
 
     ["String"] = {"net.protocol", "tls.sni",
                   "http.method", "http.host",
@@ -66,21 +68,39 @@ do
                  },
 
     ["Int"]    = {"net.port",
-                  "net.src.port", "net.dst.port",
+                 },
+  }
+
+  local STREAM_FIELDS = {
+
+    ["String"] = {"net.protocol", "tls.sni",
+                 },
+
+    ["Int"]    = {"net.src.port", "net.dst.port",
                  },
 
     ["IpAddr"] = {"net.src.ip", "net.dst.ip",
                  },
   }
 
-  CACHED_SCHEMA = schema.new()
+  local function generate_schema(fields)
+    local s = schema.new()
 
-  for typ, fields in pairs(FIELDS) do
-    for _, v in ipairs(fields) do
-      assert(CACHED_SCHEMA:add_field(v, typ))
+    for t, f in pairs(fields) do
+      for _, v in ipairs(f) do
+        assert(s:add_field(v, t))
+      end
     end
+
+    return s
   end
 
+  -- used by validation
+  HTTP_SCHEMA   = generate_schema(HTTP_FIELDS)
+  STREAM_SCHEMA = generate_schema(STREAM_FIELDS)
+
+  -- used by running router
+  CACHED_SCHEMA = is_http and HTTP_SCHEMA or STREAM_SCHEMA
 end
 
 
@@ -871,7 +891,26 @@ function _M._set_ngx(mock_ngx)
 end
 
 
-_M.schema          = CACHED_SCHEMA
+do
+  local protocol_to_schema = {
+    http  = HTTP_SCHEMA,
+    https = HTTP_SCHEMA,
+    grpc  = HTTP_SCHEMA,
+    grpcs = HTTP_SCHEMA,
+
+    tcp   = STREAM_SCHEMA,
+    udp   = STREAM_SCHEMA,
+    tls   = STREAM_SCHEMA,
+
+    tls_passthrough = STREAM_SCHEMA,
+  }
+
+  -- for db schema validation
+  function _M.schema(protocols)
+    return assert(protocol_to_schema[protocols[1]])
+  end
+end
+
 
 _M.LOGICAL_OR      = LOGICAL_OR
 _M.LOGICAL_AND     = LOGICAL_AND

spec/01-unit/01-db/01-schema/06-routes_spec.lua

@@ -1329,7 +1329,7 @@ describe("routes schema (flavor = traditional_compatible)", function()
   reload_flavor("traditional_compatible")
   setup_global_env()
 
-  it("validates a valid route", function()
+  it("validates a valid http route", function()
     local route = {
       id             = a_valid_uuid,
       name           = "my_route",
@@ -1351,6 +1351,21 @@ describe("routes schema (flavor = traditional_compatible)", function()
     assert.falsy(route.strip_path)
   end)
 
+  it("validates a valid stream route", function()
+    local route = {
+      id             = a_valid_uuid,
+      name           = "my_route",
+      protocols      = { "tcp" },
+      sources        = { { ip = "1.2.3.4", port = 80 } },
+      service        = { id = another_uuid },
+    }
+    route = Routes:process_auto_fields(route, "insert")
+    assert.truthy(route.created_at)
+    assert.truthy(route.updated_at)
+    assert.same(route.created_at, route.updated_at)
+    assert.truthy(Routes:validate(route))
+  end)
+
   it("fails when path is invalid", function()
     local route = {
       id             = a_valid_uuid,
@@ -1370,6 +1385,23 @@ describe("routes schema (flavor = traditional_compatible)", function()
     assert.falsy(errs["@entity"])
   end)
 
+  it("fails when ip address is invalid", function()
+    local route = {
+      id             = a_valid_uuid,
+      name           = "my_route",
+      protocols      = { "tcp" },
+      sources        = { { ip = "x.x.x.x", port = 80 } },
+      service        = { id = another_uuid },
+    }
+    route = Routes:process_auto_fields(route, "insert")
+    local ok, errs = Routes:validate_insert(route)
+    assert.falsy(ok)
+    assert.truthy(errs["sources"])
+
+    -- verified by `schema/typedefs.lua`
+    assert.falsy(errs["@entity"])
+  end)
+
   it("won't fail when rust.regex update to 1.8", function()
     local route = {
       id             = a_valid_uuid,
@@ -1384,3 +1416,98 @@ describe("routes schema (flavor = traditional_compatible)", function()
     assert.is_nil(errs)
   end)
 end)
+
+
+describe("routes schema (flavor = expressions)", function()
+  local a_valid_uuid = "cbb297c0-a956-486d-ad1d-f9b42df9465a"
+  local another_uuid = "64a8670b-900f-44e7-a900-6ec7ef5aa4d3"
+
+  reload_flavor("expressions")
+  setup_global_env()
+
+  it("validates a valid http route", function()
+    local route = {
+      id             = a_valid_uuid,
+      name           = "my_route",
+      protocols      = { "http" },
+      expression     = [[http.method == "GET" && http.host == "example.com" && http.path == "/ovo"]],
+      priority       = 100,
+      strip_path     = false,
+      preserve_host  = true,
+      service        = { id = another_uuid },
+    }
+    route = Routes:process_auto_fields(route, "insert")
+    assert.truthy(route.created_at)
+    assert.truthy(route.updated_at)
+    assert.same(route.created_at, route.updated_at)
+    assert.truthy(Routes:validate(route))
+    assert.falsy(route.strip_path)
+  end)
+
+  it("validates a valid stream route", function()
+    local route = {
+      id             = a_valid_uuid,
+      name           = "my_route",
+      protocols      = { "tcp" },
+      expression     = [[net.src.ip == 1.2.3.4 && net.src.port == 80]],
+      priority       = 100,
+      service        = { id = another_uuid },
+    }
+    route = Routes:process_auto_fields(route, "insert")
+    assert.truthy(route.created_at)
+    assert.truthy(route.updated_at)
+    assert.same(route.created_at, route.updated_at)
+    assert.truthy(Routes:validate(route))
+  end)
+
+  it("fails when path is invalid", function()
+    local route = {
+      id             = a_valid_uuid,
+      name           = "my_route",
+      protocols      = { "http" },
+      expression     = [[http.method == "GET" && http.path ~ "/[abc/*/user$"]],
+      priority       = 100,
+      service        = { id = another_uuid },
+    }
+    route = Routes:process_auto_fields(route, "insert")
+    local ok, errs = Routes:validate_insert(route)
+    assert.falsy(ok)
+
+    -- verified by `schema/typedefs.lua`
+    assert.truthy(errs["@entity"])
+  end)
+
+  it("fails when ip address is invalid", function()
+    local route = {
+      id             = a_valid_uuid,
+      name           = "my_route",
+      protocols      = { "tcp" },
+      expression     = [[net.src.ip in 1.2.3.4/16 && net.src.port == 80]],
+      priority       = 100,
+      service        = { id = another_uuid },
+    }
+    route = Routes:process_auto_fields(route, "insert")
+    local ok, errs = Routes:validate_insert(route)
+    assert.falsy(ok)
+
+    -- verified by `schema/typedefs.lua`
+    assert.truthy(errs["@entity"])
+  end)
+
+  it("fails if http route's field appears in stream route", function()
+    local route = {
+      id             = a_valid_uuid,
+      name           = "my_route",
+      protocols      = { "tcp" },
+      expression     = [[http.method == "GET" && net.src.ip == 1.2.3.4 && net.src.port == 80]],
+      priority       = 100,
+      service        = { id = another_uuid },
+    }
+    route = Routes:process_auto_fields(route, "insert")
+    local ok, errs = Routes:validate_insert(route)
+    assert.falsy(ok)
+
+    -- verified by `schema/typedefs.lua`
+    assert.truthy(errs["@entity"])
+  end)
+end)