From 67baddd5874a621da77a8aac61a24bb55ca35bd7 Mon Sep 17 00:00:00 2001 From: Rian Stockbower Date: Tue, 30 Jun 2026 17:30:56 -0400 Subject: [PATCH 1/2] chore: remove local Keychain repair wrapper --- .github/workflows/ci.yml | 6 - Makefile | 7 +- docs/development.md | 3 +- scripts/README.md | 38 +- scripts/repair-codereview-keychain-acl.sh | 532 ---------------------- 5 files changed, 12 insertions(+), 574 deletions(-) delete mode 100755 scripts/repair-codereview-keychain-acl.sh diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index cc643a86..b2f2bd74 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -58,12 +58,6 @@ jobs: with: go-version-file: go.mod - test-scripts: - runs-on: ubuntu-latest - steps: - - uses: actions/checkout@v4 - - run: make test-scripts - static-smoke: runs-on: ubuntu-latest env: diff --git a/Makefile b/Makefile index 7e48f5c4..b572a772 100644 --- a/Makefile +++ b/Makefile @@ -1,4 +1,4 @@ -.PHONY: all build test test-cover test-scripts test-static-smoke lint fmt tidy deps check install snapshot package-render-check release clean +.PHONY: all build test test-cover test-static-smoke lint fmt tidy deps check install snapshot package-render-check release clean # Standard keyring tags: enable 1Password support, keep passage disabled. GOFLAGS ?= -tags=keyring_nopassage @@ -12,9 +12,6 @@ build: test: go test -v ./... -test-scripts: - scripts/repair-codereview-keychain-acl.sh --self-test - test-cover: go test -coverprofile=coverage.out ./... @@ -41,7 +38,7 @@ deps: go mod download go mod verify -check: tidy fmt lint test test-scripts build +check: tidy fmt lint test build install: go install ./cmd/cr diff --git a/docs/development.md b/docs/development.md index 2e537634..077dd226 100644 --- a/docs/development.md +++ b/docs/development.md @@ -62,11 +62,10 @@ progress interface rather than importing Cobra or root options. ```bash make build # compile the binary make test # go test ./... -make test-scripts # run script self-tests make lint # golangci-lint run make tidy # go mod tidy and verify go.mod is unchanged make deps # download and verify Go modules -make check # tidy + fmt + lint + test + test-scripts + build +make check # tidy + fmt + lint + test + build make clean # remove build artifacts ``` diff --git a/scripts/README.md b/scripts/README.md index 04007e46..10f004c1 100644 --- a/scripts/README.md +++ b/scripts/README.md @@ -1,40 +1,20 @@ # Scripts -This directory contains repository support and verification tools. These scripts -are not part of the shipped `cr` command surface. +Repository support and verification tools. These scripts are not part of the +shipped `cr` command surface. -## `repair-codereview-keychain-acl.sh` - -Repairs macOS Keychain generic-password ACLs for the `codereview` service when -older items still trust only ad-hoc or per-build `cdhash` identities instead of -the stable-signed `cr` release binary. - -Use it when `cr` repeatedly prompts for the same Keychain items after upgrading -from older unsigned or ad-hoc-signed builds. Run it as the normal macOS user who -owns the login Keychain, not with `sudo`. - -Default mode is a dry-run: +macOS Keychain credential ACL repair is shared across the Open CLI Collective +tool family. Use the canonical helper from the sibling `cli-common` checkout: ```bash -./scripts/repair-codereview-keychain-acl.sh +../cli-common/scripts/repair-macos-keychain-credentials.sh --tool cr ``` -Apply repairs explicitly: +Preview and apply the additive heal path: ```bash -./scripts/repair-codereview-keychain-acl.sh --apply +../cli-common/scripts/repair-macos-keychain-credentials.sh --tool cr --heal +../cli-common/scripts/repair-macos-keychain-credentials.sh --tool cr --heal --apply ``` -The script does not read or print secret values. In `--apply` mode it updates -only `cdhash-only` generic-password items for the selected service and keychain -so they trust the current stable-signed `cr` binary. Items classified as -`missing-current-cr` are reported for manual inspection rather than changed. - -Apply mode is intentionally additive. For explicit app-list ACL entries, the -helper appends current `cr` trust while preserving existing trusted applications. -It skips decrypt ACLs with `NULL` or non-explicit app lists instead of narrowing -their meaning to one trusted application. - -`stable+stale-cdhash` means current `cr` is already trusted. macOS may still -report older cdhash grants or partition metadata for that item; the script treats -that state as repaired and does not try to purge historical metadata. +The heavier cleanup and rebuild paths also live in that shared helper. diff --git a/scripts/repair-codereview-keychain-acl.sh b/scripts/repair-codereview-keychain-acl.sh deleted file mode 100755 index f906021e..00000000 --- a/scripts/repair-codereview-keychain-acl.sh +++ /dev/null @@ -1,532 +0,0 @@ -#!/usr/bin/env bash -set -euo pipefail - -DEFAULT_SERVICE="codereview" -EXPECTED_IDENTIFIER="org.open-cli-collective.cr" -# Source of truth: Open CLI Collective macOS release-signing MACOS_CERT_LEAF_SHA. -# Do not derive this from the candidate cr binary being validated. -EXPECTED_LEAF_SHA="42e1afd02aae8666c09c15f171e1639550f301c2" - -usage() { - cat <<'USAGE' -Usage: scripts/repair-codereview-keychain-acl.sh [--apply] [--service codereview] [--keychain PATH] [--cr PATH] - scripts/repair-codereview-keychain-acl.sh --self-test - -Find codereview Keychain generic-password items that do not yet trust the -current stable-signed cr binary. In apply mode, update cdhash-only items to trust -current cr. Items that already trust current cr are considered repaired even if -macOS still shows historical cdhash grants in ACL or partition metadata. - -Default mode is dry-run. Pass --apply to modify matching Keychain items. - -Run this as your normal macOS user, not with sudo. Login Keychain ACL changes are -authorized through the user's security session and may show macOS prompts. -USAGE -} - -lower() { - printf '%s' "$1" | tr '[:upper:]' '[:lower:]' -} - -expected_dr() { - printf 'identifier "%s" and certificate leaf = H"%s"' "$EXPECTED_IDENTIFIER" "$EXPECTED_LEAF_SHA" -} - -classify_keychain_dump() { - perl -Mstrict -Mwarnings -e ' - my ($service, $dr) = @ARGV; - my $block = ""; - - sub flush_block { - my ($block, $service, $dr) = @_; - return if $block !~ /\Q"svce"="$service"\E/; - return if $block !~ /^class: "genp"$/m; - my ($account) = $block =~ /"acct"="([^"]*)"/; - return if !defined $account || $account eq ""; - my @reqs = $block =~ /requirement: ([^\n]*)/g; - my $has_current = grep { lc($_) eq lc($dr) } @reqs; - my $has_cdhash = grep { /cdhash H"/i } @reqs; - my $state = $has_current ? ($has_cdhash ? "stable+stale-cdhash" : "stable") : - ($has_cdhash ? "cdhash-only" : "missing-current-cr"); - print "$account\t$state\n"; - } - - while (my $line = ) { - if ($line =~ /^keychain:/) { - flush_block($block, $service, $dr); - $block = ""; - next; - } - $block .= $line; - } - flush_block($block, $service, $dr); - ' "$1" "$2" -} - -run_self_test() { - command -v perl >/dev/null || { echo "perl not found" >&2; exit 2; } - - local dr fixture actual expected - dr="$(expected_dr)" - fixture=$(cat <="codereview" - "type"= - "acct"="stable/git_token" - requirement: $dr -keychain: "/tmp/login.keychain-db" -class: "genp" - "svce"="codereview" - "type"= - "acct"="mixed/git_token" - requirement: $dr - requirement: cdhash H"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" -keychain: "/tmp/login.keychain-db" -class: "genp" - "svce"="codereview" - "type"= - "acct"="old/git_token" - requirement: cdhash H"bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb" -keychain: "/tmp/login.keychain-db" -class: "genp" - "svce"="codereview" - "type"= - "acct"="missing/git_token" - requirement: identifier "other.tool" and certificate leaf = H"cccccccccccccccccccccccccccccccccccccccc" -keychain: "/tmp/login.keychain-db" -class: "genp" - "svce"="other-service" - "type"= - "acct"="ignored/git_token" - requirement: cdhash H"dddddddddddddddddddddddddddddddddddddddd" -keychain: "/tmp/login.keychain-db" -class: "genp" - "svce"="codereview" - "type"= - requirement: cdhash H"eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee" -keychain: "/tmp/login.keychain-db" -class: 0x0000000F - "svce"="codereview" - "acct"="not-generic/git_token" - requirement: cdhash H"ffffffffffffffffffffffffffffffffffffffff" -EOF -) - expected=$(cat <<'EOF' -missing/git_token missing-current-cr -mixed/git_token stable+stale-cdhash -old/git_token cdhash-only -stable/git_token stable -EOF -) - actual="$(printf '%s\n' "$fixture" | classify_keychain_dump "$DEFAULT_SERVICE" "$dr" | sort)" - if [ "$actual" != "$expected" ]; then - echo "self-test failed: classifier output mismatch" >&2 - printf 'expected:\n%s\n' "$expected" >&2 - printf 'actual:\n%s\n' "$actual" >&2 - exit 1 - fi - - actual="$(printf '%s\n' 'malformed output without keychain blocks' | classify_keychain_dump "$DEFAULT_SERVICE" "$dr" | sort)" - if [ -n "$actual" ]; then - echo "self-test failed: malformed output should produce no matches" >&2 - printf 'actual:\n%s\n' "$actual" >&2 - exit 1 - fi - - echo "self-test OK" -} - -build_helper() { - command -v cc >/dev/null || { echo "cc not found; install Xcode command line tools" >&2; exit 2; } - ensure_work_tmp - helper="$work_tmp/kc_acl_set" - - cat >"$work_tmp/kc_acl_set.c" <<'C' -#include -#include -#include -#include - -static void print_status(const char *what, OSStatus st) { - fprintf(stderr, "%s: %d\n", what, (int)st); -} - -static int authorizations_contain(CFArrayRef auths, CFStringRef target) { - if (auths == NULL) { - return 0; - } - CFRange range = CFRangeMake(0, CFArrayGetCount(auths)); - return CFArrayContainsValue(auths, range, target); -} - -int main(int argc, char **argv) { - if (argc != 5) { - fprintf(stderr, "usage: kc_acl_set \n"); - return 2; - } - - const char *kc_path = argv[1]; - const char *service = argv[2]; - const char *account = argv[3]; - const char *app_path = argv[4]; - - SecKeychainRef kc = NULL; - OSStatus st = SecKeychainOpen(kc_path, &kc); - if (st != errSecSuccess) { - print_status("SecKeychainOpen", st); - return 3; - } - - SecKeychainItemRef item = NULL; - st = SecKeychainFindGenericPassword( - kc, (UInt32)strlen(service), service, (UInt32)strlen(account), account, - NULL, NULL, &item); - if (st != errSecSuccess) { - print_status("SecKeychainFindGenericPassword", st); - CFRelease(kc); - return 4; - } - - SecTrustedApplicationRef app = NULL; - st = SecTrustedApplicationCreateFromPath(app_path, &app); - if (st != errSecSuccess) { - print_status("SecTrustedApplicationCreateFromPath", st); - CFRelease(item); - CFRelease(kc); - return 5; - } - - CFStringRef fallback_label = - CFStringCreateWithCString(kCFAllocatorDefault, service, kCFStringEncodingUTF8); - - SecAccessRef access = NULL; - st = SecKeychainItemCopyAccess(item, &access); - if (st != errSecSuccess) { - print_status("SecKeychainItemCopyAccess", st); - CFRelease(fallback_label); - CFRelease(app); - CFRelease(item); - CFRelease(kc); - return 6; - } - - CFArrayRef acl_list = NULL; - st = SecAccessCopyACLList(access, &acl_list); - if (st != errSecSuccess) { - print_status("SecAccessCopyACLList", st); - CFRelease(access); - CFRelease(fallback_label); - CFRelease(app); - CFRelease(item); - CFRelease(kc); - return 7; - } - - int changed = 0; - CFIndex acl_count = CFArrayGetCount(acl_list); - for (CFIndex i = 0; i < acl_count; i++) { - SecACLRef acl = (SecACLRef)CFArrayGetValueAtIndex(acl_list, i); - CFArrayRef auths = SecACLCopyAuthorizations(acl); - if (!authorizations_contain(auths, kSecACLAuthorizationDecrypt)) { - if (auths != NULL) { - CFRelease(auths); - } - continue; - } - - CFArrayRef existing_apps = NULL; - CFStringRef description = NULL; - SecKeychainPromptSelector prompt_selector = 0; - st = SecACLCopyContents(acl, &existing_apps, &description, &prompt_selector); - if (st != errSecSuccess) { - print_status("SecACLCopyContents", st); - if (auths != NULL) { - CFRelease(auths); - } - CFRelease(acl_list); - CFRelease(access); - CFRelease(fallback_label); - CFRelease(app); - CFRelease(item); - CFRelease(kc); - return 8; - } - - if (existing_apps == NULL) { - if (description != NULL) { - CFRelease(description); - } - if (auths != NULL) { - CFRelease(auths); - } - continue; - } - - CFMutableArrayRef new_apps = - CFArrayCreateMutableCopy(kCFAllocatorDefault, 0, existing_apps); - CFArrayAppendValue(new_apps, app); - st = SecACLSetContents(acl, new_apps, - description == NULL ? fallback_label : description, - prompt_selector); - if (st != errSecSuccess) { - print_status("SecACLSetContents", st); - CFRelease(new_apps); - if (description != NULL) { - CFRelease(description); - } - if (existing_apps != NULL) { - CFRelease(existing_apps); - } - if (auths != NULL) { - CFRelease(auths); - } - CFRelease(acl_list); - CFRelease(access); - CFRelease(fallback_label); - CFRelease(app); - CFRelease(item); - CFRelease(kc); - return 9; - } - changed = 1; - - CFRelease(new_apps); - if (description != NULL) { - CFRelease(description); - } - if (existing_apps != NULL) { - CFRelease(existing_apps); - } - if (auths != NULL) { - CFRelease(auths); - } - } - - if (!changed) { - fprintf(stderr, "no decrypt-capable ACL found for item\n"); - CFRelease(acl_list); - CFRelease(access); - CFRelease(fallback_label); - CFRelease(app); - CFRelease(item); - CFRelease(kc); - return 10; - } - - st = SecKeychainItemSetAccess(item, access); - if (st != errSecSuccess) { - print_status("SecKeychainItemSetAccess", st); - CFRelease(acl_list); - CFRelease(access); - CFRelease(fallback_label); - CFRelease(app); - CFRelease(item); - CFRelease(kc); - return 11; - } - - CFRelease(acl_list); - CFRelease(access); - CFRelease(fallback_label); - CFRelease(app); - CFRelease(item); - CFRelease(kc); - return 0; -} -C - - echo "Building temporary ACL helper..." >&2 - cc "$work_tmp/kc_acl_set.c" -Wno-deprecated-declarations \ - -framework Security -framework CoreFoundation -o "$helper" >/dev/null -} - -cleanup() { - if [ -n "$work_tmp" ]; then - rm -rf "$work_tmp" - fi - if [ -n "$items_file" ]; then - rm -f "$items_file" - fi -} - -ensure_work_tmp() { - if [ -z "$work_tmp" ]; then - work_tmp="$(mktemp -d)" - fi -} - -apply=0 -self_test=0 -service="$DEFAULT_SERVICE" -keychain="$HOME/Library/Keychains/login.keychain-db" -cr_bin="" -helper="" -work_tmp="" -items_file="" -trap cleanup EXIT - -while [ "$#" -gt 0 ]; do - case "$1" in - --apply) - apply=1 - shift - ;; - --self-test) - self_test=1 - shift - ;; - --service) - service="${2:?--service requires a value}" - shift 2 - ;; - --keychain) - keychain="${2:?--keychain requires a value}" - shift 2 - ;; - --cr) - cr_bin="${2:?--cr requires a value}" - shift 2 - ;; - -h|--help) - usage - exit 0 - ;; - *) - echo "unknown argument: $1" >&2 - usage >&2 - exit 2 - ;; - esac -done - -if [ "$self_test" -eq 1 ]; then - run_self_test - exit 0 -fi - -if [ "$(uname -s)" != "Darwin" ]; then - echo "This helper only supports macOS Keychain on Darwin." >&2 - exit 2 -fi - -if [ "${EUID:-$(id -u)}" -eq 0 ]; then - echo "Do not run this with sudo; run it as the user who owns the login Keychain." >&2 - exit 2 -fi - -command -v codesign >/dev/null || { echo "codesign not found" >&2; exit 2; } -command -v security >/dev/null || { echo "security not found" >&2; exit 2; } -command -v perl >/dev/null || { echo "perl not found" >&2; exit 2; } - -if [ -z "$cr_bin" ]; then - cr_bin="$(command -v cr || true)" -fi -[ -n "$cr_bin" ] || { echo "cr not found on PATH; pass --cr /path/to/cr" >&2; exit 2; } -[ -x "$cr_bin" ] || { echo "cr is not executable: $cr_bin" >&2; exit 2; } -[ -f "$keychain" ] || { echo "keychain not found: $keychain" >&2; exit 2; } - -dr="$(codesign -d -r- "$cr_bin" 2>&1 | sed -n 's/^designated => //p' | head -n 1)" -expected="$(expected_dr)" -[ -n "$dr" ] || { echo "could not read designated requirement from $cr_bin" >&2; exit 1; } -if [ "$(lower "$dr")" != "$(lower "$expected")" ]; then - echo "refusing to repair with unexpected cr designated requirement" >&2 - echo "expected: $expected" >&2 - echo "actual: $dr" >&2 - exit 1 -fi - -list_items() { - ensure_work_tmp - local dump_file err_file - dump_file="$work_tmp/keychain.dump" - err_file="$work_tmp/keychain.err" - if ! security dump-keychain -a "$keychain" >"$dump_file" 2>"$err_file"; then - echo "failed to scan Keychain ACL metadata: $keychain" >&2 - if [ -s "$err_file" ]; then - sed 's/^/ /' "$err_file" >&2 - fi - return 1 - fi - classify_keychain_dump "$service" "$expected" <"$dump_file" -} - -all_items=() -echo "Scanning Keychain ACL metadata for service '$service' in $keychain ..." >&2 -echo "This can take 10-30 seconds on a large login Keychain." >&2 -items_file="$(mktemp)" -list_items | sort -u >"$items_file" -while IFS= read -r item; do - all_items+=("$item") -done <"$items_file" -echo "Scan complete." >&2 - -if [ "${#all_items[@]}" -eq 0 ]; then - echo "No generic-password items found for service '$service' in $keychain." - exit 0 -fi - -echo "Current cr: $cr_bin" -echo "Current DR: $dr" -echo -echo "Items:" - -targets=() -manual=() -for item in "${all_items[@]}"; do - account="${item%%$'\t'*}" - state="${item#*$'\t'}" - printf ' %-24s %s\n' "$state" "$account" - case "$state" in - stable|stable+stale-cdhash) - ;; - cdhash-only) - targets+=("$account") - ;; - *) - manual+=("$account") - ;; - esac -done - -if [ "${#manual[@]}" -gt 0 ]; then - echo - echo "Manual inspection required for ${#manual[@]} non-cdhash item(s) missing current cr trust:" - for account in "${manual[@]}"; do - printf ' %s\n' "$account" - done -fi - -if [ "${#targets[@]}" -eq 0 ]; then - echo - echo "No cdhash-only items to repair. stable+stale-cdhash means current cr is trusted;" - echo "macOS is only still reporting older cdhash grants/partition metadata." - exit 0 -fi - -echo -if [ "$apply" -ne 1 ]; then - echo "Dry-run only. Re-run with --apply to repair ${#targets[@]} item(s)." - exit 0 -fi - -echo "Repairing ${#targets[@]} item(s). macOS may prompt for Keychain authorization." -build_helper -failures=0 -for account in "${targets[@]}"; do - printf ' repairing %s ... ' "$account" - if "$helper" "$keychain" "$service" "$account" "$cr_bin"; then - echo "ok" - else - echo "failed" - failures=$((failures + 1)) - fi -done - -echo -if [ "$failures" -ne 0 ]; then - echo "$failures item(s) failed to repair." >&2 - exit 1 -fi - -echo "Repair complete. Re-run without --apply to verify all items report stable or stable+stale-cdhash." From 0c771b799af742b30b461f1a0fd48376c0c3d0e3 Mon Sep 17 00:00:00 2001 From: Rian Stockbower Date: Wed, 1 Jul 2026 12:03:39 -0400 Subject: [PATCH 2/2] docs: clarify shared keychain helper source --- scripts/README.md | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/scripts/README.md b/scripts/README.md index 10f004c1..93eac87a 100644 --- a/scripts/README.md +++ b/scripts/README.md @@ -4,7 +4,13 @@ Repository support and verification tools. These scripts are not part of the shipped `cr` command surface. macOS Keychain credential ACL repair is shared across the Open CLI Collective -tool family. Use the canonical helper from the sibling `cli-common` checkout: +tool family. The canonical helper lives in +`open-cli-collective/cli-common`: + +- Source of truth: + +- If you already have `../cli-common` checked out locally, you can run it from + the sibling checkout: ```bash ../cli-common/scripts/repair-macos-keychain-credentials.sh --tool cr