Fix audit round-1 findings: README defaults, symbol count, release-signing pin, manifest name, release-create fallback - #1
Merged
Conversation
- role_claim's documented default was "—" (absent); the struct actually defaults it to "groups" via #[serde(default = "default_role_claim")], so an operator who omits it silently gets groups-claim role mapping. - add the undocumented jwks_min_refetch_secs (default 60) and jwks_ttl_secs (default 3600) config fields to the table. - the SDK's export_auth_plugin! macro emits six extern-C symbols (busbar_abi, busbar_plugin_kind, busbar_open, busbar_call, busbar_free, busbar_close), not five as both README.md and auth-oidc-plugin/src/lib.rs claimed.
assert!(!err.is_empty()) was nearly vacuous — expect_err() already
proves open() returned Err, so this only re-asserted the error string
isn't literally empty. Assert it actually names the failed step
("OIDC discovery fetch failed"), matching resolve_jwks_url's error
format, so the test would catch a regression that replaced the
descriptive message with an unrelated (but non-empty) one.
…narrow release-create fallback
- BUSBAR_REF was a mutable branch name (1.5.0-dev) checked out in the
same job that builds+runs busbar-plugin-pack and holds
BUSBAR_SIGN_KEY (with id-token: write) — anyone able to push that
branch could reach this repo's signing key. Pin to the branch's
current tip SHA instead, with a comment on what's needed to bump it
and why crates/auth-oidc being absent there doesn't matter (this
repo's OIDC logic now lives in its own auth-oidc/ crate, not
busbarAI/crates/auth-oidc).
- the signing step packed the manifest as "busbar-auth-oidc", disagreeing
with the CI signoff (plugin-ci.yml's --name "${{ inputs.plugin_crate }}"
= busbar-auth-oidc-plugin, from ci.yml) and the README's documented
pack invocation. Since manifest.name is load-bearing for the plugin
registry's resolve()/conflicts() matching, this meant CI's signoff
proved a different artifact identity than what release.yml actually
ships. Correct the outlier to busbar-auth-oidc-plugin (matching the
majority) and align the output tarball filename to match.
- gh release create ... || gh release view ... swallowed ANY create
failure (auth errors, --verify-tag rejection, --generate-notes
errors) whenever a release object with that tag already existed, not
just the intended "already exists" race between parallel/retried
runs. Capture the failure output and only fall through to
`gh release view` when it actually says "already exists"; any other
failure now fails the job.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Re-validated the round-1 audit ledger against current
origin/main(post-restructure:busbar-auth-oidcis now a same-repo sibling crate underauth-oidc/, no longer abusbarAI/crates/auth-oidcpath dependency) and fixed everything that checked out as genuinely real.role_claimdefault: documented as absent ("—"); the struct actually defaults it to"groups"via#[serde(default = "default_role_claim")]. Fixed the table.jwks_min_refetch_secs(default 60) andjwks_ttl_secs(default 3600) are realOidcConfigfields, added to the README table.export_auth_plugin!actually emitsbusbar_abi,busbar_plugin_kind,busbar_open,busbar_call,busbar_free,busbar_close. Fixed both README.md andauth-oidc-plugin/src/lib.rs's doc comment, listing all six.assert!(!err.is_empty())inauth-oidc-plugin/src/tests.rsonly re-proved whatexpect_erralready established. Strengthened to assert the message actually names the failed step ("OIDC discovery fetch failed").release.ymlsigning step checked out a mutable branch (BUSBAR_REF: 1.5.0-dev) in the same job that builds+runsbusbar-plugin-packand holdsBUSBAR_SIGN_KEY+id-token: write. Pinned to that branch's tip SHA at time of fix, with a comment on what to verify before bumping it. Note:crates/auth-oidcis indeed absent at that SHA (a separately-tracked, out-of-scope upstream extraction issue), but it no longer matters — this repo's OIDC logic now lives in its ownauth-oidc/crate; the release build only needscrates/api,crates/plugin-sdk,crates/plugin-loader,crates/plugin-abi, andcrates/plugin-pack, all of which are present at the pinned SHA (verified via the GitHub API).release.yml's signing step packed--name "busbar-auth-oidc", disagreeing with both the README's documented invocation and CI's own signoff (plugin-ci.yml's--name "${{ inputs.plugin_crate }}", fedbusbar-auth-oidc-pluginfromci.yml). Sincemanifest.nameis load-bearing for plugin-registry matching, this meant CI's signoff proved a different artifact identity than what actually ships. Corrected the outlier tobusbar-auth-oidc-plugin(majority) and aligned the output tarball filename to match.gh release create ... || gh release view ...fallback swallowed any create failure (auth errors,--verify-tagrejection,--generate-noteserrors), not just the intended "already exists" race. Narrowed it to only fall through when the failure output actually says "already exists"; anything else now fails the job.Left alone (out of scope / already correct):
--allow-unsignedwhenBUSBAR_SIGN_KEYisn't provisioned) — documented product decision (TODO(release-keys)), not touched.crates/auth-oidcextraction issue upstream — someone else is handling it; it's also no longer relevant to this repo's build now that the crate lives here.Stale/incorrect in the original ledger
The pinned-audit's premise for the release-signing finding — "verify
crates/auth-oidcexists at whatever SHA you pin to" — turned out to be moot: between the audit's pin andorigin/main, the repo was restructured sobusbar-auth-oidc's real logic moved into this repo's ownauth-oidc/crate (workspace member), and the plugin only depends onbusbarAI/crates/{api,plugin-sdk,plugin-loader,plugin-abi,plugin-pack}now, notcrates/auth-oidc. All of those are present at the pinned SHA, so the pin doesn't need to fall back to an older commit.Test plan
cargo fmt --check— cleancargo build --all-targets— succeeds (the../../busbarAI/crates/*path deps resolve against the localbusbarAI/busbarAIcheckout)cargo clippy --all-targets -- -D warnings— cleancargo test— 38/38 tests pass (28 inauth-oidc, 8 unit + 2 e2e inauth-oidc-plugin), including the strengthened discovery-failure assertionpython3 -c "import yaml; yaml.safe_load(...)"onrelease.yml— valid YAML