From 8b2b96a21a6d0d24a10b0d5402c545cab1805371 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafa=C5=82=20Cie=C5=9Blak?= Date: Thu, 17 Oct 2024 13:08:57 +0200 Subject: [PATCH] Validate ports of api/types.AppV3 --- api/types/app.go | 54 +++++++++++++++++- api/types/app_test.go | 130 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 181 insertions(+), 3 deletions(-) diff --git a/api/types/app.go b/api/types/app.go index 8460f040fc230..3ab17273f7695 100644 --- a/api/types/app.go +++ b/api/types/app.go @@ -393,13 +393,13 @@ func (a *AppV3) CheckAndSetDefaults() error { if !strings.Contains(publicAddr, "//") && strings.Contains(publicAddr, ":") { publicAddr = "//" + publicAddr } - url, err := url.Parse(publicAddr) + publicAddrURL, err := url.Parse(publicAddr) if err != nil { return trace.BadParameter("invalid PublicAddr format: %v", err) } host := a.Spec.PublicAddr - if url.Host != "" { - host = url.Host + if publicAddrURL.Host != "" { + host = publicAddrURL.Host } if strings.HasPrefix(host, constants.KubeTeleportProxyALPNPrefix) { @@ -416,6 +416,54 @@ func (a *AppV3) CheckAndSetDefaults() error { } } + if len(a.Spec.Ports) != 0 { + if err := a.checkPorts(); err != nil { + return trace.Wrap(err) + } + } + + return nil +} + +func (a *AppV3) checkPorts() error { + // Parsing the URI here does not break compatibility. The URI is parsed only if Ports are present. + // This means that old apps that do have invalid URIs but don't use Ports can continue existing. + uri, err := url.Parse(a.Spec.URI) + if err != nil { + return trace.BadParameter("invalid app URI format: %v", err) + } + + // The scheme of URI is enforced to be "tcp" on purpose. This way in the future we can add + // multi-port support to web apps without throwing hard errors when a cluster with a multi-port + // web app gets downgraded to a version which supports multi-port only for TCP apps. + // + // For now, we simply ignore the Ports field set on non-TCP apps. + if uri.Scheme != "tcp" { + return nil + } + + if uri.Port() != "" { + return trace.BadParameter("app URI %q must not include a port number when the app spec defines a list of ports", a.Spec.URI) + } + + const minPort = 1 + const maxPort = 65535 + for _, portRange := range a.Spec.Ports { + if portRange.Port < minPort || portRange.Port > maxPort { + return trace.BadParameter("app port must be between %d and %d, but got %d", minPort, maxPort, portRange.Port) + } + + if portRange.EndPort != 0 { + if portRange.EndPort < minPort+1 || portRange.EndPort > maxPort { + return trace.BadParameter("app end port must be between %d and %d, but got %d", minPort+1, maxPort, portRange.EndPort) + } + + if portRange.EndPort <= portRange.Port { + return trace.BadParameter("app end port must be greater than port (%d vs %d)", portRange.EndPort, portRange.Port) + } + } + } + return nil } diff --git a/api/types/app_test.go b/api/types/app_test.go index 269b41686b3c2..c161ff4e0e78f 100644 --- a/api/types/app_test.go +++ b/api/types/app_test.go @@ -97,6 +97,136 @@ func TestAppPublicAddrValidation(t *testing.T) { } } +func TestAppPortsValidation(t *testing.T) { + type check func(t *testing.T, err error) + + hasNoErr := func() check { + return func(t *testing.T, err error) { + require.NoError(t, err) + } + } + hasErrTypeBadParameter := func() check { + return func(t *testing.T, err error) { + require.True(t, trace.IsBadParameter(err)) + } + } + hasErrTypeBadParameterAndContains := func(msg string) check { + return func(t *testing.T, err error) { + require.True(t, trace.IsBadParameter(err), "err should be trace.BadParameter") + require.ErrorContains(t, err, msg) + } + } + hasErrAndContains := func(msg string) check { + return func(t *testing.T, err error) { + require.ErrorContains(t, err, msg) + } + } + + tests := []struct { + name string + ports []*PortRange + uri string + check check + }{ + { + name: "valid ranges and single ports", + ports: []*PortRange{ + &PortRange{Port: 22, EndPort: 25}, + &PortRange{Port: 26}, + &PortRange{Port: 65535}, + }, + check: hasNoErr(), + }, + { + name: "valid overlapping ranges", + ports: []*PortRange{ + &PortRange{Port: 100, EndPort: 200}, + &PortRange{Port: 150, EndPort: 175}, + &PortRange{Port: 111}, + &PortRange{Port: 150, EndPort: 210}, + &PortRange{Port: 1, EndPort: 65535}, + }, + check: hasNoErr(), + }, + { + name: "valid non-TCP app with ports ignored", + uri: "http://localhost:8000", + ports: []*PortRange{ + &PortRange{Port: 123456789}, + &PortRange{Port: 10, EndPort: 2}, + }, + check: hasNoErr(), + }, + // Test cases for invalid ports. + { + name: "port smaller than 1", + ports: []*PortRange{ + &PortRange{Port: 0}, + }, + check: hasErrTypeBadParameter(), + }, + { + name: "port bigger than 65535", + ports: []*PortRange{ + &PortRange{Port: 78787}, + }, + check: hasErrTypeBadParameter(), + }, + { + name: "end port smaller than 2", + ports: []*PortRange{ + &PortRange{Port: 5, EndPort: 1}, + }, + check: hasErrTypeBadParameterAndContains("end port must be between"), + }, + { + name: "end port bigger than 65535", + ports: []*PortRange{ + &PortRange{Port: 1, EndPort: 78787}, + }, + check: hasErrTypeBadParameter(), + }, + { + name: "end port smaller than port", + ports: []*PortRange{ + &PortRange{Port: 10, EndPort: 5}, + }, + check: hasErrTypeBadParameterAndContains("end port must be greater than port"), + }, + { + name: "uri specifies port", + uri: "tcp://localhost:1234", + ports: []*PortRange{ + &PortRange{Port: 1000, EndPort: 1500}, + }, + check: hasErrTypeBadParameterAndContains("must not include a port number"), + }, + { + name: "invalid uri", + uri: "%", + ports: []*PortRange{ + &PortRange{Port: 1000, EndPort: 1500}, + }, + check: hasErrAndContains("invalid URL escape"), + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + spec := AppSpecV3{ + URI: "tcp://localhost", + Ports: tc.ports, + } + if tc.uri != "" { + spec.URI = tc.uri + } + + _, err := NewAppV3(Metadata{Name: "TestApp"}, spec) + tc.check(t, err) + }) + } +} + func TestAppServerSorter(t *testing.T) { t.Parallel()