screencapture grant: bash on ad-hoc roles + retry transient skips (RELOPS-2454)

orchestrator/orchestrator/data/screencapture-approve.sh

@@ -78,7 +78,7 @@ row() {
 }
 
 granted() {
-    for c in "${CLIENTS[@]}" "$SCREENSHOT_CLIENT"; do
+    for c in ${CLIENTS[@]+"${CLIENTS[@]}"} "$SCREENSHOT_CLIENT"; do
         case "$(row "$c")" in
             2/0|2/4) ;;
             *) return 1 ;;
@@ -92,15 +92,24 @@ granted() {
 /usr/bin/csrutil status 2>/dev/null | grep -qi disabled && \
     skip "SIP is off — macos_tcc_perms already grants this host"
 
+# Ad-hoc worker binaries (Identifier=a.out, no TeamIdentifier) can never satisfy a
+# code requirement, so a grant for them is stored and ignored. Roles without
+# taskcluster_signed_binaries -- the staging pools among them -- run ad-hoc builds.
+# Skip the worker binaries there but still grant /bin/bash: the screenshot
+# LaunchAgent does not depend on how the worker is signed.
+ident=$(/usr/bin/codesign -dvvv "${CLIENTS[0]}" 2>&1 | /usr/bin/awk -F= '/^Identifier=/{print $2; exit}')
+GRANT_WORKERS=1
+if [ "$ident" != "generic-worker-multiuser-darwin-arm64" ]; then
+    log "worker binaries are not Developer-ID signed (Identifier=${ident:-unknown}); granting /bin/bash only"
+    CLIENTS=()
+    GRANT_WORKERS=0
+fi
+
 if granted; then
-    log "already granted ($(row "${CLIENTS[0]}"), bash $(row "$SCREENSHOT_CLIENT"))"
+    log "already granted (bash $(row "$SCREENSHOT_CLIENT"))"
     exit 0
 fi
 
-ident=$(/usr/bin/codesign -dvvv "${CLIENTS[0]}" 2>&1 | /usr/bin/awk -F= '/^Identifier=/{print $2; exit}')
-[ "$ident" = "generic-worker-multiuser-darwin-arm64" ] || \
-    fail "worker binary is not Developer-ID signed (Identifier=${ident:-unknown}); the grant would be stored and ignored"
-
 overrides=$(/usr/bin/plutil -p "$OVERRIDES" 2>/dev/null | /usr/bin/grep -c kTCCServiceScreenCapture)
 [ "$overrides" = "0" ] || \
     fail "a ScreenCapture PPPC override is installed ($overrides entries); approving now yields a flags=12 row that TCC ignores"
@@ -129,17 +138,20 @@ sleep 3; /usr/bin/pkill -x "System Settings" >/dev/null 2>&1; sleep 2
 asuser /usr/bin/open "x-apple.systempreferences:com.apple.preference.security?Privacy_ScreenCapture"
 sleep 12
 
-asuser /usr/bin/osascript - "$creds" 2>&1 <<'OSA'
+asuser /usr/bin/osascript - "$creds" "$GRANT_WORKERS" 2>&1 <<'OSA'
 on run argv
   set credFile to item 1 of argv
+  set grantWorkers to (item 2 of argv) is "1"
   set adminUser to do shell script "head -1 " & quoted form of credFile
   set adminPass to do shell script "sed -n 2p " & quoted form of credFile
   do shell script "rm -f " & quoted form of credFile
 
   tell application "System Events" to tell process "System Settings"
     set frontmost to true
     delay 2
-    repeat with nm in {"generic-worker-multiuser", "start-worker"}
+    set workerNames to {}
+    if grantWorkers then set workerNames to {"generic-worker-multiuser", "start-worker"}
+    repeat with nm in workerNames
       set cb to my findCB(window 1, nm as string, 0)
       if cb is missing value then error "checkbox not found: " & (nm as string)
       if value of cb is 0 then
@@ -269,7 +281,7 @@ sleep 3; /usr/bin/pkill -x "System Settings" >/dev/null 2>&1
 
 # --- verify ------------------------------------------------------------------
 
-for c in "${CLIENTS[@]}" "$SCREENSHOT_CLIENT"; do
+for c in ${CLIENTS[@]+"${CLIENTS[@]}"} "$SCREENSHOT_CLIENT"; do
     r=$(row "$c")
     case "$r" in
         2/0|2/4) log "granted $c ($r)" ;;

orchestrator/orchestrator/workflow.py

@@ -169,6 +169,10 @@ def _try(label: str, getter, *, required: bool = True) -> None:
 # /var/root so it is root-only by location as well as by mode.
 OS_UPGRADE_REMOTE = "/var/root/macos-upgrade.sh"
 SCREENCAPTURE_REMOTE = "/var/root/screencapture-approve.sh"
+# Skips that clear on their own shortly after the bootstrap, so worth waiting out.
+SCREENCAPTURE_TRANSIENT_SKIPS = ("does not own the console session", "running a task")
+SCREENCAPTURE_ATTEMPTS = 10
+SCREENCAPTURE_RETRY_SECONDS = 30
 
 
 def _os_version_matches(actual: str, expected: str) -> bool:
@@ -1333,6 +1337,29 @@ def _screencapture_script() -> str:
     return body
 
 
+def _run_screencapture_script(ctx: HostContext) -> tuple[int, str]:
+    """Stage the approval script, run it once, remove it. Returns (exit code, output)."""
+    ui.wire(
+        f"scp -> {SCREENCAPTURE_REMOTE} (0700, credential substituted from the vault)"
+    )
+    ssh.write_file_as_root(
+        ctx.fqdn, SCREENCAPTURE_REMOTE, _screencapture_script().encode(), mode="0700"
+    )
+
+    ui.wire(
+        f"ssh admin@{ctx.hostname} sudo {SCREENCAPTURE_REMOTE}  (drives System Settings as cltbld)"
+    )
+    cp = ssh.run(ctx.fqdn, f"sudo {SCREENCAPTURE_REMOTE}; echo rc=$?", check=False)
+    out = cp.stdout.decode(errors="replace").strip()
+    ssh.run(ctx.fqdn, f"sudo rm -f {SCREENCAPTURE_REMOTE}", check=False)
+
+    rc = 1
+    for line in out.splitlines():
+        if line.startswith("rc="):
+            rc = int(line[3:] or 1)
+    return rc, out
+
+
 def step_screencapture_grant(ctx: HostContext) -> None:
     """Grant Screen Recording to the worker binaries and /bin/bash. SIP-on hosts only.
 
@@ -1356,40 +1383,35 @@ def step_screencapture_grant(ctx: HostContext) -> None:
     one host at a time.
 
     Exit 3 from the script means "not applicable / not now" (SIP off, host busy, no
-    console session) and is reported, not raised -- the host is still fine to hand back,
-    and the ronin detector (macos_screencapture_check) will keep the gap visible.
+    console session). "Not now" is retried for a few minutes first: the step runs
+    right after the bootstrap sentinel, when cltbld's autologin session may not own
+    the console yet, and a skip there would hand the host back with no grant at all.
+    Whatever skip remains is reported, not raised -- the host is still fine to hand
+    back, and the ronin detector (macos_screencapture_check) will keep the gap visible.
     """
     ui.step(
         "SCREEN RECORDING",
         "grant the worker binaries + /bin/bash ScreenCapture TCC (SIP-on hosts only)",
     )
-    ui.wire(
-        f"scp -> {SCREENCAPTURE_REMOTE} (0700, credential substituted from the vault)"
-    )
-    ssh.write_file_as_root(
-        ctx.fqdn, SCREENCAPTURE_REMOTE, _screencapture_script().encode(), mode="0700"
-    )
-
-    ui.wire(
-        f"ssh admin@{ctx.hostname} sudo {SCREENCAPTURE_REMOTE}  (drives System Settings as cltbld)"
-    )
-    cp = ssh.run(ctx.fqdn, f"sudo {SCREENCAPTURE_REMOTE}; echo rc=$?", check=False)
-    out = cp.stdout.decode(errors="replace").strip()
-    ssh.run(ctx.fqdn, f"sudo rm -f {SCREENCAPTURE_REMOTE}", check=False)
-
-    rc = 1
-    for line in out.splitlines():
-        if line.startswith("rc="):
-            rc = int(line[3:] or 1)
+    for attempt in range(1, SCREENCAPTURE_ATTEMPTS + 1):
+        rc, out = _run_screencapture_script(ctx)
+        reason = next(
+            (ln for ln in out.splitlines() if ln.startswith("[SKIP]")),
+            "[SKIP] not applicable",
+        )
+        transient = rc == 3 and any(m in reason for m in SCREENCAPTURE_TRANSIENT_SKIPS)
+        if not transient or attempt == SCREENCAPTURE_ATTEMPTS:
+            break
+        ui.wire(
+            f"{reason.replace('[SKIP] ', '')} -- retrying in "
+            f"{SCREENCAPTURE_RETRY_SECONDS}s ({attempt}/{SCREENCAPTURE_ATTEMPTS})"
+        )
+        time.sleep(SCREENCAPTURE_RETRY_SECONDS)
 
     if rc == 0:
         ui.ok("Screen Recording granted (auth_value 2, flags 0)")
         return
     if rc == 3:
-        reason = next(
-            (ln for ln in out.splitlines() if ln.startswith("[SKIP]")),
-            "[SKIP] not applicable",
-        )
         ui.warn(reason.replace("[SKIP] ", "skipped: "))
         return
     raise ReprovisionError(

orchestrator/tests/test_screencapture_grant.py

@@ -69,10 +69,72 @@ def test_skip_conditions_do_not_raise(reason):
             "orchestrator.workflow._screencapture_script", return_value="#!/bin/bash\n"
         ),
         _run_with(f"{reason}\nrc=3"),
+        patch("orchestrator.workflow.time.sleep"),
     ):
         workflow.step_screencapture_grant(_ctx())  # must not raise
 
 
+def _run_sequence(*outputs: str):
+    """Patch ssh so successive payload runs return successive outputs."""
+    it = iter(outputs)
+
+    def fake(_host, cmd, **_k):
+        return _CP(next(it) if "rc=$?" in cmd else "")
+
+    return patch("orchestrator.workflow.ssh.run", side_effect=fake)
+
+
+def test_transient_skip_is_retried_until_granted():
+    """Right after the bootstrap cltbld may not own the console yet. Giving up on the
+    first skip hands the host back with no grant, so wait it out instead."""
+    with (
+        patch("orchestrator.workflow.ssh.write_file_as_root"),
+        patch(
+            "orchestrator.workflow._screencapture_script", return_value="#!/bin/bash\n"
+        ),
+        _run_sequence(
+            "[SKIP] cltbld does not own the console session yet\nrc=3",
+            "[SKIP] cltbld does not own the console session yet\nrc=3",
+            "[screencapture] Screen Recording granted\nrc=0",
+        ),
+        patch("orchestrator.workflow.time.sleep") as sleep,
+        patch("orchestrator.workflow.ui.ok") as ok,
+    ):
+        workflow.step_screencapture_grant(_ctx())
+    assert sleep.call_count == 2
+    ok.assert_called_once()
+
+
+def test_permanent_skip_is_not_retried():
+    """SIP off will not change while we wait; do not burn five minutes on it."""
+    with (
+        patch("orchestrator.workflow.ssh.write_file_as_root") as write,
+        patch(
+            "orchestrator.workflow._screencapture_script", return_value="#!/bin/bash\n"
+        ),
+        _run_with("[SKIP] SIP is off — macos_tcc_perms already grants this host\nrc=3"),
+        patch("orchestrator.workflow.time.sleep") as sleep,
+    ):
+        workflow.step_screencapture_grant(_ctx())
+    sleep.assert_not_called()
+    write.assert_called_once()
+
+
+def test_transient_skip_gives_up_after_the_retry_budget():
+    with (
+        patch("orchestrator.workflow.ssh.write_file_as_root") as write,
+        patch(
+            "orchestrator.workflow._screencapture_script", return_value="#!/bin/bash\n"
+        ),
+        _run_with("[SKIP] host is running a task — retry when idle\nrc=3"),
+        patch("orchestrator.workflow.time.sleep"),
+        patch("orchestrator.workflow.ui.warn") as warn,
+    ):
+        workflow.step_screencapture_grant(_ctx())  # must not raise
+    assert write.call_count == workflow.SCREENCAPTURE_ATTEMPTS
+    warn.assert_called_once()
+
+
 @pytest.mark.parametrize(
     "output",
     [
@@ -150,10 +212,23 @@ def test_script_grants_and_verifies_bash():
     with patch("orchestrator.workflow.ssh_admin_password", return_value="s3cr3t"):
         body = workflow._screencapture_script()
     assert "SCREENSHOT_CLIENT=/bin/bash" in body
-    assert body.count('"${CLIENTS[@]}" "$SCREENSHOT_CLIENT"') == 2  # granted() + verify
+    # granted() + verify; the guard keeps an emptied CLIENTS legal under bash 3.2 set -u
+    assert body.count('${CLIENTS[@]+"${CLIENTS[@]}"} "$SCREENSHOT_CLIENT"') == 2
     assert 'keystroke "/bin/bash"' in body
 
 
+def test_unsigned_worker_binaries_still_grant_bash():
+    """Roles without taskcluster_signed_binaries (the staging pools) run ad-hoc worker
+    builds. That used to be a hard fail, which ended every staging reprovision in an
+    error before bash was granted. It must skip only the worker binaries."""
+    with patch("orchestrator.workflow.ssh_admin_password", return_value="s3cr3t"):
+        body = workflow._screencapture_script()
+    assert 'fail "worker binary is not Developer-ID signed' not in body
+    assert "CLIENTS=()" in body and "GRANT_WORKERS=0" in body
+    assert 'osascript - "$creds" "$GRANT_WORKERS"' in body
+    assert "if grantWorkers then set workerNames to" in body
+
+
 def test_step_is_in_both_flows():
     """Regression guard: the grant must not silently drop out of the sequences.