mirror of
https://github.com/taylanbakircioglu/haproxy-openmanager.git
synced 2026-09-16 23:55:13 +00:00
main
5 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 taylanbakircioglu
|
fbe223250b |
perf(requestlog): gate the embedded-secret scan behind a substring pre-check
The auth_pass / stats-auth / userlist / URI patterns added by the redaction
fixes on this branch run on the writer task, on every string value of every
captured body, so their cost is paid per row forever. Measured, they were:
writer-side redaction, before the redaction fixes 12.2 us/row
writer-side redaction, with them 359.1 us/row
A 29x regression, and none of it was spent matching anything - almost every
body contains none of these keywords. Profiling the four patterns on an 8 KB
config body, 300 iterations:
one combined alternation, IGNORECASE, \b-anchored 308.2 us
the same four run separately (sum) 219.7 us
text.lower() once + four substring pre-checks 5.3 us
`\b` and IGNORECASE each defeat the regex engine's literal-prefix scan, so
every alphanumeric position in 8 KB became a candidate start and the engine
walked the whole body four times to find nothing. Isolated, `auth_pass` costs
27.5 us with IGNORECASE and 2.4 us without.
Split the alternation and gate each pattern behind a substring test on one
lowercased copy. `str.lower()` and `in` are C-level scans; a pattern now runs
only when its keyword is actually present, and then on text that genuinely
contains it. Cost becomes O(total string bytes) instead of O(bytes x patterns).
The URI pattern also drops IGNORECASE and `\b` - its character class already
covers both cases, and `://` gives the engine a literal to scan for.
writer-side redaction, after 18.8 us/row
= 3.4s of CPU/day
at 180 000 rows/day
6.6 us/row over the pre-fix baseline, for four secret classes that were
previously written to the table in cleartext.
The pre-checks are on the lowercased copy, so the patterns stay IGNORECASE: the
marker may well have been `AUTH_PASS` in the original. Request-path cost is
unchanged at 27.7 us p50 - none of this ever ran there.
All 123 redaction and payload tests still pass, so the behaviour is identical;
only the path to it is cheaper.
|
||
|
taylanbakircioglu
|
82e6fe3f9c |
fix(requestlog): mask HAProxy credentials in uploaded config bodies
This application never RENDERS a credential into a haproxy.cfg - grepping the
generator and every sample config for `stats auth`, `userlist` and
`insecure-password` returns nothing - so none of our own output is at risk. The
exposure comes from the other direction: the agent uploads the node's REAL
on-disk file.
linux_install.sh: config_content=$(cat "$config_path")
-> POST /api/configuration/agents/{n}/config-response
and `POST /api/config/validate` plus the bulk import take whatever the operator
pastes. A production haproxy.cfg routinely carries `stats auth admin:<password>`
and a `userlist` block, so what is captured is credential-bearing even though
what we generate is not.
Two patterns, folded into the existing single-pass alternation:
stats auth admin:S3cr3t -> stats auth admin:********
user ops password $6$... -> user ops password ********
user dev insecure-password Hunter2 -> user dev insecure-password ********
The username on `stats auth` is deliberately kept: an operator debugging a 401
still needs to know WHICH account it was about.
The load-bearing detail is where a value ENDS. A config upload is routinely
larger than the 8 KB capture cap, so for exactly these payloads the common case
is not the parsed body - it is the truncated `{"_raw": ...}` fallback, where the
line breaks are still the two-character escape `\n` rather than real newlines.
A "rest of the line" match that does not know that runs past every apparent
line break: measured on a 17 KB upload, `auth_pass ...` masked the entire
remainder of the captured string. No leak, but the row is then worthless. Every
value pattern here stops at a real newline OR at a literal backslash-n, so the
same three credentials are masked and the surrounding config stays readable in
both forms. Both paths are verified.
Anchoring is to HAProxy keyword syntax, not to the bare word "password", so
ordinary prose survives: "invalid password format" and "the password must be 8
chars" are untouched. One accepted false positive: "user admin password reset
requested" masks the word "reset", because it is indistinguishable from a
userlist line without parsing the file. That is the same trade the existing
REDACT_CONTAINS entry for `token` already makes - a blanked word costs a little
readability, an unmasked credential costs a credential.
|
||
|
taylanbakircioglu
|
190d45fe09 |
fix(requestlog): scrub credentials carried inside URI values
`POST /api/mfa/enroll` returns the new TOTP secret twice: once as `secret`,
which redaction already caught, and once inside `otpauth_uri` as a query
parameter, which it did not. Blanking one field while the same value sits three
keys away in cleartext is not redaction.
before: {"secret": "***REDACTED***",
"otpauth_uri": "otpauth://totp/OpenManager:admin?secret=JBSWY3DP..."}
after: {"secret": "***REDACTED***",
"otpauth_uri": "otpauth://totp/OpenManager:admin?secret=***REDACTED***"}
routers/mfa.py states the rule this restores: its own activity-log call records
`{"secret_len": len(secret_plain)}` with the comment "NEVER log the secret
itself".
The fix is not otpauth-specific. Any URI found in any captured string value now
goes through scrub_url, which was already written for exactly this and was only
ever pointed at the request line. That also covers:
* userinfo credentials - `https://user:pass@host/path` keeps only the host;
* any query-string credential in a body value or an error message, using the
same is_secret_key rules as the request line, so `?token=`, `?api_key=`,
`?password=` are all handled without naming them again here;
* fragments, which are dropped - they never reach a server and can carry
tokens.
Folded into the existing alternation rather than added as a second pass, so a
captured body is still scanned once. Two guards keep it from doing harm: a URI
with neither `?` nor `@` is returned untouched rather than rebuilt, and if
scrub_url reports a parse failure the original text is kept - inside a larger
string its placeholder would corrupt the surrounding sentence.
Verified: the TOTP secret and a userinfo password are removed, an in-text URL
inside an error message is scrubbed in place, and two innocent ACME URLs
(directory_url, account_url) come through byte-identical.
Also withdrawn here: the review flagged `POST /api/agents/generate-install-script`
as leaking the agent API key through its `script` field. That was wrong. The
finding was built from the endpoint's docstring EXAMPLE (routers/agent.py:497),
which shows an `api_key` field and an `API_KEY="agt_..."` line; the handler's
actual return is {script, platform, cluster_id, filename} and the template
substitution map has no token placeholder. The agent token reaches a node by a
different path entirely, and is issued as `api_key` (routers/security.py:144),
which REDACT_CONTAINS already covers. No change was needed and none is made.
|
||
|
taylanbakircioglu
|
c50408026b |
fix(requestlog): keep the VRRP password out of the request log
Measured against the redaction module from the previous commit, using the real
field names this codebase uses. Three paths wrote the keepalived `auth_pass`
secret to `request_logs` in cleartext:
POST/PUT /api/vip request body, `auth_pass` field
GET /api/agents/{n}/keepalived-config response, keepalived.config_content
POST /api/agents/{n}/keepalived-discovery request body, config_content
The middle one is the worst: the agent polls it on the SSL cadence, so the
secret was re-written to the audit table roughly 576 times a day per member
node.
Two independent holes, closed independently:
1. KEY NAME. `auth_pass` normalizes to "authpass". "password" is not a
substring of it, and the bare "auth" entry in REDACT_EXACT is an exact
match, not a prefix - so nothing in either set matched and the field was
kept verbatim. "authpass" is now in REDACT_EXACT.
2. EMBEDDED IN A CONFIG BLOB. The two agent endpoints do not carry the secret
as its own field at all; they carry a whole keepalived.conf as one string
under `config_content`, with `auth_pass <secret>` on a line inside it. No
key-name rule can see that, and the value-shape guards do not either: it is
neither a PEM block nor a JWT. Added scrub_embedded_secrets(), one compiled
alternation applied in a single pass per string value, masking the whole
remainder of the line so a secret containing whitespace cannot partially
leak - the same pattern and the same reasoning as routers/vip.py's
_redact_secret and routers/agent.py's _AUTH_PASS_MASK_RE.
It runs BEFORE the _MAX_STRING truncation, not after: `auth_pass` sits in the
first few hundred bytes of a rendered keepalived.conf, so letting the cut
"handle" it would be relying on where the secret happens to fall in the file.
This restores an invariant the codebase already states and already enforces
elsewhere. routers/vip.py: "the secret never leaves the server in cleartext ...
only the at-rest Fernet token and the agent-delivery endpoint ever see the real
value". The discovery handler in routers/agent.py goes to three separate
lengths to uphold it - it pops auth_pass out of the parsed analysis, replaces it
with a has_auth_pass boolean, Fernet-encrypts the secret into its own column,
and stores only a masked copy as vip_discoveries.raw_config_masked. Capturing
the request that produced all that, unmasked, put the plaintext right back next
to it.
Verified: the three cases above now redact, and the ten cases that already
worked (login password, JWTs, PEM private keys under any key name, DNS provider
credentials, ACME JWS) are unchanged.
|
||
|
mustafa.ulukaya
|
ef26860df9 |
feat(logging): unified request/response log with configurable retention (v1.11.0)
Until now the only record of what happened was `user_activity_logs`, which stores non-GET 2xx operations with no bodies. When something failed you could see that a counter went up, never what was sent or what came back. This adds one queryable timeline covering both directions: - inbound: every API call, including GETs and including 4xx/5xx, with the user, client IP, status, duration and — redacted, size-capped — the request and response bodies. - outbound: every HTTP call the backend makes, tagged with who it went to (ACME/Let's Encrypt, Cloudflare, GoDaddy, HAProxy stats, agents, the ACME diagnostics probe). Outbound rows inherit the inbound request's id, so one operator action and the CA/DNS calls it triggered read as a single trace: opening a failed "Request Certificate" shows the exact POST /acme/new-order and the CA's 429 underneath. Implementation notes: - Capture is a pure-ASGI middleware that TEES the request and response streams rather than draining them. `await request.body()` inside a BaseHTTPMiddleware would consume the receive channel and break the raw-body agent heartbeat handler. Registered last so it is outermost: it then sees the final client-visible response and seeds correlation_id_context before the error handler reads it. - Rows are written by a batching background writer with a bounded queue, so the request path never awaits the database and a saturated logger drops rows visibly (surfaced on the page) instead of blocking. Redaction runs on the writer, off the request coroutine. - Secrets never land: headers are an allowlist with Authorization/Cookie kept only as a presence marker; body keys and value shapes are redacted (passwords, tokens, api_token, API keys, private-key PEMs, JWTs); the ACME JWS request body is never stored, because a stored protected+signature pair is a replayable credential — a summary is logged instead; DNS-provider errors record only the exception type; the ACME HTTP-01 challenge endpoint is excluded so key_authorization is never captured. - Retention is operator-configurable in Settings -> Request Log: separate day counts for successful and failed rows (7 / 30) plus a hard row cap (500k), whichever is reached first. Pruned in batches under a Postgres advisory lock, with the day counts bound as parameters, never interpolated. - New permissions requestlog.read / requestlog.manage. super_admin and security_admin get both, operator gets read, viewer gets neither. Schema: one new table (request_logs) plus its settings seed, SCHEMA_VERSION 10 -> 11, auto-migrated. No existing table altered, no agent or rendered-config change. Kill switches: REQUEST_LOG_ENABLED=false (middleware never registered) or the `enabled` toggle in Settings. Tests: 245 new (7 backend files + 1 frontend), full suite 1655 backend + 17 frontend passing. |