video: support five streams and improve per-entry access controls - #43
video: support five streams and improve per-entry access controls#43tridge wants to merge 10 commits into
Conversation
An aircraft can carry more than three cameras, and one port carries one stream, so the cap was the limit on how many a single entry could proxy. The three fields that hold per-slot state -- video_ports, video_flags and video_rtmp_path -- all sit in the middle of the record, so none of them could simply grow: every field after them would shift, every record already on disk would be misparsed, and an older binary would read garbage. The append-only contract at the top of keydb.h exists to make that unnecessary, so slots 3 and 4 are carried in new fields appended after reserved[], and accessors join the two halves. A record written before they existed zero-extends into them, which reads as two unused slots, so nothing needs converting and the live database keeps working. video_flags could not be widened for the same reason, and it was already full: three slot bytes plus the entry-wide byte is exactly 32 bits. A fourth slot byte at shift 24 would have landed on the entry options -- which is what happened first time round, and is why there is now a test that sets slots 3 and 4 to 0xFF and checks the audio flag survives. The record grows 344 -> 456 bytes. Also fixes video_port_count() tripping over a short list, which callers that build a KeyEntry by hand were relying on not happening. The README's video section had been pasted in three times: the edit that added it replaced on "## Building", which matches three headings. Only one copy remains.
Bit 3 of each slot's option byte, which was free, so no record growth and no migration -- an existing record reads it clear, which is the current behaviour. It marks a slot whose publisher may be admitted by the entry's MAVLink session even though a publish password is set. Some publishers cannot present one: a camera speaking RTMP from its own firmware has nowhere to put a credential unless its stream-key field tolerates a query, and plain MPEG-TS over UDP never does. Without this an entry faced an all-or-nothing choice between a password and those streams.
admit() gains the slot's session_ok bit. With it set and no credential offered, admission falls through to the MAVLink-session path instead of refusing; without it, nothing changes. The fallback deliberately does not apply to a credential that was offered and is wrong. Downgrading that to address matching would turn a clear rejection into a silent weakening, so a typo cannot succeed on the strength of the source address. Five tests, of which two guard the behaviour being preserved: an unflagged slot still refuses a session-only publisher, and a wrong password is still refused on a flagged one. Verified RED by forcing the bit false -- the three that assert the new path fail, the two guards still pass.
Rendered in the existing per-slot options row and documented in the template beside it, as the other three are. The forms.py tooltip check exempts the per-slot booleans, so the guard that they really are documented where they are rendered is extended to cover it.
Pasting into it with the tooltip showing wedges Chrome's renderer: the tab stops responding to input entirely, and it does not recover. The text it carried moves into the blurb above the form, which already introduced the field, so nothing is lost. The forms.py "every option is documented" guard exempts the field and records why, so it cannot be quietly reinstated.
A tooltip over a password field wedges Chrome's renderer: the tab stops accepting input and does not recover. Reproduced on the login form and removed there; this covers the rest, which share the markup and so presumably share the fault. The help itself is worth keeping -- the publish-password text explains that it replaces the address check, and the new-passphrase one that blank means unchanged -- so the macro renders a password field's description in flow as .field-hint rather than dropping it. The dotted underline that advertises a tooltip goes with it, and aria-describedby is now emitted only when there is something to point at. Two guards: no password input on any page carries a .tip, and the text still reaches the page. Both verified RED.
Previous review (2026-08-25, at head 1174f36)Automated review note — AI-generated (Claude), validated against the live diff. Please sanity-check before acting. Reviewed at head ISSUE — a supplied-but-wrong publish password can be read as "absent" and take the address fallback.
Not an access-control bypass — every vector needs the attacker already at the authorized session address on a flag-enabled, non-bidi slot, where credential-free publish is intended — but it means the README's "a typo cannot quietly succeed on the strength of the address" isn't honoured. A uniform fix: represent presence with an explicit flag (independent of the decoded value), require a complete bounded request line before deciding, reject embedded NUL, and use the robust query parser on the publish path too. ISSUE — signed-session fallback can never succeed (fails closed). NOTE — stale comments: Refuted from the cold pass and not issues: the session-name collision check is not still bounded to 3 (every video-slot loop uses Verified clean: append-only ABI (old 344-byte records zero-extend so the new flag defaults off on migration), per-slot/per-entry isolation, port allocation stops correctly at 65535, and the log gate re-validates the viewer each request with traversal closed. Please confirm CI green before merge given this touches an auth path. |
Preserve the distinction between absent and supplied publish credentials across RTSP and RTMP parsing, and export signed MAVLink authentication state for bidi session fallback. Update stale five-slot schema documentation and add regression coverage.
|
Automated review note — AI-generated (Claude), validated against the live diff (Claude + Codex cross-checked). Please sanity-check before acting. Full report: https://uav.tridgell.net/DevCallReviews/2026_08_26/devcall_pr_reviews.html#prSupportProxy_43 Reviewed at head Re-reviewed at the same head as my 2026-08-25 comment; the auth/ABI work verified clean again (credential-presence bits end-to-end, 456-byte split-record ABI offsets, per-slot session_ok, RTSP request guarding), but the must-fix from that round is still unaddressed, so the verdict stands. Must fix:
Should fix:
Notes (non-blocking):
Previous round triage: SESSION_EXTS BUG — still open (impact narrowed on re-analysis: |
Reject duplicate RTMP connect properties, preserve the first credential across RTMP setup commands, and continue validating credentials on every RTSP request. Bound RTSP framing, reject ambiguous content lengths, and cover the reported downgrade cases with integration tests.
1 participant
Summary
Compatibility
Existing key records remain valid: slots four and five use fields appended after the established record layout, and the new access flags default existing entries to their previous private behavior.
Testing