security(oauth): restrict OAuth signing-key settings to system admins - #265
Conversation
There was a problem hiding this comment.
Code Review
This pull request enhances security by restricting access to OAuth signing keys to system administrators. It removes broad read/write access to res.config.settings for standard users and overrides default_get to filter out sensitive keys for non-admins, backed by new integration tests. The feedback suggests ensuring that custom group checks bypass restrictions when running in superuser mode (self.env.su) to prevent unexpected behavior during elevated executions.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 19.0-staging-sec-batch1 #265 +/- ##
===========================================================
+ Coverage 71.16% 71.25% +0.08%
===========================================================
Files 221 227 +6
Lines 15396 15441 +45
===========================================================
+ Hits 10957 11002 +45
Misses 4439 4439
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
ba0b5f2 to
cc6a56d
Compare
|
Staff review (post-rebase, head Review traced every enumerable read path against Odoo 19 core source and found none open: Also confirmed: the post-rebase field names still match (no upstream rename — Two optional hardening items deliberately not taken here, to keep the reviewed diff minimal:
Standing constraint unchanged: #265 must merge before #114, whose rebase must rename the |
The module granted base.group_user read/write on res.config.settings, which exposes oauth_priv_key (the RS256 JWT signing key) and oauth_pub_key. Any internal user could read the private key via RPC and forge OAuth/JWT tokens, or overwrite the keypair to break/subvert authentication. - Remove the over-broad ir.model.access row. Odoo core already grants res.config.settings to base.group_system (read/write/create), so admins keep full access and can still save settings. - Override default_get to strip the signing-key fields for non-system-admins. default_get reads config_parameter values via sudo() with no model/field access check, so the ACL change alone would not stop key exfiltration over RPC. - Add regression tests asserting a plain internal user is denied model access and cannot read the keys via default_get, while a system admin retains both. Operators should treat the deployed keypair as compromised and rotate it.
Per review: env.user stays the original (possibly non-admin) user under sudo() while env.su is True. Bypass the group check when env.su is set so trusted server-side sudo contexts still receive the keys. The RPC attack path is never in superuser mode, so the exposure stays closed.
The version bump shipped without a HISTORY entry, so the key-rotation step operators must perform was undocumented.
04feb20 to
e9dc451
Compare
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
…g keys, GRM rule ACL (#327, #329, #265, #266) (#399) * security(dci): stop DCI Administrator group from granting system admin (#327) Reviewed head: d171acb * security(key_management): stop Key Management Admin group from granting system admin (#329) Reviewed head: 180c7fc * security(oauth): restrict OAuth signing-key settings to system admins (#265) Reviewed head: e9dc451 * security(grm): restrict GRM automation rules to GRM staff (drop portal write/create) (#266) Reviewed head: bebd609 * fix(spp_oauth): restore @api.model on default_get, field-gate signing keys (#399) Review response on PR #399 (findings 1-2): - default_get() override lacked @api.model. call_kw reads the dispatch marker off the most-derived method, so every external RPC call to res.config.settings.default_get crashed with a TypeError once this module was installed (web Settings UI unaffected - it resolves defaults in-process). Regression-tested through the real dispatcher. - oauth_priv_key/oauth_pub_key now carry groups="base.group_system": a settings save by an unauthorized principal fails closed with AccessError instead of silently deleting the stored parameters via set_param(False). The default_get pop stays - field groups are not enforced in default_get. Defence-in-depth: core's ACL already limits the model to system admins; the gate holds if that is ever re-widened. * fix(spp_dci): make DCI PII visibility opt-in per admin, harden migration (#399) Review response on PR #399 (findings 3, 7, 8): - Drop the spp_security.group_spp_admin -> group_dci_admin implication: PII rendering is now opt-in per administrator via an explicit, reviewable Access Rights grant instead of an automatic side effect of adminship, mirroring the deliberate key-custody separation in spp_key_management. The pinning test is inverted to guard against a consistency sweep reintroducing the link. The link never shipped in any release and no production DB is built from this staging branch, so no unlink migration is needed; the upgrade-test seed DB is rebuilt for the refreshed evidence run. - Migration comment refresh now strips only the stale sentence ("Members must already be system administrators") so operator rewrites and extensions survive, honouring noupdate. - Migration warning counts all_user_ids (transitive membership), matching the spp_key_management migration's counting basis - safe now that nothing implies the group. - Declare the spp_security dependency explicitly: the security tests pin design decisions against spp_security.group_spp_admin, and the dependency was previously only transitive via spp_registry. * fix(spp_key_management): correct encrypted_key wrap comment, add app icon (#399) Review response on PR #399 (findings 5, 6): - The encrypted_key field comment claimed the value is always KMS ciphertext; for the database provider it is wrapped by the master KEK, which in zero-config setups is derived from database.uuid. State both provider families accurately. The field gate itself is unchanged (the KMS-provider sudo rework is tracked in #330/#331). - The top-level Key Management menu now carries the module icon - every other top-level app menu sets web_icon; without it the app switcher shows a generic placeholder tile. Pinned in the existing load_menus test via web_icon_data. * test(spp_grm_cel): internal manager fixture, escalation counter regression (#399) Review response on PR #399 (findings 10, 11, 12): - The GRM manager fixture now links base.group_user: the spp_grm group chain carries no user-type group, so the manager was created as a share=True external principal and the staff-retention tests proved less than intended. - New regression test: a caller with read-only rule access gets a fully applied escalation with the counter incremented. The actor is a portal user - the population the sudo'd counter write serves in practice (officers/managers hold rule write; internal base users and GRM viewers are read-only too but do not drive ticket flow). Reverting the 19.0.2.0.1 sudo fix now fails loudly instead of passing the suite. - Portal-read docstrings reworded: the read row is a current implementation dependency (rule evaluation runs as the acting user, reachable by portal via direct-RPC ticket create/stage-write), not a security requirement. Tightening it - sudo evaluation, dropping the read rows, and the missing portal record rule on spp.grm.ticket - is tracked in #413. - HISTORY: scope the half-way claim to the counter/chatter state this fix addresses; notification/case partial failures are pre-existing. * docs: regenerate READMEs from CI's pinned generator (#399) Applied verbatim from the pre-commit CI run's printed diff (run 31679116024) - local regeneration is not byte-reproducible against CI's hook env, so CI output is the sole authority for generated files. * fix(spp_dci): put noupdate on the odoo root, drop deprecated data node (#399) With the group_spp_admin record removed, the noupdate <data> wrapper became the file's sole element, which oca-checks-odoo-module flags as xml-deprecated-data-node (the failing 'Checks for Odoo modules' CI hook). Same noupdate semantics, modern form.
Problem
spp_oauthgrantedbase.group_user(every internal user) read + write onres.config.settings, which exposesoauth_priv_key— the RS256 JWT signing key usedby
calculate_signature()— andoauth_pub_key. A low-privileged internal user could:verify_and_decode_signature()accepts (full API auth bypass / impersonation), and
Severity: High/Critical — this is a signing credential, not a config toggle.
Why the ACL fix alone is not enough
res.config.settings.default_get()(Odoo corebase/models/res_config.py) readsconfig_parameterfields viair.config_parameter.sudo()and performs no model-ACL orfield-group check. It is an
@api.modelmethod reachable over RPC, soenv['res.config.settings'].default_get(['oauth_priv_key'])leaks the key regardless of theACL. This was reproduced by a failing test before the fix (the plain-user
default_getresult contained the private key). Both changes below are required.
Changes
ir.model.accessrow. Odoo core already grantsres.config.settingstobase.group_system(read/write/create), so system admins keepfull access and can still Save settings. This was the only module in the repo widening this
model to
base.group_user.default_getto stripoauth_priv_key/oauth_pub_keyfor users who are notbase.group_system, closing the sudo RPC path. (Agroups=field attribute would not fixthis — field groups are not enforced in
default_get's config loop.)19.0.2.0.0→19.0.2.0.1.Tests
New
spp_oauth/tests/test_config_settings_acl.py:res.config.settings(AccessError);default_get;default_get.Written test-first (failing → passing).
./spp test spp_oauth→ 14 passed, 0 failed;./spp lintclean.Every internal user has had read access to the signing key historically, so the deployed
RSA keypair must be treated as compromised. After upgrading, operators should rotate
(regenerate) the keypair and invalidate any outstanding tokens. A code fix cannot un-leak an
already-exposed key.
Out of scope (noted for awareness)
data/ir_config_parameter_data.xml(YourPrivateKeyHere/YourPublicKeyHere) are non-secret placeholders.spp_oauth/tools/private_key.pem/public_key.pubare unused (keys are readonly from
ir.config_parameter).password="true"on the settings view fields is display masking only, not an access control.Merge order (vs #114 spp_oauth stable prep)
Per the cross-PR interaction analysis (2026-07-24,
internal/plans/security-prs-interaction-analysis.md): this PR merges before #114. #114 renames the exact fields this PR's guard protects (oauth_priv_key/oauth_pub_key→oauth_private_key/oauth_public_key, same lines inres_config_settings.py) — a guaranteed conflict whose naive resolution (keep this guard verbatim + #114's renames) leavesOAUTH_KEY_FIELDSpointing at dead field names:res.pop()removes nothing and the signing-key leak this PR closes silently reopens, with green-looking code. #114's post-merge rebase must rename theOAUTH_KEY_FIELDSentries AND port this PR'stest_config_settings_acl.pyto the new names — the ported test is the safety net. The ACL-csv conflict is semantically compatible either way (both end admin-only). Versions sequence cleanly in this order (2.0.1→2.1.0).