Validate ports of api/types.AppV3

gravitational · Oct 18, 2024 · 8b2b96a · 8b2b96a
1 parent 84e60bd
commit 8b2b96a
Show file tree

Hide file tree

Showing 2 changed files with 181 additions and 3 deletions.
diff --git 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
@@ -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()