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.
The clim `preferred_username` is optional in Azure AD. Although is listed as preferred, in some enterprise environment it's not possible to add this additional claim. `unique_name` and `upn` are legacy alternatives
If using the filesystem storage provider
for session store it can be set than a larger value than 4kb
as it is not tied to the restriction of a cookie anymore.
