Skip to content

security(oauth): restrict OAuth signing-key settings to system admins - #265

Merged
gonzalesedwin1123 merged 4 commits into
19.0-staging-sec-batch1from
security-oauth-signing-keys-acl
Jul 28, 2026
Merged

security(oauth): restrict OAuth signing-key settings to system admins#265
gonzalesedwin1123 merged 4 commits into
19.0-staging-sec-batch1from
security-oauth-signing-keys-acl

Conversation

@gonzalesedwin1123

@gonzalesedwin1123 gonzalesedwin1123 commented Jul 2, 2026

Copy link
Copy Markdown
Member

Problem

spp_oauth granted base.group_user (every internal user) read + write on
res.config.settings, which exposes oauth_priv_key — the RS256 JWT signing key used
by calculate_signature() — and oauth_pub_key. A low-privileged internal user could:

  • Read the private key and forge OAuth/JWT tokens that verify_and_decode_signature()
    accepts (full API auth bypass / impersonation), and
  • Overwrite the keypair to break or subvert authentication (tamper / DoS).

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 core base/models/res_config.py) reads
config_parameter fields via ir.config_parameter.sudo() and performs no model-ACL or
field-group check. It is an @api.model method reachable over RPC, so
env['res.config.settings'].default_get(['oauth_priv_key']) leaks the key regardless of the
ACL
. This was reproduced by a failing test before the fix (the plain-user default_get
result contained the private key). Both changes below are required.

Changes

  1. Remove the over-broad ir.model.access row. Odoo core already grants
    res.config.settings to base.group_system (read/write/create), so system admins keep
    full access and can still Save settings. This was the only module in the repo widening this
    model to base.group_user.
  2. Override default_get to strip oauth_priv_key/oauth_pub_key for users who are not
    base.group_system, closing the sudo RPC path. (A groups= field attribute would not fix
    this — field groups are not enforced in default_get's config loop.)
  3. Bump manifest 19.0.2.0.019.0.2.0.1.

Tests

New spp_oauth/tests/test_config_settings_acl.py:

  • plain internal user is denied model access to res.config.settings (AccessError);
  • plain internal user cannot read the keys via default_get;
  • system admin retains model access and can read both keys via default_get.

Written test-first (failing → passing). ./spp test spp_oauth14 passed, 0 failed;
./spp lint clean.

⚠️ Operational remediation required

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)

  • Placeholder values in data/ir_config_parameter_data.xml (YourPrivateKeyHere /
    YourPublicKeyHere) are non-secret placeholders.
  • Empty 0-byte spp_oauth/tools/private_key.pem / public_key.pub are unused (keys are read
    only 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_keyoauth_private_key/oauth_public_key, same lines in res_config_settings.py) — a guaranteed conflict whose naive resolution (keep this guard verbatim + #114's renames) leaves OAUTH_KEY_FIELDS pointing 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 the OAUTH_KEY_FIELDS entries AND port this PR's test_config_settings_acl.py to 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.12.1.0).

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread spp_oauth/models/res_config_settings.py Outdated
@codecov

codecov Bot commented Jul 2, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 71.25%. Comparing base (a92701b) to head (e9dc451).

Additional details and impacted files

Impacted file tree graph

@@                     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              
Flag Coverage Δ
spp_base_common 91.07% <ø> (ø)
spp_oauth 100.00% <100.00%> (?)
spp_programs 65.27% <ø> (ø)
spp_registry 86.94% <ø> (ø)
spp_security 69.56% <ø> (ø)

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

Files with missing lines Coverage Δ
spp_oauth/models/res_config_settings.py 100.00% <100.00%> (ø)

... and 6 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.

@gonzalesedwin1123

Copy link
Copy Markdown
Member Author

Staff review (post-rebase, head cc6a56d6): SHIP-WITH-NITS. The one must-fix is addressed in abe64750: added the readme/HISTORY.md fragment for 19.0.2.0.1, including the operator-facing key-rotation instruction. That mattered more than usual here — the remediation is manual, and the changelog is how a deployment learns the deployed keypair must be treated as compromised. (CI does not catch a missing fragment: no fragment change means no README drift for the oca-gen hook to diff.)

Review traced every enumerable read path against Odoo 19 core source and found none open: default_get over RPC (the original sink), read/search_read/web_read, the web client onchange path, direct ir.config_parameter reads, the write/tamper path via execute(), get_values(), API controllers, exports, and MRO re-add by another inheritor. The default_<field> context-forge vector from #364 was checked specifically and is not applicable here: res_config.default_get unconditionally overwrites config-parameter fields, and the pop runs last.

Also confirmed: the post-rebase field names still match (no upstream rename — git log 02de7ba7..cb7f4c6b -- spp_oauth is empty), and the superuser-mode commit is necessary rather than a widening — without it a sudo'd settings write would have wiped the live signing key.

Two optional hardening items deliberately not taken here, to keep the reviewed diff minimal:

  1. Add groups="base.group_system" to both field definitions as defence-in-depth. The current barrier is core's ACL plus the default_get pop; field groups would also close the write path if any future module widens the res.config.settings ACL.
  2. Three cheap test asserts pinning invariants that currently rest on Odoo core behaviour (context injection, get_param as an unprivileged user, create({})read()).

Standing constraint unchanged: #265 must merge before #114, whose rebase must rename the OAUTH_KEY_FIELDS entries and port test_config_settings_acl.py — a naive resolution silently reopens the leak. Post-rebase wrinkle: #265 now targets 19.0-staging-sec-batch1 while #114 targets 19.0, so the ordering must be enforced when the batch lands on 19.0, not just between the two PRs.

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.
@gonzalesedwin1123
gonzalesedwin1123 force-pushed the security-oauth-signing-keys-acl branch from 04feb20 to e9dc451 Compare July 28, 2026 08:48
@gonzalesedwin1123
gonzalesedwin1123 marked this pull request as ready for review July 28, 2026 08:57
@gemini-code-assist

Copy link
Copy Markdown

Caution

The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased.

@gonzalesedwin1123
gonzalesedwin1123 merged commit f0a58b0 into 19.0-staging-sec-batch1 Jul 28, 2026
20 checks passed
@gonzalesedwin1123
gonzalesedwin1123 deleted the security-oauth-signing-keys-acl branch July 28, 2026 08:57
gonzalesedwin1123 added a commit that referenced this pull request Aug 13, 2026
…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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant