Skip to content

feat(auth): always enforce management API scopes - #174

Merged
flemzord merged 1 commit into
mainfrom
feat/always-enable-auth-scopes
Oct 22, 2026
Merged

flemzord merged 1 commit into
mainfrom
feat/always-enable-auth-scopes

Conversation

@flemzord

Copy link
Copy Markdown
Member

Auth previously accepted valid management API tokens without checking their scopes by default. Scope checks are now always enabled: reads require auth:read or auth:write, and mutations require auth:write.

Remove --auth-check-scopes and the test-server toggle. Add 12 integration cases for missing, unrelated, read and write scopes on /clients and /users, plus CLI coverage for the removed flag.

Upgrade: remove --auth-check-scopes from deployment arguments (including explicit true/false), configure the required client scopes, and obtain new tokens before upgrading. Token issuance rules and other services are unchanged.

Validation:

  • nix develop --command just pc
  • nix develop --command go test -race -tags it ./...
  • Mandatory scope integration tests rerun after the lint correction.

@flemzord
flemzord requested review from a team as code owners October 22, 2026 08:57
@strix-security

strix-security Bot commented Oct 22, 2026 •

Copy link
Copy Markdown

Strix Security Review

✅ No open security findings.

PR summary: The pull request makes scope enforcement unconditional for the auth service's management API by removing the `--auth-check-scopes` flag and hardcoding scope checking on, updating the test-server plumbing, documentation and tests accordingly.

Review summary

Reviewed all six changed files. The production change is narrow and strictly tightening: cmd/serve.go removes the optional --auth-check-scopes flag and hardcodes scope checking on in both places the authenticator is constructed (authlib.ModuleConfig.CheckScopes and authlib.NewJWTAuth), while pkg/testserver/server.go drops the now-meaningless test-server toggle. I verified the enforcement semantics against the pinned library implementation (go-libs/v3@v3.6.1/auth: GET/HEAD/OPTIONS/TRACE require <service>:read or <service>:write, all other methods require <service>:write, with a denial surfaced as HTTP 401 through authlib.Middleware), and the new scope matrix in the added integration test matches that behaviour for the two route groups that actually carry the middleware (/clients, /users). Removal of the flag fails closed: no unknown-flag whitelist exists on the root command, so a deployment still passing the old argument (including =false) is rejected at startup rather than silently running with weaker checks, which the added CLI test exercises and which passes. No input handling, sinks, query construction, secrets, cryptographic material, redirect targets or serialization were added or altered, and no existing authorization control was deleted beyond the ability to disable scope checks. The upgrade impact (argument removal plus required client scopes) is a documented, intentional breaking change rather than a defect. No security issues were found in the changed code.

Ruled out (3)
  • Can a caller still reach the management API without scope checks after the flag is removed?
    Both authenticator construction sites in `cmd/serve.go` now hardcode the scope check to true, and the pinned library denies requests lacking `<service>:read`/`<service>:write`, so there is no remaining configuration path that disables enforcement.
  • Does removing the flag fail open for deployments that still pass `--auth-check-scopes=false`?
    The root command has no unknown-flag whitelist, so the stale argument produces an `unknown flag` startup error (asserted by the updated CLI test) instead of silently starting with checks disabled.
  • Does the new integration test assert a scope matrix the library does not actually implement?
    The twelve cases match the library's method-based rules exactly (read methods accept `auth:read` or `auth:write`, mutating methods require `auth:write`, denials return 401), and the covered routes are the ones wrapped by the auth middleware.

Review 1 on this PR · 1846d7d · requested Oct 22 08:57 UTC when the PR was opened


Reviewed by Strix
Re-run review · Configure security review settings

@NumaryBot NumaryBot added risk: medium bot-reviewed review-approved The NumaryBot review gate is satisfied for the current head. labels Oct 22, 2026

@NumaryBot NumaryBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The required automated review completed with no remaining findings.

@flemzord flemzord changed the title fix(auth): always enforce management API scopes feat(auth): always enforce management API scopes Oct 22, 2026
@flemzord
flemzord merged commit 08ff861 into main Oct 22, 2026
24 checks passed
@flemzord
flemzord deleted the feat/always-enable-auth-scopes branch October 22, 2026 10:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bot-reviewed review-approved The NumaryBot review gate is satisfied for the current head. risk: medium

Development

Successfully merging this pull request may close these issues.

2 participants