From 754896b47381f0ed238976377dfffbd839e9fdf3 Mon Sep 17 00:00:00 2001 From: bolkedebruin Date: Thu, 30 Apr 2026 11:58:47 +0200 Subject: [PATCH] Add type-safe URL override mechanism for RDP options (#182) Introduce a generic, allow-list-gated way for the /connect endpoint to accept RDP setting overrides via URL query parameters. Operators opt in via client.rdpoverridablekeys; absent that allow-list, URL-driven overrides are rejected with 400. Override values are routed through rdp.Builder.ApplyOverrides, which matches query keys against the rdp struct tags of RdpSettings and validates per Go field type. Overridden fields are tracked so explicit values always serialize even when they equal the field default. The override pass runs before authoritative server-controlled fields (gateway address, access token, full address, username) so those always win. This replaces the per-option string-splice approach considered in #181: multimon now works via ?usemultimon=1 against an operator allow-list containing "use multimon", and any other RDP key follows the same path without bespoke handler code. --- README.md | 39 ++++++ cmd/rdpgw/config/configuration.go | 5 + cmd/rdpgw/main.go | 7 +- cmd/rdpgw/rdp/rdp.go | 108 ++++++++++++++++- cmd/rdpgw/rdp/rdp_test.go | 161 +++++++++++++++++++++++++ cmd/rdpgw/web/web.go | 17 +++ cmd/rdpgw/web/web_test.go | 194 ++++++++++++++++++++++++++++++ 7 files changed, 525 insertions(+), 6 deletions(-) diff --git a/README.md b/README.md index 4e1e77b..47a7e4c 100644 --- a/README.md +++ b/README.md @@ -287,6 +287,45 @@ and RDP file will download to your desktop. This file can be opened by one of the remote desktop clients and it will try to connect to the gateway and desktop host behind it. +### Overriding RDP options from the URL +The `/connect` endpoint can apply caller-supplied RDP setting overrides from +URL query parameters when (and only when) the operator explicitly opts in. +This lets users vary settings such as multi-monitor, audio mode or +clipboard redirection without maintaining separate RDP templates. + +The query parameter name is the underlying RDP key with whitespace removed +and lowercased. For example: + +| RDP key (in the file) | Query parameter | +|-----------------------|-----------------| +| `use multimon` | `usemultimon` | +| `audiomode` | `audiomode` | +| `redirectclipboard` | `redirectclipboard` | +| `screen mode id` | `screenmodeid` | + +Values are validated against the field type: booleans accept `0`/`1` or +`true`/`false`, integers must parse as base-10 numbers, strings are taken +verbatim. Unknown query parameters are ignored so the existing `host`, +etc. continue to work. + +To enable, list the keys you want to expose under `client.rdpoverridablekeys` +in your YAML configuration (or via the `RDPGW_CLIENT__RDPOVERRIDABLEKEYS` +environment variable as a space-separated list): + +```yaml +client: + rdpoverridablekeys: + - use multimon + - audiomode +``` + +With the snippet above, `https://your-gateway/connect?usemultimon=1` adds +`use multimon:i:1` to the generated RDP file. Keys that are not on the +allow-list are rejected with `400 Bad Request`. Authoritative settings +controlled by the gateway (gateway address, access token, full address, +username) always win over URL parameters even if an operator adds those +keys to the allow-list — but adding sensitive keys is still discouraged. + ## Integration The gateway exposes an endpoint for the verification of user tokens at https://yourserver/tokeninfo . The query parameter is 'access_token' so diff --git a/cmd/rdpgw/config/configuration.go b/cmd/rdpgw/config/configuration.go index b289072..d7c461d 100644 --- a/cmd/rdpgw/config/configuration.go +++ b/cmd/rdpgw/config/configuration.go @@ -108,6 +108,11 @@ type ClientConfig struct { NoUsername bool `koanf:"nousername"` SigningCert string `koanf:"signingcert"` SigningKey string `koanf:"signingkey"` + // RdpOverridableKeys is the operator allow-list of RDP setting keys that + // the /connect endpoint may override from URL query parameters. Empty + // disables URL-based overrides. Entries are normalized (lowercase, no + // whitespace), so "use multimon" and "usemultimon" are equivalent. + RdpOverridableKeys []string `koanf:"rdpoverridablekeys"` } func ToCamel(s string) string { diff --git a/cmd/rdpgw/main.go b/cmd/rdpgw/main.go index f5fffab..25384c8 100644 --- a/cmd/rdpgw/main.go +++ b/cmd/rdpgw/main.go @@ -109,9 +109,10 @@ func main() { Hosts: conf.Server.Hosts, HostSelection: conf.Server.HostSelection, RdpOpts: web.RdpOpts{ - UsernameTemplate: conf.Client.UsernameTemplate, - SplitUserDomain: conf.Client.SplitUserDomain, - NoUsername: conf.Client.NoUsername, + UsernameTemplate: conf.Client.UsernameTemplate, + SplitUserDomain: conf.Client.SplitUserDomain, + NoUsername: conf.Client.NoUsername, + OverridableRdpKeys: conf.Client.RdpOverridableKeys, }, GatewayAddress: url, TemplateFile: conf.Client.Defaults, diff --git a/cmd/rdpgw/rdp/rdp.go b/cmd/rdpgw/rdp/rdp.go index 7c69fe6..ce7872b 100644 --- a/cmd/rdpgw/rdp/rdp.go +++ b/cmd/rdpgw/rdp/rdp.go @@ -95,6 +95,7 @@ type RdpSettings struct { type Builder struct { Settings RdpSettings Metadata mapstructure.Metadata + forced map[string]struct{} } func NewBuilder() *Builder { @@ -137,15 +138,92 @@ func NewBuilderFromFile(filename string) (*Builder, error) { func (rb *Builder) String() string { var sb strings.Builder - addStructToString(rb.Settings, rb.Metadata, &sb) + rb.addStructToString(rb.Settings, &sb) return sb.String() } -func addStructToString(st interface{}, metadata mapstructure.Metadata, sb *strings.Builder) { +// NormalizeRdpKey returns the canonical, URL-friendly form of an rdp tag +// (lowercase, no whitespace). Used to look up fields by query parameter name +// and to compare entries in the operator-supplied allow-list. For example, +// "use multimon", "Use Multimon" and "usemultimon" all normalize to +// "usemultimon". +func NormalizeRdpKey(s string) string { + s = strings.ToLower(strings.TrimSpace(s)) + return strings.ReplaceAll(s, " ", "") +} + +// ApplyOverrides applies a set of caller-supplied RDP option overrides onto +// the Builder's settings. Each input key is normalized via NormalizeRdpKey and +// matched against the rdp struct tags of RdpSettings. Keys that do not match +// any field are ignored (so passing the entire request query is safe). Keys +// that match a field but are not present in allowed return an error: the +// allow-list is opt-in by design and empty by default. +// +// Values are validated against the field's Go type: bools accept 0/1 or +// true/false; ints must parse as base-10 integers; strings are taken +// verbatim. Overridden fields are tracked so they always serialize, even when +// the new value equals the field's default. +func (rb *Builder) ApplyOverrides(values map[string][]string, allowed []string) error { + if len(values) == 0 { + return nil + } + + allow := make(map[string]struct{}, len(allowed)) + for _, k := range allowed { + allow[NormalizeRdpKey(k)] = struct{}{} + } + + s := structs.New(&rb.Settings) + byKey := make(map[string]*structs.Field, len(s.Fields())) + for _, f := range s.Fields() { + tag := f.Tag("rdp") + if tag == "" { + continue + } + byKey[NormalizeRdpKey(tag)] = f + } + + if rb.forced == nil { + rb.forced = make(map[string]struct{}) + } + + for k, vs := range values { + nk := NormalizeRdpKey(k) + f, ok := byKey[nk] + if !ok { + continue + } + if _, ok := allow[nk]; !ok { + return fmt.Errorf("rdp option %q is not allowed to be overridden", k) + } + if len(vs) == 0 { + continue + } + v := strings.TrimSpace(vs[0]) + if v == "" { + return fmt.Errorf("rdp option %q has empty value", k) + } + if err := setRdpFieldFromString(f, v); err != nil { + return fmt.Errorf("invalid value for rdp option %q: %w", k, err) + } + rb.forced[f.Name()] = struct{}{} + } + return nil +} + +func (rb *Builder) isForced(f *structs.Field) bool { + if rb.forced == nil { + return false + } + _, ok := rb.forced[f.Name()] + return ok +} + +func (rb *Builder) addStructToString(st interface{}, sb *strings.Builder) { s := structs.New(st) for _, f := range s.Fields() { - if isZero(f) && !isSet(f, metadata) { + if isZero(f) && !isSet(f, rb.Metadata) && !rb.isForced(f) { continue } sb.WriteString(f.Tag("rdp")) @@ -251,3 +329,27 @@ func setVariable(f *structs.Field, v string) error { return errors.New("invalid field type") } } + +func setRdpFieldFromString(f *structs.Field, v string) error { + switch f.Kind() { + case reflect.String: + return f.Set(v) + case reflect.Int: + i, err := strconv.Atoi(v) + if err != nil { + return fmt.Errorf("expected integer, got %q", v) + } + return f.Set(i) + case reflect.Bool: + switch strings.ToLower(v) { + case "0", "false": + return f.Set(false) + case "1", "true": + return f.Set(true) + default: + return fmt.Errorf("expected 0/1 or true/false, got %q", v) + } + default: + return fmt.Errorf("unsupported rdp field kind %s", f.Kind()) + } +} diff --git a/cmd/rdpgw/rdp/rdp_test.go b/cmd/rdpgw/rdp/rdp_test.go index f7d0ce5..5c94b58 100644 --- a/cmd/rdpgw/rdp/rdp_test.go +++ b/cmd/rdpgw/rdp/rdp_test.go @@ -44,3 +44,164 @@ func TestLoadFile(t *testing.T) { t.Fatalf("LoadFile failed: %v", err) } } + +func TestNormalizeRdpKey(t *testing.T) { + cases := map[string]string{ + "use multimon": "usemultimon", + "USE MULTIMON": "usemultimon", + " Use Multimon ": "usemultimon", + "audiomode": "audiomode", + "screen mode id": "screenmodeid", + } + for in, want := range cases { + if got := NormalizeRdpKey(in); got != want { + t.Errorf("NormalizeRdpKey(%q) = %q, want %q", in, got, want) + } + } +} + +func TestApplyOverrides_AllowedBoolApplies(t *testing.T) { + b := NewBuilder() + err := b.ApplyOverrides( + map[string][]string{"usemultimon": {"1"}}, + []string{"use multimon"}, + ) + if err != nil { + t.Fatalf("ApplyOverrides returned error: %v", err) + } + if !b.Settings.UseMultimon { + t.Errorf("UseMultimon = false, want true") + } + if !strings.Contains(b.String(), "use multimon:i:1"+CRLF) { + t.Errorf("rendered file does not contain use multimon:i:1, got:\n%s", b.String()) + } +} + +func TestApplyOverrides_AllowListNormalizes(t *testing.T) { + // allow-list given with the human/rdp form; query uses URL-friendly form. + b := NewBuilder() + err := b.ApplyOverrides( + map[string][]string{"USEMULTIMON": {"1"}}, + []string{"Use Multimon"}, + ) + if err != nil { + t.Fatalf("ApplyOverrides returned error: %v", err) + } + if !strings.Contains(b.String(), "use multimon:i:1"+CRLF) { + t.Errorf("expected normalized match to apply override, got:\n%s", b.String()) + } +} + +func TestApplyOverrides_DefaultValueStillSerializes(t *testing.T) { + // UseMultimon defaults to false; the bare struct would skip serialization. + // An explicit override of "0" must still emit `use multimon:i:0` so that + // the operator's intent is signaled to the client. + b := NewBuilder() + err := b.ApplyOverrides( + map[string][]string{"usemultimon": {"0"}}, + []string{"use multimon"}, + ) + if err != nil { + t.Fatalf("ApplyOverrides returned error: %v", err) + } + if !strings.Contains(b.String(), "use multimon:i:0"+CRLF) { + t.Errorf("expected use multimon:i:0 to render despite matching default, got:\n%s", b.String()) + } +} + +func TestApplyOverrides_RejectedWhenNotInAllowList(t *testing.T) { + b := NewBuilder() + err := b.ApplyOverrides( + map[string][]string{"usemultimon": {"1"}}, + nil, // empty allow-list + ) + if err == nil { + t.Fatalf("ApplyOverrides accepted a key with empty allow-list") + } +} + +func TestApplyOverrides_UnknownKeysIgnored(t *testing.T) { + // Query strings carry unrelated params (host=, etc.); these must not + // trip an error. + b := NewBuilder() + err := b.ApplyOverrides( + map[string][]string{"host": {"example.com"}, "totally-bogus": {"x"}}, + []string{"use multimon"}, + ) + if err != nil { + t.Errorf("ApplyOverrides should ignore unknown keys, got error: %v", err) + } +} + +func TestApplyOverrides_BoolValidation(t *testing.T) { + cases := []struct { + v string + wantErr bool + }{ + {"0", false}, {"1", false}, + {"true", false}, {"false", false}, + {"TRUE", false}, + {"yes", true}, {"2", true}, {"", true}, {"abc", true}, + } + for _, c := range cases { + b := NewBuilder() + err := b.ApplyOverrides( + map[string][]string{"usemultimon": {c.v}}, + []string{"use multimon"}, + ) + if (err != nil) != c.wantErr { + t.Errorf("value %q: got err=%v, wantErr=%v", c.v, err, c.wantErr) + } + } +} + +func TestApplyOverrides_IntValidation(t *testing.T) { + b := NewBuilder() + if err := b.ApplyOverrides( + map[string][]string{"audiomode": {"2"}}, + []string{"audiomode"}, + ); err != nil { + t.Fatalf("ApplyOverrides returned error: %v", err) + } + if b.Settings.AudioMode != 2 { + t.Errorf("AudioMode = %d, want 2", b.Settings.AudioMode) + } + b2 := NewBuilder() + if err := b2.ApplyOverrides( + map[string][]string{"audiomode": {"hello"}}, + []string{"audiomode"}, + ); err == nil { + t.Errorf("expected int parse error for value 'hello'") + } +} + +func TestApplyOverrides_StringField(t *testing.T) { + b := NewBuilder() + if err := b.ApplyOverrides( + map[string][]string{"alternateshell": {"explorer.exe"}}, + []string{"alternate shell"}, + ); err != nil { + t.Fatalf("ApplyOverrides returned error: %v", err) + } + if !strings.Contains(b.String(), "alternate shell:s:explorer.exe"+CRLF) { + t.Errorf("expected alternate shell to be set, got:\n%s", b.String()) + } +} + +func TestApplyOverrides_EmptyValueRejected(t *testing.T) { + b := NewBuilder() + err := b.ApplyOverrides( + map[string][]string{"usemultimon": {""}}, + []string{"use multimon"}, + ) + if err == nil { + t.Errorf("expected error for empty value") + } +} + +func TestApplyOverrides_NilValuesNoOp(t *testing.T) { + b := NewBuilder() + if err := b.ApplyOverrides(nil, []string{"use multimon"}); err != nil { + t.Errorf("ApplyOverrides(nil) should be a no-op, got: %v", err) + } +} diff --git a/cmd/rdpgw/web/web.go b/cmd/rdpgw/web/web.go index 9640680..290b290 100644 --- a/cmd/rdpgw/web/web.go +++ b/cmd/rdpgw/web/web.go @@ -72,6 +72,13 @@ type RdpOpts struct { UsernameTemplate string SplitUserDomain bool NoUsername bool + // OverridableRdpKeys is the operator-supplied allow-list of RDP setting + // keys that the /connect endpoint may override from URL query parameters. + // Empty (the default) disables URL-based RDP overrides entirely. Each + // entry is matched against the rdp struct tag of an RdpSettings field + // after normalization (lowercase, no whitespace), so "use multimon", + // "Use Multimon" and "usemultimon" are equivalent. + OverridableRdpKeys []string } type Handler struct { @@ -484,6 +491,16 @@ func (h *Handler) HandleDownload(w http.ResponseWriter, r *http.Request) { } } + // Apply URL-driven RDP option overrides (e.g. ?usemultimon=1) before + // the server-controlled fields below, so authoritative gateway/auth + // settings always win regardless of what an operator put on the + // allow-list. + if err := d.ApplyOverrides(r.URL.Query(), h.rdpOpts.OverridableRdpKeys); err != nil { + log.Printf("rejected rdp override for user %s: %s", id.UserName(), err) + http.Error(w, err.Error(), http.StatusBadRequest) + return + } + if !h.rdpOpts.NoUsername { d.Settings.Username = render if domain != "" { diff --git a/cmd/rdpgw/web/web_test.go b/cmd/rdpgw/web/web_test.go index a57aa96..2e29394 100644 --- a/cmd/rdpgw/web/web_test.go +++ b/cmd/rdpgw/web/web_test.go @@ -182,6 +182,200 @@ func TestHandler_HandleDownload(t *testing.T) { } +func TestHandler_HandleDownload_RdpOverrides(t *testing.T) { + tests := []struct { + name string + query string + allow []string + template string + wantStatus int + wantContain []string // substrings expected in body when status==200 + wantMissing []string + }{ + { + name: "allowed bool override applies", + query: "?usemultimon=1", + allow: []string{"use multimon"}, + wantStatus: http.StatusOK, + wantContain: []string{"use multimon:i:1\r\n"}, + }, + { + name: "allowed bool override emits default value explicitly", + query: "?usemultimon=0", + allow: []string{"use multimon"}, + wantStatus: http.StatusOK, + wantContain: []string{"use multimon:i:0\r\n"}, + }, + { + name: "override beats template", + query: "?usemultimon=0", + allow: []string{"use multimon"}, + template: "use multimon:i:1\r\n", + wantStatus: http.StatusOK, + wantContain: []string{"use multimon:i:0\r\n"}, + wantMissing: []string{"use multimon:i:1\r\n"}, + }, + { + name: "no allow list disables overrides", + query: "?usemultimon=1", + wantStatus: http.StatusBadRequest, + }, + { + name: "key not in allow list is rejected", + query: "?audiomode=2", + allow: []string{"use multimon"}, + wantStatus: http.StatusBadRequest, + }, + { + name: "invalid bool value rejected", + query: "?usemultimon=hello", + allow: []string{"use multimon"}, + wantStatus: http.StatusBadRequest, + }, + { + name: "invalid int value rejected", + query: "?audiomode=loud", + allow: []string{"audiomode"}, + wantStatus: http.StatusBadRequest, + }, + { + name: "allow list normalizes (caller uses url-friendly form)", + query: "?usemultimon=1", + allow: []string{"USE MULTIMON"}, + wantStatus: http.StatusOK, + wantContain: []string{"use multimon:i:1\r\n"}, + }, + { + name: "unrelated query params are ignored", + query: "?host=10.0.0.1:3389&usemultimon=1", + allow: []string{"use multimon"}, + wantStatus: http.StatusOK, + wantContain: []string{"use multimon:i:1\r\n"}, + }, + { + name: "string field override applies", + query: "?alternateshell=explorer.exe", + allow: []string{"alternate shell"}, + wantStatus: http.StatusOK, + wantContain: []string{"alternate shell:s:explorer.exe\r\n"}, + }, + { + name: "url override cannot escape authoritative server fields", + query: "?username=evil@attacker", + allow: []string{"username"}, // operator footgun: still must not leak past server set + wantStatus: http.StatusOK, + wantContain: []string{"username:s:" + testuser + "\r\n"}, + wantMissing: []string{"username:s:evil@attacker\r\n"}, + }, + } + + u, _ := url.Parse(gateway) + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + req, err := http.NewRequest("GET", "/connect"+tt.query, nil) + if err != nil { + t.Fatal(err) + } + rr := httptest.NewRecorder() + id := identity.NewUser() + id.SetUserName(testuser) + id.SetAuthenticated(true) + req = identity.AddToRequestCtx(id, req) + + var templateFile string + if tt.template != "" { + f, err := os.CreateTemp("", "rdp") + if err != nil { + t.Fatal(err) + } + defer os.Remove(f.Name()) + if _, err := f.WriteString(tt.template); err != nil { + t.Fatal(err) + } + if err := f.Close(); err != nil { + t.Fatal(err) + } + templateFile = f.Name() + } + + c := Config{ + HostSelection: "roundrobin", + Hosts: hosts, + PAATokenGenerator: paaTokenMock, + GatewayAddress: u, + RdpOpts: RdpOpts{OverridableRdpKeys: tt.allow}, + TemplateFile: templateFile, + } + h := c.NewHandler() + http.HandlerFunc(h.HandleDownload).ServeHTTP(rr, req) + + if rr.Code != tt.wantStatus { + t.Fatalf("status: got %d, want %d (body=%q)", rr.Code, tt.wantStatus, rr.Body.String()) + } + if rr.Code != http.StatusOK { + return + } + body := rr.Body.String() + for _, s := range tt.wantContain { + if !strings.Contains(body, s) { + t.Errorf("body missing %q\nbody=\n%s", s, body) + } + } + for _, s := range tt.wantMissing { + if strings.Contains(body, s) { + t.Errorf("body contains forbidden %q\nbody=\n%s", s, body) + } + } + }) + } +} + +func TestHandler_HandleSignedDownload_RdpOverrideApplies(t *testing.T) { + // The override must take effect on the signed path too: ApplyOverrides + // runs on the same Builder used to render the signed content. + req, err := http.NewRequest("GET", "/connect?usemultimon=1", nil) + if err != nil { + t.Fatal(err) + } + rr := httptest.NewRecorder() + id := identity.NewUser() + id.SetUserName(testuser) + id.SetAuthenticated(true) + req = identity.AddToRequestCtx(id, req) + + u, _ := url.Parse(gateway) + c := Config{ + HostSelection: "roundrobin", + Hosts: hosts, + PAATokenGenerator: paaTokenMock, + GatewayAddress: u, + RdpOpts: RdpOpts{OverridableRdpKeys: []string{"use multimon"}}, + } + h := c.NewHandler() + + fs := afero.NewMemMapFs() + if err := genKeypair(fs); err != nil { + t.Fatalf("could not generate key pair: %s", err) + } + signer, err := rdpsign.New("test.crt", "test.key", rdpsign.WithFs(fs)) + if err != nil { + t.Fatalf("could not create signer: %s", err) + } + h.rdpSigner = signer + + http.HandlerFunc(h.HandleDownload).ServeHTTP(rr, req) + if rr.Code != http.StatusOK { + t.Fatalf("status: got %d, want 200 (body=%q)", rr.Code, rr.Body.String()) + } + body := rr.Body.String() + if !strings.Contains(body, "use multimon:i:1\r\n") { + t.Errorf("signed body missing use multimon:i:1\nbody=\n%s", body) + } + if !strings.Contains(body, "signature:s:") { + t.Errorf("signed body missing signature\nbody=\n%s", body) + } +} + func TestHandler_HandleSignedDownload(t *testing.T) { req, err := http.NewRequest("GET", "/connect", nil) if err != nil {