Fix x11 server config issues

lib/config/fileconf.go

@@ -855,26 +855,33 @@ func (ssh *SSH) X11ServerConfig() (*x11.ServerConfig, error) {
 		return cfg, nil
 	}
 
-	cfg.MaxDisplay = x11.DefaultMaxDisplay
-	if ssh.X11.MaxDisplay != nil {
-		cfg.MaxDisplay = int(*ssh.X11.MaxDisplay)
-		// If set, max display must not be greater than the
-		// max display number supported by X Server
-		if cfg.MaxDisplay > x11.MaxDisplayNumber {
+	cfg.DisplayOffset = x11.DefaultDisplayOffset
+	if ssh.X11.DisplayOffset != nil {
+		cfg.DisplayOffset = int(*ssh.X11.DisplayOffset)
+
+		if cfg.DisplayOffset > x11.MaxDisplayNumber {
 			cfg.DisplayOffset = x11.MaxDisplayNumber
 		}
 	}
 
-	cfg.DisplayOffset = x11.DefaultDisplayOffset
-	if ssh.X11.DisplayOffset != nil {
-		cfg.DisplayOffset = int(*ssh.X11.DisplayOffset)
-		// If set, display offset must not be greater than the
-		// max display number
-		if cfg.DisplayOffset > cfg.MaxDisplay {
-			cfg.DisplayOffset = cfg.MaxDisplay
+	// DELETE IN 10.0.0 (Joerger): yaml typo, use MaxDisplay.
+	if ssh.X11.MaxDisplays != nil && ssh.X11.MaxDisplay == nil {
+		ssh.X11.MaxDisplay = ssh.X11.MaxDisplays
+	}
+
+	cfg.MaxDisplay = cfg.DisplayOffset + x11.DefaultMaxDisplays
+	if ssh.X11.MaxDisplay != nil {
+		cfg.MaxDisplay = int(*ssh.X11.MaxDisplay)
+
+		if cfg.MaxDisplay < cfg.DisplayOffset {
+			return nil, trace.BadParameter("x11.MaxDisplay cannot be smaller than x11.DisplayOffset")
 		}
 	}
 
+	if cfg.MaxDisplay > x11.MaxDisplayNumber {
+		cfg.MaxDisplay = x11.MaxDisplayNumber
+	}
+
 	return cfg, nil
 }
 
@@ -977,9 +984,11 @@ type X11 struct {
 	// DisplayOffset tells the server what X11 display number to start from when
 	// searching for an open X11 unix socket for XServer proxies.
 	DisplayOffset *uint `yaml:"display_offset,omitempty"`
-	// DisplayOffset tells the server what X11 display number to stop at when
+	// MaxDisplay tells the server what X11 display number to stop at when
 	// searching for an open X11 unix socket for XServer proxies.
-	MaxDisplay *uint `yaml:"max_displays,omitempty"`
+	MaxDisplay *uint `yaml:"max_display,omitempty"`
+	// DELETE IN 10.0.0 (Joerger): yaml typo, use MaxDisplay.
+	MaxDisplays *uint `yaml:"max_displays,omitempty"`
 }
 
 // Databases represents the database proxy service configuration.

lib/config/fileconf_test.go

@@ -390,42 +390,6 @@ func TestSSHSection(t *testing.T) {
 				cfg["ssh_service"].(cfgMap)["port_forwarding"] = "banana"
 			},
 			expectError: require.Error,
-		}, {
-			desc: "X11 enabled",
-			mutate: func(cfg cfgMap) {
-				cfg["ssh_service"].(cfgMap)["x11"] = cfgMap{
-					"enabled": "yes",
-				}
-			},
-			expectError: require.NoError,
-		}, {
-			desc: "X11 disabled",
-			mutate: func(cfg cfgMap) {
-				cfg["ssh_service"].(cfgMap)["x11"] = cfgMap{
-					"enabled": "no",
-				}
-			},
-			expectError: require.NoError,
-		}, {
-			desc:        "X11 display offset default",
-			mutate:      func(cfg cfgMap) {},
-			expectError: require.NoError,
-		}, {
-			desc: "X11 display offset 100",
-			mutate: func(cfg cfgMap) {
-				cfg["ssh_service"].(cfgMap)["x11"] = cfgMap{
-					"display_offset": 100,
-				}
-			},
-			expectError: require.NoError,
-		}, {
-			desc: "X11 display offset invalid value",
-			mutate: func(cfg cfgMap) {
-				cfg["ssh_service"].(cfgMap)["x11"] = cfgMap{
-					"display_offset": -100,
-				}
-			},
-			expectError: require.Error,
 		},
 	}
 
@@ -449,25 +413,26 @@ func TestSSHSection(t *testing.T) {
 }
 
 func TestX11Config(t *testing.T) {
-	for _, tc := range []struct {
-		desc            string
-		mutate          func(cfgMap)
-		expectError     require.ErrorAssertionFunc
-		expectX11Config *x11.ServerConfig
+	testCases := []struct {
+		desc              string
+		mutate            func(cfgMap)
+		expectReadError   require.ErrorAssertionFunc
+		expectConfigError require.ErrorAssertionFunc
+		expectX11Config   *x11.ServerConfig
 	}{
 		{
 			desc:            "default",
 			mutate:          func(cfg cfgMap) {},
-			expectError:     require.NoError,
 			expectX11Config: &x11.ServerConfig{},
-		}, {
-			desc: "disabled",
+		},
+		// Test x11 enabled
+		{
+			desc: "x11 disabled",
 			mutate: func(cfg cfgMap) {
 				cfg["ssh_service"].(cfgMap)["x11"] = cfgMap{
 					"enabled": "no",
 				}
 			},
-			expectError:     require.NoError,
 			expectX11Config: &x11.ServerConfig{},
 		}, {
 			desc: "x11 enabled",
@@ -476,128 +441,112 @@ func TestX11Config(t *testing.T) {
 					"enabled": "yes",
 				}
 			},
-			expectError: require.NoError,
 			expectX11Config: &x11.ServerConfig{
 				Enabled:       true,
 				DisplayOffset: x11.DefaultDisplayOffset,
-				MaxDisplay:    x11.DefaultMaxDisplay,
+				MaxDisplay:    x11.DefaultDisplayOffset + x11.DefaultMaxDisplays,
 			},
-		}, {
-			desc: "enabled value invalid",
-			mutate: func(cfg cfgMap) {
-				cfg["ssh_service"].(cfgMap)["x11"] = cfgMap{
-					"enabled":        "no",
-					"display_offset": "100",
-				}
-			},
-			expectError: require.Error,
-		}, {
+		},
+		// Test display offset
+		{
 			desc: "display offset set",
 			mutate: func(cfg cfgMap) {
 				cfg["ssh_service"].(cfgMap)["x11"] = cfgMap{
 					"enabled":        "yes",
 					"display_offset": "100",
 				}
 			},
-			expectError: require.NoError,
 			expectX11Config: &x11.ServerConfig{
 				Enabled:       true,
 				DisplayOffset: 100,
-				MaxDisplay:    x11.DefaultMaxDisplay,
+				MaxDisplay:    100 + x11.DefaultMaxDisplays,
 			},
 		}, {
-			desc: "not enabled, display offset set",
+			desc: "display offset value capped",
 			mutate: func(cfg cfgMap) {
 				cfg["ssh_service"].(cfgMap)["x11"] = cfgMap{
-					"enabled":        "no",
-					"display_offset": "100",
+					"enabled":        "yes",
+					"display_offset": math.MaxUint32,
 				}
 			},
-			expectError: require.NoError,
 			expectX11Config: &x11.ServerConfig{
-				Enabled: false,
-			},
-		}, {
-			desc: "display offset value invalid",
-			mutate: func(cfg cfgMap) {
-				cfg["ssh_service"].(cfgMap)["x11"] = cfgMap{
-					"enabled":        "yes",
-					"display_offset": "banana",
-				}
+				Enabled:       true,
+				DisplayOffset: x11.MaxDisplayNumber,
+				MaxDisplay:    x11.MaxDisplayNumber,
 			},
-			expectError: require.Error,
-		}, {
-			desc: "display offset value capped",
+		},
+		// Test max display
+		{
+			desc: "max display set",
 			mutate: func(cfg cfgMap) {
 				cfg["ssh_service"].(cfgMap)["x11"] = cfgMap{
-					"enabled":        "yes",
-					"display_offset": math.MaxUint32,
+					"enabled":     "yes",
+					"max_display": "100",
 				}
 			},
-			expectError: require.Error,
 			expectX11Config: &x11.ServerConfig{
 				Enabled:       true,
 				DisplayOffset: x11.DefaultDisplayOffset,
+				MaxDisplay:    100 + x11.DefaultDisplayOffset,
 			},
 		}, {
+			// DELETE IN 10.0.0 (Joerger): yaml typo, use max_display.
 			desc: "max displays set",
 			mutate: func(cfg cfgMap) {
 				cfg["ssh_service"].(cfgMap)["x11"] = cfgMap{
 					"enabled":      "yes",
 					"max_displays": "100",
 				}
 			},
-			expectError: require.NoError,
 			expectX11Config: &x11.ServerConfig{
 				Enabled:       true,
 				DisplayOffset: x11.DefaultDisplayOffset,
-				MaxDisplay:    100,
+				MaxDisplay:    100 + x11.DefaultDisplayOffset,
 			},
 		}, {
-			desc: "not enabled, max displays set",
+			desc: "max display value capped",
 			mutate: func(cfg cfgMap) {
 				cfg["ssh_service"].(cfgMap)["x11"] = cfgMap{
-					"enabled":      "no",
-					"max_displays": "100",
+					"enabled":     "yes",
+					"max_display": math.MaxUint32,
 				}
 			},
-			expectError: require.NoError,
 			expectX11Config: &x11.ServerConfig{
-				Enabled: false,
+				Enabled:       true,
+				DisplayOffset: x11.DefaultDisplayOffset,
+				MaxDisplay:    x11.MaxDisplayNumber,
 			},
 		}, {
-			desc: "max displays value invalid",
+			desc: "max display smaller than display offset",
 			mutate: func(cfg cfgMap) {
 				cfg["ssh_service"].(cfgMap)["x11"] = cfgMap{
-					"enabled":      "yes",
-					"max_displays": "banana",
+					"enabled":        "maybe",
+					"display_offset": "1000",
+					"max_display":    "100",
 				}
 			},
-			expectError: require.Error,
-		}, {
-			desc: "max displays value capped",
-			mutate: func(cfg cfgMap) {
-				cfg["ssh_service"].(cfgMap)["x11"] = cfgMap{
-					"enabled":      "yes",
-					"max_displays": math.MaxUint32,
-				}
-			},
-			expectError: require.Error,
-			expectX11Config: &x11.ServerConfig{
-				Enabled:       true,
-				DisplayOffset: x11.DefaultDisplayOffset,
-				MaxDisplay:    x11.MaxDisplayNumber - x11.DefaultDisplayOffset,
+			expectConfigError: func(t require.TestingT, err error, i ...interface{}) {
+				require.Error(t, err)
+				require.True(t, trace.IsBadParameter(err), "got err = %v, want BadParameter", err)
 			},
 		},
-	} {
+	}
+
+	for _, tc := range testCases {
 		text := bytes.NewBuffer(editConfig(t, tc.mutate))
+
 		cfg, err := ReadConfig(text)
-		tc.expectError(t, err)
-		if err != nil {
+		if tc.expectReadError != nil {
+			tc.expectReadError(t, err)
 			return
 		}
+		require.NoError(t, err)
 
 		serverCfg, err := cfg.SSH.X11ServerConfig()
+		if tc.expectConfigError != nil {
+			tc.expectConfigError(t, err)
+			return
+		}
 		require.NoError(t, err)
 		require.Equal(t, tc.expectX11Config, serverCfg)
 	}

lib/srv/regular/sshserver_test.go

@@ -698,7 +698,7 @@ func TestX11Forward(t *testing.T) {
 	f.ssh.srv.x11 = &x11.ServerConfig{
 		Enabled:       true,
 		DisplayOffset: x11.DefaultDisplayOffset,
-		MaxDisplay:    x11.DefaultMaxDisplay,
+		MaxDisplay:    x11.DefaultMaxDisplays,
 	}
 
 	ctx := context.Background()

lib/sshutils/x11/display.go

@@ -33,9 +33,9 @@ const (
 	// DefaultDisplayOffset is the default display offset when
 	// searching for an open XServer unix socket.
 	DefaultDisplayOffset = 10
-	// DefaultMaxDisplay is the default maximum display number
+	// DefaultMaxDisplays is the default maximum number of displays
 	// supported when searching for an open XServer unix socket.
-	DefaultMaxDisplay = 1000
+	DefaultMaxDisplays = 1000
 	// MaxDisplay is the theoretical max display value which
 	// X Clients and serverwill be able to parse into a unix socket.
 	MaxDisplayNumber = math.MaxInt32

lib/sshutils/x11/forward.go

@@ -137,11 +137,11 @@ func ServeChannelRequests(ctx context.Context, clt *ssh.Client, handler x11Chann
 // ServerConfig is a server configuration for X11 forwarding
 type ServerConfig struct {
 	// Enabled controls whether X11 forwarding requests can be granted by the server.
-	Enabled bool `yaml:"enabled"`
+	Enabled bool
 	// DisplayOffset tells the server what X11 display number to start from when
 	// searching for an open X11 unix socket for XServer proxies.
-	DisplayOffset int `yaml:"display_offset,omitempty"`
+	DisplayOffset int
 	// MaxDisplay tells the server what X11 display number to stop at when
 	// searching for an open X11 unix socket for XServer proxies.
-	MaxDisplay int `yaml:"max_displays,omitempty"`
+	MaxDisplay int
 }

lib/sshutils/x11/forward_test.go

@@ -56,7 +56,7 @@ func TestForward(t *testing.T) {
 	}()
 
 	// Create a fake XServer proxy just like the one in sshserver.
-	sl, serverDisplay, err := OpenNewXServerListener(DefaultDisplayOffset, DefaultMaxDisplay, 0)
+	sl, serverDisplay, err := OpenNewXServerListener(DefaultDisplayOffset, DefaultMaxDisplays, 0)
 	require.NoError(t, err)
 	t.Cleanup(func() {
 		require.NoError(t, sl.Close())