Skip to content

osctrl: follow-up fixes for #813 — per-service --auth defaults + carve path SQL escape#817

Open
alvarofraguas wants to merge 1 commit into
jmpsec:mainfrom
alvarofraguas:pr/round-1-followup-auth-split-carve-escape
Open

osctrl: follow-up fixes for #813 — per-service --auth defaults + carve path SQL escape#817
alvarofraguas wants to merge 1 commit into
jmpsec:mainfrom
alvarofraguas:pr/round-1-followup-auth-split-carve-escape

Conversation

@alvarofraguas
Copy link
Copy Markdown
Contributor

Follow-up to #813 addressing the two open review threads that landed after the merge:

1. Per-service --auth defaults

#813 flipped the shared --auth default to jwt, but the flag lives in initServiceFlags which is called by all three services. That produced misleading --help output for osctrl-tls (which authenticates osquery nodes via the per-environment enroll secret, not via this flag).

Split --auth out into a new initAuthFlag(params, defaultValue) helper and call it from each Init*Flags with the correct default per service:

Service Default Why
osctrl-api jwt Closes U-AUTH-1 (audit-flagged "unauthenticated by default")
osctrl-admin db Its existing browser login form
osctrl-tls none osquery nodes auth via enroll secret; tls's main.go doesn't read this field

Usage text reverted to the original "Authentication mechanism for the service" per your other comment on #813. The OSCTRL_INSECURE_NO_AUTH env-var note is no longer in help text — operators who set --auth=none on osctrl-api see the actionable error message at startup from guardAuthMode (unchanged from #813).

2. Carve path: replace regex gate with SQL string-literal escape

The previous ValidCarvePath regex (introduced in #813) rejected legitimate paths containing spaces, parentheses, or non-ASCII characters — C:\Program Files\Common Files\app.log, /Library/Application Support/com.example/cfg, /home/álvaro/notes.txt all failed it.

Replaced with a splice-site defense in GenCarveQuery:

  • escapeSQLString doubles single quotes — '; SELECT 1; -- becomes ''; SELECT 1; --, parsed by SQLite as the literal contents of one string, no way to break out
  • escapeLikePattern escapes LIKE meta-chars (\, %, _) so existing characters in the path stay literal
  • globToLike runs LIKE-escape first, then maps */?%/_, then SQL-quote-escapes; order documented
  • Query emits ESCAPE '\' so the parser honors the escape character

Removed ValidCarvePath / validCarvePath and the precheck call in CarvesRunHandler. Test coverage:

  • Classic injection on both exact and glob branches (single quote doubled, payload becomes literal)
  • Legitimate Windows / macOS / UTF-8 paths that the old regex would have rejected
  • Glob mapping with literal-meta-char escapes (%, _, \)
  • SQL shape sanity for happy paths

Verified

  • go build ./... clean
  • go vet ./... clean
  • go test ./... green across all packages
  • osctrl-api/admin/tls --help all show the correct per-service auth default
  • Live smoke on a dev stack against real Postgres: 4 carve requests (Windows-with-spaces, macOS Application Support, UTF-8, classic injection) all land as expected. The injection's single quote is doubled in the persisted SQL, and SQLite parses the result as one literal string.

Test plan

  • CI: lint + build + tests pass once workflows are approved
  • Manual: osctrl-tls --help | grep '\-\-auth' shows default: "none"
  • Manual: carve a file with a path containing spaces (e.g. C:\Program Files\app.log); confirm the carve completes and the path round-trips

@alvarofraguas
Copy link
Copy Markdown
Contributor Author

Hi @javuto — caught your "leave it like this, including the usage" on #813 this morning, but it landed after #817 was already up with the short Usage form. Apologies for the cross-thread mismatch — I didn't have your preference yet when I drafted #817.

The per-service flag split in #817 (api=jwt, admin=db, tls=none) addresses your "tls is AuthNone for sure" remark and isn't affected. The only piece that diverges from your latest guidance is the Usage string on osctrl-api's --auth flag:

Your preference (long, what #813 shipped with):

--auth   Authentication mechanism for the service
         (jwt|none — `none` requires OSCTRL_INSECURE_NO_AUTH=1)

Currently in #817 (short):

--auth   Authentication mechanism for the service

Happy to amend #817 to restore the long form. Before I do, I want to surface the trade-off so you can pick with full context — I went with the short form for security-leaning reasons that are worth a moment.

Why the short Usage in #817:

  • The long form literally tells operators "here's how to turn auth off." A frustrated operator who can't get JWT working can shave seconds off finding the disable path by reading --help instead of hitting the fatal first.
  • The fatal-then-read-error path forces a brief reconsider moment that the inline help text skips.
  • It avoids cargo-cult propagation — fewer copy-pasted compose examples with OSCTRL_INSECURE_NO_AUTH=1 in the wild because fewer operators discover the env var via --help.

Why the long Usage is reasonable:

  • The compensating controls (default flip to jwt, fatal guard on auth=none without the env var, 60s warning ticker) absorb most of the actual risk. The Usage style is mostly UX.
  • Discoverability helps the legitimate dev/test case — operators who need --auth=none for local development find it cleanly.
  • You see far more osctrl deployments than I do; if you've watched operators struggle with this flag, the discoverability win probably outweighs the small disclosure risk.

My read: the security delta is small — ~15 seconds of friction to disable auth. The guard catches the dangerous outcomes either way. The choice is really UX-vs-defense-in-depth at the margin, and you have better visibility into which side of that margin matters in practice.

Tell me which way to go and I'll amend.

Follow-up on jmpsec#813 (merged) addressing two review threads:

== Per-service --auth defaults ==

The shared `initServiceFlags` was applying a single `--auth` default
across all three services, which produced the wrong `--help` output
for osctrl-tls (which authenticates osquery nodes via the per-env
enroll secret, not via this flag — `none` is the correct default for
it).

Split the `--auth` flag out of `initServiceFlags` into a new
`initAuthFlag(params, defaultValue)` helper and call it from each
service's `Init*Flags` with the semantically correct default:

  - osctrl-api   → `jwt`   (closes U-AUTH-1, the audit-flagged
                            "unauthenticated by default" finding)
  - osctrl-admin → `db`    (its existing browser login form)
  - osctrl-tls   → `none`  (osquery nodes auth via enroll secret;
                            tls's main.go does not read this field)

Usage text reverted to "Authentication mechanism for the service" per
review feedback — the env-var note is intentionally not in help text;
operators who set `--auth=none` on osctrl-api see the actionable
error message at startup from `guardAuthMode` instead.

== Carve path: replace regex gate with SQL string-literal escape ==

The previous `ValidCarvePath` regex blocked legitimate paths that
contain spaces, parentheses, or non-ASCII characters (e.g.
`C:\Program Files\Common Files\app.log`,
`/Library/Application Support/com.example/cfg`,
`/home/álvaro/notes.txt`). Replaced with a splice-site defense:

  - `escapeSQLString` doubles single quotes so an attempted injection
    like `'; SELECT 1; --` becomes the literal contents of the SQL
    string literal — there is no way to break out of the surrounding
    quotes.
  - For glob mode, `escapeLikePattern` escapes the LIKE meta-chars
    (`\`, `%`, `_`) with `\` so pre-existing LIKE characters in the
    path stay literal, then `*`/`?` map to `%`/`_`. The query emits
    `ESCAPE '\'` so the parser respects the escapes.
  - `globToLike` runs the LIKE escape first, then the glob mapping,
    then the SQL-quote escape — the order matters and is documented.

Removed `ValidCarvePath` / `validCarvePath` and the precheck call in
`CarvesRunHandler`. Test coverage:

  - injection attempts on both exact and glob branches
  - legitimate Windows / macOS / UTF-8 paths that the old regex
    would have rejected
  - glob mapping with literal-meta-char escapes
  - SQL shape sanity for happy paths

Verified locally:
  - go build ./...  (clean)
  - go vet ./...    (clean)
  - go test ./...   (all packages green; 4 new carve tests pass)
  - osctrl-api/admin/tls --help all show the correct per-service
    auth default
  - Live smoke on a dev stack against real Postgres: 4 carve requests
    with Windows-with-spaces, macOS Application Support, UTF-8, and
    the classic injection string all land as expected — the
    injection's single quote is doubled to `''` in the persisted SQL,
    and SQLite parses the result as one literal string.
@alvarofraguas alvarofraguas force-pushed the pr/round-1-followup-auth-split-carve-escape branch from 46f6777 to 39ca450 Compare May 16, 2026 08:06
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