Skip to content

[Codex] fix(auth): require admin for adapter config and binding mutations#521

Open
Micsi wants to merge 2 commits into
abeggled:mainfrom
Micsi:codex/fix-mqtt-adapter-authorization-vulnerability
Open

[Codex] fix(auth): require admin for adapter config and binding mutations#521
Micsi wants to merge 2 commits into
abeggled:mainfrom
Micsi:codex/fix-mqtt-adapter-authorization-vulnerability

Conversation

@Micsi
Copy link
Copy Markdown
Collaborator

@Micsi Micsi commented May 19, 2026

Motivation

  • Close a privilege-escalation path where non-admin users could configure external MQTT brokers and attach bindings that exfiltrate or inject DataPoint values.

Description

  • Require admin-level dependency get_admin_user for legacy adapter config mutations and reads (PATCH /{adapter_type}/config and GET /{adapter_type}/config) by updating obs/api/v1/adapters.py imports and dependencies.
  • Require admin-level dependency get_admin_user for binding mutation endpoints (create/update/delete) in obs/api/v1/bindings.py, while keeping read-only listing (GET /{dp_id}/bindings) accessible to authenticated users.
  • The changes are intentionally minimal and limited to authorization checks and imports to avoid changing adapter runtime behavior.

Testing

  • Compiled the modified modules with python -m compileall obs/api/v1/adapters.py obs/api/v1/bindings.py and the compilation succeeded.
  • Performed a code inspection to verify that only the intended endpoints were switched to get_admin_user and that read-only endpoints remain protected by get_current_user.

Codex Task

@Micsi Micsi added the Security Security-related changes label May 19, 2026
@Micsi Micsi requested a review from abeggled May 19, 2026 21:29
@Micsi Micsi self-assigned this May 19, 2026
@Micsi
Copy link
Copy Markdown
Collaborator Author

Micsi commented May 20, 2026 •
edited
Loading

Nachtrag zum Follow-up-Fix (Commit 277d090):

Was war das konkrete Problem?

  • Bei Requests mit X-API-Key hat get_current_user bisher den Wert aus api_keys.name zurückgegeben (also den frei wählbaren Anzeigenamen des Schlüssels), nicht den tatsächlichen Besitzer (api_keys.owner).
  • get_admin_user prüft Admin-Rechte aber gegen users.username.

Folgefehler / Risiko

  • Die neue Admin-Absicherung aus diesem PR (Depends(get_admin_user) auf Adapter-Config- und Binding-Mutationsrouten) war damit für API-Keys nicht verlässlich:
    • Ein Nicht-Admin konnte einen API-Key mit Namen z. B. admin anlegen; dieser Name floss dann in die Admin-Prüfung ein (Privilege-Confusion).
    • Umgekehrt konnten legitime Admin-Automationskeys mit beliebigem Namen fälschlich abgewiesen werden.

Was wurde geändert?

  • obs/api/auth.py: API-Key-Authentifizierung löst die Identität jetzt über api_keys.owner auf (statt über api_keys.name).
  • Legacy-Keys ohne Owner werden mit 401 abgewiesen, damit keine unklare Privilegzuordnung mehr möglich ist.

Absicherung durch Tests

  • pytest -q tests/integration/test_auth.py -> 11 passed
  • Neue Regressionstests:
    • Nicht-Admin-Key mit Name admin darf keinen Adminzugriff erhalten.
    • Admin-Key mit beliebigem Namen behält Adminzugriff.

This was referenced May 20, 2026
Closed
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@abeggled abeggled Awaiting requested review from abeggled

Assignees

@Micsi Micsi

Labels

Security Security-related changes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@Micsi