Register Buildkite as an OCEAN source - #19
Closed
p4gs wants to merge 7 commits into
Closed
Conversation
…controls Buildkite had zero presence in OCEAN — no modules, no checks, no controls, and it was absent from KNOWN_SOURCES. Registration (src/fleet/manifest.rs): adds the buildkite arm to allowed_credentials and to KNOWN_SOURCES. Worth recording that KNOWN_SOURCES is narrower than it looks — it is referenced only in this file and gates fleet manifest targets, not check loading. checks/ already ships 25 vendor directories against a four-entry list, so the 12 checks here never depended on this edit; it enables Buildkite as a fleet target. Checks (checks/buildkite/, 12): organization 2FA and public visibility, API IP allowlist and inactive-token revocation, pipeline visibility and fork builds, agent-token expiry and IP restriction, cluster secret policies, cross-pipeline rules inventory, admin count bounds, cluster maintainer bounds. Two structural constraints shaped every one of them: jsonpath_extract resolves only at the root array, so nested Relay traversal (edges -> node -> field) cannot be expressed. GraphQL checks therefore read only scalar Organization fields or connection counts, and anything list-shaped goes through REST, whose responses are root arrays. Admin enumeration is REST for exactly this reason, compounded by GraphQL's members connection having no role filter. implementation: native is vestigial — the field exists on the struct but nothing dispatches on it, so no check declares it. Native logic has to be reached through a control's observers/testers refs instead. Controls (controls/cicd/, 5): identity, API token posture, pipeline exposure, agent credentials, authorization boundaries. Authored against the shipped YAMLs and the Rust struct rather than schemas/control.schema.json, which disagrees with both — the schema requires evaluation_logic and offers collectors, while every shipped YAML uses evaluation with observers/testers, and the struct omits threat_mitigated and evidence_requirements that the YAMLs carry. A control written strictly to the published schema will not load. Every control description states the scope ceiling explicitly: roughly 40% of the Buildkite hardening surface is agent-side configuration (no-command-eval, allowed-plugins, redacted-vars, verification-jwks-file, pre-bootstrap hooks) and is invisible to any control plane. It can only be attested from the agent host. Omitting that would let a passing result overstate what OCEAN actually covers. Enterprise gating is handled as a reporting distinction rather than a silent pass: inactive-token revocation is readable on any plan but settable only on Enterprise, so on lower plans the finding is a real gap the operator cannot remediate without a plan change, and the check says so. Verified: all 12 checks and 5 controls parse; no key falls outside the Rust struct; no check uses a nested Relay extract or declares implementation: native; every credential name is in allowed_credentials; every check carries an hth reference back to the guide control it mirrors. Not verified: cargo check fails with three pre-existing E0063 errors in okta_pr_mfa_downgrade.rs, okta_default_policy_bypass.rs and storage/sqlite.rs — the sibling grc-controls Evidence struct gained connected_account, population and evaluation fields that this crate does not set. No error references fleet/manifest.rs; the breakage predates this change.
Two defects in the controls as first committed, both of which produced the "hollow shape" this integration exists to avoid — a control that looks correctly wired and can never return a finding. Phantom observer ids. The controls referenced buildkite.org_mfa, buildkite.pipeline_visibility and similar dotted ids, copying the convention of the shipped controls. But those reference NATIVE Rust modules, and there are zero buildkite.* observers in the registry. register_check (src/check/loader.rs) registers a passive YAML check as a YamlObserver keyed on def.id, and YamlObserver::id() returns that id verbatim, so the only ids that can resolve for a YAML-backed check are the check ids themselves. Rewired all 12 to BK-*. The failure was non-crashing and total: src/cli/mod.rs catches the lookup error per-module as status ERROR, leaving zero evidence and a fail-closed "no evidence available". Unpassable CEL. The evaluation copied `&& has_active` from branch_protection. src/eval/engine.rs:76 binds has_active to active_count > 0, where active counts evidence at ConfidenceLevel::ActiveVerification — produced only by testers. All five controls are observer-only and every BK check is Passive by default, so active_count is structurally always zero and the expression could not return true on a perfectly hardened tenant. Dropped it, which matches the shipped convention exactly: every control with testers: 0 (scm/actions_security, commit_signing, code_scanning, dependency_management, frameworks/soc2) omits has_active, and only the three with testers use it. Also documents an invocation limitation in each control rather than leaving it to be discovered. These resolve under --target '*' but not --target buildkite, because target_matches_module compares the first dot-separated segment of a module_id and "BK-1" is never "buildkite". This is a property of the YAML-check namespace rather than of Buildkite — GH-*, OKTA-*, SLACK-* and VERCEL-* behave identically. Using dotted buildkite.* ids would satisfy the target filter while resolving to nothing, which is the worse of the two failures. Closing it properly means native Rust observers, or registering YAML checks under a vendor alias. Verified: 12 checks to 12 observer refs, no phantoms, no orphans, no duplicates; all 17 YAMLs parse; the parity ceiling and the new invocation note are present in all five descriptions.
An independent adversarial review, then a second-round verifier built against the pinned cel-interpreter 0.10.0, found the checks committed in be83ec2 were incomplete in a way that produced two opposite failure modes on the same class of input. GraphQL answers request-level errors with HTTP 200 and either a null data key or no data key at all — the standard shape, not an edge case; BK-1.02's own description names an Enterprise-gated field on a lower plan as a trigger, which is exactly this. The four GraphQL-backed checks (BK-1.02, BK-2.05a, BK-2.05b, BK-4.01) only guarded against a resolved-but-null organization, so on the standard error shape the extracted field was unbound, the CEL expression raised, and the fail-closed default re-emitted the accusation the checks exist to make only when actually warranted — a critical "2FA is NOT enforced" finding against an organization that was never successfully read. The fix a prior pass applied to unblock this closed that hole but opened a worse one: because OCEAN emits an assertion's pass_message on every Effective status, and abstaining IS Effective, nine assertions started asserting facts they had not observed. On a 404, $length's non-array fallback binds a count of 1, so the token-expiry check would print "All 1 agent token(s) in cluster X carry an expiry" for a cluster whose token list could not be read. A compliance check that affirmatively attests to a control it never observed is worse than one that wrongly accuses — the accusation gets investigated and disproven; the attestation is trusted and closes the loop on nothing. Both are fixed together, verified by an independent probe crate that ports the real interpreter (jsonpath_extract, navigate_fields, evaluate_assertion, the unwrap_or(false) fail-closed default) against the pinned CEL version and drives the shipped YAML rather than reasoning about it: baseline 10 false accusations and 14 false attestations, final 128/128 pass across all nine body shapes per check (transport failure, 200/data-null-with-errors, 200/no-data-key, 200/organization-null, and true-positive/true-negative). The structural fix: every GraphQL-backed assertion now extracts the response root ($, which jsonpath_extract always binds) and guards on !has(root.data) || root.data == null || !has(root.data.organization) || root.data.organization == null before dereferencing anything nested — closing a further hole the brief's first proposal missed (data present, organization key absent still raised). Twelve assertions across all twelve checks — not the nine originally scoped — had the abstain-emits-pass_message defect; each message now states what was checked and that no violation was observed, hedged on the sibling reachability assertion also having passed, rather than asserting the compliance fact outright. Four assertions had hardcoded thresholds in their messages that would go stale the moment an operator overrode the bound via input; they now interpolate the actual input value. Four residual fail_messages still used the retired UNKNOWN framing this convention replaced; corrected. Two cross-cutting fixes land alongside: BUILDKITE_API_TOKEN was on the fleet credential allowlist but absent from the masking plane in src/harden/mod.rs — a credential that is allowed but not masked can reach stdout, the dry-run JSON, and the audit log verbatim. And the remediation-URL allowlist rejected every Buildkite API host, which meant BK-2.03's suggested remediation call could never authenticate; both api.buildkite.com and graphql.buildkite.com are now accepted. New: tests/check_buildkite_parity.rs, mirroring tests/check_okta_parity.rs, including a case that pins Effective plus no finding plus no unrendered template plus no forbidden affirmative claim on every abstained path — so a future edit that reintroduces an affirmative pass_message on an unreachable endpoint fails the suite instead of passing it. Also regenerates parity/hth-parity.json (was stale after be83ec2 added checks without regenerating it, which fails the parity validate CI gate at ci.yml:335) and patches scripts/hth_parity.py to record the HTH checkout path relative to this repo rather than as the invoking machine's absolute home directory, closing a path leak the prior manifest already carried into this public repo. Verified: parity validate: OK (254 mapped checks consistent); every check and control still parses; zero implementation: native declarations; parity-ceiling and INVOCATION paragraphs intact in all five controls; rustfmt --check clean on every touched file. Not verified: cargo check/test still fail on the pre-existing sibling grc-controls Evidence-struct drift (connected_account/population/evaluation), 142 errors before and after this commit, none referencing any file it touches. The new test file is therefore syntax-verified only, not executed.
The fresh-context review dispatched after 33746c8 found that fix was correctly scoped to the 4 GraphQL-backed checks but left the same failure class live in 7 REST-backed checks the same commit touched: BK-2.02, BK-2.02b, BK-2.03, BK-2.07, BK-3.01a, BK-3.01b, BK-3.05. Root cause, confirmed by execution against the real interpreter code, not by reading the YAML: each check's reachability assertion guards only on status_code != 200. On an HTTP 200 whose body is not a JSON array — an error object, an empty object, a non-JSON error string, which any of these endpoints can return — the $[*] wildcard extraction never binds, referencing it in the compliance assertion raises, and the interpreter's fail-closed unwrap_or(false) turns that into a hard accusation the reachability assertion doesn't catch either, because it only checked the status code. Reproduced independently: the identical scenario against the pre-fix interpreter produces the exact false accusation on all 7 checks, e.g. "More than 3 organization members hold the admin role" against a body that was never actually read. Fixed with the same shape as the GraphQL guard: a new always-bound JSONPath form, $is_array (cel-interpreter 0.10.0 has no type() builtin to do this any other way), extracted as body_is_array in each check, folded into both the reachability assertion (status == 200 && body_is_array) and the compliance assertion's short-circuit chain. BK-3.07 was left alone — it only ever uses $length, which stays bound regardless of body shape, so it was never exposed to this class. Also fixes the rustfmt drift the same review flagged in src/harden/mod.rs: 11 hunks this repo's own new code introduced, corrected surgically rather than via a blanket rustfmt pass, which would have also silently reformatted 29 unrelated pre-existing mismatches in the same file. Verified by an independent probe (not the fixer's own) built by copying the real jsonpath_extract/evaluate_assertion_with_inputs/build_input_context byte-for-byte from src/check/interpreter.rs and driving the real shipped YAML: 35 scenarios across the 7 checks (empty array, populated true-positive and true-negative, non-array body, 404), zero violations; every abstain-path pass_message rendered and read, none asserts an unobserved fact; the false-accusation reproduced cleanly against the pre-fix code as a baseline control. cargo check --all-targets error count unchanged at 142 before and after, fingerprinted by (file, occurrence-count) rather than line number since this diff shifts lines — all pre-existing sibling grc-controls Evidence-struct drift, zero new errors. parity/hth-parity.json and controls/ untouched by this diff; hth_parity.py --validate still OK.
… and PR #19 Root-caused every distinct CI failure category (7 parallel diagnosis agents, each backed by real logs/local reproduction, never inferred from job names) and fixed everything genuinely code-fixable without changing runtime behavior. Fixed: - Evidence-struct drift: sibling grc-controls's Evidence gained 4 required fields (schema_version, connected_account, population, evaluation). 71 construction sites updated; 70 use EVIDENCE_SCHEMA_VERSION + None (the sibling crate's own documented convention), 1 (storage/sqlite.rs scan_evidence, reading persisted pre-custody rows) correctly uses PRE_CUSTODY_SCHEMA instead. - fuzz.yml: added the sibling ../grc-controls checkout step every other workflow already has (verbatim from ci.yml) — fixes all 3 fuzz matrix jobs. - CodeQL: deleted the redundant, permanently-conflicting advanced-config codeql.yml (actions-only, sscsb-bootstrap leftover); GitHub's broader default-setup scan (actions+python+rust) already covers this and passes. - osv-scanner: cargo update -p anyhow --precise 1.0.103 and -p ratatui --precise 0.30.2 (drags ratatui-core -> lru 0.18.2) — both in-range Cargo.lock-only bumps, clear RUSTSEC-2026-0190/0253. paste's RUSTSEC-2024-0436 has no upstream fix (cel-interpreter 0.10.0, latest, still requires it) — left as an owner risk-acceptance decision, not applied. - cargo-deny: swapped the Docker-based action (which can't see the sibling checkout one directory up) for a native install+run, mirroring the already-working vet job. - Lint/clippy: 37 pre-existing lints fixed (redundant borrows, two real FromStr impls replacing shadowed inherent methods, two params-struct refactors for too_many_arguments, tempfile::TempDir idiom in 12 spots), plus 4 more that only became visible once the Evidence-struct fix unblocked compilation (a genuinely-dead CheckResult sarif field trio and two more too_many_arguments functions needing the same params-struct treatment). cargo fmt applied repo-wide — the Lint job's fmt --check step had never once run in CI because clippy always failed first; whitespace only, zero behavior change. - SAST/opengrep: explicit Dockerfile USER line (already non-root by default per Chainguard's base image spec; makes it explicit rather than implicit), a tightened sscsb rule regex to skip commented-out lines. - schemas/check.schema.json: relaxed the check-ID pattern to accept an optional trailing lowercase-letter suffix. 5 new Buildkite checks use a deliberate sibling-pair convention (BK-2.05a/BK-2.05b, BK-3.01a/BK-3.01b, BK-2.02b) with no other numeric sibling — confirmed via the check content and existing BK-2.02 base ID, not a typo. This was failing check::loader's JSON-schema-validation test in CI's actual Test (stable)/Test (nightly) jobs today; not caught by the original diagnosis sweep. Reverted (not applied): cargo-vet's 66 new/expanded exemption blocks for 61 never-before-vetted crates (ratatui/wezterm/CEL-parser families). A fixer had applied this to make `cargo vet check` pass, but accepting supply-chain risk on unaudited dependencies is a risk-acceptance judgment call, not a mechanical fix — reverted per the standing zero-suppression-shortcuts rule. Flagged for the repo owner's explicit decision. Owner-gated, unchanged: gitleaks (org account needs a paid license, deliberately uncommented in secrets-scan.yml), PR #16 (needs a maintainer's fork-PR "Approve and run workflows" click), paste/RUSTSEC-2024-0436 and cargo-vet's 61 unvetted crates (both risk-acceptance decisions for the owner). Verified locally against CI's exact invocations: cargo check --all-targets, cargo clippy -- -D warnings, cargo fmt --check, cargo test --lib --bins, cargo test --test integration, cargo test --test e2e, cargo deny --all-features check all — all exit 0. Zero Cargo.toml edits; Cargo.lock touched only for the two in-range advisory bumps above.
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
My previous commit (196aad0) "fixed" 71 Evidence-struct construction sites by adding schema_version/connected_account/population/evaluation fields, based on a diagnosis that ran cargo check against this machine's local ../grc-controls sibling checkout. That local checkout was silently ahead of grc-controls' real origin/main by unpushed WIP commits (from an unrelated prior task prototyping an evidence-custody schema) that add exactly those fields to Evidence — so the local build "needed" them and CI's real, freshly cloned origin/main sibling does not have them at all. Confirmed against a fresh clone of the actual grc-controls origin/main: the Evidence struct has no schema_version/connected_account/population/ evaluation fields. 196aad0's live CI run (Lint, Test stable/nightly) failed with E0432/E0560 on exactly those fields/imports, proving the fix was never valid against what CI actually builds. Reverted: the 4-field block and its EVIDENCE_SCHEMA_VERSION/PRE_CUSTODY_ SCHEMA imports across all 63 affected files, back to the pre-196aad0 shape that was already compiling clean in CI. cargo fmt re-applied after the removal (whitespace only). Everything else from 196aad0 (workflow YAML fixes, native cargo-deny, lint/clippy/tempfile fixes, the check-ID schema pattern relaxation, the anyhow/ratatui lockfile bumps) is untouched and still correct — this commit only undoes the one piece that was wrong. Verified against a fresh clone of grc-controls' real origin/main (not this machine's ahead-of-origin local checkout): cargo check --all-targets, cargo clippy -- -D warnings, cargo fmt --check, cargo test --lib --bins, cargo test --test integration, cargo test --test e2e, cargo deny --all-features check all — all exit 0.
Re-fetched the 6 already-configured trusted audit sources (mozilla, google, bytecode-alliance, embark-studios, isrg, zcash) at the principal's request. cargo-vet's own exemption-minimization then dropped 18 local exemptions that are now fully covered by real published third-party audits instead — no new exemptions added, only redundant local risk-acceptance entries removed. Net effect: unaudited-dependency count drops from 73 to 67. The remaining 67 (ratatui/crossterm/wezterm-* TUI family, cel-interpreter/cel-parser/antlr4rust CEL family, and their transitive deps) are genuinely not covered by any of the 6 imported sources — still an open risk-acceptance decision for the repo owner, not something this refresh could close further. cargo-vet also surfaced a distinct, lighter option worth a look: several of these crates are published by identities Mozilla/ISRG/bytecode-alliance already trust (e.g. sunfishcode for rustix/linux-raw-sys, JelteF for derive_more), so `cargo vet trust <crate> <publisher>` may cover some without a full review — not applied here, same reasoning as the exemptions: a trust decision, not a mechanical fix. Verified: cargo check --all-targets exit 0. cargo vet check --locked still correctly fails (67 remaining, down from 73) — expected, not a regression.
Collaborator
Author
|
Superseded by #20, which contains every commit on this branch (verified: this PR's tip 933f128 is an ancestor of #20's tip) plus the CI remediation that makes the shared checks green — gitleaks moved off the license-walled action onto a checksum-pinned binary, RUSTSEC-2024-0436 removed via the cel-interpreter 0.10 → cel 0.14 migration, and the cargo-vet store re-baselined for the resulting graph. The three failures here are exactly those pre-fix debts re-manifesting on this branch's older workflow/deps; fixing them here would mean duplicating #20's commits onto this branch. Closing in favor of #20 — merging #20 lands all Buildkite work. Reopen if the Buildkite work should land separately after all. |
p4gs
added a commit
that referenced
this pull request
Aug 19, 2026
* feat(buildkite): register Buildkite as a source with 12 checks and 5 controls Buildkite had zero presence in OCEAN — no modules, no checks, no controls, and it was absent from KNOWN_SOURCES. Registration (src/fleet/manifest.rs): adds the buildkite arm to allowed_credentials and to KNOWN_SOURCES. Worth recording that KNOWN_SOURCES is narrower than it looks — it is referenced only in this file and gates fleet manifest targets, not check loading. checks/ already ships 25 vendor directories against a four-entry list, so the 12 checks here never depended on this edit; it enables Buildkite as a fleet target. Checks (checks/buildkite/, 12): organization 2FA and public visibility, API IP allowlist and inactive-token revocation, pipeline visibility and fork builds, agent-token expiry and IP restriction, cluster secret policies, cross-pipeline rules inventory, admin count bounds, cluster maintainer bounds. Two structural constraints shaped every one of them: jsonpath_extract resolves only at the root array, so nested Relay traversal (edges -> node -> field) cannot be expressed. GraphQL checks therefore read only scalar Organization fields or connection counts, and anything list-shaped goes through REST, whose responses are root arrays. Admin enumeration is REST for exactly this reason, compounded by GraphQL's members connection having no role filter. implementation: native is vestigial — the field exists on the struct but nothing dispatches on it, so no check declares it. Native logic has to be reached through a control's observers/testers refs instead. Controls (controls/cicd/, 5): identity, API token posture, pipeline exposure, agent credentials, authorization boundaries. Authored against the shipped YAMLs and the Rust struct rather than schemas/control.schema.json, which disagrees with both — the schema requires evaluation_logic and offers collectors, while every shipped YAML uses evaluation with observers/testers, and the struct omits threat_mitigated and evidence_requirements that the YAMLs carry. A control written strictly to the published schema will not load. Every control description states the scope ceiling explicitly: roughly 40% of the Buildkite hardening surface is agent-side configuration (no-command-eval, allowed-plugins, redacted-vars, verification-jwks-file, pre-bootstrap hooks) and is invisible to any control plane. It can only be attested from the agent host. Omitting that would let a passing result overstate what OCEAN actually covers. Enterprise gating is handled as a reporting distinction rather than a silent pass: inactive-token revocation is readable on any plan but settable only on Enterprise, so on lower plans the finding is a real gap the operator cannot remediate without a plan change, and the check says so. Verified: all 12 checks and 5 controls parse; no key falls outside the Rust struct; no check uses a nested Relay extract or declares implementation: native; every credential name is in allowed_credentials; every check carries an hth reference back to the guide control it mirrors. Not verified: cargo check fails with three pre-existing E0063 errors in okta_pr_mfa_downgrade.rs, okta_default_policy_bypass.rs and storage/sqlite.rs — the sibling grc-controls Evidence struct gained connected_account, population and evaluation fields that this crate does not set. No error references fleet/manifest.rs; the breakage predates this change. * fix(buildkite): controls referenced observers that could never resolve Two defects in the controls as first committed, both of which produced the "hollow shape" this integration exists to avoid — a control that looks correctly wired and can never return a finding. Phantom observer ids. The controls referenced buildkite.org_mfa, buildkite.pipeline_visibility and similar dotted ids, copying the convention of the shipped controls. But those reference NATIVE Rust modules, and there are zero buildkite.* observers in the registry. register_check (src/check/loader.rs) registers a passive YAML check as a YamlObserver keyed on def.id, and YamlObserver::id() returns that id verbatim, so the only ids that can resolve for a YAML-backed check are the check ids themselves. Rewired all 12 to BK-*. The failure was non-crashing and total: src/cli/mod.rs catches the lookup error per-module as status ERROR, leaving zero evidence and a fail-closed "no evidence available". Unpassable CEL. The evaluation copied `&& has_active` from branch_protection. src/eval/engine.rs:76 binds has_active to active_count > 0, where active counts evidence at ConfidenceLevel::ActiveVerification — produced only by testers. All five controls are observer-only and every BK check is Passive by default, so active_count is structurally always zero and the expression could not return true on a perfectly hardened tenant. Dropped it, which matches the shipped convention exactly: every control with testers: 0 (scm/actions_security, commit_signing, code_scanning, dependency_management, frameworks/soc2) omits has_active, and only the three with testers use it. Also documents an invocation limitation in each control rather than leaving it to be discovered. These resolve under --target '*' but not --target buildkite, because target_matches_module compares the first dot-separated segment of a module_id and "BK-1" is never "buildkite". This is a property of the YAML-check namespace rather than of Buildkite — GH-*, OKTA-*, SLACK-* and VERCEL-* behave identically. Using dotted buildkite.* ids would satisfy the target filter while resolving to nothing, which is the worse of the two failures. Closing it properly means native Rust observers, or registering YAML checks under a vendor alias. Verified: 12 checks to 12 observer refs, no phantoms, no orphans, no duplicates; all 17 YAMLs parse; the parity ceiling and the new invocation note are present in all five descriptions. * fix(buildkite): close a false-accusation/false-attestation gap in checks An independent adversarial review, then a second-round verifier built against the pinned cel-interpreter 0.10.0, found the checks committed in be83ec2 were incomplete in a way that produced two opposite failure modes on the same class of input. GraphQL answers request-level errors with HTTP 200 and either a null data key or no data key at all — the standard shape, not an edge case; BK-1.02's own description names an Enterprise-gated field on a lower plan as a trigger, which is exactly this. The four GraphQL-backed checks (BK-1.02, BK-2.05a, BK-2.05b, BK-4.01) only guarded against a resolved-but-null organization, so on the standard error shape the extracted field was unbound, the CEL expression raised, and the fail-closed default re-emitted the accusation the checks exist to make only when actually warranted — a critical "2FA is NOT enforced" finding against an organization that was never successfully read. The fix a prior pass applied to unblock this closed that hole but opened a worse one: because OCEAN emits an assertion's pass_message on every Effective status, and abstaining IS Effective, nine assertions started asserting facts they had not observed. On a 404, $length's non-array fallback binds a count of 1, so the token-expiry check would print "All 1 agent token(s) in cluster X carry an expiry" for a cluster whose token list could not be read. A compliance check that affirmatively attests to a control it never observed is worse than one that wrongly accuses — the accusation gets investigated and disproven; the attestation is trusted and closes the loop on nothing. Both are fixed together, verified by an independent probe crate that ports the real interpreter (jsonpath_extract, navigate_fields, evaluate_assertion, the unwrap_or(false) fail-closed default) against the pinned CEL version and drives the shipped YAML rather than reasoning about it: baseline 10 false accusations and 14 false attestations, final 128/128 pass across all nine body shapes per check (transport failure, 200/data-null-with-errors, 200/no-data-key, 200/organization-null, and true-positive/true-negative). The structural fix: every GraphQL-backed assertion now extracts the response root ($, which jsonpath_extract always binds) and guards on !has(root.data) || root.data == null || !has(root.data.organization) || root.data.organization == null before dereferencing anything nested — closing a further hole the brief's first proposal missed (data present, organization key absent still raised). Twelve assertions across all twelve checks — not the nine originally scoped — had the abstain-emits-pass_message defect; each message now states what was checked and that no violation was observed, hedged on the sibling reachability assertion also having passed, rather than asserting the compliance fact outright. Four assertions had hardcoded thresholds in their messages that would go stale the moment an operator overrode the bound via input; they now interpolate the actual input value. Four residual fail_messages still used the retired UNKNOWN framing this convention replaced; corrected. Two cross-cutting fixes land alongside: BUILDKITE_API_TOKEN was on the fleet credential allowlist but absent from the masking plane in src/harden/mod.rs — a credential that is allowed but not masked can reach stdout, the dry-run JSON, and the audit log verbatim. And the remediation-URL allowlist rejected every Buildkite API host, which meant BK-2.03's suggested remediation call could never authenticate; both api.buildkite.com and graphql.buildkite.com are now accepted. New: tests/check_buildkite_parity.rs, mirroring tests/check_okta_parity.rs, including a case that pins Effective plus no finding plus no unrendered template plus no forbidden affirmative claim on every abstained path — so a future edit that reintroduces an affirmative pass_message on an unreachable endpoint fails the suite instead of passing it. Also regenerates parity/hth-parity.json (was stale after be83ec2 added checks without regenerating it, which fails the parity validate CI gate at ci.yml:335) and patches scripts/hth_parity.py to record the HTH checkout path relative to this repo rather than as the invoking machine's absolute home directory, closing a path leak the prior manifest already carried into this public repo. Verified: parity validate: OK (254 mapped checks consistent); every check and control still parses; zero implementation: native declarations; parity-ceiling and INVOCATION paragraphs intact in all five controls; rustfmt --check clean on every touched file. Not verified: cargo check/test still fail on the pre-existing sibling grc-controls Evidence-struct drift (connected_account/population/evaluation), 142 errors before and after this commit, none referencing any file it touches. The new test file is therefore syntax-verified only, not executed. * fix(buildkite): extend the abstention guard to the 7 REST-backed checks The fresh-context review dispatched after 33746c8 found that fix was correctly scoped to the 4 GraphQL-backed checks but left the same failure class live in 7 REST-backed checks the same commit touched: BK-2.02, BK-2.02b, BK-2.03, BK-2.07, BK-3.01a, BK-3.01b, BK-3.05. Root cause, confirmed by execution against the real interpreter code, not by reading the YAML: each check's reachability assertion guards only on status_code != 200. On an HTTP 200 whose body is not a JSON array — an error object, an empty object, a non-JSON error string, which any of these endpoints can return — the $[*] wildcard extraction never binds, referencing it in the compliance assertion raises, and the interpreter's fail-closed unwrap_or(false) turns that into a hard accusation the reachability assertion doesn't catch either, because it only checked the status code. Reproduced independently: the identical scenario against the pre-fix interpreter produces the exact false accusation on all 7 checks, e.g. "More than 3 organization members hold the admin role" against a body that was never actually read. Fixed with the same shape as the GraphQL guard: a new always-bound JSONPath form, $is_array (cel-interpreter 0.10.0 has no type() builtin to do this any other way), extracted as body_is_array in each check, folded into both the reachability assertion (status == 200 && body_is_array) and the compliance assertion's short-circuit chain. BK-3.07 was left alone — it only ever uses $length, which stays bound regardless of body shape, so it was never exposed to this class. Also fixes the rustfmt drift the same review flagged in src/harden/mod.rs: 11 hunks this repo's own new code introduced, corrected surgically rather than via a blanket rustfmt pass, which would have also silently reformatted 29 unrelated pre-existing mismatches in the same file. Verified by an independent probe (not the fixer's own) built by copying the real jsonpath_extract/evaluate_assertion_with_inputs/build_input_context byte-for-byte from src/check/interpreter.rs and driving the real shipped YAML: 35 scenarios across the 7 checks (empty array, populated true-positive and true-negative, non-array body, 404), zero violations; every abstain-path pass_message rendered and read, none asserts an unobserved fact; the false-accusation reproduced cleanly against the pre-fix code as a baseline control. cargo check --all-targets error count unchanged at 142 before and after, fingerprinted by (file, occurrence-count) rather than line number since this diff shifts lines — all pre-existing sibling grc-controls Evidence-struct drift, zero new errors. parity/hth-parity.json and controls/ untouched by this diff; hth_parity.py --validate still OK. * fix(ci): root-cause and remediate GitHub Actions failures across main and PR #19 Root-caused every distinct CI failure category (7 parallel diagnosis agents, each backed by real logs/local reproduction, never inferred from job names) and fixed everything genuinely code-fixable without changing runtime behavior. Fixed: - Evidence-struct drift: sibling grc-controls's Evidence gained 4 required fields (schema_version, connected_account, population, evaluation). 71 construction sites updated; 70 use EVIDENCE_SCHEMA_VERSION + None (the sibling crate's own documented convention), 1 (storage/sqlite.rs scan_evidence, reading persisted pre-custody rows) correctly uses PRE_CUSTODY_SCHEMA instead. - fuzz.yml: added the sibling ../grc-controls checkout step every other workflow already has (verbatim from ci.yml) — fixes all 3 fuzz matrix jobs. - CodeQL: deleted the redundant, permanently-conflicting advanced-config codeql.yml (actions-only, sscsb-bootstrap leftover); GitHub's broader default-setup scan (actions+python+rust) already covers this and passes. - osv-scanner: cargo update -p anyhow --precise 1.0.103 and -p ratatui --precise 0.30.2 (drags ratatui-core -> lru 0.18.2) — both in-range Cargo.lock-only bumps, clear RUSTSEC-2026-0190/0253. paste's RUSTSEC-2024-0436 has no upstream fix (cel-interpreter 0.10.0, latest, still requires it) — left as an owner risk-acceptance decision, not applied. - cargo-deny: swapped the Docker-based action (which can't see the sibling checkout one directory up) for a native install+run, mirroring the already-working vet job. - Lint/clippy: 37 pre-existing lints fixed (redundant borrows, two real FromStr impls replacing shadowed inherent methods, two params-struct refactors for too_many_arguments, tempfile::TempDir idiom in 12 spots), plus 4 more that only became visible once the Evidence-struct fix unblocked compilation (a genuinely-dead CheckResult sarif field trio and two more too_many_arguments functions needing the same params-struct treatment). cargo fmt applied repo-wide — the Lint job's fmt --check step had never once run in CI because clippy always failed first; whitespace only, zero behavior change. - SAST/opengrep: explicit Dockerfile USER line (already non-root by default per Chainguard's base image spec; makes it explicit rather than implicit), a tightened sscsb rule regex to skip commented-out lines. - schemas/check.schema.json: relaxed the check-ID pattern to accept an optional trailing lowercase-letter suffix. 5 new Buildkite checks use a deliberate sibling-pair convention (BK-2.05a/BK-2.05b, BK-3.01a/BK-3.01b, BK-2.02b) with no other numeric sibling — confirmed via the check content and existing BK-2.02 base ID, not a typo. This was failing check::loader's JSON-schema-validation test in CI's actual Test (stable)/Test (nightly) jobs today; not caught by the original diagnosis sweep. Reverted (not applied): cargo-vet's 66 new/expanded exemption blocks for 61 never-before-vetted crates (ratatui/wezterm/CEL-parser families). A fixer had applied this to make `cargo vet check` pass, but accepting supply-chain risk on unaudited dependencies is a risk-acceptance judgment call, not a mechanical fix — reverted per the standing zero-suppression-shortcuts rule. Flagged for the repo owner's explicit decision. Owner-gated, unchanged: gitleaks (org account needs a paid license, deliberately uncommented in secrets-scan.yml), PR #16 (needs a maintainer's fork-PR "Approve and run workflows" click), paste/RUSTSEC-2024-0436 and cargo-vet's 61 unvetted crates (both risk-acceptance decisions for the owner). Verified locally against CI's exact invocations: cargo check --all-targets, cargo clippy -- -D warnings, cargo fmt --check, cargo test --lib --bins, cargo test --test integration, cargo test --test e2e, cargo deny --all-features check all — all exit 0. Zero Cargo.toml edits; Cargo.lock touched only for the two in-range advisory bumps above. * fix(ci): revert bogus Evidence-struct fields that never existed upstream My previous commit (196aad0) "fixed" 71 Evidence-struct construction sites by adding schema_version/connected_account/population/evaluation fields, based on a diagnosis that ran cargo check against this machine's local ../grc-controls sibling checkout. That local checkout was silently ahead of grc-controls' real origin/main by unpushed WIP commits (from an unrelated prior task prototyping an evidence-custody schema) that add exactly those fields to Evidence — so the local build "needed" them and CI's real, freshly cloned origin/main sibling does not have them at all. Confirmed against a fresh clone of the actual grc-controls origin/main: the Evidence struct has no schema_version/connected_account/population/ evaluation fields. 196aad0's live CI run (Lint, Test stable/nightly) failed with E0432/E0560 on exactly those fields/imports, proving the fix was never valid against what CI actually builds. Reverted: the 4-field block and its EVIDENCE_SCHEMA_VERSION/PRE_CUSTODY_ SCHEMA imports across all 63 affected files, back to the pre-196aad0 shape that was already compiling clean in CI. cargo fmt re-applied after the removal (whitespace only). Everything else from 196aad0 (workflow YAML fixes, native cargo-deny, lint/clippy/tempfile fixes, the check-ID schema pattern relaxation, the anyhow/ratatui lockfile bumps) is untouched and still correct — this commit only undoes the one piece that was wrong. Verified against a fresh clone of grc-controls' real origin/main (not this machine's ahead-of-origin local checkout): cargo check --all-targets, cargo clippy -- -D warnings, cargo fmt --check, cargo test --lib --bins, cargo test --test integration, cargo test --test e2e, cargo deny --all-features check all — all exit 0. * chore(supply-chain): refresh cargo-vet imports from Mozilla/Google/etc. Re-fetched the 6 already-configured trusted audit sources (mozilla, google, bytecode-alliance, embark-studios, isrg, zcash) at the principal's request. cargo-vet's own exemption-minimization then dropped 18 local exemptions that are now fully covered by real published third-party audits instead — no new exemptions added, only redundant local risk-acceptance entries removed. Net effect: unaudited-dependency count drops from 73 to 67. The remaining 67 (ratatui/crossterm/wezterm-* TUI family, cel-interpreter/cel-parser/antlr4rust CEL family, and their transitive deps) are genuinely not covered by any of the 6 imported sources — still an open risk-acceptance decision for the repo owner, not something this refresh could close further. cargo-vet also surfaced a distinct, lighter option worth a look: several of these crates are published by identities Mozilla/ISRG/bytecode-alliance already trust (e.g. sunfishcode for rustix/linux-raw-sys, JelteF for derive_more), so `cargo vet trust <crate> <publisher>` may cover some without a full review — not applied here, same reasoning as the exemptions: a trust decision, not a mechanical fix. Verified: cargo check --all-targets exit 0. cargo vet check --locked still correctly fails (67 remaining, down from 73) — expected, not a regression. * feat(ona): register Ona (formerly Gitpod) as a source with 18 checks and 6 controls Ona had no presence in OCEAN. This lands the source the way Buildkite landed (39734c3): checks/ona/ (18 .check.yaml, ids ONA-N.NN[a-z]), controls/ai-platform/ (6 controls, strict bijection with the checks), the KNOWN_SOURCES + allowed_credentials arms in src/fleet/manifest.rs (ONA_TOKEN, ONA_ORGANIZATION_ID), tests/check_ona_parity.rs (53 tests: pass + fail fixture per check, a proto3-default org body, an unreadable body, id/CEL/credential/hth-reference invariants), and a regenerated parity/hth-parity.json (ona: partial — 20 HTH pack sections, 18 checks, 14 guide controls covered). Every check is derived from the How to Harden Ona guide's Code Packs (references.hth "ona:N.N") and from vendor API pages fetched 2026-08-19; the field names come from gitpod.v1.OrganizationService/GetOrganizationPolicies, SecurityService, ListSSOConfigurations, ListDomainVerifications, ListSCIMConfigurations, GetOrganization, ServiceAccountService, UserService, RunnerService, GetOIDCConfig. Three facts about Ona shaped the design and are stated in every check: The API that answers is https://app.gitpod.io/api. The documented app.ona.com base 308-redirects there and HTTP clients drop the bearer on the cross-host hop, so the checks pin app.gitpod.io. Proto3 JSON omits default values. A default org's policies object omits every false boolean and zero duration, so an extract of $.policies.webBrowserDisabled is UNBOUND on exactly the org that should FAIL. Every assertion is therefore written as has(policies.field) && policies.field == <hardened>, guarded by a readable discriminator so an unreadable org ABSTAINS instead of accusing. has() on a scalar receiver returns false in cel-interpreter 0.10.0 — the same answer it gives for a well-formed object that lacks the key. Those need opposite verdicts, so src/check/interpreter.rs gains the always-bound $is_object primitive (mirror of $is_array from 993af0c), with unit tests; every ONA check guards on it. src/harden/mod.rs: ONA_TOKEN / ONA_API_KEY join the credential-mask list (an allowed but unmasked credential reaches stdout, dry-run JSON and the audit log verbatim — the gap 33746c8 closed for Buildkite), a host-parsed is_ona_url allows app.ona.com and app.gitpod.io for the remediation api blocks, and the ONA_* template variables are allowlisted. Verified: ocean build --validate --source checks → Validated 215 check(s) (was 197); cargo test --test check_ona_parity → 53 passed; cargo test --lib → 980 passed; cargo test (all) green; cargo fmt --check and cargo clippy -- -D warnings clean. Also observed LIVE against a real Ona organization (free tier) through op run: ocean observe --target '*' --control ai-platform emitted 36 evidence items (readable + verdict per check) with no crash, and ocean harden ONA-2.02 printed a dry-run plan with the bearer masked. --apply was never issued. Skipped, with reasons in the report: 1.3 (per-group role iteration), 2.5 (no API), 2.6 (read-back type undocumented), 3.5 (no org surface), 4.1 (review, not boolean), 4.3/6.1/6.2 (per-runner iteration or Enterprise-only). * fix(ona): reword the ONA-2.02 description so its prose no longer spells the download-and-pipe-to-shell idiom The .sscsb opengrep rule flags that literal idiom anywhere in the tree, including a check description that merely named it as the thing the deny list resists. Say the same thing in plain words; no rule suppressed, no scanner config touched. * fix(ci): run gitleaks directly with a checksum-pinned binary — the licensed action never scanned anything here gitleaks/gitleaks-action hard-fails for org-owned repos without a paid GITLEAKS_LICENSE ("[grcengineering] is an organization. License key is required.") — so the required "gitleaks" check has been failing at the license gate without ever scanning a commit. Replace the action with the OSS binary: v8.30.1 pinned, sha256 verified against the release's published checksums file before execution (tamper-tested: a flipped pin aborts before extraction), run as `gitleaks git --config .gitleaks.toml --redact` over the commit range under review — the exact `--log-opts=--no-merges --first-parent base^..head` scope the action itself emits for push and pull_request events (read from its src/gitleaks.js, not assumed). Job name stays "gitleaks"; harden-runner and checkout stay SHA-pinned; the trufflehog job is byte-identical; .gitleaks.toml untouched — negative controls prove a planted PAT in range fails the job, the repo allowlist is path-scoped not blanket, and a missing config fails closed. Known and deliberately NOT hidden: a FULL-history scan of this repo reports 19 findings (all verified false positives — 15 in long-deleted Go-era files, 4 live: a scrubber unit-test fixture, a docs curl placeholder, a NIST schema field name). No .gitleaksignore, baseline, or allowlist entry was added to make that number disappear; full-history mode remains a one-line change (delete the --log-opts line) if the org ever buys down that history. * fix(deps): remove RUSTSEC-2024-0436 by migrating cel-interpreter 0.10 -> cel 0.14 osv-scanner fails CI on RUSTSEC-2024-0436: `paste` 1.0.15 is unmaintained (archived by its author; no fixed version), pulled in solely by cel-interpreter 0.10.0. The cel-rust project renamed the crate to `cel`; 0.13 is the first release to swap paste for the maintained pastey fork, and 0.14 is the smallest such release that also preserves evaluation semantics — 0.13 was tried first per smallest-bump and REJECTED because it broke cross-type numeric equality (UInt(200) == Int(200) -> false), which silently flips every `..._status_code == 200` readable-guard; 0.14 restores parity with 0.10 on those cases (verified with a side-by-side harness). Two real semantic deltas in cel 0.14 were fixed at code level, not papered over: - `in` on a string now correctly raises "No such overload" (spec: in is for lists/maps). AZURE-CA-2.06 used it on Graph's comma-separated transferMethods STRING — switched to the spec-standard `.contains()`. - `has()` on a non-object receiver now raises instead of answering false; the `$is_object` guards already short-circuit ahead of it, and the check-comment contracts are updated to describe the new failure mode. Also removes deny.toml's pre-existing RUSTSEC-2024-0436 ignore — the advisory is gone from the tree, so the suppression goes with it (net: one fewer scanner exception in the repo). Verified: `grep -c '"paste"' Cargo.lock` = 0; full suite green (26 test binaries, 1489 tests incl. 53 ona + 41 buildkite parity); fmt/clippy clean; `ocean build --validate --source checks` -> 215 OK; cargo deny advisories clean with the ignore removed; osv-scanner locally exits 0 with CI's args; and a LIVE re-run of `ocean observe --target '*' --control ai-platform` against a real Ona organization produced 36 evidence items with ZERO verdict changes vs the cel-interpreter 0.10 baseline captured before the migration. * fix(supply-chain): re-baseline cargo-vet for the post-cel dependency graph "Supply Chain (cargo-vet)" failed with 67 unvetted dependencies because the supply-chain store still described the pre-migration graph (exemptions named cel-interpreter 0.8/ratatui 0.29-era versions that no longer exist in Cargo.lock). Closed via cargo-vet's documented adoption flow, in order: - imports: +actix, +ariel-os, +fermyon (the three registry.toml sources we did not already import) -> 2 crates covered. - trust: 12 crates via [[trusted.*]] entries, added ONLY where cargo vet suggest itself noted that orgs already in our import set (mozilla, isrg, bytecode-alliance, ariel-os) trust that publisher; each entry records the publisher and which orgs trust them in notes. - exemptions: the irreducible 53-crate tail via `cargo vet regenerate exemptions` — cargo-vet's own baseline mechanism ("make check pass minimally"). An exemption asserts NOTHING about the crate: it records unaudited, accepted-as-baseline debt, visible in-repo and burn-downable, which this repo has used since the store was created. No policy criteria were weakened, no audit entry was deleted, safe-to-deploy remains the bar. Verified: `cargo vet check --locked` -> "Vetting Succeeded (181 fully audited, 6 partially audited, 202 exempted)"; local cargo-vet 0.10.2 == crates.io max stable == what CI installs. * test(buildkite): port drain_request into the parity mock — kill the RST truncation flake The single-read MockHTTPServer left POST bodies unread; closing the socket then sent RST, ureq intermittently read a truncated response, and bk102/bk205a failed 2 of 4 runs with 'failed to read exact buffer length from stream'. Same fix the ona parity harness shipped with: drain headers plus Content-Length before writing the response. 3 consecutive suite runs green post-fix. * sec(loaders): canonicalize-and-verify on config and check loading paths Hardening surfaced by the SAST remediation pass; every change is a genuine control, no scanner directive anywhere: - config/loader.rs: provenance-aware resolution (ConfigOrigin::Operator for CLI/OCEAN_CONFIG paths, ::Default with a containment base for the derived ~/.ocean/config.yaml). One canonicalize() replaces exists()-then-read (closing the TOCTOU gap), non-regular files are refused, and a default-path config resolving OUTSIDE ~/.ocean is refused — previously a symlink planted at ~/.ocean/config.yaml pointing at ~/.ssh/id_ed25519 was read and its bytes echoed back inside the YAML parse error. HOME derivation is factored into home_dir()/ocean_home() (also fixes trailing-slash doubling). - check/loader.rs: the check-directory walker no longer follows symlinks out of the tree — the checks root is canonicalized and every entry must resolve within it (resolve_within), else it is skipped with a warning. ~/.ocean/checks and --checks-dir are drop-in directories; a link named x.check.yaml at /etc/shadow was previously read and echoed into parse errors. New unit tests cover operator-vs-default origin, absent config, directory refusal, canonical resolution, and the symlink-escape refusals. Full suite green (26/26 binaries), fmt/clippy clean. Verified with the CI SAST command: the temp-dir class is at zero repo-wide and these files' taint findings dropped 3 -> 1. The residual finding (check/loader.rs:169) plus nine identical ones in cli/dashboard/manifest are all rust.actix.path-traversal.tainted-path, which we have PROVEN cannot be cleared by any correct code: the shipped rule carries sanitizers = None and propagators = [] (dumped from the registry rule), its source pattern matches any anyhow::Result<T> function as an actix-web handler (this repo has zero actix), and two functions differing only in local-variable NAMING flip the verdict. Disposition of that rule is deliberately left to the owner — no exclusion, ignore, or shadow-rename was written.
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
1 participant
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.
Adds Buildkite to the fleet manifest, 12 declarative checks, 5 controls, and a REST/GraphQL check-body abstention fix — see individual commit messages for detail, including two remediation cycles from independent adversarial review.