Skip to content

#410 slice 2b required residual: desktop lease beat (renew/retry/skip-transient) + NITs from #453 #454

Closed

Description

@chayapats

Follow-up to #453 (#410 slice 2a), required residual for #410 slice 2b. Source: Duo review #453 (review), Tria ruling #453 (comment).

Slice 2b must not merge until every item below is done. Tria ruled these REQUIRED.

Problem

The desktop browser proxy lease in src/s15/vm/qemu.ts, as of 4fa48e5, has two gaps:

  1. Transient inspect() failure kills the lease (Duo SHOULD 1, qemu.ts:376-382). The 10-minute renewal treats any inspect() throw as "not running", and closeDesktopLease() cancels the timer. A snapshot holds the QMP socket for up to 300s (qemu.ts:469). The QEMU chardev stops accepting clients while one is connected, so inspect() times out after 30s (qemu.ts:241). The lease then dies until the next boot or re-attach, while the VM keeps running.
  2. A missed lease is never retried (Duo SHOULD 2, qemu.ts:369, 372-374). If issue() fails (full 64-lease pool) or delivery fails (SSH or the writer), the timer is never armed. Browsing stays dead until a reboot. Also, openDesktopLease() runs at the end of launch(), before provision() waits for cloud-init, so the jit-desktop group may not exist yet when the lease is delivered.

Acceptance criteria

One 10-minute beat per running VM, with these rules:

  • Running, lease held: renew().
  • Running, no lease held: call openDesktopLease() again (covers a full pool and failed delivery).
  • inspect() throws transiently (QMP busy or timed out, management unavailable): skip this beat and keep the timer. The 30-minute expiry already covers one missed beat.
  • Proven non-running, or vm_boundary_unproven: revoke the lease (stays fail-closed).
  • The jit-desktop group exists before launch() returns, or the lease is re-issued after cloud-init finishes.
  • Test (a): a snapshot that holds QMP for more than 30s does not kill the lease.
  • Test (b): with the pool full at boot, a later beat gets a lease.

NITs (fix in 2b)

  • In the contract s15-working-vm-v1, the "Desktop browser lease" paragraph should say that each renewal resets the 512 MiB (bytesPerSession) allowance for the period.
  • Add $ to the env-file character check at qemu.ts:623. systemd EnvironmentFile expands $. Today's URL cannot contain it, but the check should reject it.

Open question for Champ (not a defect)

Duo found no bypass of the listed names in the sensitive-site filter. These are reachable today and could be added before 2b if Champ wants: gmail.com, outlook.com, outlook.office365.com, idmsa.apple.com, oauth2.googleapis.com.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions