diff --git a/orchestrator/orchestrator/data/screencapture-approve.sh b/orchestrator/orchestrator/data/screencapture-approve.sh index 22f21e6..2865927 100644 --- a/orchestrator/orchestrator/data/screencapture-approve.sh +++ b/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,9 +138,10 @@ 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 @@ -139,7 +149,9 @@ on run argv 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)" ;; diff --git a/orchestrator/orchestrator/workflow.py b/orchestrator/orchestrator/workflow.py index fdf6ae4..17d333f 100644 --- a/orchestrator/orchestrator/workflow.py +++ b/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( diff --git a/orchestrator/tests/test_screencapture_grant.py b/orchestrator/tests/test_screencapture_grant.py index 4223950..cd57a30 100644 --- a/orchestrator/tests/test_screencapture_grant.py +++ b/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.