security: batch 1 — DCI/Key admin privilege escalations, OAuth signing keys, GRM rule ACL (#327, #329, #265, #266)

[gonzalesedwin1123]

Lands security batch 1 — four independent security fixes, validated together on a staging branch so 19.0 was never exposed to an unverified intermediate state.

What's in it

PR	Fix	Modules (version)
#327	DCI Administrator no longer grants system administration (privilege escalation)	spp_dci 19.0.2.0.1 → 2.0.2 + migration
#329	Key Management Admin no longer grants system administration (privilege escalation)	spp_key_management 19.0.2.0.0 → 2.0.1 + migration
#265	OAuth RSA signing keys restricted to system admins (were readable by every internal user)	spp_oauth 19.0.2.0.0 → 2.0.1
#266	GRM routing/escalation rules restricted to GRM staff (portal write/create dropped)	spp_grm_cel 19.0.2.0.0 → 2.0.1

33 files. Every fix was individually adversarially reviewed, and #265/#266 were re-reviewed after rebasing because upstream had moved substantially under them.

⚠ Merge with a MERGE COMMIT — not squash

Squashing would collapse four distinct security fixes into one opaque commit. The merge commit preserves one commit per fix (each carrying its (#NNN) reference) for bisect and attribution.

Verification

Each PR was squash-merged into staging pinned with --match-head-commit <reviewed sha>, so what merged is provably what was reviewed. Manifest: internal/plans/batch1-manifest.tsv.

• Tree identity — each squash commit's tree is byte-identical to its reviewed head (all 4 ✅)
• Structural — staging's first-parent history is exactly the 4 recorded squash commits + the back-merge; no direct pushes
• File union — the delta vs 19.0 is exactly 33 files, precisely the union of the 4 commits; no riders
• CI — targeted ci.yml dispatch on the staging head: green on all four module test jobs (run 31352167158)
• Drift — 19.0 advanced 12 commits after the batch merged; 19.0 was back-merged into staging (not rebased, which would have destroyed the audit trail above). git show --remerge-diff on the merge is empty — purely mechanical, nothing smuggled in. The drift touched none of the four modules.

Upgrade test (-u) — the gate CI does not provide

CI only ever runs -i (fresh install), so the upgrade path is untested by the pipeline. Run manually, seeded at current 19.0 (208d9758) → staging (07a0ad3a); exit 0, zero errors. Evidence: internal/plans/batch1-upgrade-test-evidence.md.

Baseline confirmed all four vulnerabilities live on current 19.0, then after upgrade:

Assertion	Before	After
ir_module_module.latest_version matches manifests	—	✅ all 4
group_dci_admin / group_key_admin imply base.group_system	1 / 1	0 / 0
spp_oauth widening ir.model.access row	1	0
GRM portal perms on both rule models	r,w,c	read only

Both migrations fired with their audit warnings.

🔴 Release notes — required

1. security(dci): stop DCI Administrator group from granting system admin #327 — "DCI Administrator" no longer grants system administration. Deployments must explicitly grant base.group_system to whoever administers the DCI server (tokens, sender registries, subscriptions, server keys, settings) — every one of those surfaces is system-gated.
2. security(key_management): stop Key Management Admin group from granting system admin #329 (+security(oauth): restrict OAuth signing-key settings to system admins #265) — "Key Management Admin" no longer grants system administration; OAuth signing-key rotation now requires explicit base.group_system.
3. security(oauth): restrict OAuth signing-key settings to system admins #265 — operator action required: rotate the deployed OAuth RSA keypair and invalidate outstanding tokens. Every internal user previously had read access to the private key. No code change fixes this retroactively.
4. security(grm): restrict GRM automation rules to GRM staff (drop portal write/create) #266 — portal users lose write/create on GRM routing/escalation rules (read retained). No action expected.

Both migrations log warnings, but upgrade-log warnings are easily missed — the notes are the real channel.

Ordering constraint that survives this merge

#265 must land before #114. #114 renames the exact fields #265's default_get guard pops (oauth_priv_key/oauth_pub_key → oauth_private_key/oauth_public_key). Merging this PR satisfies the ordering, but #114's rebase must rename the OAUTH_KEY_FIELDS entries AND port tests/test_config_settings_acl.py — a naive conflict resolution leaves the guard pointing at dead field names and silently reopens the signing-key leak with green-looking code.

After merging

• Audit: git show --remerge-diff <merge-sha> should be empty, and git diff <merge-sha> origin/19.0-staging-sec-batch1 should be empty — check before the branch auto-deletes (delete_branch_on_merge is on)
• Tag the release by the merge-commit SHA, not branch HEAD (another PR can land in between; releases are immutable)
• Follow-ups already filed from these reviews: security(spp_grm_cel): escalation/routing rules evaluate as superuser — GRM officer can seize every ticket [Severity: High] #379, security(spp_grm): portal users have read/write/create on EVERY grievance ticket (no record rule targets portal) [Severity: High] #380, security(spp_grm_cel): rule-engine entry points are unguarded RPC-callable methods [Severity: Medium] #381 (from security(grm): restrict GRM automation rules to GRM staff (drop portal write/create) #266's review), security(spp_dci): rename the 'DCI Administrator' group — the name no longer describes what it grants [follow-up to #327] #398 (rename the DCI group — its name no longer describes what it grants)
• security(graduation): enforce manager-only assessment approval server-side #267 (graduation self-approval) was dropped from this batch after review returned NEEDS-CHANGES; it remains open against 19.0 with its full fix set recorded on the PR

Plan and checklist: internal/plans/security-staging-merge-plan.md.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 75.20%. Comparing base (208d975) to head (07a0ad3).

Additional details and impacted files

@@            Coverage Diff             @@
##             19.0     #399      +/-   ##
==========================================
+ Coverage   70.17%   75.20%   +5.03%     
==========================================
  Files         205      407     +202     
  Lines       17751    29863   +12112     
==========================================
+ Hits        12457    22459   +10002     
- Misses       5294     7404    +2110     

Flag	Coverage Δ
spp_analytics	93.25% <ø> (?)
spp_api_v2	79.99% <ø> (?)
spp_api_v2_change_request	66.53% <ø> (ø)
spp_api_v2_cycles	71.03% <ø> (?)
spp_attachment_av_scan	85.35% <ø> (?)
spp_base_common	91.07% <ø> (ø)
spp_change_request_v2	77.83% <ø> (ø)
spp_claim_169	58.18% <ø> (?)
spp_cr_type_assign_program	?
spp_dci	89.59% <ø> (?)
spp_dci_client	89.25% <ø> (?)
spp_dci_client_compliance	99.25% <ø> (?)
spp_dci_client_crvs	92.51% <ø> (?)
spp_dci_client_dr	85.77% <ø> (?)
spp_dci_client_ibr	92.47% <ø> (?)
spp_dci_client_sr	98.10% <ø> (?)
spp_dci_compliance	93.01% <ø> (?)
spp_dci_demo	94.28% <ø> (ø)
spp_dci_indicators	96.23% <ø> (?)
spp_dci_server	90.36% <ø> (?)
spp_farmer_registry_cr	?
spp_farmer_registry_demo	?
spp_grm_cel	77.23% <100.00%> (?)
spp_key_management	38.52% <ø> (?)
spp_mis_demo_v2	70.38% <ø> (ø)
spp_oauth	100.00% <100.00%> (?)
spp_programs	65.27% <ø> (ø)
spp_registry	87.22% <ø> (+0.07%)	⬆️
spp_security	69.56% <ø> (ø)
spp_starter_sp_mis	?
spp_studio_change_requests	?

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
spp_grm_cel/models/grm_escalation_rule.py	76.76% <100.00%> (ø)
spp_key_management/models/encryption_key.py	91.66% <ø> (ø)
spp_oauth/models/res_config_settings.py	100.00% <100.00%> (ø)

... and 278 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[emjay0921]

LGTM — four correct, independently-verified security fixes. CI fully green (every module test job) with 100% patch coverage; migrations idempotent; tests genuinely adversarial (transitive-closure guards, migration replay + idempotency, positive/negative ACL paths). Exemplary security work.

Verified against the code:

• #327 DCI — implied_ids → base.group_system removed (that grant escalated every PII-visibility holder to system admin); inverted so spp_security.group_spp_admin implies group_dci_admin. Group stays noupdate; the implication sits outside the noupdate block so it re-applies on upgrade; migration strips the stale link + comment on existing DBs.
• #329 Key mgmt — same escalation removed, admin now implies Key Operator. Confirmed the crux: every key model (spp.encryption.key, spp.key.purpose, spp.key.provider.registry, spp.asymmetric.key) has its own explicit group_key_admin AND base.group_system ACL rows, so removing the system implication strips nothing from key admins or real system admins. Menu correctly moved out of Settings.
• #265 OAuth — removes the row widening res.config.settings to base.group_user, and guards default_get() (reads config_parameter via sudo() with no ACL check) to strip the signing keys for non-system, non-su callers; honors env.su. Textbook fix.
• #266 GRM — portal users dropped to read-only on the two rule models (no record rules → ACL was the only boundary; portal submission runs sudo). The self.sudo().write() on escalation_count is a narrow, correct consequence, documented with nosemgrep.

Surfacing (not blockers):

1. #265 operator action (must hit release notes): the deployed OAuth RSA keypair was readable by every internal user — rotate it and invalidate outstanding tokens; no code change fixes that retroactively.
2. #265 → #114 ordering hazard: OAUTH_KEY_FIELDS names the current field names; when #114 (rename) rebases it must update those entries AND port the ACL test, or the guard silently points at dead names and reopens the leak with green tests.
3. Merge mechanics: merge commit (not squash); branch auto-deletes, so run git show --remerge-diff <merge-sha> (and git diff vs the branch) before it disappears; tag the release by the merge-commit SHA.
4. Minor — spp_dci deps: dci_groups.xml references spp_security.group_spp_admin but the manifest depends is only ["base","spp_registry"]; it resolves transitively (CI proves install) — recommend declaring spp_security explicitly so it can't break if spp_registry ever drops it.
5. Note: encrypted_key field access widened to group_key_admin — fine as KMS-wrapped ciphertext with operators still blocked (tested); rests on the assumption no provider stores plaintext there.

Leaving the merge to you given the merge-commit / tag-by-SHA requirements.