The dev container image generated a TLS keypair at build time and
shipped it inside the image, so every pull of the same image tag was
serving the same private key. The entrypoint also reverted to USER 0
to support a dead `createusers.txt` loop and a `chmod u+s` that was a
no-op (set on a binary owned by 1001). Net result was that any RCE
in the gateway landed as root and the wire-trust posture relied on a
shared private key.
Stop generating the cert at build time: the runtime image now carries
openssl and the entrypoint mints an ephemeral self-signed cert at
first start when no cert is mounted at the configured path. Each
container instance gets its own key. Drop USER 0 entirely; the
entrypoint runs as 1001 throughout. Prune the dead createusers loop
and the `chmod u+s`.
Separately, the README and the dev compose files publish a small set
of literal placeholder values for SessionKey, SessionEncryptionKey,
and the various Token*Key fields. Operators copy-paste these into
real deployments. Refuse to start when any of those literals appear
in the corresponding config field.
The auth daemon's gRPC socket was world-writable and accepted any
local UID that could connect to it. On a multi-tenant host any user
on the box could speak the gRPC API and run an arbitrary username/
password through PAM -- effectively an unauthenticated PAM oracle.
Create the socket with mode 0660 (Umask(0117)) and gate Accept on
SO_PEERCRED: only the daemon's own UID is allowed by default, plus
any operator-supplied --allow-uid / --allow-gid. Privilege-separated
deployments (rdpgw and rdpgw-auth as different users) need to list
the gateway's UID, or share a group; the existing path otherwise
would have been permissive.
The peer-credentials check is Linux-only; the non-Linux build keeps
the listener as-is and logs a warning, since rdpgw-auth itself
requires libpam and is effectively Linux-only in practice.
EnrichContext used to copy the first X-Forwarded-For entry into the
request identity unconditionally. The resulting AttrClientIp drives
client-IP comparisons later in the gateway-access flow, and a direct
caller could set XFF to anything they liked.
Add a small package-level allow-list:
* InitTrustedProxies(cidrs) parses operator-supplied CIDRs once at
startup. A bad CIDR is fatal, an empty list disables XFF entirely.
* EnrichContext takes the client IP from r.RemoteAddr (host portion)
and only swaps in the first X-Forwarded-For entry when r.RemoteAddr
itself sits in a trusted-proxy CIDR. AttrProxies is set from the
remaining XFF entries on the same condition.
Wire Server.TrustedProxies through configuration.go to web.
The `roundrobin`, `signed`, and `unsigned` host-selection modes route
requests against an operator-curated `Server.Hosts` list. The `any`
mode does not -- it forwards to whatever `?host=` value the request
carries, which makes the gateway usable as a generic TCP relay
against whatever the gateway can reach (loopback, RFC1918, link-local,
the cloud metadata service, arbitrary high-numbered ports on public
hosts).
Add a small destination policy applied only in `any` mode:
* Reject hosts that resolve to loopback, RFC1918, IPv6 ULA,
link-local, unspecified, or multicast addresses. Operators can opt
back in with `Server.AllowPrivateDestinations: true`.
* Restrict the destination port to `Server.AllowedDestinationPorts`
(default {3389}).
The other host-selection modes are unaffected -- the operator already
curates their hosts list.
The DestinationPolicy zero value is the secure default, so direct
&Handler{} constructions in tests still get the expected behavior.
DNS names are resolved at validation time and every returned address
is checked.
CheckPAACookie used to call OIDCProvider.UserInfo with a copy of the
IdP access token embedded in the cookie itself, just to recover the
Subject. The gateway already signs Subject at issue time, so the
roundtrip is redundant -- and copying the IdP access token into the
.rdp file makes the cookie much larger and ties gateway availability
to IdP availability.
Drop the AccessToken field from customClaims and from GeneratePAAToken
(no other consumer exists), set tunnel.User.SetUserName from the
signed Subject claim, and remove the UserInfo call from
CheckPAACookie.
Add Audience: "rdpgw-paa" to standard claims at issue and
AnyAudience to the validation expectation so a JWS minted with the
same signing key for any other purpose can't be presented as a PAA.
For a representative RS256 access token the cookie shrinks from
~961 bytes to ~259 bytes.
Adds tests:
- TestPAACookieDoesNotEmbedAccessToken
- TestPAACookieHasAudienceClaim
- TestCheckPAACookieIsSelfContained
Three independent corrections to functions that crash the worker on
malformed or concurrent input:
* protocol/common.readHeader: validate the on-wire size field before
slicing data[8:size]. Introduce a named headerLen constant and reject
declared sizes outside [headerLen, headerLen+maxFragmentSize], so a
size of 0..7 (which previously panicked with slice bounds out of
range) and an oversized size both surface as an error instead.
* protocol/track: serialize access to the global Connections map with
a sync.RWMutex. Concurrent RegisterTunnel/RemoveTunnel calls would
otherwise be caught by the runtime as a fatal `concurrent map
writes`. Also correct the inverted condition in Disconnect (the
previous code dereferenced a nil Monitor when the id was missing and
returned "does not exist" when the id was present).
* web/ntlm.getAuthPayload: switch from authorisationEncoded[0:5] /
[0:10] to strings.HasPrefix so an Authorization header shorter than
the prefixes returns an error instead of a slice-bounds panic.
Adds:
- TestReadHeaderRejectsUndersizedSize (sizes 0/1/2/7).
- TestTunnelTrackerConcurrent (200 goroutine pairs).
- TestDisconnectKnownConnection / TestDisconnectMissingConnectionDoesNotPanic.
- TestGetAuthPayloadShortHeader (missing/empty/3/4/5/9 character values).
HandleGatewayProtocol caches a *Tunnel keyed on the client-supplied
Rdg-Connection-Id header so the two halves of a session (RDG_OUT_DATA
and RDG_IN_DATA) can rendezvous on a single record. The cache hit path
previously reused the tunnel without checking who was making the
follow-up request.
Add tunnelOwnerMatches(t, id) to compare the cached tunnel's
UserName() and AttrClientIp against the request identity. On
mismatch, refuse with 401 instead of attaching the new request to the
existing tunnel. The helper is conservative: nil tunnel/user/identity,
empty username, or missing client-IP attribute all fail closed.
The legitimate case (the same client returns to attach its second
half-channel to its own first half) is unchanged.
Adds TestTunnelOwnershipEnforced.
Header auth previously trusted any request that carried the configured
user header, with no check that the request came from a known upstream
proxy. Anyone reaching rdpgw directly could mint an authenticated
session as any user by setting the header.
Add `Header.TrustedProxies` (CIDR list) checked against `RemoteAddr`
before reading the user header. Refuse the request with 401 when the
remote is outside the allow-list. Refuse to start when header
authentication is enabled but `Header.TrustedProxies` is empty.
The CIDR allow-list gates the immediate upstream only; operators must
still configure their proxy to strip duplicate inbound copies of the
user header so a client cannot smuggle one through the trusted hop.
Documented in docs/header-authentication.md.
TestHeaderAuthRequiresTrustedProxy is a 3-case table covering: no
allow-list (refused), outside allow-list (refused), inside allow-list
(allowed). Existing TestHeaderAuthenticated cases updated to declare
trust for httptest.NewRequest's default RemoteAddr (192.0.2.1).
The .rdp file format is line-delimited (key:type:value\r\n), and an
unfiltered \r, \n, or NUL inside a string field is reinterpreted by RDP
clients as a directive boundary. A username flowing in from any of OIDC,
header auth, NTLM, or the URL-override path could therefore inject
arbitrary additional directives — e.g. `alternate shell:s:cmd.exe` — and
when RDP signing is enabled the malicious payload is signed as
authentic, producing a one-click client-side RCE on every user who
opens the file.
Strip bytes < 0x20 and 0x7F at the renderer chokepoint
(addStructToString), so every source path — caller, template file,
ApplyOverrides, anything future — passes through the same filter.
Legitimate values (usernames, base64url tokens, hostnames) contain no
such bytes, so the filter is a no-op for normal input. Stripping is
logged so operators can spot rejected input.
Adds TestStringFieldBoundaryHygiene covering CRLF in username, domain
and full address; bare LF in alternate shell; and embedded NUL.
Co-authored-by: Bolke de Bruin <bolke.debruin@metyis.com>
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.
* Fix protocol fallback
* Use token-aware header matching, drop dead fallback log
Connection is a list header; plain equality misses legitimate
"keep-alive, Upgrade" clients. Switch to case-insensitive token
matching for the Connection/Upgrade checks.
Remove the "falling back to old protocol" log on upgrade failure.
upgrader.Upgrade commits an HTTP error response before returning, so
the follow-up legacy path cannot produce a coherent reply. The real
fallback happens at the header pre-check for clients and reverse
proxies that strip the upgrade tokens.
Add tests for the header helper and RDGOUT routing.
Running the gateway as non-tls, but using an external TLS gateway in
kubernetes+istio, I determined that the istio TLS gateway would join
messages frames into a single TCP packet. The packet read code assumed
that a single packet is a message. This is not the case for a TCP
stream, since you don't know how the frames are segmented via proxies,
etc.
The fix turned out more complex that I would have liked, but added a
number of unit tests to cover all the corner cases. Likely fragmentation
was not working correctly as well, as there was some cases that were
previously not handled.
Note that this might address issue #126 as well.
* Support for NTLM authentication added
To support NTLM authentication, a database is added as an authentication source.
Currently, only the configuration file is supported as a database.
Database authentication supports Basic and NTLM authentication protcols.
ServerConfig.BasicAuthEnabled renamed to LocalEnabled as Basic auth can be used with NTLM or Local.
