configure-repo: check/grant org secret visibility - #122
Conversation
27ff0d9 to
8d34398
Compare
Robie Basak (basak-qcom)
left a comment
There was a problem hiding this comment.
My intention in the separation of configure-repo and set-repo-secrets is that configure-repo merely requires ACLs inside GitHub, whereas set-repo-secrets needs access to password vault, etc. In this model I see set-repo-secrets as only relevant to actual setting of secrets, not all secrets operations, and configuration of visibility of a secret belongs in configure-repo really. This makes it convenient to run configure-repo without having to fetch the secrets from the vault. Otherwise your org secret setting wouldn't get run during configure-repo, which I think would be suboptimal (if in theory it's run by an org admin, which I guess is a use case you care about since you're adding that functionality).
Please could you move it across? Feel free to update README.md to clarify if the distinction is non-obvious to you.
Claude also reports a pagination issue that seems plausible to me. Please investigate:
1. Pagination — real false-negative risk (both files)
get_visible_org_secrets reads only the first page:
stdout = run_gh_command(["api", f"repos/{repo}/actions/organization-secrets"])
data = json.loads(stdout)
return [secret["name"] for secret in data.get("secrets", [])]gh api does not paginate automatically. The organization-secrets endpoint returns {total_count, secrets:[...]} with per_page=30 by default. In an org with >30 secrets visible to a repo, a secret that is
visible can fall off the first page — so configure-repo reports a false ❌, and set-repo-secrets attempts a
redundant (harmless but confusing) grant. qualcomm-linux plausibly has more than 30 org secrets.
Cheap fix, e.g.:
stdout = run_gh_command(
["api", "--paginate", f"repos/{repo}/actions/organization-secrets",
"-q", ".secrets[].name"]
)
return stdout.split()(With --paginate -q, gh streams each page's names; no wrapper-object concatenation problem.) This is the one item
I'd consider blocking, since it undermines the check's core purpose.
8d34398 to
4342e6b
Compare
4342e6b to
0a4e411
Compare
Add a check that the organization-level secrets DEB_PKG_BOT_CI_QSC_TOKEN
and DEB_PKG_BOT_CI_TOKEN (used by reusable workflows/scripts and the
Ubuntu apt artifactory upload step) are visible to the repository, and
grant that visibility automatically if the caller's own gh auth carries
the admin:org scope. Uses the dedicated
PUT .../actions/secrets/{name}/repositories/{repo_id} endpoint, which
only adds this one repository to the secret's existing access list and
never touches the secret's value, so no admin:org-scoped write ever
needs to know or resupply it. If granting fails (most likely: missing
admin:org), this reports it and continues rather than blocking the rest
of what configure-repo configures.
This lives in configure-repo rather than set-repo-secrets: granting org
secret visibility is a GitHub ACL change, not a secret value, so it
never needs vault access the way set-repo-secrets' actual secret-value
operations do. README.md now spells out that distinction explicitly.
get_visible_org_secrets uses --paginate: the organization-secrets
endpoint returns 30 secrets per page by default and gh does not
paginate automatically, so a secret visible to the repository but
sitting past the first page would otherwise read as a false negative.
Splitting the paginated output on whitespace relies on secret names
never containing spaces, which GitHub itself enforces (alphanumeric and
underscore only) - flagged as a fragile assumption in a comment.
Reuses repo_info's own id field for the grant call instead of a
separate get_repo_id lookup - configure_repository() already fetches
repo_info earlier, so a dedicated function/call was redundant.
Confirmed against the real API: get_visible_org_secrets (with
pagination), and grant_org_secret_access were each verified directly
against pkg-example; the grant endpoint 403s with our current gh auth
(lacks admin:org) and is caught gracefully.
Signed-off-by: Simon Beaudoin <sbeaudoi@qti.qualcomm.com>
0a4e411 to
82c0af1
Compare
Summary
Adds a check that the organization-level secrets
DEB_PKG_BOT_CI_QSC_TOKENand
DEB_PKG_BOT_CI_TOKENare visible to the repository (i.e. thesecret's organization visibility setting is "All repositories", or
"Selected repositories" with this repository selected), and grants
that visibility automatically if the caller's own
ghauth carriesthe
admin:orgscope — using the dedicatedPUT .../actions/secrets/{name}/repositories/{repo_id}endpoint, whichonly adds this one repository to the secret's existing access list and
never touches the secret's value. If granting fails (most likely:
missing
admin:org), this reports it and continues rather thanblocking the rest of what
configure-repoconfigures.This lives in
configure-reporather thanset-repo-secrets: grantingorg secret visibility is a GitHub ACL change, not a secret value, so it
never needs vault access the way
set-repo-secrets' actualsecret-value operations do.
README.mdnow spells out thatdistinction explicitly.
get_visible_org_secretsuses--paginate: the organization-secretsendpoint returns 30 secrets per page by default and
ghdoes notpaginate automatically, so a secret visible to the repository but
sitting past the first page would otherwise read as a false negative.
Review history
Originally this PR put the granting capability in
set-repo-secrets.Per review feedback:
set-repo-secretstoconfigure-repo(set-repo-secretsis reserved for actual secretvalues, which this never touches).
get_visible_org_secretsthat could causefalse negatives in orgs with more than 30 secrets visible to a repo.
Squashed to a single commit.
Validation
python3 -m py_compile tools/configure-repo tools/set-repo-secrets./tools/configure-repo --check pkg-example— both secrets report ✓./tools/set-repo-secrets --check pkg-example— no org-secretmention at all (fully reverted to its original scope)
get_visible_org_secrets(withpagination),
get_repo_id, andgrant_org_secret_accessverifieddirectly against
pkg-example; the grant endpoint 403s with ourcurrent
ghauth (lacksadmin:org) and is caught gracefully