The encrypted drive for RDP file transfer is the only question this installer
asks, and the only way to answer it ahead of time was an environment variable
that --help did not mention. So the one thing a scripted install needs to know
was the one thing the usage text left out, next to --no-deps and --no-tls,
which are flags precisely because skipping a step is a normal thing to want.
--no-drive is an alias for RUSTGUAC_DRIVE_SETUP=no rather than a second switch
reaching the same place, so there is one variable to read when the answer comes
out wrong. Setting it during argument parsing also lets it win over an
inherited RUSTGUAC_DRIVE_SETUP=yes, which is the precedence an explicit flag
should have.
--help now lists all five drive variables and, more usefully, the three ways
the prompt already answers itself: an existing container, a missing cryptsetup,
or stdin that is not a terminal. Someone seeing the prompt on one machine and
not another is looking at one of those rather than at a bug, and someone not
seeing it where they expected it is looking at the first.
It also points at PREFIX/guacd.env, since the neighbouring trap is setting
something directly in a unit file the next install rewrites.
(cherry picked from commit 8d2f2c000a62d5125e71f2e5395c9edef08ea81a)
Three failures, all of which cost time during a day of deploys and all of
which present as something other than what they are.
The binary could not be overwritten while the service ran. cp reported "Text
file busy" and left the old binary in place, after the script had already said
it built the new one -- a deploy that changed nothing and looked like a deploy
that worked. It now stops the service first and restarts it afterwards, but
only when it was already running: a first install still leaves it down so an
admin can be created before anything is reachable, and it checks the service
actually came back rather than assuming.
The static assets were copied over the top, so nothing ever left. That is not
just clutter, because ServeDir is rooted at the install's static directory and
whatever sits there is reachable: a stray nested copy of the entire tree lived
at static/static from July until September 2026, serving two-month-old HTML
and JavaScript at /static/... to anyone who asked, and three files from an
abandoned branch outlived the branch the same way. Mirrored now, with rsync
where it exists and a replace where it does not, and copying `static/.`
instead of `static/*` -- the latter being one slip from `cp -r static dest/`,
which nests the tree rather than filling the directory, and is how that copy
got there.
And the optional drive prompt blocked for ever without a terminal. It sits
after the binary and the assets are installed, so the hang looked like a
completed deploy whose service simply had not come back. It is skipped when
stdin is not a tty, and says how to answer it from a script.
(cherry picked from commit b24d3c1b80c152a20558301a14dfeb3edd7a5a1a)
The branded HTML handlers returned `Html(...)` and nothing else: no ETag,
no Last-Modified, no Cache-Control. With no freshness information at all
Chrome caches heuristically and does not revalidate, so a restart served
the old page for hours -- incognito was the only reliable way to see a
change. The static assets had the same gap from the other side: ServeDir
already sent ETag and Last-Modified and honoured both, it was just never
asked, so a rebuilt /guac/*.js was invisible too.
The pages are branded once at startup into a map, so the tag is computed
there as well -- one sha256 over each, against a rewrite already walking
the same strings. A request is then a string compare, and an unchanged
page costs a 304 rather than 136K of client.html or 255K of
connections.html. `no-cache` is the revalidate directive, not
do-not-store: the browser keeps the body and asks. Tags are
content-derived, not boot-derived, so a restart that changes nothing
still answers 304.
Revalidating every asset is correct but not free, and this is reached
over a WAN. The same startup pass therefore rewrites the 36 script tags
and the stylesheet to carry a hash of the bytes on disk, and a URL
bearing one is served with a year and `immutable`. The round trip it
saves is a single one, since the browser-to-proxy leg is HTTP/2 and the
conditional requests go out together -- but client.html reloads on every
session launch and relaunch, so it is a round trip paid exactly when the
link is worst. Only a 2xx or 304 earns the long directive: a 404 under a
versioned URL is a deployment that has gone wrong, and pinning it in the
cache until next year would make it outlive its cause.
Query strings rather than hashed filenames because there is no build step
to rename anything and the rewrite already existed; the old caution about
proxies declining to cache them has not been true in many years.
The cost is that editing an asset now needs a restart where a reload was
enough, since the URL and the bytes it names are produced together --
hence RUSTGUAC_NO_ASSET_VERSIONING, which leaves everything revalidating.
Verified against a running instance: matching and weak If-None-Match give
304 on the HTML, a stale tag gives 200, a versioned asset is immutable on
both its 200 and its 304, a bare one and a 404 are not, the kill switch
emits bare URLs, and API responses are untouched -- the middleware is on
the ServeDir fallback only.
(cherry picked from commit 966aba765c8a8db043b5f62603abdb6a18dd0eb5)
Unrelated fixes carried by the fork: session times and date filters in local
time, no local resolution of the target when a jump host is set, no resize
request that changes no pixels, and rustfmt on three hunks left unformatted.
(cherry picked from commit d9b361db6eb216d871f3fdac41c423a586e2d559)
Upstream changes on pick: the client.html resize skip is kept but
without the native-resolution sizing (displayScale/deviceSize), which
belongs to the HiDPI commit not taken here, so sizes stay in CSS pixels.
The oidc.rs hunk was formatting only and is superseded.
The installer rewrites both unit files on every run, so anything set directly in
them -- or through "systemctl edit" -- is lost at the next install. guacd's
environment moves to $PREFIX/guacd.env, created only when absent and never
overwritten, and the unit reads it through EnvironmentFile. That is where
GUACD_LOG_LEVEL belongs, since trace is what prints per-surface-command codec
detail and debug what prints it per frame.
Systemd drop-ins are the other half of the same trap. They merge with the unit
just written and take precedence over it, but live in a directory this script
never touches, so a forgotten one silently overrides everything here --
including ExecStart, which discards the log-level wrapper entirely. Any that
exist are now listed with their contents, and the command that shows the unit
as systemd actually assembles it.
BUILD-CONTAINER.md records the container build this runs in, since guacd is
built from source against FreeRDP 3 rather than installed from a package, and
the environment a measurement was taken in is otherwise left to be guessed at.
(cherry picked from commit 30558b38be1ea807d1271fe7b3a89f7717c7abc4)
Upstream: BUILD-CONTAINER.md and the fork notes were left out.
Provider discovery is lazy with retry, so SSO recovers on its own once the
provider comes up, and /api/auth/status reports availability separately from
enablement. Includes the callback diagnostics for state-cookie mismatch.
(cherry picked from commit dffe34660601a223021819c8efbed2ad503bfc14)
Upstream changes on pick: resolved against the ID token diagnostics from
#237 (the provider key ids now travel with the cached client and are
logged on rediscovery), and the unauthenticated /api/auth/status warm-up
is single-flight via OidcState::warm_in_background, so polling it while
the provider is down cannot queue up unbounded discovery tasks.
guacd sends the magic packet to whatever broadcast address and UDP port
it is given and then waits the given time before connecting. On an
ad-hoc request from a non-admin those came from the caller, so a
poweruser could have guacd send UDP to an arbitrary address and port,
or hold a guacd process for as long as they liked. Wake-on-LAN is now
ignored on those requests (it is configured on connection entries), the
wait is capped at 10 minutes, and the SSH font size is clamped to 6-72.
Wake-on-LAN (per entry): new wol_send_packet/wol_mac_addr/wol_broadcast_addr/
wol_udp_port/wol_wait_time fields on AddressBookEntry, threaded through
CreateSessionRequest into the SSH/VNC/RDP guacd param structs and emitted as
guacd's native wol-* connection args (guacd sends the magic packet and polls the
target before connecting). New 'Wake-on-LAN' section in the entry editor;
Guacamole imports carry existing wol-* params over.
SSH terminal font: new per-entry ssh_font_size (default 12), replacing the
hardcoded font-size. SSH DPI is now pinned to 96 so terminal text no longer
renders oversized on HiDPI displays — the client auto-scales the canvas, so a
fixed baseline keeps text size consistent across normal and HiDPI screens.
(RDP/VNC keep the device-scaled DPI.)
Onboarding modal: add a corner close button that permanently dismisses the
welcome tour (re-show still available via Settings -> Welcome Tour).
(cherry picked from commit e881bbc1e8933b46f7f2a8de691c86343dbbf15b)
openidconnect reports every signature problem as "Signature
verification failed" and keeps the reason (no matching key, crypto
error, disallowed algorithm) in the error's source chain, which we
logged with Display and so dropped. Log the whole chain, plus the
token's alg and kid from its header and whether that kid is among the
provider keys fetched at startup. When it is not, say so and name the
two usual causes: a key rotation since startup, and an Entra app with
an app-specific signing key (#237).
The login `next` check allowed anything starting with a single `/`.
Browsers treat `\` as `/` and drop tabs and newlines in Location, so
`/\evil.com`, and `/%09/evil.com` once the query was decoded, both
redirected to another site straight after a successful login on the
real login page. `next` must now be visible ASCII with no backslash,
and must not start with an encoded slash or backslash. That also keeps
it safe to place in the rustguac_next cookie unescaped, where a `;`
could previously add cookie attributes. Checked both when the cookie is
set and when it is consumed.
The network allowlists are what fence powerusers creating ad-hoc
sessions, and several paths went around them:
- POST /api/ssh/probe-host-key dialled any host:port, a port scanner and
banner grabber for anything rustguac can reach (guacd, Vault, metadata
services). Non-admin probes are now checked against
ssh_allowed_networks and dialled at the checked address.
- Jump hosts were never checked, though rustguac dials the first one
itself. For non-admin ad-hoc sessions it is now fenced by
ssh_allowed_networks and pinned.
- Ad-hoc Proxmox sessions called any proxmox_url and let guacd dial
whatever SPICE proxy that API returned. For non-admin ad-hoc sessions
both are now fenced by vnc_allowed_networks, as direct SPICE is.
The jump host and Proxmox fences are deliberately limited to non-admin
ad-hoc requests: connection entries and admins name bastions and
clusters that sit outside the target lists, and on the canary every
entry-based Proxmox and jump host session would otherwise have broken.
The flag is serde(skip), so a request body cannot set it.
The allowlist passed a hostname if any resolved address was allowed and
then handed guacd the hostname to resolve again, so a name whose DNS the
caller controls could pass the check and connect elsewhere. SSH and VNC
now dial the checked address when there are no jump hosts. RDP, SPICE
and web keep the hostname (TLS names, Kerberos) and the residual risk is
documented. Docs also no longer claim every resolved address must match.
client_ip took the LEFTMOST X-Forwarded-For entry from a trusted proxy.
Behind any proxy that appends to an incoming header (nginx
$proxy_add_x_forwarded_for, as our own reverse-proxy doc recommended, or
Apache mod_proxy), that entry is whatever the client sent, so a client
could pick an address inside an IP allowlist and walk through it. The
header is now walked from the right, skipping trusted proxies, across
every header line, split on raw bytes so a non-UTF-8 left entry cannot
hide the address a proxy appended.
Rate limiters used tower_governor's SmartIpKeyExtractor, which believes
forwarded headers from any peer: a different forged X-Forwarded-For per
request meant a fresh bucket each time, including on the always-on OIDC
limiter. They now key on the same trusted-proxy resolution. When
rate_limit is enabled, the API and WebSocket limiters sit outside
authentication so requests with bad credentials are counted. The OIDC
burst goes from 5 to 20 to cover an office behind one NAT address.
Docs: nginx recipe overwrites X-Forwarded-For with $remote_addr, and the
rate-limit section now states what is actually limited and when.
The first WebSocket to a pending session takes its guacd stream, already
logged in to the target. Any operator could claim another user's pending
session that way, and session ids are visible in reports. Now only the
session's creator may make that connection, with no admin bypass: an admin
"shadowing" a pending session used to silently become its owner and lock
the real owner out, so shadow now refuses sessions that have not started
(409). handle_ws only takes the stream when the owner check passed at
upgrade time.
POST /api/sessions accepts an admin-only `owner` field so a backend using
an API key can create a session for a user who then connects with their
own login. That user is recorded as the creator.
Share links and shadow tokens gain an optional read-only mode; the default
stays read/write. Each shareable session gets a second token
(share_url_readonly), and shadow takes {"read_only": true}. guacd enforces
it per joining user via the read-only connect arg, which was previously
hardcoded to false. The Share dialog offers both links, the Sessions page
offers watch and shadow, and docs no longer claim a read-only mode that
did not exist.
They call assert_cmd's cargo_bin, which needs CARGO_BIN_EXE_rustguac.
Cargo exports that only to integration tests under tests/, never to a
#[cfg(test)] module inside src/. As unit tests they passed locally purely
because a stale target/debug/rustguac was lying around for assert_cmd's
fallback to find; on a clean checkout, and so in CI, all five failed with
"`CARGO_BIN_EXE_rustguac` is unset". They have been broken since they were
written and nothing noticed, because the suite had never run in CI.
Caught by the cargo test step added in 83a9110, on its first run.
Database assertions now go through rusqlite directly rather than the
crate's own db helpers, since rustguac is a binary crate with no lib target
for an integration test to import. That is the better boundary regardless:
it checks the state the CLI actually leaves behind. rusqlite joins
dev-dependencies, which adds nothing to the tree as it is already a runtime
dependency.
CI ran fmt, clippy, build and audit but never `cargo test`, so none of the
310 tests had ever run on a pull request. That included the regression
tests written specifically to guard past incidents: the recording rotation
that deleted in-progress files, and the credential-variable folder scan
that hid subfolder-granted entries from operators. Both could have been
reintroduced without CI noticing.
Runs the debug profile rather than --release on purpose. debug_assertions
and integer overflow checks are compiled in there and switched off in a
release build, and the wire-protocol parser is where an overflow would
otherwise pass silently.
Noted by @joshsol1 on #230.
`Config::load` exited with "[oidc] is configured but no client_secret
was provided" for every subcommand, including `add-admin`. The secret
normally lives in /opt/rustguac/env, which only systemd's
EnvironmentFile loads, so running the admin CLI from an interactive
shell on a freshly installed host failed unless the secret was also
copied into config.toml, which defeats the point of the env file.
Keep the OIDC_CLIENT_SECRET override in `Config::load`, but move the
presence check into `Config::validate_oidc_secret` and call it only on
the `serve` path. Admin subcommands touch just the SQLite database and
never contact the IdP. Add unit tests for the validation function.
rustls 0.23.45, uuid 1.26.1, russh 0.63.3, tokio-rustls 0.26.5, aes 0.9.3,
rcgen 0.14.10, tower-http 0.7.1, toml 1.1.6. All are patch level within the
existing Cargo.toml constraints, so this is a lockfile-only change.
Two transitive majors ride along: rcgen takes pem 3.0.6 to 4.0.0, and rustls
takes aws-lc-rs 1.17.3 to 1.18.1 with aws-lc-sys 0.43.0 to 0.45.0. Release
build, 273 tests, clippy and cargo audit all clean.
Closes#215, #216, #217, #219, #224, #225, #226, #227.
Supersedes the cherry-picked 2449aa5 (install wrappers once), which stopped
the recursion crash but left passthrough silently dead after the RDPGFX
channel reconnect that xrdp performs at login. b3b2cce guards per callback
on what is actually installed, covering both failure directions.
Also brings 869c738, which releases the sync gate at the pipeline depth
waitForPending() gates on rather than at zero. Releasing only at zero meant
a continuously decoding session never drained fully, so every sync waited
out its 200ms timeout: choppy audio and a framerate throttled by guacd
pacing against an inflated lag figure.
# Conflicts:
# patches/004-h264-passthrough.patch
Documents the map-group subcommand in the group-to-role mappings section:
creating a mapping, the one-mapping-per-group rule and the --force re-map,
and that mappings are applied at the user's next OIDC login rather than
immediately, which matters in both directions when revoking access.
Documents the new cli_audit_log table: what is and is not recorded, that
the actor prefers SUDO_USER so sudo names the person rather than root,
that only successful actions are logged, how to read it with sqlite3, and
the 90 day retention.
Also corrects two pre-existing errors found while writing the above:
- deployment-guide.md showed an [oidc.group_role_mappings] config table.
No such config field exists, so anyone following the guide would add it
and silently get no mappings at all. Replaced with the CLI commands that
actually create them. The same block had session_ttl_secs under [oidc];
the real key is auth_session_ttl_secs and it is top level.
- roles-and-access-control.md documented an enable-user subcommand that
does not exist. Re-enabling is done from the Admin page or the API, so
the doc now says that, with a warning about disabling your own account.
Follow-up to the map-group subcommand.
oidc_group is UNIQUE, so mapping a group that already had a mapping failed
on the constraint and surfaced a raw "UNIQUE constraint failed" error, with
no way to change a mapping from the CLI at all. The command now looks the
group up first: an unchanged mapping is a no-op, a conflicting one explains
itself and points at the new --force flag, and --force re-maps through
db::update_group_mapping. This matches the API, which already answers 409
rather than 500 on a duplicate.
CLI commands hold the database directly and so bypass the API's
authenticated audit trail, leaving role grants and account deletions made
from a shell with no record anywhere. A new cli_audit_log table and
db::log_cli_event record the admin, user-role and group-mapping mutations,
attributed to SUDO_USER where present so an action taken through sudo names
the person rather than root. Entries are headline only, never API keys or
passwords, and are covered by the existing retention cleanup.
Tests: assert_cmd's Command is aliased so it no longer shadows the clap
Command enum that use super::* brings into scope, and the CLI tests run in
a per-test scratch directory removed on drop rather than writing a database
into tests/cli inside the repo. Adds coverage for role rejection, the
duplicate path, --force re-mapping, and the audit row.
This reverts 8988165, restoring the unconditional full-range BT.709
conversion. The commit it undoes identified a real inconsistency and
resolved it toward the wrong side.
Its reasoning was that the 4:2:0 path never reaches the shader, so the
combined 4:4:4 picture converted differently from the 4:2:0 one beside
it, and that adopting the decoded frame's colour space would make the two
agree. The two did then agree -- as limited range, on hosts that send
full range. Before it, 4:4:4 was correct and 4:2:0 was crushed; after it,
both were crushed. The symptom it describes, "the combined 4:4:4 picture
renders visibly flatter", was the correct picture beside a crushed one.
What every RDP host actually sends is now read out of the SPS rather than
assumed. Windows writes video_full_range_flag=1 with no colour
description. The xrdp fork writes it with a complete BT.709 description.
Stock xrdp 0.10.6 omits the video_signal_type block altogether -- it
passes x264 no VUI parameters -- while naming its own conversion
XRDP_yuv444_709fr. All three encode full-range BT.709, which is what
MS-RDPEGFX defines.
What the browser reports for those streams is a different question.
Chrome's hardware decoder ignores a bare range flag with no description
beside it and reports limited for a host that plainly declared full, so
a shader that adopts the frame's colour space expands 16-235 to 0-255 on
full-range samples: blacks crushed to zero, whites clipped, chroma
over-saturated by 255/224.
The inconsistency this reinstates is real and is not being dismissed: a
session can render 4:4:4 correctly and 4:2:0 crushed. One path right
beats both wrong, and the repair belongs upstream of the browser --
completing the host's SPS on the wire, so that neither path has to guess
and third-party clients benefit too. That follows separately; it is
server-side and does not belong in this PR.
The coefficients are exact BT.709 rather than the 8-bit approximations
the original cited from FreeRDP's prim_internal.h. Under passthrough
FreeRDP never decodes these streams, so matching its rounding was never
the point.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013Rxg4cT87G9czw4nUxKhHG
waitForPending() holds the Guacamole sync response while pendingDecodes
exceeds MAX_PIPELINE_DEPTH, but resolveIfIdle() only fired its resolvers at
pendingDecodes <= 0. So once the backlog crossed the depth it had to drain
completely before a sync could go out, and a session decoding continuously
never reaches zero. Every sync therefore waited out its full timeout,
logging "sync wait timeout" and reporting that much processing lag however
fast the client was really going.
guacd paces frames against the client's processing lag, so the session runs
slow while the client is not in fact behind. The function's own
documentation already said the callback fires "when the backlog is within
the allowed pipeline depth"; only the implementation disagreed.
It shows up first on AVC444, where a picture is two access units and the
backlog therefore runs twice as deep for the same frame rate, making it far
less likely to touch zero between frames. That presents as AVC444 being
expensive rather than as a threshold mismatch, so an A/B that varies the
server between AVC444 and AVC420 while holding the client constant cannot
tell the two apart.
The colour-space change introduced "float y" for the scaled luma, but y is
already the int pixel row in that scope. GLSL rejected the redefinition and
then every arithmetic line that mixed the two, so the shader failed to
compile and the renderer fell back to 4:2:0 for the whole session:
ERROR: 0:89: 'y' : redefinition
ERROR: 0:93: '+' : no operation exists that takes highp int and highp float
Name it luma.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The combining shader hardcoded BT.709 at full range, citing the coefficients
FreeRDP decodes these streams with. But in passthrough guacd never decodes --
the browser does, and the 4:2:0 path never reaches the shader at all: that
VideoFrame is drawn by the browser, which applies the frame's own colour
space including the limited-range expansion of 16-235 to 0-255.
So the two paths converted differently on the same session. An H.264 stream
carrying no VUI signalling is limited range, and treating it as full leaves
black at RGB 16 and white at 235, with chroma unscaled to match: the combined
4:4:4 picture renders visibly flatter than the 4:2:0 one beside it, depending
only on which codec the server chose.
Take the range and matrix from the decoded frame's colorSpace instead, read
once from the first main view, and default to limited range BT.709 when the
decoder reports nothing -- which is what the absence of signalling means.
VideoFrame.colorSpace was available in scope and never consulted.
The previous commit stopped guac_rdp_rdpgfx_channel_connected() from wrapping
its own wrapper, which fixed the recursion crash, but it guarded on
orig_surface_command being NULL:
if (rdp_client->settings->enable_h264
&& rdp_client->orig_surface_command == NULL) {
orig_surface_command lives on guac_rdp_client, so it is per-connection, while
the handler it protects is per-channel-instance. FreeRDP's
gdi_graphics_pipeline_uninit() sets SurfaceCommand to NULL and
gdi_graphics_pipeline_init() assigns gdi_SurfaceCommand back
(libfreerdp/gdi/gfx.c:1864, :1911), so a reconnect restores FreeRDP's own
handler -- and the once-only guard, seeing a non-NULL orig_surface_command
from the first install, declines to wrap it again. Passthrough is then dead
for the rest of the session.
xrdp reconnects the channel at login, for the display resize the previous
commit already identified, so this fired on essentially every xrdp session.
It was silent: the surface commands still reached FreeRDP's GDI decoder and
guacd re-encoded them as JPEG/WebP, so the desktop rendered correctly at
~100% of a core while no H.264 reached the browser. A trace on xrdp 0.10.6
showed one keyframe forwarded, the channel unloading 52ms later, and no
"H.264 passthrough enabled" line after the reconnect that followed.
Guard on what is actually installed, per callback. The two are restored on
different schedules -- gdi_graphics_pipeline_init() assigns SurfaceCommand
but never touches CapsConfirm -- so after a reconnect CapsConfirm is still
ours while SurfaceCommand is FreeRDP's again. A single guard keyed on
SurfaceCommand would pass and then point orig_caps_confirm at
guac_rdp_gfx_caps_confirm itself, reaching the same stack exhaustion by the
other callback.
With this, the same host forwards 807 AVC420 deltas and 3 keyframes over 60s
of desktop use, with guacd at 3.3% of a core.
guac_rdp_rdpgfx_channel_connected() installed the SurfaceCommand and
CapsConfirm wrappers unconditionally, every time it ran. It runs again when
the RDPGFX channel reconnects, which xrdp provokes with the display resize it
performs at login, and the second pass saved the wrapper the first pass had
installed:
rdp_client->orig_surface_command = (void*) rdpgfx->SurfaceCommand;
rdpgfx->SurfaceCommand = guac_rdp_gfx_surface_command;
leaving orig_surface_command pointing at guac_rdp_gfx_surface_command itself.
Any surface command that is not H.264 falls through to orig(context, cmd),
re-enters the wrapper, and recurses until the stack is exhausted and the guacd
child dies -- a white screen in the browser, with no "Client terminated
successfully" in the log and the segfault visible only outside a container.
H.264 commands take skip_decode and never reach that call, so a server sending
nothing but H.264 is unaffected. A stock xrdp is the opposite case: one trace
showed 7440 planar and 12 progressive surface commands against 437 AVC420, so
it dies on the first frame. Distro xrdp packages (0.10.1 on Ubuntu) are built
--enable-rfxcodec with no H.264 encoder at all, so they send planar for
everything and hit this immediately.
The wrappers were also installed for every GFX session regardless of
enable_h264, so this reproduced with RFX alone and H.264 unticked -- no
H.264-capable server required.
Both halves are fixed: install only when the connection asked for H.264, and
only when nothing is installed yet. guac_rdp_client is guac_mem_zalloc'd, so
the NULL check is sound on first entry.
guac_rdp_rdpgfx_channel_connected() installed the SurfaceCommand and
CapsConfirm wrappers unconditionally, every time it ran. It runs again when
the RDPGFX channel reconnects, which xrdp provokes with the display resize it
performs at login, and the second pass saved the wrapper the first pass had
installed:
rdp_client->orig_surface_command = (void*) rdpgfx->SurfaceCommand;
rdpgfx->SurfaceCommand = guac_rdp_gfx_surface_command;
leaving orig_surface_command pointing at guac_rdp_gfx_surface_command itself.
Any surface command that is not H.264 falls through to orig(context, cmd),
re-enters the wrapper, and recurses until the stack is exhausted and the guacd
child dies -- a white screen in the browser, with no "Client terminated
successfully" in the log and the segfault visible only outside a container.
H.264 commands take skip_decode and never reach that call, so a server sending
nothing but H.264 is unaffected. A stock xrdp is the opposite case: one trace
showed 7440 planar and 12 progressive surface commands against 437 AVC420, so
it dies on the first frame. Distro xrdp packages (0.10.1 on Ubuntu) are built
--enable-rfxcodec with no H.264 encoder at all, so they send planar for
everything and hit this immediately.
The wrappers were also installed for every GFX session regardless of
enable_h264, so this reproduced with RFX alone and H.264 unticked -- no
H.264-capable server required.
Both halves are fixed: install only when the connection asked for H.264, and
only when nothing is installed yet. guac_rdp_client is guac_mem_zalloc'd, so
the NULL check is sound on first entry.
Restores GfxAVC444=TRUE, skips the server-side H.264 decode, forwards both
AVC444 views (new 'view' field on the h264 instruction + Yuv444.js WebGL2
4:4:4 reconstruction), and fixes blank H.264 recording playback. Review
findings folded in (f16eed7). guacd CPU ~100%/90% of a core -> ~2%.
russh 0.63 changed the check_server_key callback to take
&PublicKeyOrCertificate; resolve it to a PublicKey via .public_key() in
both the tunnel and probe handlers, and widen the russh constraint to
0.63. Closes the retargeted dependabot PRs #204 (uuid) and #205 (russh).
The CI `cargo audit` step was failing on RUSTSEC-2026-0258 (h2 unbounded
empty DATA frames, a HTTP/2 DoS), pulled in transitively via reqwest/hyper.
Bump h2 0.4.15 -> 0.4.19 (patched) and chacha20 0.10.1 -> 0.10.2 (the prior
version was yanked).
Adds a per-entry RDP keyboard layout, wiring guacd's `server-layout`
parameter (previously hardcoded to empty) through the address-book entry,
the connect request, and the guacd handshake, with a dropdown in the
connections editor and a migration mapping from Guacamole's `server-layout`
on import. Empty keeps guacd's default (en-us-qwerty).
list_credential_variables walked from top-level folders using
check_folder_access + continue, short-circuiting the whole subtree when an
ancestor was not directly accessible. An operator granted access at a
subfolder level saw "No credential variables" in My Credentials even though
the connections tree showed the entry (the tree is descendant-aware). It now
descends past inaccessible ancestors, collecting variables only from folders
the user can directly access. Adds regression tests.
CI's runners updated to clippy 0.1.98, which enforces two lints that
existing code tripped under -D warnings. Rewrite the two late-init
if/else blocks in the SSH tunnel builder as tuple `let` bindings
(needless-late-init), and allow result-large-err on check_folder_access,
whose Err is an axum Response by design.
Review notes from the upstream issue thread:
- The auxiliary-view snapshot was sized to the auxiliary frame's own
dimensions. The v1 chroma layout pads that view to a multiple of 16
rows, so the snapshot could be taller than the picture the renderer
draws; with no rects the whole thing is blitted, painting a blank
strip below the image. Size it from the rendered canvas instead.
- The per-frame watchdog was cleared before combineFrame() ran, so the
4:4:4 path had no timeout despite the comment saying otherwise: a
copyTo() that never settled would hold the ordered display queue with
nothing able to release it. Clear it once the frame is done with
instead -- in the finally for the synchronous paths, and after the
copy settles for the combine.
- Drop the unreachable view != 0 branch after the combine hand-off.
- '0' no longer coerces to false in override(), so ?h264ChromaFilter=0
sets threshold zero rather than switching the filter off. Boolean
overrides are unaffected: they coerce, and 0 is falsy.
- Drop the stale stats() reference from client.html; the decoder has no
such method.
Recordings capture the raw protocol stream, so a recording of an H.264
session contains h264 instructions. recordings.html loaded neither
H264Decoder.js nor Yuv444.js, so playback of those sessions rendered
nothing.
H.264 passthrough had two independent defects. On Windows hosts it
negotiated no H.264 at all, and where it did engage it delivered about
half the saving it should.
GfxAVC444 = FALSE was set to make the client advertise AVC420-only,
which is not what the combination does. FreeRDP gates the entire RDPGFX
V10+ capability block on
!GfxH264 || GfxAVC444
(rdpgfx_main.c:254, rdpgfx_send_caps_advertise_pdu), so with GfxH264 set
and GfxAVC444 clear no V10 capability set is sent at all -- the client
advertises only V8 and V8.1. Windows will not offer H.264 at V8.1: it
falls back to CLEARCODEC and CAPROGRESSIVE, which guacd then decodes and
re-encodes as JPEG/WebP, exactly the cost the feature exists to avoid.
The display stays correct, so the regression is invisible without
measuring CPU or counting codecs on the wire. Over a minute of 1080p
video on Windows 11 Pro: 4050 CLEARCODEC + 1447 CAPROGRESSIVE + zero
H.264 frames before, all frames codec 15 after. xrdp is unaffected,
which is why this went unnoticed -- it accepts AVC420 at V8.1 and never
needed the V10 capsets.
AVC444 was disabled because forwarding it was broken, not because
AVC420 was wanted: only bitstream[0] reached the client, rendering as a
green/magenta split. Forwarding one view cannot work -- both bitstreams
decode through the same H264_CONTEXT in FreeRDP's avc444_decompress, so
they are one H.264 sequence with alternating views. Dropping either
breaks reference frames for the other, and the decoder reports no error
because what arrives is well formed. Both views are now forwarded,
tagged with a view field on the h264 instruction; the client decodes
both, draws only view 0, and combines the auxiliary view's chroma into
full 4:4:4 in a WebGL2 pass, falling back to 4:2:0 if it cannot.
The GDI decode is now skipped for captured H.264 commands. Previously
the wrapper always called through to the original handler, so only the
re-encode was saved. RemoteFX, planar and progressive still decode
normally, so a server mixing codecs within a frame stays correct.
guacd CPU with 1080p video playing, sampled per-thread over 30s:
xrdp (AVC420) ~100% of a core -> 2.0%
Windows 11 (AVC444) 90.6% of a core -> 2.1%
Configuration is unchanged: the per-connection enable-h264 argument is
the only switch, and there are no environment variables.
Maps Apache Guacamole passthrough tokens (${GUAC_USERNAME} etc.) to rustguac
credential variables during import, so imported connections arrive wired to
My Credentials. Repeatable FROM=TO; applied to the username, password,
domain, and private_key fields. Adds tests and docs.
Disk-based rotation deleted the oldest .guac unconditionally until statvfs
usage fell below max_disk_percent. Recordings for live sessions are held
open by the recording tee, so unlinking one frees no space and statvfs never
drops: the loop then deleted every recording in the directory in a single
pass while the disk stayed full (a production host lost all recordings this
way).
rotate() now excludes live-session recordings (SessionManager::
active_recording_paths) and stops if a deletion frees no space. Adds
regression tests.
pletch